Skip to content

wasi-http: reject p3 serve requests with no authority or Host header - #14983

Closed
xia-chao wants to merge 1 commit into
bytecodealliance:mainfrom
xia-chao:p3-serve-missing-authority
Closed

xia-chao wants to merge 1 commit into
bytecodealliance:mainfrom
xia-chao:p3-serve-missing-authority

Conversation

@xia-chao

Copy link
Copy Markdown
Contributor

#14432 has already fixed the "missing authority/Host ⇒ 400" for p2; this just makes p3 consistent (p3 still passes the request straight to the handler).

@xia-chao
xia-chao requested review from a team as code owners October 24, 2026 12:01
@xia-chao
xia-chao requested review from cfallin and rvolosatovs and removed request for a team October 24, 2026 12:01
@xia-chao
xia-chao force-pushed the p3-serve-missing-authority branch from eafb7cd to 83d4a8d Compare October 24, 2026 12:43
Comment thread tests/all/cli_tests.rs Outdated
stream.write_all(b"GET / HTTP/1.1\r\n\r\n").await?;
let mut response = Vec::new();
let mut buf = [0; 512];
while !response.ends_with(b"</html>") && response.len() < 8192 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reading in a timeout loop is the wrong approach - we want a timeout for the total read, which should use AsyncReadExt::read_to_end, not this loop that appends to the buffer (and uses a fragile heuristic for ending the response)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you didn't consider that read to end waits for eof, and a keep alive 400 never closes the connection, so it just times out that's why the request sends connection: close.

p2 already answers 400 for these; the p3 path left the request to the
handler with a `none` authority.
@xia-chao
xia-chao force-pushed the p3-serve-missing-authority branch from 83d4a8d to 4696408 Compare October 27, 2026 09:45

@pchickey pchickey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a conflict with #14988, I lost track with all of the PRs flying around. Wasnt the idea that this PR got closed in favor of that one?

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.

2 participants