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

send custom user agent when using got #6256

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Mar 10, 2021

There was always going to be something that slipped through the cracks.
Closes #6251

@chris48s chris48s added the core Server, BaseService, GitHub auth label Mar 10, 2021
@shields-ci
Copy link

Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against 2a4eb54

@@ -3,6 +3,8 @@
const got = require('got')
const { Inaccessible, InvalidResponse } = require('./errors')

const userAgent = 'Shields.io/2003a'
Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea what this number means. I just copied it from

const userAgent = 'Shields.io/2003a'

I decided to just duplicate this in got.js instead of importing it from legacy-request-handler as I think we can get rid of a lot of what is in there in the near future

Copy link
Member

Choose a reason for hiding this comment

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

😆 I almost asked about this too but vaguely remembered it being a magical value of sorts

Copy link
Member

Choose a reason for hiding this comment

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

CalVer! 😁

It means GHSA-6qx4-g993-2225 is patched. The intent was to make it possible for upstream services to block requests from unpatched self-hosted instances.

Now that we've got actual versions (thanks @chris48s!) if it's possible to get the actual version in there I think that'd be a nice refinement.

Copy link
Member Author

Choose a reason for hiding this comment

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

That.. sort of makes sense for self-hosted installs (unless they track next/master), although we don't pin our own deploys to a version, by which I mean

we tag server-YYYY-MM-DD and deploy
we make some more commits and deploy those too
now the version we're running in production isn't server-YYYY-MM-DD - its a future version that doesn't correspond to any tag

I suppose Shields.io/server-YYY-MM-DD would at least mean server-YYY-MM-DD (or later)

Copy link
Member

Choose a reason for hiding this comment

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

Sure. We could also use server-YYY-MM-DD for the Docker releases and includ something like server-YYY-MM-DD+dev for production.

Reading this thread makes me think we should include some extra stuff in the query string on request for the dynamic + endpoint badges, or when a user-specified URL is used, since it allows a service provider to differentiate between our official badge requests, and other requests which are being specified by the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made an issue for this as I suspect we're not going to pick this up immediately and this conversation will disappear into the github ether. #6268

@chris48s chris48s merged commit fd7eddc into badges:master Mar 10, 2021
chris48s added a commit to chris48s/shields that referenced this pull request Mar 16, 2021
chris48s added a commit that referenced this pull request Mar 17, 2021
* Revert "send custom user agent when using got (#6256)"
This reverts commit fd7eddc.

* Revert "Migrate request to got (part 1 of many) (#6160)"
This reverts commit 2359eb2.

* install got as a prod dependency, allow npm 7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different user agent
4 participants