Bug 51159 – [Qt] Permit qrc resources to load in QWebSettings::setUserStyleSheetUrl()

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jarred Nicholls]

Right now Page::userStyleSheetLocationChanged() only accepts base64 data urls or file urls.  In Qt world, a qrc resource is considered a local resource, re: SchemeRegistry::localURLSchemes()

However, Page::userStyleSheetLocationChanged() does not check the localURLSchemes registry when loading urls for the user style sheet, and instead only accepts file:// urls.  By changing this behavior, we can open the door to a qrc resource style sheet.

In Qt port, we have to handle qrc paths a little differently by making sure it's prefixed with a colon, so it's a special case in KURL::fileSystemPath().  This does change the behavior, so a new test should be created.

[Jarred Nicholls]

Created attachment 76727 [details]
Proposed patch

Need to add a new Qt test, but wanted feedback first.

[Jarred Nicholls]

Comment on attachment 76727 [details]
Proposed patch

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

> WebCore/platform/qt/KURLQt.cpp:57
> +    return QString();

String not QString ;-)

[Ariya Hidayat]

Comment on attachment 76727 [details]
Proposed patch

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

> WebCore/platform/qt/KURLQt.cpp:51
> +    if (isValid() && (protocolIs("file") || protocolIs("qrc"))) {
> +        // A valid qrc resource path begins with a colon
> +        if (protocolIs("qrc"))
> +            return ":" + path();

Is there a security implication of this? For example, can now any application which is granted the access to local file also upload/peek any resource (in particular in a hybrid QtWebKit-based app)?

> WebCore/platform/qt/KURLQt.cpp:54
> +        return static_cast<QUrl>(*this).toLocalFile();

This static_cast is rather scary. I can't think of a better solution right now :(

[Jarred Nicholls]

(In reply to comment #3)
> Is there a security implication of this? For example, can now any application which is granted the access to local file also upload/peek any resource (in particular in a hybrid QtWebKit-based app)?

AFAICT, the answer would be "no".  The security worry would be if remote sources could access/open local resources.  For the same reason you can't open a file:// url from some http:// document, loading a style sheet from a qrc resource should not open any security holes.

fileSystemPath() is only used in one other location in WebCore, and that is for resource loading in the Windows port.  This same function first ensures the resource is a file:// url anyways, so qrc wouldn't even be considered.

But I digress...fileSystemPath is only universally used in this one spot in WebCore; to load a style sheet file.

> 
> This static_cast is rather scary. I can't think of a better solution right now :(

The other solution is just a semantic change, doing an implicit cast:

((QUrl)(*this)).toLocalFile();

The method directly proceeding KURLQt::fileSystemPath() is an overloaded cast for KURL -> QUrl.

[Ariya Hidayat]

> For the same reason you can't open a file:// url from some http:// document, loading 
> a style sheet from a qrc resource should not open any security holes.

My previous question was more: if an app has access to file system, does the patch make it able to access qrc as well?

Looking at the code in WebCore, seems that it is unlikely.


In all cases, this definitely needs a test (WebKit/qt/tests).

[Jarred Nicholls]

(In reply to comment #5)
> > For the same reason you can't open a file:// url from some http:// document, loading 
> > a style sheet from a qrc resource should not open any security holes.
> 
> My previous question was more: if an app has access to file system, does the patch make it able to access qrc as well?
> 
> Looking at the code in WebCore, seems that it is unlikely.

Indeed it is unlikely, since fileSystemPath is not used anywhere else in WebCore, aside from one platform-specific spot that is 1) guarded behind an isLocalFile() check, and 2) can't handle qrc resources anyways :)

With that said, if a resource is local (file, qrc, whatever), then by default they can access each other - anything local on a user's machine is automatically considered to be in the same security origin and is trustworthy.  This, however, can be turned off via QWebSettings::LocalContentCanAccessFileUrls.

This would only warrant further discussion if fileSystemPath() was used in critical resource loading functions and SecurityOrigin.  Note: resource loading looks to the local scheme registry to determine local vs remote, and qrc is registered by default...just something to consider.

> 
> 
> In all cases, this definitely needs a test (WebKit/qt/tests).

Definitely.  Just curious if this change would fly or not.

[Ariya Hidayat]

Comment on attachment 76727 [details]
Proposed patch

r- until the test is available

[Robert Hogan]

Comment on attachment 76727 [details]
Proposed patch

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

> WebCore/ChangeLog:10
> +        Permit any url with a local scheme to set a user style sheet. Allow qrc
> +        resource URLs as arguments to QWebSettings::setUserStyleSheetUrl().
> +

I think a less intrusive way to do this would be to convert the qrc URL to a file URL in QWebSetttings::setUserStyleSheerUrl().

[Jarred Nicholls]

Created attachment 106909 [details]
Proposed Patch

[Jarred Nicholls]

Created attachment 106917 [details]
Proposed Patch

[Andreas Kling]

Comment on attachment 106917 [details]
Proposed Patch

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

I like where this is going, but let me put my nitpick hat on for a sec.

> Source/WebCore/page/Page.cpp:649
> +    // Allow any local file URI scheme to be loaded

URI here and URL elsewhere? I admit I'm a little foggy on the distinction, but the usage here should be consistent, no?
Also, period at end of sentence.

> Source/WebCore/platform/qt/KURLQt.cpp:46
> +    // Permit valid file or qrc URLs

Ditto.

> Source/WebCore/platform/qt/KURLQt.cpp:47
> +    if (isValid() && (protocolIs("file") || protocolIs("qrc"))) {

I'd prefer isLocalFile() to protocolIs("file") here.
Also, the double occurrence of the "qrc" literal is ugly. Please rephrase this so we only need it once.

> Source/WebCore/platform/qt/KURLQt.cpp:48
> +        // A valid qrc resource path begins with a colon

Ditto.

> Source/WebCore/platform/qt/KURLQt.cpp:52
> +        // Convert the file URL into a proper platform file path

Ditto.

> Source/WebCore/platform/qt/KURLQt.cpp:53
> +        return static_cast<QUrl>(*this).toLocalFile();

Since this line was already changed by a recent patch of yours, this will need a reupload.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:489
> +    QVERIFY(networkManager->requestedUrls.count() >= 1);

Is there an exact value that we could check against instead of ">=1"?

[Robert Hogan]

(In reply to comment #8)
> (From update of attachment 76727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76727&action=review
> 
> > WebCore/ChangeLog:10
> > +        Permit any url with a local scheme to set a user style sheet. Allow qrc
> > +        resource URLs as arguments to QWebSettings::setUserStyleSheetUrl().
> > +
> 
> I think a less intrusive way to do this would be to convert the qrc URL to a file URL in QWebSetttings::setUserStyleSheerUrl().

Out of curiosity why not this way? Seems odd to put a Qt'ism in KURL if there's an alternative in a Qt specific file.

[Jarred Nicholls]

(In reply to comment #11)
> (From update of attachment 106917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106917&action=review
> 
> I like where this is going, but let me put my nitpick hat on for a sec.
> 

This is good, it was written 9 months ago and I didn't give the comments much attention.

> > Source/WebCore/page/Page.cpp:649
> > +    // Allow any local file URI scheme to be loaded
> 
> URI here and URL elsewhere? I admit I'm a little foggy on the distinction, but the usage here should be consistent, no?
> Also, period at end of sentence.
> 
> > Source/WebCore/platform/qt/KURLQt.cpp:46
> > +    // Permit valid file or qrc URLs
> 
> Ditto.
> 
> > Source/WebCore/platform/qt/KURLQt.cpp:47
> > +    if (isValid() && (protocolIs("file") || protocolIs("qrc"))) {
> 
> I'd prefer isLocalFile() to protocolIs("file") here.

Sure

> Also, the double occurrence of the "qrc" literal is ugly. Please rephrase this so we only need it once.

Agreed

> 
> > Source/WebCore/platform/qt/KURLQt.cpp:48
> > +        // A valid qrc resource path begins with a colon
> 
> Ditto.
> 
> > Source/WebCore/platform/qt/KURLQt.cpp:52
> > +        // Convert the file URL into a proper platform file path
> 
> Ditto.
> 
> > Source/WebCore/platform/qt/KURLQt.cpp:53
> > +        return static_cast<QUrl>(*this).toLocalFile();
> 
> Since this line was already changed by a recent patch of yours, this will need a reupload.

Yeah the two patches are related.  I knew up front this would cause a merge conflict depending on which went first; but I figured a committer could resolve it easily.  I will rebase...

> 
> > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:489
> > +    QVERIFY(networkManager->requestedUrls.count() >= 1);
> 
> Is there an exact value that we could check against instead of ">=1"?

Perhaps, though that's not what we're testing.  This (like in other userStyleSheet test) is just a sanity check before accessing at(0), in my opinion.

[Jarred Nicholls]

(In reply to comment #12)
> (In reply to comment #8)
> > (From update of attachment 76727 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=76727&action=review
> > 
> > > WebCore/ChangeLog:10
> > > +        Permit any url with a local scheme to set a user style sheet. Allow qrc
> > > +        resource URLs as arguments to QWebSettings::setUserStyleSheetUrl().
> > > +
> > 
> > I think a less intrusive way to do this would be to convert the qrc URL to a file URL in QWebSetttings::setUserStyleSheerUrl().
> 
> Out of curiosity why not this way? 

How would you go from qrc => file?

> Seems odd to put a Qt'ism in KURL if there's an alternative in a Qt specific file.

To be fair, it's Qt's KURL, so it is Qt specific regardless...

If you meant qrc => base64 data url, then that would work though is less performant and qrc is already loaded in memory as it is.  Also, I think the smarter check in Page.cpp opens it up more for future "local" schemes, but it's a stretch.

[Jarred Nicholls]

Actually a data URL won't work either, additional resources in the stylesheet like background images wouldn't be able to load relative to the stylesheet.

[Jarred Nicholls]

Created attachment 106995 [details]
Proposed DRT Patch

kling's comments

[Andreas Kling]

Comment on attachment 106995 [details]
Proposed DRT Patch

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

> Source/WebCore/page/Page.cpp:689
> -    if (url.isLocalFile())
> +    
> +    // Allow any local file URL scheme to be loaded.
> +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()))

Is this change observable from DRT?

[WebKit Review Bot]

Comment on attachment 106995 [details]
Proposed DRT Patch

Attachment 106995 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9631882

New failing tests:
platform/chromium/fast/text/text-stroke-with-border.html
platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html

[Jarred Nicholls]

(In reply to comment #17)
> (From update of attachment 106995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106995&action=review
> 
> > Source/WebCore/page/Page.cpp:689
> > -    if (url.isLocalFile())
> > +    
> > +    // Allow any local file URL scheme to be loaded.
> > +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()))
> 
> Is this change observable from DRT?

Yes, DRT can set the user style sheet for tests.  And to answer your next question, yes I'll add a qt DRT test :-P

[Jarred Nicholls]

(In reply to comment #18)
> (From update of attachment 106995 [details])
> Attachment 106995 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9631882
> 
> New failing tests:
> platform/chromium/fast/text/text-stroke-with-border.html
> platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html

What the actual fuck...this has got to be a coincidence !? :)

[Jarred Nicholls]

Created attachment 107204 [details]
Proposed Patch

added qt layout test

[Kenneth Rohde Christiansen]

Comment on attachment 107204 [details]
Proposed Patch

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

> Source/WebCore/page/Page.cpp:689
> +    // Allow any local file URL scheme to be loaded.
> +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()))

I wonder if this should be a separate change with tests? Especially since it is in shared code

> Source/WebCore/platform/qt/KURLQt.cpp:54
> +    if (isValid()) {
> +        if (isLocalFile())
> +            return static_cast<QUrl>(*this).toLocalFile();
> +            
> +        // A valid qrc resource path begins with a colon.
> +        if (protocolIs("qrc"))
> +            return ":" + path();
> +    }

We generally try avoiding indentation when possible. So what about

if (!isValid)
    return String();

[Jarred Nicholls]

Created attachment 107206 [details]
Proposed Patch

Missed WebKitTestRunner

[Jarred Nicholls]

(In reply to comment #22)
> (From update of attachment 107204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107204&action=review
> 
> > Source/WebCore/page/Page.cpp:689
> > +    // Allow any local file URL scheme to be loaded.
> > +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()))
> 
> I wonder if this should be a separate change with tests? Especially since it is in shared code

Unfortunately it can't be a separate change.  I think the current user style sheet tests, with the addition of these qrc tests, still cover this functionality.

> 
> > Source/WebCore/platform/qt/KURLQt.cpp:54
> > +    if (isValid()) {
> > +        if (isLocalFile())
> > +            return static_cast<QUrl>(*this).toLocalFile();
> > +            
> > +        // A valid qrc resource path begins with a colon.
> > +        if (protocolIs("qrc"))
> > +            return ":" + path();
> > +    }
> 
> We generally try avoiding indentation when possible. So what about
> 
> if (!isValid)
>     return String();

Yes this would be better, thanks.

[Jarred Nicholls]

(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 107204 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=107204&action=review
> > 
> > > Source/WebCore/page/Page.cpp:689
> > > +    // Allow any local file URL scheme to be loaded.
> > > +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()))
> > 
> > I wonder if this should be a separate change with tests? Especially since it is in shared code
> 
> Unfortunately it can't be a separate change.  I think the current user style sheet tests, with the addition of these qrc tests, still cover this functionality.
> 
> > 
> > > Source/WebCore/platform/qt/KURLQt.cpp:54
> > > +    if (isValid()) {
> > > +        if (isLocalFile())
> > > +            return static_cast<QUrl>(*this).toLocalFile();
> > > +            
> > > +        // A valid qrc resource path begins with a colon.
> > > +        if (protocolIs("qrc"))
> > > +            return ":" + path();
> > > +    }
> > 
> > We generally try avoiding indentation when possible. So what about
> > 
> > if (!isValid)
> >     return String();
> 
> Yes this would be better, thanks.

On second thought, since we can't guarantee the protocol is either file or qrc, we'd still need the second return String() at the bottom and I was avoiding that...seems redundant I suppose.  But I understand the indentation.  What'cha think?

Thanks for the review.

[Kenneth Rohde Christiansen]

> On second thought, since we can't guarantee the protocol is either file or qrc, we'd still need the second return String() at the bottom and I was avoiding that...seems redundant I suppose.  But I understand the indentation.  What'cha think?
> 
> Thanks for the review.

Sure feel free to do that

[Jarred Nicholls]

Created attachment 107231 [details]
Proposed Patch - indentation cleanup

Indentation cleanup in KURLQt.cpp.  Pick one to commit :)

[Jarred Nicholls]

Created attachment 107353 [details]
Proposed Patch

Rebase from trunk, ready to rock.

[Kenneth Rohde Christiansen]

Comment on attachment 107353 [details]
Proposed Patch

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

> Source/WebCore/page/Page.cpp:689
> -    if (url.isLocalFile())
> +    
> +    // Allow any local file URL scheme to be loaded.
> +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol()))

Is this change tested by non qt specific tests?

[WebKit Review Bot]

Comment on attachment 107353 [details]
Proposed Patch

Clearing flags on attachment: 107353

Committed r95548: <http://trac.webkit.org/changeset/95548>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.