Allow access if security origin access is asked for is the same to the this

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[anton muhin]

Allow access if security origin access is asked for is the same to the this

[anton muhin]

Created attachment 80664 [details]
Patch

[Adam Barth]

Comment on attachment 80664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80664&action=review

> Source/WebCore/ChangeLog:5
> +        Allow access if security origin access is asked for is the same to the this

"is the same to the this"
=> "is the same as this" ?

> Source/WebCore/ChangeLog:8
> +        Covered by the existing tests.

How is this covered by existing tests if none of the tests changed as a result of this patch?

[anton muhin]

(In reply to comment #2)
> (From update of attachment 80664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80664&action=review
> 
> > Source/WebCore/ChangeLog:5
> > +        Allow access if security origin access is asked for is the same to the this
> 
> "is the same to the this"
> => "is the same as this" ?

Done.

> 
> > Source/WebCore/ChangeLog:8
> > +        Covered by the existing tests.
> 
> How is this covered by existing tests if none of the tests changed as a result of this patch?

I mean that nothing started to break with this change.  Does that make sense?  If not, may you suggest how I can reword this phrase?  And I am afraid it'd be pretty much difficult to write a test which will trigger this path right now.

[Adam Barth]

Yeah, I don't think it's possible to trigger that branch now.  We should just say that in the ChangeLog.

To give folks reading this bug more context, Anton and I have been discussing adding some defense-in-depth security checks at various points in the bindings.  Those points have SecurityOrigin objects, but not necessarily DOMWindow objects.  Currently, the "you can always access yourself" check is done by comparing DOMWindows.  Adding this branch here is redundant today, but won't be redundant in the future once we add these extra checks.

[anton muhin]

Created attachment 80669 [details]
Patch

[anton muhin]

Adam, may you have another look?

(In reply to comment #5)
> Created an attachment (id=80669) [details]
> Patch

[Sam Weinig]

(In reply to comment #4)
> Yeah, I don't think it's possible to trigger that branch now.  We should just say that in the ChangeLog.
> 
> To give folks reading this bug more context, Anton and I have been discussing adding some defense-in-depth security checks at various points in the bindings.  Those points have SecurityOrigin objects, but not necessarily DOMWindow objects.  Currently, the "you can always access yourself" check is done by comparing DOMWindows.  Adding this branch here is redundant today, but won't be redundant in the future once we add these extra checks.

Can we remove some of the DOMWindow comparing to themselves checks then?

[Adam Barth]

> Can we remove some of the DOMWindow comparing to themselves checks then?

That's a good idea.  We should be able to remove them after/in this patch.

[anton muhin]

Guys, if you're fine with this patch, may I ask you to r+ it?

(In reply to comment #8)
> > Can we remove some of the DOMWindow comparing to themselves checks then?
> 
> That's a good idea.  We should be able to remove them after/in this patch.

[Adam Barth]

Comment on attachment 80669 [details]
Patch

Sure.  I think a followup patch to remove the DOMWindow equality comparison might be worth doing too.

[anton muhin]

Comment on attachment 80669 [details]
Patch

Thanks a lot, Adam.  I'll try to do further cleanup with a separate patch.

[WebKit Commit Bot]

Comment on attachment 80669 [details]
Patch

Clearing flags on attachment: 80669

Committed r77272: <http://trac.webkit.org/changeset/77272>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.