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 better user-agent values (and got config changes) #7309

Merged
merged 5 commits into from
Nov 25, 2021
Merged

Conversation

chris48s
Copy link
Member

Refs #4655
Closes #6268

There is not loads of changed code in this PR, but there's a couple of things going on in here which are kind of linked and kind of not.

  • One is a bit of cleanup. The request --> got work left us with this slightly clunky thing where got.js exports a fetchFactory which takes fetchLimitBytes and returns a function. That allows us to inject fetchLimitBytes from config in the server but then sometimes it is useful to construct it outside the context of the server object.
  • We've also had this long-standing issue Send better user-agent values #6268 which I had a look at before and was also a bit clunky/fiddly for the same reason.

I did have a look at completely decoupling config parsing from the server, but what I realised was that because we can set some of the args at runtime (see

shields/server.js

Lines 28 to 33 in 95a439a

if (+process.argv[2]) {
config.public.bind.port = +process.argv[2]
}
if (process.argv[3]) {
config.public.bind.address = process.argv[3]
}
) this didn't seem like the right approach as the whole config object doesn't depend exclusively on the yml/env vars and we'd have to double validate the config at server start time.
The approach I settled on was making a subset of config available outside the context of the server object, which seemed like the right tradeoff.

The other bit of this is adding a feature. Basically before this PR, every instance of shields (our own, self-hosted, dev copies) sends the exact same user agent value (Shields.io/2003a, for..reasons).
As of this PR:

  • The default user agent is shields (self-hosted)/dev
  • We (or users who self-host their own instance) can override the base value (the part before the slash)
  • If either HEROKU_SLUG_COMMIT (we have this set in production) or DOCKER_SHIELDS_VERSION (this PR adds it to the image) are set, that will be used for the second part of the UA string (after the slash)
  • If neither of those vars are set it will be /dev

This should mean in most cases the userAgent string will be reasonably meaningful.

- add userAgentBase setting
- send short SHA in user agent on heroku
- set a version (tag or short SHA) in Dockefile and use
  it to report server version in UA for docker users
@chris48s chris48s added the core Server, BaseService, GitHub auth label Nov 22, 2021
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7309 November 22, 2021 19:44 Inactive
@shields-ci
Copy link

shields-ci commented Nov 22, 2021

Warnings
⚠️

📚 Remember to ensure any changes to config.private in services/github/auth/acceptor.js are reflected in the server secrets documentation

⚠️ This PR modified the server but none of its tests.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against d41fc55

core/base-service/got.js Show resolved Hide resolved

const fetchLimitBytes = bytes(publicConfig.fetchLimit)

function getUserAgent(userAgentBase = publicConfig.userAgentBase) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We will need to set USER_AGENT_BASE=Shields.io in prod before we deploy

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

I'm good with the changes, both code and strategic, and approving accordingly so you can move forward if you'd like. One minor inline item that could be ignored or handled in a follow up, but happy to re-:+1: if you decide to make any other changes

@chris48s chris48s temporarily deployed to shields-staging-pr-7309 November 24, 2021 20:20 Inactive
@chris48s
Copy link
Member Author

I've set the env var, deployed and confirmed it is working in production by using the endpoint badge to make a request to an endpoint where I can see the header.
Hopefully this is all fine and I think it is a good change, but there is this slight worry at the back of my mind that somewhere out there in the world is a rate limit or deny list with a specific exception for Shields.io/2003a hard-coded which is going to break something now that we're sending a different suffix each time. We will have to see..

@calebcartwright
Copy link
Member

but there is this slight worry at the back of my mind that somewhere out there in the world is a rate limit or deny list with a specific exception for Shields.io/2003a hard-coded which is going to break something now that we're sending a different suffix each time

This was in the back of my mind as well, though I actually think this change is perhaps the best way forward to surface any potential issues. I don't think we can (or should) perpetually pin a user agent, and it feels like changing it as we've done here may be the only way for us to truly discover any such cases, which will then allow us to figure out next steps

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.

Send better user-agent values
4 participants