Stream: git-wasmtime

Topic: wasmtime / PR #7383 wasi-http: Fallible fields set and ap...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:06):

elliottt requested fitzgen for a review on PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:06):

elliottt requested wasmtime-core-reviewers for a review on PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:06):

elliottt opened PR #7383 from elliottt:trevor/fallible-fields-operations to bytecodealliance:main:

Add the ability to return errors when calls to fields.set or fields.append contain headers that are forbidden, and add the validation functions to WasiHttpView for extensibility. This change also necessitates tracking the intended use of fields values, implicitly set when the fields is constructed from an incoming-request or incoming-response, or explicitly set when using fields.constructor.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:07):

elliottt edited PR #7383:

Add the ability to return errors when calls to fields.set or fields.append contain headers that are forbidden, and add the validation functions to WasiHttpView for extensibility. This change also necessitates tracking the intended use of fields values, implicitly set when the fields is constructed from an incoming-request or incoming-response, or explicitly set when using fields.constructor.

Fixes #7270

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:07):

elliottt requested pchickey for a review on PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:10):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:10):

elliottt created PR review comment:

I don't like this trap when the fields provided aren't meant for a request. This pushes me towards thinking that we shouldn't use a constructor here, but instead a static method for making a request that can fail.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:11):

elliottt created PR review comment:

I'd appreciate some input on what a good default set of forbidden headers is for requests and responses.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:11):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 22:59):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 23:04):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 23:04):

elliottt created PR review comment:

Would it be useful to add the HeaderMap that's being added to as an argument here as well?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 01:18):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 01:54):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 02:02):

elliottt edited PR #7383:

This PR includes two approaches to handling fallible fields methods. Both make the return values of fields.set and fields.append fallible, though they differ in what they consider errors to report at that point.

  1. Adds the ability to return errors when calls to fields.set or fields.append contain headers that are forbidden, and add the validation functions to WasiHttpView for extensibility. This change also necessitates tracking the intended use of fields values, implicitly set when the fields is constructed from an incoming-request or incoming-response, or explicitly set when using fields.constructor.
  2. Rejects malformed header names in fields.set and fields.append, but allows forbidden headers at that point. Instead of plumbing through the intended ultimate use in the fields type, validation of forbidden headers is done in the consumer of the outbound-response or outbound-request values (response-outparam.set or the outgoing-handler.handle functions).

The last commit implements approach 2, and approach 1 can be seen by reverting it.

Fixes #7270

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 02:03):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 04:08):

lukewagner submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 04:08):

lukewagner created PR review comment:

I think in the case of header errors, the set of failure reasons is disjoint from all the other places that produce errors, which I think makes it worth having a separate variant just for header errors such as:

variant header-error {
  invalid-syntax,
  forbidden
}

(I know we don't usually maximally-specialize error variants, but it seems like in cases where they're fully disjoint like this, it's worthwhile.)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 04:10):

lukewagner submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 04:10):

lukewagner created PR review comment:

If we reject invalid headers eagerly, then I think there shouldn't be any error possible here. In particular, the set operation shouldn't be describing the actual sending of the response over the network (that kind of error would be reflecting in the streaming operations for writing to and finishing the response body); this operation should be the equivalent of *outparam = result;. Or am I missing something else that can happen here?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 04:23):

lukewagner submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 04:23):

lukewagner created PR review comment:

Note also that if we expand the WASI HTTP spec to distinguish "definitely-forbidden" headers (listed by the WASI HTTP spec) and "host-forbidden" headers (that's purely up to the current running host), we'd probably split forbidden into two cases to reflect the difference.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 15:57):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 15:57):

elliottt created PR review comment:

I like that specific error type more than extending the more general error variant, thanks! I'm not sure about differentiating between definitely forbidden and host forbidden though: what would a guest do differently if it knew the difference between definitely forbidden and host forbidden?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:21):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:22):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:22):

elliottt created PR review comment:

Good point. Switching to having forbidden headers rejected at fields.set and fields.append removes the need to allow validation here.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:25):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:28):

elliottt edited PR #7383:

This PR makes the fields.set and fields.append methods fallible, and introduces a new error type for describing the ways in which those operations can fail. Additionally it adds a notion of forbidden headers, which will always include the following:

There's an embedder-defined hook for adding additional header name validation, but that underlying set of headers can't be altered.

Fixes #7270

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:29):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:32):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:36):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 17:41):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 18:01):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 18:39):

lukewagner created PR review comment:

    unexpected-error(string)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 18:39):

lukewagner submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 18:42):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2023 at 18:51):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 30 2023 at 17:39):

pchickey submitted PR review:

Looks great overall - rather than trap in the fields constructor, lets make the constructor create an empty set of fields, and have a static function that takes the list<tuple<...>> repr and returns result<fields, header-error>

view this post on Zulip Wasmtime GitHub notifications bot (Oct 30 2023 at 18:07):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 30 2023 at 18:08):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 30 2023 at 18:56):

elliottt updated PR #7383.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 30 2023 at 21:27):

alexcrichton merged PR #7383.

Last updated: Dec 23 2024 at 12:05 UTC