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

add more unsignable headers #1231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cfbao
Copy link
Contributor

@cfbao cfbao commented Dec 25, 2024

Description

Add more unsignable headers, including the user-agent header.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Comment on lines +48 to +52
"user-agent",
"via",
"x-forwarded-for",
"x-forwarded-port",
"x-forwarded-proto",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

API Gateway REST API (which uses CloudFront) always modifies "Via" and these three "X-Forwarded-*" headers. It also adds/modifies the "User-Agent" header if the incoming request doesn't have it or have it as an empty string.

@cfbao
Copy link
Contributor Author

cfbao commented Dec 25, 2024

The more I look into this rabbit hole, the more I feel like allowing users to control which headers are signed would be a good idea.

The current list is still relatively conservative. If the user has a custom proxy in front of them, a lot more headers can be modified.

E.g.

It's probably not a good idea to include all these possibilities in the built-in ignore list of this library.
Offloading such configuration to users would provide more flexibility and lessen maintenance burden.

If there's no other concerns, I'll look into adding some new overloads that accepts a header include/exclude list.

@cfbao cfbao marked this pull request as ready for review December 25, 2024 20:01
@FantasticFiasco
Copy link
Owner

I also think allowing the client to control this would be a splendid idea, but I don't have an idea of how to accomplish this yet. I'm currently engaged in another initiative, but would love to collaborate with you on this in a week or two, if you would like that?

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.

Should we skip the User-Agent header?
2 participants