Replace Request.type based logic with Request.destination #231

yoavweiss · 2017-08-21T10:20:49Z

As Fetch is eliminating Request.type (whatwg/fetch#582), this PR (tries to) replace the related logic to Request.destination.

At the same time, CSP had references to no-longer-existing destinations. I've aligned it with latest Fetch definitions.

Preview | Diff

annevk · 2017-08-21T12:02:40Z

index.src.html

@@ -1901,8 +1899,7 @@ <h5 algorithm id="connect-src-pre-request">
  1.  Assert: |policy| is unused.

  2.  If |request|'s <a for="request">initiator</a> is "`fetch`", or its


No comma needed before or anymore.

annevk · 2017-08-21T12:02:46Z

index.src.html

@@ -1922,8 +1919,7 @@ <h5 algorithm id="connect-src-post-request">
  1.  Assert: |policy| is unused.

  2.  If |request|'s <a for="request">initiator</a> is "`fetch`", or its


No comma needed before or anymore.

annevk · 2017-08-21T12:03:17Z

index.src.html

@@ -2169,7 +2165,7 @@ <h5 algorithm id="frame-src-pre-request">

  1.  Assert: |policy| is unused.

-  2.  If |request|'s <a for="request">type</a> is "`document`" and


This seems like it was already bogus. "document" was never a request type.

Yeah, but with destination that works, right?

Yeah, would be good to get @mikewest to double check though. Might also want to note this in the eventual commit message.

Yeah, it looks like this should have been destination all along. Glad you caught it!

annevk · 2017-08-21T12:04:01Z

index.src.html

@@ -2305,8 +2301,7 @@ <h5 algorithm id="manifest-src-pre-request">

  1.  Assert: |policy| is unused.

-  2.  If |request|'s <a for="request">type</a> is "", and its
-      <a for="request">initiator</a> is "`manifest`":
+  2.  If |request|'s <a for="request">initiator</a> is "`manifest`":


This can use destination as well.

(Perhaps we should drop "manifest" and "xslt" as initiator. Initiator is only there for when it's different from another column.)

changing to destination

annevk · 2017-08-21T12:04:10Z

index.src.html

@@ -2325,8 +2320,7 @@ <h5 algorithm id="manifest-src-post-request">

  1.  Assert: |policy| is unused.

-  2.  If |request|'s <a for="request">type</a> is "", and its
-      <a for="request">initiator</a> is "`manifest`":
+  2.  If |request|'s <a for="request">initiator</a> is "`manifest`":


This can use destination as well.

changing to destination

annevk · 2017-08-21T12:06:03Z

index.src.html

@@ -2546,7 +2538,7 @@ <h5 algorithm id="script-src-pre-request">

      Note: If `worker-src` is present, we'll defer to it when handling worker requests.

-  2.  If |request|'s <a for="request">type</a> is "`script`":
+  2.  If |request|'s <a for="request">destination</a> is a for="request">script-like destination</a>:


Missing <a here.

oops. Added

annevk · 2017-08-21T12:06:07Z

index.src.html

@@ -2620,7 +2612,7 @@ <h5 algorithm id="script-src-post-request">

      Note: If `worker-src` is present, we'll defer to it when handling worker requests.

-  2.  If |request|'s <a for="request">type</a> is "`script`":
+  2.  If |request|'s <a for="request">destination</a> is a for="request">script-like destination</a>:


Missing <a here.

annevk · 2017-08-21T12:07:02Z

index.src.html

+            the empty string, return `connect-src`.
+      : "`manifest`"
+      ::
+        1.  If the |request|'s <a for="request">initiator</a> is
            "`manifest`", return `manifest-src`.


Again, it doesn't seem like we need initiator for this case anymore.

ok, returning manifest-src regardless of initiator.

annevk · 2017-08-21T12:08:09Z

index.src.html

@@ -3778,20 +3772,14 @@ <h5 id="effective-directive-for-a-request" algorithm>
        1.  Return `style-src`.

      : "`script`"
+      : "`xslt`"
+      ::
+        1. Return `script-src`.


This seems like new behavior for "xslt".

Yeah, but seems like the intended behavior based on https://w3c.github.io/webappsec-csp/#directive-script-src

Hrm. Yes, you're both right: this wasn't in the doc, but it should have been.

annevk · 2017-08-21T12:08:16Z

index.src.html

-            : "`worker`"
-            ::
-              1.  Return `worker-src`.
+        1. Return `child-src`.


What happened to worker-src?

should've been worker-src (and we should update the table at https://fetch.spec.whatwg.org/#concept-request-destination)

mikewest

Dropping type seems reasonable. Thanks for putting this change together, Yoav! I'm happy to merge it in once the Fetch bit lands.

mikewest · 2017-08-21T14:44:03Z

index.src.html

@@ -2169,7 +2165,7 @@ <h5 algorithm id="frame-src-pre-request">

  1.  Assert: |policy| is unused.

-  2.  If |request|'s <a for="request">type</a> is "`document`" and


Yeah, it looks like this should have been destination all along. Glad you caught it!

mikewest · 2017-08-21T14:50:30Z

index.src.html

@@ -2374,7 +2368,7 @@ <h5 algorithm id="media-src-pre-request">

  1.  Assert: |policy| is unused.

-  2.  If |request|'s <a for="request">type</a> is one of "`audio`", "`video`",
+  2.  If |request|'s <a for="request">destination</a> is one of "`audio`", "`video`",
      or "`track`":


Hrm. I thought we had MIME type checks hanging off of the distinction of audio vs video vs track. Skimming through Fetch, though, I don't see them.

Still, would you mind keeping the distinctions for the moment? I have naive dreams of poking at mixed content checks for these types at some point as folks continue to transition to TLS. It would be nice to not have to go back and add the detail in that we already have now.

mikewest · 2017-08-21T14:51:13Z

index.src.html

@@ -2374,7 +2368,7 @@ <h5 algorithm id="media-src-pre-request">

  1.  Assert: |policy| is unused.

-  2.  If |request|'s <a for="request">type</a> is one of "`audio`", "`video`",
+  2.  If |request|'s <a for="request">destination</a> is one of "`audio`", "`video`",
      or "`track`":


Oh. You mean introducing a new definition that wraps these three destinations up? Sure, I'm fine with that. It seems like a useful concept.

mikewest · 2017-08-21T14:52:14Z

index.src.html

@@ -3778,20 +3772,14 @@ <h5 id="effective-directive-for-a-request" algorithm>
        1.  Return `style-src`.

      : "`script`"
+      : "`xslt`"
+      ::
+        1. Return `script-src`.


Hrm. Yes, you're both right: this wasn't in the doc, but it should have been.

yoavweiss · 2017-08-28T09:25:09Z

Relevant Fetch changes: whatwg/fetch#582

annevk · 2017-08-28T09:26:41Z

Oh. You mean introducing a new definition that wraps these three destinations up? Sure, I'm fine with that. It seems like a useful concept.

I haven't done this yet as I think we should probably find a second consumer of such functionality first.

mikewest · 2017-10-09T08:36:44Z

Changes to Fetch landed, Yoav assures me that IP weirdness is no longer weird, landing.

Thanks!

* 'master' of https://github.com/w3c/webappsec-csp: (209 commits) Fix a few typos (w3c#280) Introduce 'prefetch-src'. (w3c#283) Clarify navigation behavior for 'script-src'. Incorrect indentation of the navigation check algorithm. IDL amendments and small misc issues. (w3c#271) Regenerate HTMLs. Origin link. NoncedElement link. link up inline css issue (w3c#228) Replaced 'alias' with 'copy' for less ambiguity (w3c#273) Cleanup `global object` usage to make sense with `Documents` (w3c#254) Elements with duplicated attributes are not nonceable. s/not-example.com/example.org/ Linked testing policy and fixed a few links (w3c#263) Rebuild HTML. Fix linking errors to 'script-like' and 'applet'. Adds WorkletGlobalScope as a concept to CSP. (w3c#205) Slight correction of host matching description (w3c#251) Fixed ambigous grammar (w3c#250) Replace Request.type based logic with Request.destination (w3c#231) ...

Replace Request.type based logic to Request.destination

1d422a0

annevk reviewed Aug 21, 2017

View reviewed changes

yoavweiss added 2 commits August 21, 2017 14:48

Fixed based on comments by annevk

6f98fb9

Added "a "

65895a1

mikewest approved these changes Aug 21, 2017

View reviewed changes

annevk mentioned this pull request Aug 28, 2017

Fold request type into destination whatwg/fetch#581

Closed

mikewest merged commit 6638ad4 into w3c:master Oct 9, 2017

yoavweiss deleted the replace_request_type_with_destination branch October 9, 2017 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Request.type based logic with Request.destination #231

Replace Request.type based logic with Request.destination #231

yoavweiss commented Aug 21, 2017 •

edited by pr-preview bot

Loading

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

annevk Aug 21, 2017

mikewest Aug 21, 2017

annevk Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

mikewest Aug 21, 2017

annevk Aug 21, 2017

yoavweiss Aug 21, 2017

mikewest left a comment

mikewest Aug 21, 2017

mikewest Aug 21, 2017

mikewest Aug 21, 2017

mikewest Aug 21, 2017

yoavweiss commented Aug 28, 2017

annevk commented Aug 28, 2017

mikewest commented Oct 9, 2017

		@@ -1901,8 +1899,7 @@ <h5 algorithm id="connect-src-pre-request">
		1. Assert: \|policy\| is unused.

		2. If \|request\|'s <a for="request">initiator</a> is "`fetch`", or its

		@@ -1922,8 +1919,7 @@ <h5 algorithm id="connect-src-post-request">
		1. Assert: \|policy\| is unused.

		2. If \|request\|'s <a for="request">initiator</a> is "`fetch`", or its

		@@ -2169,7 +2165,7 @@ <h5 algorithm id="frame-src-pre-request">

		1. Assert: \|policy\| is unused.

		2. If \|request\|'s <a for="request">type</a> is "`document`" and

Replace Request.type based logic with Request.destination #231

Replace Request.type based logic with Request.destination #231

Conversation

yoavweiss commented Aug 21, 2017 • edited by pr-preview bot Loading

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

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

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

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

mikewest 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

yoavweiss commented Aug 28, 2017

annevk commented Aug 28, 2017

mikewest commented Oct 9, 2017

yoavweiss commented Aug 21, 2017 •

edited by pr-preview bot

Loading