Add 'unsafe-webtransport-hashes' keyword to connect-src

[jan-ivar]

Fixes #683. Assumes a unsafe-WebTransport-hash list flag on request (seems simplest post w3c/webtransport#697).

@dveditz @annevk @martinthomson WDYT?

---

Preview | Diff

[annevk]

Looks reasonable overall. @antosart probably needs to review.

[antosart]

Do we also need to adapt the post-request check? The interaction between fetch an webtransport is not totally clear to me, but I guess it would also go through https://fetch.spec.whatwg.org/#ref-for-should-block-response%E2%91%A0 ?

[antosart]

Also, we probably need WPTs.

@mikewest after conversations at TPAC, do we already have an updated process for changes to the spec?

[mikewest]

No, it's been on my list since TPAC, but I haven't gotten a proposal out yet.

Certainly will require WPT coverage and signals from other vendors, though. I think we have the latter here, but not the former?

[jan-ivar]

I've added the post-request check and used "unsafe-webtransport-hashes is not empty" for now. Thanks for the reviews!

(Unless there are any takers) I'll work on WPT next!

[jan-ivar]

Build is failing on "No 'dfn' refs found for 'webtransport-hashes'" which are provided by whatwg/fetch#1896.

WPT tests are in https://phabricator.services.mozilla.com/D276378.

[antosart]

Thank you. Let's wait for whatwg/fetch#1896. to be merged then.

A comment on the WPT, which I'll leave here for simplicity: Could you wait for the violation to be reported (only if CSP is expected to block, of course) in order to avoid flakiness of the test? Also, could you avoid the variable logs and instead separate the assertions? Something like:

const spvPromise = newPromise(resolve =>  { 
  window.onsecuritypolicyviolation = e => resolve(e.violatedDirective) 
}; 
const webTransportPromise = new Promise(resolve => {
 try {
    const wt = new WebTransport(url, options);
    t.add_cleanup(() => wt.close());
    wt.closed.catch(() => {});
    await wt.ready;
    resolve(true);
  } catch (e) {
    if (e.name != "WebTransportError") throw e;
    resolve(false);
  }
});
assert_equals(await webTransportPromise, expectAllowed);
if (expectViolation) {
  assert_equals(await spvPromise, "connect-src");
}

(feel free to improve further).

[jan-ivar]

Thanks for the feedback @antosart. But wouldn't that relax the test, which also tests that the event is NOT always fired?

I tried to align with the other connect-src tests (like connect-src-websocket-blocked.sub.html) here (modulo the arguably outdated async_test-based logTest.sub.js due to its poor error handling, timing out on any TypeError).

The CSP tests seem to observe a time window and test both that events are fired and NOT fired when appropriate. — I misread how logTest.js works.

Awaiting the event would cause timeouts on failure instead of reporting said failure like the other CSP tests do.

If we're concerned about intermittents I can increase the max time window to 1 second to match the other CSP tests. — I can add a 1 second timeout for when an event is expected.

[antosart]

Ah sorry. I was thinking about reports (which have a history of making tests flaky) but you are actually testing securitypolicyviolations, which should be fired deterministically (it should be enough to let the message loop run enough times). I'll take that back :)

[jan-ivar]

@antosart no the feedback was good. I've restructured the tests to be less timing dependent and match existing CSP tests. They now await the event with a 1 second timeout, same as logTest.js (once I understood it properly). Thanks!

[jan-ivar]

Aligned with upstream name tweak. Should be good to merge right after whatwg/fetch#1896.

[jan-ivar]

Tests merged in web-platform-tests/wpt#57217.

[antosart]

Thanks. IIUC there is a typo in the third test case in connect-src-webtransport-allowed.sub.https.html right? Since we want to check that even when specifying serverCertificateHashes, the request is blocked if csp does not allow unsafe-webtransport-hashes. Does the fix in https://chromium-review.googlesource.com/c/chromium/src/+/7502859 make sense to you?

[jan-ivar]

Hi @antosart, apologies for the late response. Yes your fix corrects the WPT to match the text...

Unfortunately, I suspect it's my text that is wrong, not the WPT. I believe the WPT (before your fix) matched our intent stated in #683 (comment).

I'll update the text tomorrow. Sorry for the churn on this. Thank goodness for tests, and your keen eye!

[jan-ivar]

@antosart @annevk algorithm updated (now stricter) PTAL. (Note: PR Preview appears out of date)