Allow payment requests for same origin-domain subdocuments #2217
Conversation
zcorpan
added
the
needs tests
|
|
||
| <li><var>document</var>'s <span data-x="concept-document-bc">browsing context</span> has a | ||
| <span>browsing context container</span> whose <span>node document</span>'s <span>origin</span> | ||
| has the <span>same origin-domain</span> as <var>document</var>'s <span>origin</span></li> |
There was a problem hiding this comment.
Why not use same origin here? I think we should keep same origin-domain for legacy cases only.
There was a problem hiding this comment.
See the issue for discussion with @bzbarsky. In particular using same-origin would allow cases where script DOM access is otherwise not allowed.
There was a problem hiding this comment.
Right, similar to how access to the network or storage would remain "shared", but script access would not. I guess we could go either way... Or we could require both meaning that when you use document.domain you're out of luck with this feature, but that might not be compatible either.
There was a problem hiding this comment.
Since it is snapshotted at document creation time it will not work if document.domain is used with the proposed spec. But it would work if "same origin" is used. So same origin-domain + snapshot allows fewer cases without the attribute.
There was a problem hiding this comment.
I tried to clarify this a bit now.
|
LGTM, but it would be good to have tests (as noted by the label), but also some implementer sign-off. I have the feeling we'll keep tweaking these allow* attributes forever otherwise. |
|
This seems generally ok to me, but have you talked to the feature policy people about the origin-domain aspect of things? |
|
@igrigorik @mikewest @raymeskhoury @ojanvafai @clelland It would be good if the spec here for I would be happy to add any hooks to HTML that FP can use if necessary. |
|
|
||
| <li><var>document</var>'s <span data-x="concept-document-bc">browsing context</span> has a | ||
| <span>browsing context container</span> whose <span>node document</span>'s <span>origin</span> | ||
| has the <span>same origin-domain</span> as <var>document</var>'s <span>origin</span></li> |
There was a problem hiding this comment.
I think that "has the same origin-domain as" should probably read "is same origin-domain with" here.
There was a problem hiding this comment.
I used the same wording as elsewhere in the spec.
There was a problem hiding this comment.
Probably wrong elsewhere too then, since it's not really a property of origins, but a check you perform on them.
There was a problem hiding this comment.
https://html.spec.whatwg.org/#svg-0:same-origin-domain is the only instance I could find with that wording; all the other cross-referenced uses seem to use some variant of "is same origin-domain with"
|
The wording seems good to me; Same origin-domain seems like the right boundary to be using. I think this behavior matches the way that Feature Policy should work for allowpaymentrequest -- at least as far as the iframe attribute goes. There may be additional complications when we introduce policies via respone headers, but as long as we can amend this at that point, then this should be good right now. (It would be great to eventually be able to define 'allowed to use' in terms of feature policy, but that spec isn't quite there yet.) |
|
Thanks, @clelland do you think we should double check with @igrigorik or should we move ahead and merge? |
| <li> | ||
| <p>If <var>allowattribute</var> is <code | ||
| data-x="attr-iframe-allowfullscreen">allowfullscreen</code> or <code | ||
| data-x="attr-iframe-allowusermedia">allowusermedia</code>, then follow these substeps:</p> |
There was a problem hiding this comment.
Since nobody has shipped allowusermedia we can probably exclude it here.
Ditto. |
|
Sorry for being slow to the thread and if this is a silly question, but I'm unsure why we want to check for same origin-domain as opposed to same-origin? |
|
@raymeskhoury it was discussed in #2184 (comment). Since the check is made during document creation using same origin-domain will allow slightly less than using same-origin would. In particular if the parent had already set document.domain it would not be treated as same-origin. The argument was made that this would be slightly better since the documents can't access each other through script in that case either. |
|
Ah so using same origin-domain would be less permissive in this case? I was just looking at the definition of same origin-domain here: https://w3c.github.io/html/browsers.html#same-origin-domain and it seemed to indicate that origins with different ports would compare equal, which would be more permissive. Will that apply in this case? Is that what we want? |
|
It would be less permissive because in this case the child global cannot have (Also, you want https://html.spec.whatwg.org/multipage/browsers.html#same-origin-domain but it happens they're the same in this case I think.) |
|
IMO we should include a note outlining all the less-permissive-vs.-more-permissive things we've discovered here. Maybe with an example or three. |
|
There is a note explaining it. |
|
Yes, I meant explicitly stating that using same origin-domain is less permissive than using same-origin, and perhaps giving an example of where that's the case. |
|
It's only less permissive in the snapshotting case, right? In the general case, we can't say anything so definite, since it could either restrict communication between documents, or allow it when it wasn't otherwise possible. (And, like @raymeskhoury, I originally thought that same origin-domain completely ignored ports, I had to re-read the wording carefully a couple of times to convince myself that it wasn't the case. An example to that end would be useful as well) |
|
We're only discussing snapshotting here. In general same origin-domain should be avoided. |
|
@annevk thanks for clarifying! Sorry for my lack of understanding. SGTM then. Any ways we can make it more obvious would be great though :) |
|
OK added a new commit here with examples, and also a new PR with an example for ports at #2257 |
|
Sweet, still need to add tests. Plan in web-platform-tests/wpt#4526 (might not need all of these to merge this, but some of them at least) |
|
I agree with @foolip's comment that we should also switch allowusermedia in this PR if possible. |
zcorpan
force-pushed
the
zcorpan/allow-same-origin-allowpaymentrequest
branch
from
f4dacfc
to
3aa2531
Compare
|
I've squashed and rebased on master. Since we added A first set of tests for |
| @@ -28717,6 +28717,65 @@ interface <dfn>HTMLIFrameElement</dfn> : <span>HTMLElement</span> { | |||
| <span>browsing context container</span> that is an <code>iframe</code> element, let | |||
| <var>iframe</var> be that element. Otherwise, abort these steps.</p></li> | |||
|
|
|||
| <li> | |||
|
|
|||
|
I had missed to |
zcorpan
removed
the
needs tests
|
I think it is OK to merge this now. |
|
I think so too. We got tests, bugs, and solid text. The only nitpick I have is that we should maybe also have allowusermedia tests, but implementers should still be writing tests too. |
|
Filed web-platform-tests/wpt#4625 for testing |
Fixes #2184.
This is on top of #2195.
Same origin-domain is used instead of same-origin to disallow by default if
document.domainhad been used in the parent document.The origin check is part of the snapshot instead of live.
Browsing context containers other than
iframe(e.g.object,embed,frame) that are same origin-domain are allowed to use payment request.(I think ideally this should apply to
allowfullscreenandallowusermediaas well, but I don't want to hold upallowpaymentrequeston compat concerns forallowfullscreenso focusing on justallowpaymentrequestright now.)Please use #2184 for comments about the general approach/concerns, and comment here for editorial review.
Implementer sign-off: