Bug 36625 – Modify NotificationPresenter::checkPermission() to pass the full source url rather than just the security origin

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Rafael Weinstein]

The chromium client needs the full URL of the requesting frame.

[Rafael Weinstein]

Created attachment 51688 [details]
Patch

[Aaron Boodman]

I think that you will need to fix the Windows port as well.

[Rafael Weinstein]

Created attachment 51691 [details]
Patch

[Eric Seidel (no email)]

This is to work around a GKURL KURL difference, correct?

[Rafael Weinstein]

Created attachment 51693 [details]
Patch

[Adam Barth]

Comment on attachment 51693 [details]
Patch

Ok.  There might be some subtle issues if a sandboxed iframe tries to use the notification API.  With this design, it might inherit the privileges of its origin.  Would you be willing to file a bug about that issue and attach a test case?

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> This is to work around a GKURL KURL difference, correct?

If this is true, then can we eliminate the difference?

[Darin Fisher (:fishd, Google)]

Comment on attachment 51693 [details]
Patch

> +++ b/WebKit/chromium/public/WebNotificationPresenter.h
> @@ -62,7 +62,7 @@ public:
>  
>      // Checks the permission level for the given origin.
>      // FIXME: This should become abstract when the below is removed.
> -    virtual Permission checkPermission(const WebSecurityOrigin& origin)
> +    virtual Permission checkPermission(const WebURL& url)

Doesn't this make for a two-sided commit?  Can we avoid such complexity
by preserving the old API in a deprecated form and then remove it via
a follow-up patch?

[Aaron Boodman]

(In reply to comment #7)
> (In reply to comment #4)
> > This is to work around a GKURL KURL difference, correct?
> 
> If this is true, then can we eliminate the difference?

It isn't true. Eric was getting this patch confused with something else I was talking to him about the same day.

There is a difference between GURL and KURL though. See:

https://bugs.webkit.org/show_bug.cgi?id=36623(In reply to comment #8)
> (From update of attachment 51693 [details])
> > +++ b/WebKit/chromium/public/WebNotificationPresenter.h
> > @@ -62,7 +62,7 @@ public:
> >  
> >      // Checks the permission level for the given origin.
> >      // FIXME: This should become abstract when the below is removed.
> > -    virtual Permission checkPermission(const WebSecurityOrigin& origin)
> > +    virtual Permission checkPermission(const WebURL& url)
> 
> Doesn't this make for a two-sided commit?  Can we avoid such complexity
> by preserving the old API in a deprecated form and then remove it via
> a follow-up patch?

No. Note the WebSecurityOrigin signature is not used in Chromium and has a default implementation. It was a previous (aborted) attempt at this same change, and is intended to be made in three non-breaking steps.

BTW, We need a name for the desired way to make webkit changes. Air-lock? Three-phase?

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
> There is a difference between GURL and KURL though. See:
> 
> https://bugs.webkit.org/show_bug.cgi?id=36623

Ah, yes.  I'm familiar with that bug.  It wasn't clear to me that it related
to differences between KURL and KURLGoogle.


(In reply to comment #8)
> No. Note the WebSecurityOrigin signature is not used in Chromium and has a
> default implementation. It was a previous (aborted) attempt at this same
> change, and is intended to be made in three non-breaking steps.

Ah, sounds good.


> BTW, We need a name for the desired way to make webkit changes. Air-lock?
> Three-phase?

Trifecta, Hat-trick, ...  I'd vote for three-phase :)

[Rafael Weinstein]

@abarth: I've created https://bugs.webkit.org/show_bug.cgi?id=36732 and attached a test for the sandboxing issue you identified.

[WebKit Commit Bot]

Comment on attachment 51693 [details]
Patch

Clearing flags on attachment: 51693

Committed r56756: <http://trac.webkit.org/changeset/56756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.