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

fix(api): Set API identifying headers on all HTTP requests (CODY-4209) #6102

Merged
merged 4 commits into from
Nov 21, 2024

Conversation

jamesmcnamara
Copy link
Contributor

@jamesmcnamara jamesmcnamara commented Nov 9, 2024

Closes CODY-4209

Sets the X-Requested-With flag on all HTTP requests. I tested this locally with VSCode, Eclipse, JetBrains, Cody Web and the Cody CLI and for each of them (in concert with these two other PRs) saw the headers appear in the logs when I made a chat interaction.

  • VSCode identifies as vscode
  • JetBrains identifies as jetbrains
  • Eclipse identifies as eclipse
  • Cody Web identifies as web
  • Cody CLI identifies as cody-cli

We might consider either prepending each name with sourcegraph- if we want to be very explicit that it's our apps, but I think it's pretty clear with the current values and it's consistent with the clientName and CodyIDE values used through out the codebase today.

The one tricky bit here is Cody Web because at least when developing locally, the requests to the SG instance are CORS requests and by default the local server is not a trusted origin. In the two linked PRs I added the headers to the trusted list and added the default vite server URL to the local trusted origins. I think that we shouldn't have to do anything for production because they shouldn't be CORS requests then right?

Are there any other places might I need to check or update to make sure this works?

Edit: I just added the if statement to exclude these headers if we're in development mode. If we like that I can probably close the dev-private PR and we won't have to make any changes to S2.

Test plan

Tested manually to see that the HTTP header was present

Changelog

Sets the X-Requested-With header on all HTTP requests.

@jamesmcnamara jamesmcnamara changed the title fix(api): Set API identifying headers on all HTTP requests fix(api): Set API identifying headers on all HTTP requests (CODY-4209) Nov 9, 2024
@dadlerj
Copy link
Member

dadlerj commented Nov 12, 2024

Approved from a property naming perspective. I defer to @vovakulikov to review the code changes :)

@jamesmcnamara jamesmcnamara merged commit d93cc15 into main Nov 21, 2024
23 of 24 checks passed
@jamesmcnamara jamesmcnamara deleted the jsm/cody-4209-client-name-header branch November 21, 2024 23:58
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