Bug 221568 – Disallow alert/confirm/prompt in cross-origin-domain subframes

• Component: WebCore Misc.
• Status: RESOLVED LATER

[Domenic Denicola]

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

[Chris Dumez]

Created attachment 419625 [details]
Patch

[Chris Dumez]

Created attachment 419631 [details]
Patch

[Chris Dumez]

Created attachment 419652 [details]
Patch

[Chris Dumez]

Created attachment 419663 [details]
Patch

[Chris Dumez]

Created attachment 419665 [details]
Patch

[Chris Dumez]

Created attachment 419673 [details]
Patch

[Geoffrey Garen]

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]

(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]

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]

(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]

Created attachment 419747 [details]
Patch

[Sam Weinig]

(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]

(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]

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]

<rdar://problem/74206634>

[Chris Dumez]

This change was reverted via Bug 229737 due to breakage.