WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(11.12 KB, patch)
2021-02-08 14:41 PST
,
Chris Dumez
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(97.44 KB, patch)
2021-02-08 16:09 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(156.22 KB, patch)
2021-02-08 18:44 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(158.23 KB, patch)
2021-02-08 18:51 PST
,
Chris Dumez
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(159.63 KB, patch)
2021-02-08 20:55 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(157.93 KB, patch)
2021-02-09 11:31 PST
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2021-02-08 14:13:57 PST
Created
attachment 419625
[details]
Patch
Chris Dumez
Comment 2
2021-02-08 14:41:25 PST
Created
attachment 419631
[details]
Patch
Chris Dumez
Comment 3
2021-02-08 16:09:10 PST
Created
attachment 419652
[details]
Patch
Chris Dumez
Comment 4
2021-02-08 18:44:05 PST
Created
attachment 419663
[details]
Patch
Chris Dumez
Comment 5
2021-02-08 18:51:07 PST
Created
attachment 419665
[details]
Patch
Chris Dumez
Comment 6
2021-02-08 20:55:28 PST
Created
attachment 419673
[details]
Patch
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
Created
attachment 419747
[details]
Patch
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
<
rdar://problem/74206634
>
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.
Top of Page
Format For Printing
XML
Clone This Bug