fix: headers WPT conformance #108

guybedford · 2024-08-08T02:52:12Z

This fixes 40 web platform tests, without any WPT regressions, related to the headers subsystem including header encoding, validation, immutable guards, header ordering, multi-value sorting, iteration.

In addition it implements the Headers.prototype.getSetCookie method, resolving all tests for it.

Resolves:

Implement headers getSetCookie: Implement Headers.prototypeg.getSetCookie #109
Non-ascii bytes in request headers (request-headers-nonascii.any.js.json): Non-ascii bytes in request headers #85
Immutable headers guards for incoming response headers (response-headers-guard.any.js.json) : Implement "immutable" headers guard #78

Headers in the CachedInContent and ContentOnly state are stored as an ordered vector of host_api::HostString to match the representation on the wire, without the need for reencoding across the host API calls.

A separate sort_list is maintained separately representing the ordered iteration through the headers, and iterator and getter functions are updated to use this list when appropriate.

In addition header keys are stored with their original casing to match WhatWG, with equality and ordering defined using case-invariant comparison operations.

Header casing tests on the wire are not passing since Wasmtime will lowercase headers, but if the WASI implementation were to support header casing then this would work (eg js-compute-runtime and Jco may choose to support header casing under this implementation).

Guard validations need to happen on every append so the guard logic is moved back into the main headers implementation, with new host calls get_forbidden_request_headers() and get_forbidden_response_headers() used to obtain the information for the header guards.

tests/integration/tests/performance.js

builtins/web/fetch/headers.h

JakeChampion · 2024-08-08T07:49:16Z

builtins/web/fetch/request-response.cpp

-    host_api::HttpHeadersGuard guard = Request::is_instance(obj)
-      ? host_api::HttpHeadersGuard::Request
-      : host_api::HttpHeadersGuard::Response;
+    // Incoming request and incoming response headers are immutable per service worker


I understand this is making the implementation spec compliant which is good, but this is also a breaking change as some applications will now throw an error because of this change. With that in my, I think this should be released as a new major version (if not in StarlingMonkey then in downstream projects)

Since this is implemented in the request-response usage of Headers, which Fastly maintains its own version of, we can choose to not implement this yet in js-compute-runtime to maintain this as a non-breaking change.

I'll make sure to include this in the Fastly PR at fastly/js-compute-runtime#844.

I agree in principle, yes. In practice I'm not too concerned about this actually being hit, so I predict that we at Fermyon will end up not treating this as a breaking change so much as a bug fix.

builtins/web/fetch/headers.cpp

tschneidereit

This is absolutely great, thank you for all the effort you put into it! <3

I left a whole bunch of comments, but I don't think addressing them should be too hard, and none of them point out major shortcomings of any kind.

I'd still like to take one more look, so please don't force-push changes so I can do an incremental diff.

builtins/web/fetch/headers.cpp

tschneidereit · 2024-08-09T15:29:07Z

builtins/web/fetch/request-response.cpp

-    host_api::HttpHeadersGuard guard = Request::is_instance(obj)
-      ? host_api::HttpHeadersGuard::Request
-      : host_api::HttpHeadersGuard::Response;
+    // Incoming request and incoming response headers are immutable per service worker


I agree in principle, yes. In practice I'm not too concerned about this actually being hit, so I predict that we at Fermyon will end up not treating this as a breaking change so much as a bug fix.

tschneidereit · 2024-08-09T15:36:40Z

host-apis/wasi-0.2.0/host_api.cpp

-    if (header_name.compare(header) == 0) {
-      return false;
-    }
+std::vector<string_view> HttpHeaders::get_forbidden_request_headers() {


Would this be better as a const& to ensure the vec itself isn't copied, while it's also not mutated?

It is mean't to be mutated so we can take ownership of this list and then ensure it is properly sorted.

So the only change would be just be moving the clone into the caller, which I'm not sure is necessary?

Ah, I see. I think I'd somewhat prefer asserting that it is already properly sorted and otherwise using the original allocation. Which I guess could be done by indeed having it return a const& and then copying it in an #ifdef DEBUG block, sorting that, and comparing it with the original. But I agree that that is a bunch of work, so feel free to land things as-is: it's certainly not the end of the world in terms of overhead.

Added something along these lines.

host-apis/wasi-0.2.0/host_api.cpp

include/host_api.h

guybedford · 2024-08-09T17:58:00Z

Thanks for the positive reception and detailed feedback here, I've resolved everything that's been addressed, leaving the outstanding comments open. The pr feedback commit contains everything in that diff. I may push some subsequent changes as additional comments and will comment about what those are if I do them.

tschneidereit

This is great, thank you! A few more comments, but nothing that holds up landing

tschneidereit · 2024-08-10T13:49:22Z

builtins/web/url.cpp

@@ -187,8 +186,7 @@ jsurl::SpecSlice URLSearchParams::serialize(JSContext *cx, JS::HandleObject self

 bool URLSearchParams::append(JSContext *cx, unsigned argc, JS::Value *vp) {
  METHOD_HEADER(2)
-  bool err = false;
-  auto value = append_impl_validate(cx, args[0], &err, "append");
+  auto value = append_impl_validate(cx, args[0], "append");


Shouldn't this also do a result check for value? If there's a reason it doesn't, mind adding a comment explaining it? (Apologies for not catching this in the last review.)

The URL iteration doesn't do key validation before value validation like the headers one does currently, although further investigation into the failing URL tests might indicate implementing this can resolve some remaining WPT there. As a headers PR I didn't want to get into that here though.

tschneidereit · 2024-08-10T13:54:04Z

host-apis/wasi-0.2.0/host_api.cpp

-    if (header_name.compare(header) == 0) {
-      return false;
-    }
+std::vector<string_view> HttpHeaders::get_forbidden_request_headers() {


Ah, I see. I think I'd somewhat prefer asserting that it is already properly sorted and otherwise using the original allocation. Which I guess could be done by indeed having it return a const& and then copying it in an #ifdef DEBUG block, sorting that, and comparing it with the original. But I agree that that is a bunch of work, so feel free to land things as-is: it's certainly not the end of the world in terms of overhead.

builtins/web/fetch/headers.cpp

fix: support headers lists

062ad1a

guybedford requested review from tschneidereit and JakeChampion August 8, 2024 02:52

guybedford mentioned this pull request Aug 8, 2024

Implement Headers.prototypeg.getSetCookie #109

Closed

JakeChampion reviewed Aug 8, 2024

View reviewed changes

tests/integration/tests/performance.js Show resolved Hide resolved

JakeChampion reviewed Aug 8, 2024

View reviewed changes

builtins/web/fetch/headers.h Show resolved Hide resolved

JakeChampion reviewed Aug 8, 2024

View reviewed changes

guybedford commented Aug 8, 2024

View reviewed changes

builtins/web/fetch/headers.cpp Outdated Show resolved Hide resolved

guybedford added 2 commits August 8, 2024 11:01

use latin1 strings when iterating

0134046

fixup

87dbb79

tschneidereit requested changes Aug 9, 2024

View reviewed changes

pr feedback

6f509bf

guybedford requested a review from tschneidereit August 9, 2024 18:58

tschneidereit approved these changes Aug 10, 2024

View reviewed changes

sorted assertion

10da780

guybedford merged commit 10e5106 into main Aug 12, 2024
3 checks passed

guybedford deleted the headers-list branch August 12, 2024 16:24

This was referenced Aug 27, 2024

Non-ascii bytes in request headers #85

Closed

Implement "immutable" headers guard #78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: headers WPT conformance #108

fix: headers WPT conformance #108

guybedford commented Aug 8, 2024 •

edited

Loading

JakeChampion Aug 8, 2024

guybedford Aug 8, 2024

tschneidereit Aug 9, 2024

tschneidereit left a comment

tschneidereit Aug 9, 2024

tschneidereit Aug 9, 2024

guybedford Aug 9, 2024

tschneidereit Aug 10, 2024

guybedford Aug 12, 2024

guybedford commented Aug 9, 2024

tschneidereit left a comment

tschneidereit Aug 10, 2024

guybedford Aug 12, 2024

tschneidereit Aug 10, 2024

fix: headers WPT conformance #108

fix: headers WPT conformance #108

Conversation

guybedford commented Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschneidereit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Aug 9, 2024

tschneidereit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Aug 8, 2024 •

edited

Loading