Bug 51159 - [Qt] Permit qrc resources to load in QWebSettings::setUserStyleSheetUrl()
Summary: [Qt] Permit qrc resources to load in QWebSettings::setUserStyleSheetUrl()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-12-15 18:35 PST by Jarred Nicholls
Modified: 2011-09-20 09:41 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (3.06 KB, patch)
2010-12-15 18:57 PST, Jarred Nicholls
ariya.hidayat: review-
Details | Formatted Diff | Diff
Proposed Patch (5.55 KB, patch)
2011-09-09 13:13 PDT, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Proposed Patch (5.48 KB, patch)
2011-09-09 13:51 PDT, Jarred Nicholls
kling: review-
Details | Formatted Diff | Diff
Proposed DRT Patch (5.15 KB, patch)
2011-09-10 18:08 PDT, Jarred Nicholls
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (9.34 KB, patch)
2011-09-13 11:47 PDT, Jarred Nicholls
kenneth: review+
Details | Formatted Diff | Diff
Proposed Patch (10.62 KB, patch)
2011-09-13 11:55 PDT, Jarred Nicholls
kenneth: review+
Details | Formatted Diff | Diff
Proposed Patch - indentation cleanup (10.54 KB, patch)
2011-09-13 14:37 PDT, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Proposed Patch (10.49 KB, patch)
2011-09-14 10:43 PDT, Jarred Nicholls
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jarred Nicholls 2010-12-15 18:35:38 PST
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.
Comment 1 Jarred Nicholls 2010-12-15 18:57:19 PST
Created attachment 76727 [details]
Proposed patch

Need to add a new Qt test, but wanted feedback first.
Comment 2 Jarred Nicholls 2010-12-15 20:36:17 PST
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 ;-)
Comment 3 Ariya Hidayat 2010-12-16 11:28:11 PST
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 :(
Comment 4 Jarred Nicholls 2010-12-16 21:23:05 PST
(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.
Comment 5 Ariya Hidayat 2010-12-17 08:40:08 PST
> 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).
Comment 6 Jarred Nicholls 2010-12-17 09:00:48 PST
(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.
Comment 7 Ariya Hidayat 2010-12-17 09:53:14 PST
Comment on attachment 76727 [details]
Proposed patch

r- until the test is available
Comment 8 Robert Hogan 2011-06-20 12:37:15 PDT
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().
Comment 9 Jarred Nicholls 2011-09-09 13:13:34 PDT
Created attachment 106909 [details]
Proposed Patch
Comment 10 Jarred Nicholls 2011-09-09 13:51:14 PDT
Created attachment 106917 [details]
Proposed Patch
Comment 11 Andreas Kling 2011-09-10 04:12:20 PDT
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"?
Comment 12 Robert Hogan 2011-09-10 04:31:43 PDT
(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.
Comment 13 Jarred Nicholls 2011-09-10 06:23:56 PDT
(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.
Comment 14 Jarred Nicholls 2011-09-10 06:37:16 PDT
(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.
Comment 15 Jarred Nicholls 2011-09-10 06:50:20 PDT
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.
Comment 16 Jarred Nicholls 2011-09-10 18:08:46 PDT
Created attachment 106995 [details]
Proposed DRT Patch

kling's comments
Comment 17 Andreas Kling 2011-09-11 17:07:54 PDT
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?
Comment 18 WebKit Review Bot 2011-09-11 17:53:21 PDT
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
Comment 19 Jarred Nicholls 2011-09-11 19:54:01 PDT
(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
Comment 20 Jarred Nicholls 2011-09-11 19:55:55 PDT
(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 !? :)
Comment 21 Jarred Nicholls 2011-09-13 11:47:26 PDT
Created attachment 107204 [details]
Proposed Patch

added qt layout test
Comment 22 Kenneth Rohde Christiansen 2011-09-13 11:54:23 PDT
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();
Comment 23 Jarred Nicholls 2011-09-13 11:55:37 PDT
Created attachment 107206 [details]
Proposed Patch

Missed WebKitTestRunner
Comment 24 Jarred Nicholls 2011-09-13 11:59:01 PDT
(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.
Comment 25 Jarred Nicholls 2011-09-13 12:01:55 PDT
(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.
Comment 26 Kenneth Rohde Christiansen 2011-09-13 13:42:22 PDT
> 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
Comment 27 Jarred Nicholls 2011-09-13 14:37:44 PDT
Created attachment 107231 [details]
Proposed Patch - indentation cleanup

Indentation cleanup in KURLQt.cpp.  Pick one to commit :)
Comment 28 Jarred Nicholls 2011-09-14 10:43:06 PDT
Created attachment 107353 [details]
Proposed Patch

Rebase from trunk, ready to rock.
Comment 29 Kenneth Rohde Christiansen 2011-09-20 04:52:52 PDT
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?
Comment 30 WebKit Review Bot 2011-09-20 09:41:06 PDT
Comment on attachment 107353 [details]
Proposed Patch

Clearing flags on attachment: 107353

Committed r95548: <http://trac.webkit.org/changeset/95548>
Comment 31 WebKit Review Bot 2011-09-20 09:41:13 PDT
All reviewed patches have been landed.  Closing bug.