New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate feature policy #177

Merged
merged 9 commits into from Mar 2, 2018

Conversation

5 participants
@clelland
Copy link
Contributor

clelland commented Dec 15, 2017

clelland added some commits Dec 15, 2017

Integrate Feature Policy for sync-xhr
This adds a policy-controlled feature, named 'sync-xhr', which can be
disabled in a document to turn off synchronous requests for that
document (and documents in all descendant frames). Calling send() on a
synchronous request in a document where sync-xhr is disabled will
result in a NetworkError being thrown.
@clelland

This comment has been minimized.

Copy link
Contributor

clelland commented Dec 15, 2017

As written, this depends on whatwg/html#3287 so that 'allowed to use' can reference a policy-controlled feature, and not just an attribute name.

@tyoshino
Copy link
Member

tyoshino left a comment

looks good

xhr.bs Outdated

<p>The feature name for <a>Synchronous XMLHttpRequest</a> is "sync-xhr".

<p>The default allowlist for Synchronous XMLHttpRequest is <code>*</code>.

This comment has been minimized.

@tyoshino

tyoshino Dec 19, 2017

Member

<a> is omitted for "Synchronous XMLHttpRequest" intentionally? just checking.

This comment has been minimized.

@clelland

clelland Dec 19, 2017

Contributor

Nope, missed the markup there. Thanks.

xhr.bs Outdated
<a>responsible document</a> and it is <em>not</em>
<a>allowed to use</a> the <a>Synchronous XMLHttpRequest</a>
feature, then run <a>handle response end-of-body</a> for a <a>network
error</a> and return.

This comment has been minimized.

@annevk

annevk Dec 19, 2017

Member

It seems nicer to throw during open(), no?

This comment has been minimized.

@clelland

clelland Dec 19, 2017

Contributor

That would be surprising to developers, I think -- there aren't currently any cases of a NetworkError being thrown during open(), since the fetch algorithm isn't called until send(). Since this policy can be imposed on pages which were written before this proposal, it seems that using an existing failure pattern is less likely to result in unforseen behaviour on the page.

I had originally proposed throwing an InvalidAccessError during open(), to align with the note at https://xhr.spec.whatwg.org/#sync-warning, but the rough consensus at TPAC (I wasn't actually present, this is second-hand) was around emulating a network-layer-error instead.

This comment has been minimized.

@annevk

annevk Dec 19, 2017

Member

Well yeah, I wouldn't want to throw a NetworkError there. I wonder what rationale was given in that discussion. Who would we ask?

This comment has been minimized.

@annevk

annevk Dec 19, 2017

Member

Note that InvalidAccessError would also be consistent with step 10 of open() today.

This comment has been minimized.

@clelland

clelland Dec 19, 2017

Contributor

I'll ask on #178, and see if we can find out who was there, and if this accurately reflects the consensus.

This comment has been minimized.

@foolip

foolip Jan 16, 2018

Member

https://chromium.googlesource.com/chromium/src/+/40126a62123c1e8704b2f92a6ef54eb3e6ce52db/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp#695 now throws InvalidAccessError in open(), which I think makes sense, has this been resolved but the comment thread just left open?

This comment has been minimized.

@clelland

clelland Jan 16, 2018

Contributor

https://chromium-review.googlesource.com/c/chromium/src/+/804057 changes it from InvalidAccessError to NetworkError, and causes it to occur on send(), like other network conditions would.

This comment has been minimized.

@foolip

foolip Jan 16, 2018

Member

I see, thanks!

xhr.bs Outdated
<h3 id=feature-policy>Feature Policy Integration</h3>

<p>This specification defines a policy-controlled feature named <dfn>Synchronous
XMLHttpRequest</dfn>.

This comment has been minimized.

@annevk

annevk Dec 19, 2017

Member

Are all features supposed to follow this naming convention?

This comment has been minimized.

@clelland

clelland Dec 19, 2017

Contributor

I wanted to decouple the name of the feature from the character string used to denote it in policies

The spec just says that there are policy-controlled features, and they have a feature name, which is defined in the ABNF as 1*( ALPHA / DIGIT / "-")

xhr.bs Outdated
<p>This specification defines a policy-controlled feature named <dfn>Synchronous
XMLHttpRequest</dfn>.

<p>The feature name for <a>Synchronous XMLHttpRequest</a> is "sync-xhr".

This comment has been minimized.

@annevk

annevk Dec 19, 2017

Member

xref "feature name"

And make it "<code>sync-xhr</code>".

This comment has been minimized.

@clelland

clelland Dec 19, 2017

Contributor

Done. (I've added explicit anchors for the unexported dfns until I fix up the FP spec to export those terms properly)

xhr.bs Outdated

<p>The feature name for <a>Synchronous XMLHttpRequest</a> is "sync-xhr".

<p>The default allowlist for Synchronous XMLHttpRequest is <code>*</code>.

This comment has been minimized.

@annevk

annevk Dec 19, 2017

Member

xref "default allowlist" (elsewhere we use safelist for things like this, is this different enough to use a new name?)

This comment has been minimized.

@clelland

clelland Dec 19, 2017

Contributor

I hadn't noticed 'safelist' being used in this context, and I'm not sure if 'safety' is the concept we're trying to convey in any case.

Originally the feature policy spec used 'whitelist', and early reviews suggested we switch to 'allowlist' to match some other specs (I don't recall which; perhaps Web Authentication?)

I'm sure this is open to change, but that should probably happen on the FP issues list :)

This comment has been minimized.

@annevk

annevk Dec 19, 2017

Member

WebAuthn doesn't seem to use it.

This comment has been minimized.

@clelland

clelland Dec 19, 2017

Contributor

w3c/webauthn#327 -- apparently not anymore (I still don't remember if this was the spec that was brought up, could be a complete coincidence)

xhr.bs Outdated

<p>When disabled in a document, calling send() on an XMLHttpRequest object with
the synchronous flag set MUST cause a DOMException named
<code>NetworkError</code> to be thrown.

This comment has been minimized.

@annevk

annevk Dec 19, 2017

Member

Please remove this. We don't want duplicate requirements.

This comment has been minimized.

@clelland

clelland Dec 19, 2017

Contributor

Done.

clelland added some commits Dec 19, 2017

Clean up FP integration
* Fix references and markup
* Remove duplicated requirement text

@annevk annevk referenced this pull request Dec 19, 2017

Open

"allowlist" #103

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Dec 19, 2017

In particular I was wondering if a feature is always named "Uppercase Feature" or "Uppercase feature" rather than just "uppercase feature" which would be "synchronous XMLHttpRequest" here. Introducing that as a "feature named X" and then saying X has a feature name is somewhat confusing by the way. Maybe it makes more sense to call one the feature name and the other the identifier.

@clelland

This comment has been minimized.

Copy link
Contributor

clelland commented Jan 11, 2018

There isn't a standard for naming features -- I've generally used whatever seems to sound right in prose -- and this is really the first time that the name of a feature and it's "feature name" string have diverged, since neither seems appropriate in the other's place.

(And I agree that it is confusing to call the character string that goes into policies the "feature name". I've opened w3c/webappsec-feature-policy#125 to address that. I'll submit PRs to all of the specs that have included that text once we have settled on a better term to use)

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jan 11, 2018

FWIW I would probably just remove the feature name concept entirely and only use the web-developer-facing string ("feature identifier") throughout all specs. Having two 1:1 things, one of which is never exposed to either browser developers or web developers, seems confusing.

@clelland

This comment has been minimized.

Copy link
Contributor

clelland commented Jan 24, 2018

Agreed, and fixed with the latest commit.

@annevk
Copy link
Member

annevk left a comment

Couple nits left, mostly looking good now. Thanks.

xhr.bs Outdated
@@ -29,6 +29,9 @@ spec:dom; type:interface; text:Document
<pre class=anchors>
urlPrefix: https://w3c.github.io/DOM-Parsing/; spec: dom-parsing
type: dfn; text: fragment serializing algorithm; url: dfn-fragment-serializing-algorithm
urlPrefix: https://wicg.github.io/feature-policy/; spec: FEATURE-POLICY
type: dfn; text: policy-controlled feature; url: policy-controlled-feature
type: dfn; text: default allowlist; url: default-allowlist

This comment has been minimized.

@annevk

annevk Jan 25, 2018

Member

These should be properly exported by Feature Policy directly. This anchors block is only there for legacy.

This comment has been minimized.

@clelland

clelland Jan 26, 2018

Contributor

They've been exported for a while -- or at least I thought they were :( Filed tabatkins/bikeshed#1173 to get that indexed for use in bikeshed.

xhr.bs Outdated
<li>
<p>If <a>context object</a>'s <a>relevant settings object</a> has a
<a>responsible document</a> and it is <em>not</em> <a>allowed to use</a> the
<code><a>sync-xhr</a></code> feature, then run <a>handle response

This comment has been minimized.

@annevk

annevk Jan 25, 2018

Member

If this is meant to be a string I think it should be "<code><a>sync-xhr</a></code>" here and below. That's how you write down strings per Infra.

Also, no wrapping inside phrasing elements.

This comment has been minimized.

@annevk

annevk Jan 25, 2018

Member

Oh, also, since the <p> is the only child you need to do <li><p>....

This comment has been minimized.

@clelland

clelland Jan 26, 2018

Contributor

All fixed

xhr.bs Outdated

<p>This specification defines a <a>policy-controlled feature</a> identified by
the string <code><dfn>sync-xhr</dfn></code>. Its <a>default allowlist</a> is
<code>*</code>.

This comment has been minimized.

@annevk

annevk Jan 25, 2018

Member

More can be on one line here. Please use 100 columns for new/modified text.

This comment has been minimized.

@clelland

clelland Jan 26, 2018

Contributor

Done.

xhr.bs Outdated
<p>This specification defines a <a>policy-controlled feature</a> identified by
the string <code><dfn>sync-xhr</dfn></code>. Its <a>default allowlist</a> is
<code>*</code>.

This comment has been minimized.

@annevk

annevk Jan 25, 2018

Member

You want an extra newline here for consistency.

This comment has been minimized.

@clelland

clelland Jan 26, 2018

Contributor

And done.

xhr.bs Outdated
@@ -1021,6 +1025,12 @@ method must run these steps:
<p>Otherwise, if the <a>synchronous flag</a> is set, run these substeps:

<ol>
<li>
<p>If <a>context object</a>'s <a>relevant settings object</a> has a
<a>responsible document</a> and it is <em>not</em> <a>allowed to use</a> the

This comment has been minimized.

@annevk

annevk Jan 25, 2018

Member

Should this be "and that"/"which is"? To me it seems that "it" refers to a settings object here.

This comment has been minimized.

@clelland

clelland Jan 26, 2018

Contributor

Fixed -- that was ambiguous, thanks.

clelland added some commits Jan 25, 2018

Addressing review comments
* Clarify which object is tested for "allowed to use"
* Denote strings correctly
* Formatting
@annevk

annevk approved these changes Jan 30, 2018

Copy link
Member

annevk left a comment

Great work!

What remains is:

  • web-platform-tests
  • Implementation bugs

if I'm not missing anything.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jan 30, 2018

You also might want to associate iclelland@google.com with your GitHub account and add your name to the Acknowledgments section of the standard.

@clelland

This comment has been minimized.

Copy link
Contributor

clelland commented Feb 1, 2018

@annevk, there are web-platform-tests for this already committed; you can see the results at https://wpt.fyi/xhr/xmlhttprequest-sync-default-feature-policy.sub.html (It fails in Chrome 63, but should pass in 65) Are there additional tests you'd like to see?

I'll see about filing bugs in other places where I can do that.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Feb 1, 2018

That's good enough I think. Other interesting tests might be header-based / same-origin restrictions.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Feb 1, 2018

I noticed one more nit. We use sentence casing for headers. I'm not entirely sure if that means "Feature Policy integration" or "Feature policy integration" though. I'm guessing the former makes the most sense.

@annevk annevk referenced this pull request Mar 2, 2018

Closed

Feature proposal: sync-xhr #126

@annevk annevk merged commit 67a423f into whatwg:master Mar 2, 2018

2 checks passed

Participation clelland participates on behalf of Google LLC
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@domenic

This comment has been minimized.

Copy link
Member

domenic commented Mar 2, 2018

Doesn't this depend on whatwg/html#3287 which is still not merged? :-/

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 2, 2018

@domenic see the commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment