Integrate with Feature Policy #822

marcoscaceres · 2019-01-15T05:22:04Z

closes #600
closes #698

The following tasks have been completed:

Confirmed there are no ReSpec errors/warnings.

Implementation commitment:

Safari (link to issue)
Chrome (link to issue)
Firefox (link to issue)
Edge (public signal)

Optional, impact on Payment Handler spec?
None.

Preview | Diff

marcoscaceres · 2019-01-15T05:23:38Z

@ianbjacobs, can you give this a once over for me? Then I'll ping clelland to make sure it's technically accurate.

marcoscaceres · 2019-01-15T13:16:50Z

Just noting some discussion happening here too:
web-platform-tests/wpt#14855

marcoscaceres · 2019-01-15T13:20:08Z

Annevk tells me that allowpaymentrequest has been moved out of the HTML spec into feature policy spec.

I'll need to update the spec to remove reference to HTML for allowpaymentrequest.

index.html

marcoscaceres · 2019-01-15T13:53:38Z

index.html

        </p>
        <ol data-link-for="PaymentDetailsBase" class="algorithm">
-          <li data-tests=
-          "allowpaymentrequest/active-document-cross-origin.https.sub.html, allowpaymentrequest/active-document-same-origin.https.html, allowpaymentrequest/removing-allowpaymentrequest.https.sub.html, allowpaymentrequest/setting-allowpaymentrequest-timing.https.sub.html, allowpaymentrequest/setting-allowpaymentrequest.https.sub.html">


Tests will be moving to Feature Policy directory as part of web-platform-tests/wpt#14855

marcoscaceres · 2019-01-15T13:55:30Z

@clelland, whenever you can, would really appreciate if you could take a look at this PR.

If you have input on web-platform-tests/wpt#14855, that would also be great.

index.html

ianbjacobs

Hi Marcos,

I suggested one small change otherwise seems ok to me editorially.

marcoscaceres · 2019-01-15T14:28:32Z

Appreciate the review @ianbjacobs!

domenic · 2019-01-15T16:12:55Z

Is there any chance that we can kill allowpaymentrequest only use allow="paymentrequest" instead? Or is there too much deployed content?

index.html

clelland · 2019-01-15T19:19:07Z

index.html

-        specified on the <a>iframe</a> element.
+      <p>
+        The <a data-cite="feature-policy">Feature Policy</a> specification
+        defines the "<code>payment</code>" feature and the <code><dfn data-cit=


This spec should probably define the actual feature -- similarly to https://html.spec.whatwg.org/#policy-controlled-features or https://fullscreen.spec.whatwg.org/#feature-policy-integration, it's just a matter of declaring the feature name and the default allowlist ('self' in this case)

And, as @annevk mentioned, the allowpaymentrequest attribute is defined in HTML.

The note in the fullscreen spec is actually a pretty good fit for the situation here. allowfullscreen affects the container policy, unless overridden by allow.

I agree. I'm kinda conflicted between us trying to get this to REC (important to a lot of members of the WG - aiming for April) and "doing the right thing" and just normatively defining "payment" here 🤔 Our plan is to publish a REC, but then quickly spin up a "v1.1" that would formally define the feature string.

You cannot get to REC without two implementations right? And implementations wouldn't be able to implement this any other way. Or would you leave this whole thing undefined or some such?

You cannot get to REC without two implementations right?

Correct.

And implementations wouldn't be able to implement this any other way. Or would you leave this whole thing undefined or some such?

The Feature Policy would be undefined, but there is sufficient prose in the spec (in the constructor) that an implementation that does implement either the allowpaymentrequest attribute or the Feature Policy spec would behave in conforming manner (and could be demonstratively shown to behave in a conforming manner).

index.html

marcoscaceres · 2019-01-16T07:19:49Z

@domenic wrote:

Is there any chance that we can kill allowpaymentrequest only use allow="paymentrequest" instead? > Or is there too much deployed content?

@rsolomakhin, worth setting up a telemetry probe? It has shipped for about 2 years in Chrome, so might be too late... but never know.

domenic

To be clear, I don't think this is good in its current state, and I think PaymentRequest needs to define the feature. Feature Policy does not define individual features; the specs do.

marcoscaceres · 2019-01-24T21:21:07Z

To be clear, I don't think this is good in its current state, and I think PaymentRequest needs to define the feature. Feature Policy does not define individual features; the specs do.

I agree with this, and it's absolutely the right thing to do - and we might need to accept it as a critical piece of the design (particularly from a security stand point... even if it delays going to REC for a bit).

Have you - or @clelland - heard anything from the WebKit folks about supporting Feature Policy? We've unfortunately not been able to get in touch with @aestes about WebKit's implementation plans.

marcoscaceres · 2019-01-25T04:29:22Z

@domenic @clelland ok, done :) I based the text on what was stated in the Fullscreen spec.

Please double check I didn't miss anything.

clelland

This looks good, @marcoscaceres -- one question about the internal cross-reference, but the text is exactly what I'd expect.

clelland · 2019-01-25T15:06:08Z

index.html

-          <a>allowed to use</a> the feature indicated by attribute name
-          <a>allowpaymentrequest</a>, then <a>throw</a> a
-          "<a>SecurityError</a>" <a>DOMException</a>.
+          <a>allowed to use</a> the "<a>payment</a>" feature, then <a>throw</a>


Should this be <a lt="payment-feature">payment</a>, to link to the definition in #feature-policy?

@clelland that would be good. However, I did it this way because Feature Policy itself doesn't actually define the "payment" feature - thus I couldn't actually link to it. However, I'm happy update the link it once Feature Policy defines it.

I meant to link to the feature-policy section in this spec, where 'payment-feature' is actually defined, not to link to the feature policy spec. Features should ideally be defined in the spec where they are used, rather than centrally in the FP spec.

Ah, sorry for misunderstanding. Will fix.

index.html

Co-Authored-By: marcoscaceres <marcos@marcosc.com>

marcoscaceres requested a review from ianbjacobs January 15, 2019 05:22

marcoscaceres commented Jan 15, 2019

View reviewed changes

index.html Show resolved Hide resolved

marcoscaceres commented Jan 15, 2019

View reviewed changes

marcoscaceres mentioned this pull request Jan 15, 2019

Move allowpaymentrequest attribute tests to HTML web-platform-tests/wpt#14855

Closed

annevk reviewed Jan 15, 2019

View reviewed changes

index.html Outdated Show resolved Hide resolved

ianbjacobs reviewed Jan 15, 2019

View reviewed changes

index.html Outdated Show resolved Hide resolved

ianbjacobs approved these changes Jan 15, 2019

View reviewed changes

marcoscaceres requested a review from domenic January 15, 2019 14:30

domenic requested changes Jan 15, 2019

View reviewed changes

index.html Show resolved Hide resolved

clelland reviewed Jan 15, 2019

View reviewed changes

marcoscaceres force-pushed the feature_policy branch from 71a332e to 30931bb Compare January 16, 2019 04:29

marcoscaceres mentioned this pull request Jan 16, 2019

Disable Payment Request API in CSP/iframe sandbox #698

Closed

domenic requested changes Jan 24, 2019

View reviewed changes

Marcos Cáceres added 4 commits January 25, 2019 13:46

Editorial: relationship to Feature Policy spec

b24a70c

allowpaymentrequest is now in Feature Policy

8f10817

Review feedback

1ce1ac5

Normatively define payment feature

7679151

marcoscaceres force-pushed the feature_policy branch from 30931bb to 7679151 Compare January 25, 2019 04:22

marcoscaceres changed the title ~~Editorial: relationship to Feature Policy spec~~ Integrate with Feature Policy Jan 25, 2019

Merge branch 'gh-pages' into feature_policy

2cb7a95

clelland reviewed Jan 25, 2019

View reviewed changes

domenic approved these changes Jan 28, 2019

View reviewed changes

index.html Outdated Show resolved Hide resolved

domenic and others added 2 commits January 29, 2019 11:31

typo

f1e80d6

Co-Authored-By: marcoscaceres <marcos@marcosc.com>

Review nit

1d9a28a

marcoscaceres merged commit 5966e82 into gh-pages Jan 29, 2019

marcoscaceres deleted the feature_policy branch January 29, 2019 01:27

clelland mentioned this pull request Jul 11, 2019

Explicit export/definition for "payment" w3c/webappsec-permissions-policy#154

Closed

Integrate with Feature Policy #822

Integrate with Feature Policy #822

Uh oh!

Conversation

marcoscaceres commented Jan 15, 2019 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcoscaceres commented Jan 15, 2019

Uh oh!

marcoscaceres commented Jan 15, 2019

Uh oh!

marcoscaceres commented Jan 15, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Jan 15, 2019

Uh oh!

Uh oh!

Uh oh!

ianbjacobs left a comment

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Jan 15, 2019

Uh oh!

domenic commented Jan 15, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcoscaceres commented Jan 16, 2019

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Jan 24, 2019

Uh oh!

marcoscaceres commented Jan 25, 2019

Uh oh!

clelland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcoscaceres commented Jan 15, 2019 •

edited by pr-preview bot

Loading