Skip to content

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

Conversation

xofyarg
Copy link
Contributor

@xofyarg xofyarg commented Jul 22, 2025

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.

@xofyarg xofyarg requested a review from a team as a code owner July 22, 2025 01:22
@xofyarg xofyarg force-pushed the wasi-http-configurable-forbidden-header branch from c9a7eb7 to d183eff Compare July 22, 2025 02:31
@xofyarg
Copy link
Contributor Author

xofyarg commented Jul 22, 2025

Not sure which is better: 3c9f4c1 looks clean to me, but it changes previous behavior if user has implemented is_forbidden_header; d183eff adds an extra method.

Copy link
Member

@rvolosatovs rvolosatovs left a 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

@pchickey pchickey added this pull request to the merge queue Jul 22, 2025
@xofyarg
Copy link
Contributor Author

xofyarg commented Jul 22, 2025

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:

choose the headers to deny with no breaking changes to the API

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.

@pchickey pchickey removed this pull request from the merge queue due to a manual request Jul 22, 2025
@xofyarg xofyarg force-pushed the wasi-http-configurable-forbidden-header branch from d183eff to d45832b Compare July 23, 2025 02:25
@xofyarg
Copy link
Contributor Author

xofyarg commented Jul 23, 2025

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.

Copy link
Member

@rvolosatovs rvolosatovs left a 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

@alexcrichton
Copy link
Member

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.
@xofyarg xofyarg force-pushed the wasi-http-configurable-forbidden-header branch from d45832b to 1506742 Compare July 23, 2025 15:09
@xofyarg xofyarg requested a review from a team as a code owner July 23, 2025 15:09
@xofyarg xofyarg requested review from fitzgen and removed request for a team July 23, 2025 15:09
@rvolosatovs rvolosatovs enabled auto-merge July 23, 2025 15:10
@xofyarg
Copy link
Contributor Author

xofyarg commented Jul 23, 2025

I put a note in the RELEASES.md file, please take another look.

@rvolosatovs rvolosatovs added this pull request to the merge queue Jul 23, 2025
Merged via the queue into bytecodealliance:main with commit ef3d260 Jul 23, 2025
42 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.

3 participants