Summary: | [Qt] Permit qrc resources to load in QWebSettings::setUserStyleSheetUrl() | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jarred Nicholls <jarred> | ||||||||||||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Enhancement | CC: | ariya.hidayat, benjamin, dglazkov, kenneth, markus, robert, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
Jarred Nicholls
2010-12-15 18:35:38 PST
Created attachment 76727 [details]
Proposed patch
Need to add a new Qt test, but wanted feedback first.
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 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 :( (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. > 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).
(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 on attachment 76727 [details]
Proposed patch
r- until the test is available
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(). Created attachment 106909 [details]
Proposed Patch
Created attachment 106917 [details]
Proposed Patch
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"? (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. (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. (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. 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. Created attachment 106995 [details]
Proposed DRT Patch
kling's comments
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 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 (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 (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 !? :) Created attachment 107204 [details]
Proposed Patch
added qt layout test
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(); Created attachment 107206 [details]
Proposed Patch
Missed WebKitTestRunner
(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. (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.
> 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
Created attachment 107231 [details]
Proposed Patch - indentation cleanup
Indentation cleanup in KURLQt.cpp. Pick one to commit :)
Created attachment 107353 [details]
Proposed Patch
Rebase from trunk, ready to rock.
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 on attachment 107353 [details] Proposed Patch Clearing flags on attachment: 107353 Committed r95548: <http://trac.webkit.org/changeset/95548> All reviewed patches have been landed. Closing bug. |