The `navigate-to` directive #290

andypaicu · 2018-01-29T15:41:58Z

There has been talk at TPAC about this and there is some generally positive feedback around this explainer I sent earlier.

WDYT?

Also who else should be added?

Preview | Diff

annevk · 2018-01-29T15:46:45Z

Bikeshed: "navigate-to". Justification: it's "connect-src", not "connection-src". (I might even go as far as just "navigate" as I'm not sure what the suffix adds.)

andypaicu · 2018-01-30T12:50:47Z

I can see navigate-to seems reasonable. I would like to keep the to suffix as I think it is somewhat clearer that we are specifying destinations and not perhaps something else about navigations.

I have some concern and I'm not sure what happens when the initiator is not a browsing context (for example a url typed in the box) but I want to ensure that this will never block those navigations.

mikewest

Thanks for putting this together! Here are some thoughts.

mikewest · 2018-01-31T10:43:17Z

index.src.html


  7.  A <dfn for="directive" export>navigation response check</dfn>, which takes a
-      <a for="/">request</a>, a <a>response</a> and two <a>browsing contexts</a> as
-      arguments, and is executed during [[#should-block-navigation-response]].
+      <a for="/">request</a>, a <a>response</a>, two <a>browsing contexts</a>,


This also takes a "navigation type" string (either form-submission or other).

mikewest · 2018-01-31T10:44:11Z

index.src.html

-      <a for="/">request</a>, type string, and two <a>browsing contexts</a> as arguments, and
-      is executed during [[#should-block-navigation-request]]. It returns
-      "`Allowed`" unless otherwise specified.
+      <a for="/">request</a>, `navigation type` string, and two <a>browsing contexts</a>,


Nit: Drop the backticks: "navigation type string" seems fine without them. Same below for "check type string".

mikewest · 2018-01-31T10:50:25Z

index.src.html

@@ -1386,7 +1383,31 @@ <h4 id="should-block-navigation-response" algorithm>

            1.  If |directive|'s <a for="directive">navigation response check</a>
                returns "`Allowed`" when executed upon |navigation request|, |type|,
-                |navigation response|, |source|, and |target|, skip to the next
+                |navigation response|, |source|, |target|, and "`target`" skip to the next


I think it's worth adding a note under step 2 above explaining that this piece of the algorithm is useful for directives that allow the response itself to assert something. Perhaps along the lines of:

Some directives, like [=frame-ancestors=], allow a policy delivered with |navigation response| to
act upon the navigation. To support these directives, we walk through the response's CSP list
and allow its directives to process their [=directive/navigation response checks=]

Under the new step 3, you might follow that up with:

Other directives, like [=navigate-to=], allow the policies associated with the navigation's |source|
to act upon the navigation. To support these directives, we walk through the relevant document's
CSP list, and allow its directives to process their [=directive/navigation response checks=].

That framing might also suggest switching the terminology here from "source"/"target" to something like "response"/"source".

mikewest · 2018-01-31T10:52:01Z

index.src.html

+                |directive|'s <a for="directive">name</a>.
+
+                Note: We use `null` for the global object, as no global exists:
+                we haven't processed the navigation to create a Document yet.


This makes sense for the target checks. I wonder if we ought to use |source|'s global for the source checks. That might not actually be possible, give the way browsers implement navigation, of course. Chrome might have thrown away the initial context if we're processing a redirect at this point...

mikewest · 2018-01-31T10:54:00Z

index.src.html

+                we haven't processed the navigation to create a Document yet.
+
+            3.  Set |violation|'s <a for="violation">resource</a> to |navigation
+                response|'s <a for="response">URL</a>.


This also makes sense for the target checks, but I think it might be wrong for the source checks. We'd want the initial URL of the request that began the navigation. That is, given the chain A => B => C, if C is a violation, we should return A in order to avoid leaking redirect targets. Same logic as the note in step 2 of https://w3c.github.io/webappsec-csp/#create-violation-for-request: something like "|navigation request|'s [=request/url=]" should do it for that section, I think.

mikewest · 2018-01-31T10:55:50Z

index.src.html

+    2.  If |check type| is "`source`", return "`Allowed`".
+
+        Note: The 'frame-ancestors' <a>directive</a> is relevant only on the
+        |target| <a>browsing context</a> and it has no impact on the |source|


It's not really relevant to the target browsing context, but to the policies delivered with the response that we're processing.

Still relevant?

mikewest · 2018-01-31T10:57:12Z

index.src.html

-    a document can navigate by any means (<{a}>, <{form}>, `window.location`,
-    `window.open`, etc.). The directive's syntax is described by the following ABNF grammar:
+  The <dfn export>navigation-to</dfn> directive restricts the {{URL}}s to which
+  a <a>document</a> can initiate navigations by any means (<{a}>, <{form}>, `window.location`,


I wonder if we should structure this as sitting under form-action? That is, given the policy navigation-to a b c; form-action d, would we allow or deny a form submission to d? It seems like we'd want to allow it.

mikewest · 2018-01-31T10:58:43Z

index.src.html

+    `navigation-to` Navigation Response Check
+  </h5>
+
+  Given a <a for="/">request</a> (|request|), a <a>response</a> (|navigation response|)


Nit: This gets the navigation type string as well.

mikewest · 2018-01-31T11:00:32Z

index.src.html

+    2.  If |check type| is "`target`", return "`Allowed`".
+
+        Note: The 'navigation-to' <a>directive</a> is relevant only on the
+        |source| <a>browsing context</a> and it has no impact on the |target|


Nit: "relevant to", not "relevant on".

mikewest · 2018-01-31T11:03:55Z

index.src.html

+
+    2.  If this directive's <a for="directive">value</a> does not contain a <a>source
+        expression</a> that is an <a>ASCII case-insensitive</a> match for
+        the "<a grammar>`'only-check-last-endpoint'`</a>"


How about 'allow-redirects'? Or, in keeping with the rest of everything in CSP, 'unsafe-allow-redirects'?

andypaicu · 2018-02-05T14:38:46Z

Thank you @annevk and @mikewest for the feedback. I have addressed all of it (hopefully):

renamed navigation-to to navigate-to
form submissions will be ignored by navigate-to if form-action is present
notes on why we need to check policies from the source context when we get a response as well as the response's own policies
using the source's global object if possible (with a note for the caveat that it might not be)
various small typos, missed parameters and so on
renamed the flag to 'unsafe-allow-redirects'

I need some help with making sure that this navigation-to check does not affect any out-of-document navigations (eg typing in the omnibox). I assume the source context won't have any policies if it's not a document that initiates the navigation but I could use some help to make sure that is the case.

mikewest

Thanks! A few more thoughts.

I think the tests in web-platform-tests/wpt#9197 reflect the earlier choice to use the target frame's policy rather than the source frame's policy. Will you update those as well?

mikewest · 2018-02-08T14:24:06Z

index.src.html

-      executed during [[#should-block-navigation-response]].
-      It returns "`Allowed`" unless otherwise specified.
+      <a for="/">request</a>, a navigation type string, a <a>response</a>, two
+      <a>browsing contexts</a>, a check type string ("`source`" or "`response`"),


Listing the values here is nice! Could you do it for the navigation type strings as well ("`form-submission`" or "`other`")?

mikewest · 2018-02-08T14:31:53Z

index.src.html

@@ -608,7 +608,7 @@ <h4 id="framework-directive-source-list">Source Lists</h4>
    ; Keywords:
    <dfn>keyword-source</dfn> = "<dfn>'self'</dfn>" / "<dfn>'unsafe-inline'</dfn>" / "<dfn>'unsafe-eval'</dfn>"
                     / "<dfn>'strict-dynamic'</dfn>" / "<dfn>'unsafe-hashed-attributes'</dfn>" /
-                     / "<dfn>'report-sample'</dfn>" / "<dfn>'only-check-last-endpoint'</dfn>"
+                     / "<dfn>'report-sample'</dfn>" / "<dfn>'unsafe-allow-redirects'</dfn>"


Would you mind adding something like:

ISSUE: Bikeshed `unsafe-allow-redirects`.

Shouldn't block this patch, but we should think about it a bit as a group.

mikewest · 2018-02-08T14:34:14Z

index.src.html

@@ -1379,12 +1379,15 @@ <h4 id="should-block-navigation-response" algorithm>
    2.  For each |policy| in |navigation response|'s
        <a for="response">CSP list</a>:

+        Note: Some directives (like <a>frame-ancestors</a>) allow a |response|'s
+        <a>Content Security Policy</a> to act on the navigation.


I liked the rest of the explanation I suggested. :) "To support these directives, we walk through the response's CSP list and allow its directives to process their [=directive/navigation response checks=]". Too much explanation?

It felt like I was using words to describe the steps of the algorithm without adding extra information.

shrug Ok. I'm fine with clarifying repetition, but if you think it's too much, I'll not push on it.

mikewest · 2018-02-08T14:35:40Z

index.src.html

@@ -1403,22 +1406,27 @@ <h4 id="should-block-navigation-response" algorithm>

    3.  For each |policy| in |source|'s <a>active document</a>'s <a for="Document">CSP List</a>:

+        Note: Some directives (like <a>navigate-to</a>) need the |response| before
+        acting on the navigation.


mikewest · 2018-02-08T14:37:37Z

index.src.html

+                Note: Depending on the browser's implementation it might not be
+                possible to obtain the |source|'s <a>relevant global object</a>
+                at this point in time. If that is the case `null` should be used
+                instead.


I'd drop this note. We're already relying on |source| active document's policy above. If that exists, then from a spec perspective, the global should exist as well. If we need to do some weird monkey patching in Chrome to store the details we need to generate the report, so be it.

mikewest · 2018-02-08T14:39:50Z

index.src.html

+    2.  If |check type| is "`source`", return "`Allowed`".
+
+        Note: The 'frame-ancestors' <a>directive</a> is relevant only on the
+        |target| <a>browsing context</a> and it has no impact on the |source|


Still relevant?

annevk · 2018-02-16T09:59:15Z

The eventual commit message should mention that this fixes #125.

andypaicu · 2018-02-16T13:54:33Z

Once this makes it in, I will update the tests in web-platform-tests/wpt#9197 of course.

mikewest

LGTM. Would you mind hopping onto the WebAppSec call this evening to solicit feedback even more explicitly from other folks? :)

Once this makes it in, I will update the tests in web-platform-tests/wpt#9197 of course.

Please update the tests so we can land them at around the same time we put this into the spec.

mikewest · 2018-02-21T09:31:18Z

index.src.html

@@ -1379,12 +1379,15 @@ <h4 id="should-block-navigation-response" algorithm>
    2.  For each |policy| in |navigation response|'s
        <a for="response">CSP list</a>:

+        Note: Some directives (like <a>frame-ancestors</a>) allow a |response|'s
+        <a>Content Security Policy</a> to act on the navigation.


shrug Ok. I'm fine with clarifying repetition, but if you think it's too much, I'll not push on it.

PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66

andypaicu · 2018-02-23T08:49:21Z

I have separated the tests into their own PR independent of the chrome implementation patch.

web-platform-tests/wpt#9644

PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66

PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66 Reviewed-on: https://chromium-review.googlesource.com/934181 Commit-Queue: Andy Paicu <[email protected]> Reviewed-by: Mike West <[email protected]> Cr-Commit-Position: refs/heads/master@{#541769}

Automatic update from web-platform-tests'navigate-to' directive tests PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66 Reviewed-on: https://chromium-review.googlesource.com/934181 Commit-Queue: Andy Paicu <[email protected]> Reviewed-by: Mike West <[email protected]> Cr-Commit-Position: refs/heads/master@{#541769} wpt-commits: c36f23851a19457614035e9937d992f36ef434e9 wpt-pr: 9644 wpt-commits: c36f23851a19457614035e9937d992f36ef434e9 wpt-pr: 9644

Automatic update from web-platform-tests'navigate-to' directive tests PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66 Reviewed-on: https://chromium-review.googlesource.com/934181 Commit-Queue: Andy Paicu <andypaicuchromium.org> Reviewed-by: Mike West <mkwstchromium.org> Cr-Commit-Position: refs/heads/master{#541769} wpt-commits: c36f23851a19457614035e9937d992f36ef434e9 wpt-pr: 9644 wpt-commits: c36f23851a19457614035e9937d992f36ef434e9 wpt-pr: 9644 UltraBlame original commit: f9f41a459b9ae92a9b9a6bbef6a759f7550d45c1

andypaicu added 2 commits January 29, 2018 16:31

First try at a 'navigation-to' directive

aeaee0d

Fixed small typos

0cf8c9d

andypaicu requested a review from mikewest January 29, 2018 15:41

mikewest reviewed Jan 31, 2018

View reviewed changes

andypaicu added 2 commits February 5, 2018 09:13

Changed directive name to 'navigate-to'

c680fde

Ammendaments and changes suggested

55c9363

shekyan mentioned this pull request Feb 6, 2018

form submissions should be ignored by navigate-to if form-action is present shapesecurity/salvation#193

Closed

mikewest self-assigned this Feb 8, 2018

mikewest reviewed Feb 8, 2018

View reviewed changes

annevk mentioned this pull request Feb 16, 2018

Allow navigation to only whitelisted URLs via navigate-to #125

Closed

Second round of CRs

2be467a

mikewest approved these changes Feb 21, 2018

View reviewed changes

chromium-wpt-export-bot mentioned this pull request Feb 23, 2018

'navigate-to' directive tests web-platform-tests/wpt#9644

Merged

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2018

'navigate-to' directive tests

62b155c

PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2018

'navigate-to' directive tests

56531a5

PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66

wsargent mentioned this pull request Feb 28, 2018

Content Security Policy Filter playframework/playframework#8242

Merged

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 5, 2018

'navigate-to' directive tests

f0e4300

PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66

mikewest changed the title ~~The navigation-to directive~~ The navigate-to directive Mar 5, 2018

andypaicu merged commit 4ef8d6c into w3c:master Mar 8, 2018

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 8, 2018

'navigate-to' directive tests

b5cfb26

PR: w3c/webappsec-csp#290 Bug: 805886 Change-Id: I5bdda65c7e70e729b33a3647135fee6453e97e66

oreoshake mentioned this pull request Mar 12, 2018

Add support for navigate-to directive github/secure_headers#387

Closed

koto mentioned this pull request Jun 22, 2018

Consider dropping TrustedURL w3c/trusted-types#65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The `navigate-to` directive #290

The `navigate-to` directive #290

andypaicu commented Jan 29, 2018 •

edited by pr-preview bot

Loading

annevk commented Jan 29, 2018

andypaicu commented Jan 30, 2018

mikewest left a comment

mikewest Jan 31, 2018

mikewest Jan 31, 2018

mikewest Jan 31, 2018

mikewest Jan 31, 2018

mikewest Jan 31, 2018

mikewest Jan 31, 2018

mikewest Feb 8, 2018

mikewest Jan 31, 2018

mikewest Jan 31, 2018

mikewest Jan 31, 2018

mikewest Jan 31, 2018

andypaicu commented Feb 5, 2018

mikewest left a comment

mikewest Feb 8, 2018

mikewest Feb 8, 2018

mikewest Feb 8, 2018

andypaicu Feb 16, 2018

mikewest Feb 21, 2018

mikewest Feb 8, 2018

mikewest Feb 8, 2018

mikewest Feb 8, 2018

annevk commented Feb 16, 2018

andypaicu commented Feb 16, 2018 •

edited

Loading

mikewest left a comment

mikewest Feb 21, 2018

andypaicu commented Feb 23, 2018

The navigate-to directive #290

The navigate-to directive #290

Conversation

andypaicu commented Jan 29, 2018 • edited by pr-preview bot Loading

annevk commented Jan 29, 2018

andypaicu commented Jan 30, 2018

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

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

andypaicu commented Feb 5, 2018

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

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

annevk commented Feb 16, 2018

andypaicu commented Feb 16, 2018 • edited Loading

mikewest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andypaicu commented Feb 23, 2018

The `navigate-to` directive #290

The `navigate-to` directive #290

andypaicu commented Jan 29, 2018 •

edited by pr-preview bot

Loading

andypaicu commented Feb 16, 2018 •

edited

Loading