[xia-chao]

wasmtime serve answers 500 when a request has neither a URI authority nor a
Host header. The rejection happens before the guest runs, so the error now
carries the status the client should see, and serve reads it back, defaulting
to 500. That gives the 400 RFC 9112 section 3.2 asks for, and it is not
version-specific: an HTTP/1.0 request without Host gets the same 400.
wasi:http/service is untouched.

[pchickey]

I think this comes down to an architectural decision for whether the guest or host code is responsible for enforcing the RFC invariant. De facto in this implementation, if hyper didn't enforce an RFC invariant, then it would be up to the guest to enforce it. That's not a principled decision, it was just the way things shook out, and other hosts very likely end up handling broken RFC invariants differently than the exact way hyper handles them.

So, we could enforce this particular RFC invariant here, but then the questions are:

1. why, exactly, do we not enforce it for p3? The comment here doesn't make sense to me, because in p2 the guest is also capable of observing whether authority is none.
2. what other RFC invariants is our implementation not enforcing in the host? Do we add them all here?
3. What from this do we need to feed into specification of the wasi-http implementation?

Want to call this out to other maintainers here, cc @lann

[lann]

If new_incoming_request is going to validate host/authority presence then by spec it should be returning a 400. Pragmatically I'm not sure where that would make a difference.

[xia-chao]

Fair — the comment was wrong. I tested both worlds: the proxy path already fails on a Host-less request, we just turned the 500 into a 400. The service path doesn't fail, it just never gets the host.

[lann]

Could you say more about your motivation for this change?

[xia-chao]

RFC 9112 §3.2 says an HTTP/1.1 request without Host must get 400, and §3.3 lets the server reject an empty authority, so rejecting is fine — ours just surfaces as a 500 with "worker trapped or panicked", which reads as our bug. Only the status code changes.

[pchickey]

@xia-chao Its possible to modify your guest code to give the correct status, correct?

[lann]

@xia-chao Its possible to modify your guest code to give the correct status, correct?

No, that's the whole problem; wasmtime-wasi-http bails before it gets to the guest:

wasmtime/crates/wasi-http/src/p2/types.rs

Line 92 in 1bc343e

None => bail!("invalid HTTP request missing authority in URI and host header"),

[xia-chao]

No — the host rejects it before the guest runs: "invalid HTTP request missing authority in URI and host header". A duplicate or invalid Host does reach the guest.

[xia-chao]

It's a bug, not a preference.

[pchickey]

OK, I apologize, I missed that, now I understand what both of you are saying.

We don't want to duplicate the logic of detecting this 400 Bad Request. Instead, we should change the place the bad request is detected to also include what status code Wasmtime serve should respond with. So, instead of this patch, the architecture I'd like to see is:

1. add a pub struct ErrorResponse { status: StatusCode } impl ErrorResponse { pub fn new(status: StatusCode) -> Self {...} pub fn status(&self) -> StatusCode {...} } to wasmtime-wasi-http, with derive Debug, impls Display ("error response: {}"), and impl std::error::Error. This struct has opaque fields so that we can extend it in the future with more, if applicable.
2. change the bail linked to have, additionally, .context(ErrorResponse::new(StatusCode::BAD_REQUEST))
3. in the Err case handler in wasmtime-cli's serve.rs, try to downcast the error to an ErrorResponse and get the status, with a default to 500 if the downcast fails. Put that status that into the status code of the response, as well as the html response body. The current hard-coded 500 response body should become a template. Use https://docs.rs/http/latest/http/status/struct.StatusCode.html#method.canonical_reason for the non-numeric description.

[xia-chao]

Your three steps are merged, but they don't actually solve the problem: the p2 prepare error gets wrapped into a TrapOrPanicError before serve gets it, so ErrorResponse is only logged and we still return a 500.

To get a 400, we should pass this error back through tx just like instantiation errors do. This touches 6 test cases, but leaves service completely unchanged.

[xia-chao]

Done in fdf05d2. Passing the error back through tx gives us the 400 now;

[pchickey]

Thanks very much for the changes. This round of feedback is mostly that Claude is awfully verbose with comments (you pay by the token, after all) and I think that all of the really obvious ones are best deleted.

[xia-chao]

Done

[pchickey]

Thanks!