-
Notifications
You must be signed in to change notification settings - Fork 1.5k
allow configurable forbidden header #11292
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
allow configurable forbidden header #11292
Conversation
c9a7eb7
to
d183eff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In p3 I've fixed this in the following manner: https://github.com/bytecodealliance/wasip3-prototyping/blob/main/crates%2Fwasi-http%2Fsrc%2Fp3%2Fmod.rs#L381-L387
This way users can choose the headers to deny with no breaking changes to the API
That said, IMO this diff is already an improvement
Should I adjust the change close to p3 then? There are two commits in the PR, I can either drop the 2nd, or squash them together. Please let me know what you think. Thanks. Edit:
The way it works in p2 is, when user override this trait method, the new customized headers as well as the default forbidden headers are denied: https://github.com/bytecodealliance/wasmtime/blob/v34.0.2/crates/wasi-http/src/types.rs#L287 I prefer the way how p3 is implemented, but it means a break change to the current API. Otherwise we have to introduce something new, which will then be dropped when the user upgrade to p3, which also not nice. |
d183eff
to
d45832b
Compare
I updated the PR to follow the p3 API, that should give the users a smooth path forward. Although it has a break change for people who override the trait method. The PR is ready to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
As you correctly point out, this is actually is a breaking change for users providing an implementation of the method. Should we record this in release notes? I'm not actually sure about the expectations/procedure here
cc @bytecodealliance/wasmtime-wasi-reviewers
Breaking changes are fine yeah, and if you're able to write a note in the release notes that'd be appreciated, but it's not required. When I go back and review merged PRs for the release notes I try to catch issues like this and write them in. |
When the HTTP library is not used as a proxy, removing of the forbidden headers by default may not make sense. This change makes it configurable via the `WasiHttpView` by overriding the is_forbidden_header method, so that the DEFAULT_FORBIDDEN_HEADERS will not be always included. The behavior is now close to p3.
d45832b
to
1506742
Compare
I put a note in the RELEASES.md file, please take another look. |
When the HTTP library is not used as a proxy, removing of the forbidden headers by default may not make sense. This change makes it configurable via the
WasiHttpView
, which keeps the default behavior.