Skip to content
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

Invoke "allowed to use" always #383

Merged
merged 2 commits into from
Jan 11, 2017
Merged

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Dec 15, 2016

The old text was only invoking "allowed to use" for non-top-level
browsing contexts, which means the active document check is not
done for the top-level document case.

The old text was only invoking "allowed to use" if a document in
the chain of ancestor browsing contexts were not same origin,
but this does not match Chromium. Chromium will throw an exception
for PaymentRequest in an iframe even if it's same origin. It also
means that if everything is same origin, then the active document
check in "allowed to use" would not be called.

The use case for allowpaymentrequest must be to allow cross-origin
documents in iframes to make payments. Otherwise, if everything is
same-origin, the document could just construct top.PaymentRequest
to bypass any checks, or set the allowpaymentrequest attribute on
its frameElement.

Fixes #361.

The active document check in "allowed to use" was added in
whatwg/html#2160.

@rsolomakhin
Copy link
Collaborator

rsolomakhin commented Dec 15, 2016

I'm working on fixing the same-origin discrepancy in Chrome. Sorry about that! The correct behavior is to allow usage of PaymentRequest in same-origin iframes, even without allowpaymentrequest attribute. This attribute should be required only for cross-origin iframes that want to use PaymentRequest.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 15, 2016

@rsolomakhin that does not match the behavior of allowfullscreen and allowusermedia. But if we want to allow same-origin use of these things without the attribute, that could be done as part of the "allowed to use" concept as well.

@rsolomakhin
Copy link
Collaborator

@zcorpan We are trying to match the behavior of FeaturePolicy attribute allow="paymentrequest:[*]". FeaturePolicy is planned to replace all allow* attributes eventually.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 15, 2016

@rsolomakhin OK. I think that makes sense, but if possible we should grand-father allowfullscreen and allowusermedia into that behavior as well. Another difference that @bzbarsky pointed out is Feature Policy is snapshotting the attribute at document creation time (like sandbox) instead of having it live. But that needs changes in HTML also.

In any case, I think this PR is a step in the right direction. You suggest relaxing it further in the same-origin case.

@bzbarsky
Copy link

The feature policy behavior for fullscreen and paymentrequest can easily differ, if they have different default feature policies.

In particular, I think the intent in feature policy is that the behavior HTML defines in "allowed to use" corresponds to a default feature policy of [] in non-toplevel browsing contexts. The behavior @rsolomakhin wants corresponds to a default whitelist of "self". Or at least that's the idea; the algorithms in the feature policy explainer don't quite do this right yet, but the intent is to be able to define both behaviors in terms of feature policy.

Note that for purposes of this discussion "same-origin iframe" means "same-origin with its parent", not with toplevel.

@clelland
Copy link

@zcorpan, we're looking at changing the behaviour of allowfullscreen and allowusermedia to match this (no attribute needed for same-origin) -- it doesn't make much sense in the same-origin case anyway, where the child could simply reach into its parent and enable any attributes it needs to access a feature, and trying to restrict it that way makes FP much more difficult to reason about.

The must-be-live nature of allowfullscreen looks like it was never part of the spec, but was an implementation detail that has come to be relied on. If that can't be changed without breaking existing sites, then we might be able to special-case that attribute, but I'd rather not do that for any new features.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 28, 2016

Allowing same-origin without an allowpaymentrequest attribute is addressed by whatwg/html#2217 -- which depends on this PR.

The old text was only invoking "allowed to use" for non-top-level
browsing contexts, which means the active document check is not
done for the top-level document case.

The old text was only invoking "allowed to use" if a document in
the chain of ancestor browsing contexts were not same origin,
but this does not match Chromium. Chromium will throw an exception
for PaymentRequest in an iframe even if it's same origin. It also
means that if everything *is* same origin, then the active document
check in "allowed to use" would not be called.

The use case for allowpaymentrequest must be to allow cross-origin
documents in iframes to make payments. Otherwise, if everything is
same-origin, the document could just construct top.PaymentRequest
to bypass any checks, or set the allowpaymentrequest attribute on
its frameElement.

Fixes #361.

The active document check in "allowed to use" was added in
whatwg/html#2160.
@zcorpan zcorpan force-pushed the zcorpan/fix-allowed-to-use-check branch from 85cb81c to 172cbaf Compare January 5, 2017 13:22
@zcorpan
Copy link
Member Author

zcorpan commented Jan 5, 2017

I rebased this now.

(Not clear to me why the ipr check is still failing; AFAICT I'm in the Web Payments Working Group per https://labs.w3.org/hatchery/repo-manager/admin/user/zcorpan . Clicking "revalidate" in https://labs.w3.org/hatchery/repo-manager/pr/id/w3c/browser-payment-api/383 I get a "Forbidden" message.)

@ianbjacobs
Copy link
Collaborator

Hi @zorpan, we fixed our database and the IPR conflict is (appropriately) gone. Thanks!
Ian

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zkoch zkoch merged commit 1f06fae into gh-pages Jan 11, 2017
@zcorpan zcorpan deleted the zcorpan/fix-allowed-to-use-check branch January 12, 2017 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants