RESOLVED LATER 221568
Disallow alert/confirm/prompt in cross-origin-domain subframes
https://bugs.webkit.org/show_bug.cgi?id=221568
Summary Disallow alert/confirm/prompt in cross-origin-domain subframes
Domenic Denicola
Reported 2021-02-08 12:40:29 PST
The spec is being updated in https://github.com/whatwg/html/pull/6297 Tests (for confirm/prompt only) in https://github.com/web-platform-tests/wpt/pull/27435
Attachments
Patch (10.94 KB, patch)
2021-02-08 14:13 PST, Chris Dumez
no flags
Patch (11.12 KB, patch)
2021-02-08 14:41 PST, Chris Dumez
ews-feeder: commit-queue-
Patch (97.44 KB, patch)
2021-02-08 16:09 PST, Chris Dumez
no flags
Patch (156.22 KB, patch)
2021-02-08 18:44 PST, Chris Dumez
no flags
Patch (158.23 KB, patch)
2021-02-08 18:51 PST, Chris Dumez
ews-feeder: commit-queue-
Patch (159.63 KB, patch)
2021-02-08 20:55 PST, Chris Dumez
no flags
Patch (157.93 KB, patch)
2021-02-09 11:31 PST, Chris Dumez
no flags
Chris Dumez
Comment 1 2021-02-08 14:13:57 PST
Chris Dumez
Comment 2 2021-02-08 14:41:25 PST
Chris Dumez
Comment 3 2021-02-08 16:09:10 PST
Chris Dumez
Comment 4 2021-02-08 18:44:05 PST
Chris Dumez
Comment 5 2021-02-08 18:51:07 PST
Chris Dumez
Comment 6 2021-02-08 20:55:28 PST
Geoffrey Garen
Comment 7 2021-02-09 09:50:10 PST
Comment on attachment 419673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419673&action=review r=me > Source/WebCore/page/SecurityOrigin.cpp:471 > +bool SecurityOrigin::isSameOriginDomain(const SecurityOrigin& other) const I'm a little surprised that this needed to be a unique check, instead of just canAccess(). Is this check more strict or less strict? (If this check is more strict, it kinda doesn't make sense, since I could always do otherWindow.alert() as a workaround.)
Chris Dumez
Comment 8 2021-02-09 10:35:36 PST
(In reply to Geoffrey Garen from comment #7) > Comment on attachment 419673 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419673&action=review > > r=me > > > Source/WebCore/page/SecurityOrigin.cpp:471 > > +bool SecurityOrigin::isSameOriginDomain(const SecurityOrigin& other) const > > I'm a little surprised that this needed to be a unique check, instead of > just canAccess(). Is this check more strict or less strict? (If this check > is more strict, it kinda doesn't make sense, since I could always do > otherWindow.alert() as a workaround.) can-access is not really a spec concept. I was trying to stay as close to possible to the spec. I just looked at SecurityOrigin::canAccess() and the concepts are almost identical except that: 1. SecurityOrigin::canAccess() returns true when the m_universalAccess flag is set. 2. SecurityOrigin::canAccess() adds the following extra check for file URLs: (canAccess && isLocal()) canAccess = passesFileCheck(other); Despite those differences. They are basically the same thing. However, I do not like the fact that our naming does not match the spec. Maybe we should consider renaming SecurityOrigin::canAccess() to match the spec naming? Or have SecurityOrigin::isSameOriginDomain() call canAccess() internally?
Domenic Denicola
Comment 9 2021-02-09 10:42:01 PST
FWIW in Blink we implement all same-origin domain checks using canAccess. (Which, I believe, has similar exceptional cases.) And, the mismatch between spec-naming and Blink code always bothers me too :).
Chris Dumez
Comment 10 2021-02-09 10:57:00 PST
(In reply to Domenic Denicola from comment #9) > FWIW in Blink we implement all same-origin domain checks using canAccess. > (Which, I believe, has similar exceptional cases.) And, the mismatch between > spec-naming and Blink code always bothers me too :). Thanks for the clarification. I see 3 ways to address this: 1. Rename canAccess() to isSameOriginDomain() 2. Add a new isSameOriginDomain() that calls canAccess() internally 3. Add a comment for canAccess() explaining that it implements the 'same origin-domain' concept in the HTML specification.
Chris Dumez
Comment 11 2021-02-09 11:31:47 PST
Sam Weinig
Comment 12 2021-02-09 11:56:30 PST
(In reply to Chris Dumez from comment #10) > (In reply to Domenic Denicola from comment #9) > > FWIW in Blink we implement all same-origin domain checks using canAccess. > > (Which, I believe, has similar exceptional cases.) And, the mismatch between > > spec-naming and Blink code always bothers me too :). > > Thanks for the clarification. I see 3 ways to address this: > 1. Rename canAccess() to isSameOriginDomain() > 2. Add a new isSameOriginDomain() that calls canAccess() internally > 3. Add a comment for canAccess() explaining that it implements the 'same > origin-domain' concept in the HTML specification. I would opt for number 1. Matching spec names is super helpful, and have fewer of these predicates is almost always better than having more.
Chris Dumez
Comment 13 2021-02-09 11:57:34 PST
(In reply to Sam Weinig from comment #12) > (In reply to Chris Dumez from comment #10) > > (In reply to Domenic Denicola from comment #9) > > > FWIW in Blink we implement all same-origin domain checks using canAccess. > > > (Which, I believe, has similar exceptional cases.) And, the mismatch between > > > spec-naming and Blink code always bothers me too :). > > > > Thanks for the clarification. I see 3 ways to address this: > > 1. Rename canAccess() to isSameOriginDomain() > > 2. Add a new isSameOriginDomain() that calls canAccess() internally > > 3. Add a comment for canAccess() explaining that it implements the 'same > > origin-domain' concept in the HTML specification. > > I would opt for number 1. Matching spec names is super helpful, and have > fewer of these predicates is almost always better than having more. Yes, I prefer 1. too. I went with 3. in this patch to keep things small and I would be happy to rename the function in a follow-up if everyone agrees.
EWS
Comment 14 2021-02-09 13:21:33 PST
Committed r272607: <https://commits.webkit.org/r272607> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419747 [details].
Radar WebKit Bug Importer
Comment 15 2021-02-10 14:33:14 PST
Chris Dumez
Comment 16 2021-09-01 09:47:35 PDT
This change was reverted via Bug 229737 due to breakage.
Note You need to log in before you can comment on or make changes to this bug.