Bug 221568 - Disallow alert/confirm/prompt in cross-origin-domain subframes
Summary: Disallow alert/confirm/prompt in cross-origin-domain subframes
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 229737
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-08 12:40 PST by Domenic Denicola
Modified: 2021-09-01 13:18 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Domenic Denicola 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
Comment 1 Chris Dumez 2021-02-08 14:13:57 PST
Created attachment 419625 [details]
Patch
Comment 2 Chris Dumez 2021-02-08 14:41:25 PST
Created attachment 419631 [details]
Patch
Comment 3 Chris Dumez 2021-02-08 16:09:10 PST
Created attachment 419652 [details]
Patch
Comment 4 Chris Dumez 2021-02-08 18:44:05 PST
Created attachment 419663 [details]
Patch
Comment 5 Chris Dumez 2021-02-08 18:51:07 PST
Created attachment 419665 [details]
Patch
Comment 6 Chris Dumez 2021-02-08 20:55:28 PST
Created attachment 419673 [details]
Patch
Comment 7 Geoffrey Garen 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.)
Comment 8 Chris Dumez 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?
Comment 9 Domenic Denicola 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 :).
Comment 10 Chris Dumez 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.
Comment 11 Chris Dumez 2021-02-09 11:31:47 PST
Created attachment 419747 [details]
Patch
Comment 12 Sam Weinig 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.
Comment 13 Chris Dumez 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.
Comment 14 EWS 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].
Comment 15 Radar WebKit Bug Importer 2021-02-10 14:33:14 PST
<rdar://problem/74206634>
Comment 16 Chris Dumez 2021-09-01 09:47:35 PDT
This change was reverted via Bug 229737 due to breakage.