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

Pass headers to feed generator #2030

Merged

Conversation

CooperEdmunds
Copy link
Contributor

Forwards headers from getFeed req to the feed generator. In light of this PR adding an Accept-Language header to requests from the frontend, this will enable feed generators to respect the user's language preferences.

Comment on lines 41 to 46
const headers: Record<string, string> = {}
for (const [key, value] of Object.entries(req.headers)) {
if (typeof value === 'string') {
headers[key] = value
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll want to avoid passing through hop-by-hop headers, which include the following:

  • connection
  • keep-alive
  • proxy-authenticate
  • proxy-authorization
  • te
  • trailer
  • transfer-encoding
  • upgrade

The connection header itself may list additional hop-by-hop headers as well—this is all described a bit on mdn: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection. We probably also want to avoid passing through the host header, since the host is different in the upstream request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's enough complexity with the hop-by-hop headers that we may just want to special-case accept-language for now until we can do a more rigorous pass on this.

@CooperEdmunds are there any other headers that you'd like to have passed through to the feed generator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accept-language is sufficient. I'll make that change.

Copy link
Collaborator

@bnewbold bnewbold Jan 9, 2024

Choose a reason for hiding this comment

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

yeah, my intuition would be to pass-through only an allow-list of headers, not all-by-default.

In addition to divy's list above, there are some security/privacy sensitive headers like X-Real-IP, X-Forwarded-Proto, Strict-Transport-Security, Cookie, etc.

Other ones I could see wanting to pass through are:

  • Accept (content-negotiation)
  • Cache-Control (eg, if trying to cache-bust?)
  • If-Modified-Since (cache control)
  • Max-Forwards (prevent loops!)
  • Origin (for CORS? and logging? possible privacy concern?)
  • Referer (possible privacy concern?)
  • User-Agent
  • DNT (do-not-track)

Copy link
Collaborator

@dholms dholms left a comment

Choose a reason for hiding this comment

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

Sweet this looks good! Thanks for taking the initiative on this, have been meaning to do it for awhile but it kept slipping.

Running CI then can merge

@dholms dholms merged commit 50f7045 into bluesky-social:main Jan 9, 2024
10 checks passed
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.

None yet

4 participants