Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return an error when additional headers conflicts #910

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

jhchabran
Copy link
Contributor

Passing SRC_ADDITIONAL_HEADERS_AUTHORIZATION=... and SRC_ACCESS_TOKEN at the same time leads to the latter overriding the former silently, making it quite difficult to spot.

This PR surfaces the conflict as an error explictly advising to never set SRC_ACCESS_TOKEN in conjunction with the additional header.

Test plan

Unit test + local run:

# OK:

~/work/src-cli/cmd  jh/fix-authorization-header-conflict $ SRC_HEADER_AUTHORIZATION="sourcegraph" SRC_ENDPOINT=https://sourcegraph.test:3443 go run ./... api -query='query { currentUser { username } }'
{
  "data": {
    "currentUser": {
      "username": "sourcegraph"
    }
  }
}

# Prints an error because that's doomed to fail: 

~/work/src-cli/cmd  jh/fix-authorization-header-conflict $ SRC_ACCESS_TOKEN="foobar" SRC_HEADER_AUTHORIZATION="sourcegraph" SRC_ENDPOINT=https://sourcegraph.test:3443 go run ./... api -query='query { currentUser { username } }'
reading config: when passing an 'Authorization' additional headers, SRC_ACCESS_TOKEN must never be set
exit status 1
~/work/src-cli/cmd  jh/fix-authorization-header-conflict $ 

@jhchabran jhchabran requested a review from efritz January 4, 2023 17:32
@jhchabran jhchabran merged commit eb68e5f into main Jan 4, 2023
@jhchabran jhchabran deleted the jh/fix-authorization-header-conflict branch January 4, 2023 17:37
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.

2 participants