Skip to content

bug: only require CI=true and AccessToken when making API requests#1278

Open
burmudar wants to merge 7 commits intomainfrom
wb/bug-src-token-and-ci
Open

bug: only require CI=true and AccessToken when making API requests#1278
burmudar wants to merge 7 commits intomainfrom
wb/bug-src-token-and-ci

Conversation

@burmudar
Copy link
Contributor

@burmudar burmudar commented Mar 25, 2026

In https://github.com/sourcegraph/sourcegraph/actions/runs/23518169374/job/68455311352?pr=11118 the action failed on command src version -client-only, where it should not have failed because it's a client only check and no API interaction thus not requiring a Access Token to be set.

In this PR we update the config:

  • record whether we are in CI

Update APIClient:

  • add a check to see if we're in CI and our AuthMode is "AccessToken"
  • fail on any request if we require a CI AccessToken and it is not set.

Closes CPL-291

Test plan

unit tests + manual tests

@burmudar burmudar requested a review from a team March 25, 2026 08:15
@burmudar burmudar self-assigned this Mar 25, 2026
return c.inCI
}

func (c *config) requireCIAccessToken() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a doc string. Additionally at callsites this reads kinda weird. Maybe it should be called checkIfCIAccessTokenRequired (matching up with the err it returns)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated


func (c *config) requireCIAccessToken() error {
if c.InCI() && c.AuthMode() != AuthModeAccessToken {
return errCIAccessTokenRequired
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there might be an access token set but it is oauth instead? Maybe we need to include a little more context when this fails to help debug weird issues. Or does oauth + CI seem impossible?

Copy link
Contributor Author

@burmudar burmudar Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add some context

Or does oauth + CI seem impossible?

Not entirely impossible but it's definitely the more difficult path. If there is no keyring the OAuth token you get will only be used for that session and then forgotten (we do warn when this happens)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also can't use OAuth access tokens as SRC_ACCESS_TOKEN values, since OAuth requires Bearer in the Authorization header and SRC_ACCESS_TOKEN requires token

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to provide more context in the error!

- rename `RequireAccessToken` to `checkIfCIAccessTokenRequired`
- provide more context in error on why it happened
@burmudar burmudar force-pushed the wb/bug-src-token-and-ci branch from e6c2532 to cd32acb Compare March 25, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants