Bug 31875 – QWebView: Impossible to make XMLHttpRequest from locally stored HTML page

• Component: WebKit Qt
• Status: RESOLVED DUPLICATE

[Juhana]

Created attachment 43848 [details]
demonstrates the problem

XMLHttpRequests from locally stored HTML file always fails in QWebView. Security restrictions are understandable when running JavaScript code inside QWebView component. But I think it should be possible somehow for developer to allow/whitelist some URLS where XMLHttpRequest can be established. This was possible in Qt 4.6.0 beta using WebSecurityOrigin::whiteListAccessFromOrigin() function but this function is removed from 4.6.0 Release candidate 1. Can similar functionality be restored to next Qt 4.6 release?

Following example shows the problem. 
QWebView is created in following way: 

---
QWebView view;
view.load(QUrl("qrc:/script/index.html"));
view.show();
---

index.html has following script inside body tags:
---
<script type="text/javascript">
var xmlhttp=new XMLHttpRequest()
try
{
xmlhttp.open('get', 'http://api.flickr.com/services/rest/?method=flickr.test.echo&api_key=bd9cc5d539f02a3833dcde44ddfe9d5b')
xmlhttp.onreadystatechange = httpCallBack;
xmlhttp.send(null);
}
catch (e)
{
alert("An exception occurred in the script. Error name: " + e.name + ". Error message: " + e.message); 
}
function httpCallBack()
{
 if (xmlhttp.readyState == 4) 
 {
 alert("state 4" + xmlhttp.responseText);
 }
 else
 {
 alert("state " + xmlhttp.readyState)
 }
}
</script>
---

onreadystatechange/httpCallBack gets called with readyState 4. But responseText is empty and no actual HTTP request goes out. 
Qt version: 4.6.0 Release candidate 1

Reference discussion:
http://discussion.forum.nokia.com/forum/showthread.php?t=186338

(full example attached)

[Carol Szabo]

Working on this. Patch to come soon

[Carol Szabo]

Created attachment 45124 [details]
Proposed Patch

This patch exposes in the Qt port the already existing WebCore whitelist API.
Most of the patch is the test harness for this API.
I suggest that tests that refer to QWebSecurityOrigin and are now included in tst_wqebpage be moved to tst_qwebsecurityorigin.

[Laszlo Gombos]

Comment on attachment 45124 [details]
Proposed Patch

I think the need for this API is quite evident. Excellent patch. 

Looks good to me but I think we can do better documenting the testcase. Please create comments inside the code for the followings: 

 - function runTest() in test.html.
 - class CannedResponseNetworkReply - why do we need this how does it work
 - explanation on why these tests can not be implemented as LayoutTests.

You can maybe also get a better summary for this change/ChangeLog.

r-, only because of the lack of documentation.

[Carol Szabo]

Created attachment 45223 [details]
Proposed Patch; Addressed Laszlo's concerns

[WebKit Review Bot]

Attachment 45223 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:30:  Should have a space between // and comment  [whitespace/comments] [4]
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:31:  Should have a space between // and comment  [whitespace/comments] [4]
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:42:  create_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:44:  whiteList_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp:251:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:350:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 8

[Adam Barth]

The stylebot isn't that great at understanding Qt unit tests.  Some of those warnings are real, but some are bogus.  Can you fix the real ones and let me know the bogus ones so we can fix the stylebot?  Thanks.

[Laszlo Gombos]

Carol, comments looks good to me, thanks !

Can you also address the style issues as suggested by Adam ?

[Simon Hausmann]

Comment on attachment 45223 [details]
Proposed Patch; Addressed Laszlo's concerns

>  /*!
> +    \since 4.7
> +    Constructs a security origin for \a url.
> +*/
> +QWebSecurityOrigin* QWebSecurityOrigin::create(const QUrl& url)
> +{
> +    return new QWebSecurityOrigin(new QWebSecurityOriginPrivate(SecurityOrigin::createFromString(url.toString()).get()));
> +};

QWebSecurityOrigin is a class that is designed to be value based, so returning a pointer seems wrong to me. In addition
the "create" pattern is not typically used in Qt APIs.

> +*/
> +void QWebSecurityOrigin::addToWhiteList(const QUrl& url, bool includeSubDomains)
> +{
> +    SecurityOrigin::whiteListAccessFromOrigin(*d->origin.get(), url.scheme(), url.host(), includeSubDomains);
> +}

I think the boolean as second argument is not a good choice for the API, as it
makes the caller code hard to read.

See also

    http://doc.trolltech.com/qq/qq13-apis.html#thebooleanparametertrap

for a more detailed explanation of this pattern.

I think we sohuld discuss this API on the mailing list first ( see https://trac.webkit.org/wiki/QtWebKitAPI )

[Laszlo Gombos]

Commit http://trac.webkit.org/changeset/52444 probably regressed this patch - please take this commit into account.

[Kenneth Rohde Christiansen]

> a pointer seems wrong to me. In addition
> the "create" pattern is not typically used in Qt APIs.

We have createWindow and createPlugin in our Qt WebKit API's, though, but not on the class itself.

[Simon Hausmann]

We've had a long API discussion in the office here in Oslo today about this, and concluded the following proposal for an API:


-- qwebsecurityorigin.h

class QWebSecurityOrigin {
    ...

    class AccessEntry {
    public:
        AccessEntry();
        AccessEntry(const QString& destination);
        ~AccessEntry();

        QString domain() const { return m_domain; }
        void setDomain(const QString& domain) { m_domain = domain; }

        void setScheme(const QString& scheme) const { m_scheme = scheme; }
        QString scheme() const { return m_scheme; }

        void setAllowSubDomains(bool allow) { m_allowSubDomains = allow; }
        bool allowSubDomains() const { return m_allowSubDomains; }

    private:
        QString m_domain;
        QString m_scheme;
        bool m_allowSubDomains;
        struct Private;
        Private *d;
    };

    ...
    static QList<QWebSecurityOrigin::AccessEntry> crossOriginAccessList(const QString& origin);
    static void setCrossOriginAccessList(const QString& origin, const QList<QWebSecurityOrigin::AcessEntry>& list);
    static clearCrossOriginAccessLists();
    ...

-- qwebsecurityorigin.cpp

QWebSecurityOrigin::AccessEntry::AcessEntry()
    : m_allowSubDomains(false)
    , d(0)
{
}

QWebSecurityOrigin::AccessEntry::AcessEntry(const QString& destination)
    : m_allowSubDomains(false)
    , d(0)
{
    RefPtr<SecurityOrigin> dest = SecurityOrigin::createFromString(destination);
    m_domain = dest.host();
    m_scheme = dest.protocol();
}

QWebSecurityOrigin::AccessEntry::~AcessEntry()
{
    delete d;
}

-- example use-cases

QList<QWebSecurityOrigin::AccessEntry> accessList = QWebSecuritOrigin::crossOriginAccessList("wheatherwidget.com");

for (...) {
    QWebSecurityOrigin::AccessEntry entry;
    entry.setDomain("google.com");
    entry.setAllowSubDomains(true);

    accessList.append(entry);
}

accessList.append(QWebSecurityOrigin::AccessEntry("https://google.com"));

QWebSecurityOrigin::AccessEntry entry("https://google.com");
entry.setAllowSubDomains(true);
accessList.append(entry);

QWebSecurityOrigin::setCrossOriginAccessList("weatherwidget.com", accessList);

QWebSecurityOrigin::clearCrossOriginAccessLists();

[Carol Szabo]

(In reply to comment #11)
> We've had a long API discussion in the office here in Oslo today about this,
> and concluded the following proposal for an API:
> 
> 
> -- qwebsecurityorigin.h
> 
> class QWebSecurityOrigin {
>     ...
> 
>     class AccessEntry {
>     public:
>         AccessEntry();
>         AccessEntry(const QString& destination);
>         ~AccessEntry();
> 
>         QString domain() const { return m_domain; }
>         void setDomain(const QString& domain) { m_domain = domain; }
> 
>         void setScheme(const QString& scheme) const { m_scheme = scheme; }
>         QString scheme() const { return m_scheme; }
> 
>         void setAllowSubDomains(bool allow) { m_allowSubDomains = allow; }
>         bool allowSubDomains() const { return m_allowSubDomains; }
> 
>     private:
>         QString m_domain;
>         QString m_scheme;
>         bool m_allowSubDomains;
>         struct Private;
>         Private *d;
>     };
> 
>     ...
>     static QList<QWebSecurityOrigin::AccessEntry> crossOriginAccessList(const
> QString& origin);
>     static void setCrossOriginAccessList(const QString& origin, const
> QList<QWebSecurityOrigin::AcessEntry>& list);
>     static clearCrossOriginAccessLists();
>     ...
> 
> -- qwebsecurityorigin.cpp
> 
> QWebSecurityOrigin::AccessEntry::AcessEntry()
>     : m_allowSubDomains(false)
>     , d(0)
> {
> }
> 
> QWebSecurityOrigin::AccessEntry::AcessEntry(const QString& destination)
>     : m_allowSubDomains(false)
>     , d(0)
> {
>     RefPtr<SecurityOrigin> dest =
> SecurityOrigin::createFromString(destination);
>     m_domain = dest.host();
>     m_scheme = dest.protocol();
> }
> 
> QWebSecurityOrigin::AccessEntry::~AcessEntry()
> {
>     delete d;
> }
> 
> -- example use-cases
> 
> QList<QWebSecurityOrigin::AccessEntry> accessList =
> QWebSecuritOrigin::crossOriginAccessList("wheatherwidget.com");
> 
> for (...) {
>     QWebSecurityOrigin::AccessEntry entry;
>     entry.setDomain("google.com");
>     entry.setAllowSubDomains(true);
> 
>     accessList.append(entry);
> }
> 
> accessList.append(QWebSecurityOrigin::AccessEntry("https://google.com"));
> 
> QWebSecurityOrigin::AccessEntry entry("https://google.com");
> entry.setAllowSubDomains(true);
> accessList.append(entry);
> 
> QWebSecurityOrigin::setCrossOriginAccessList("weatherwidget.com", accessList);
> 
> QWebSecurityOrigin::clearCrossOriginAccessLists();

I hope you guys realize that this interface cannot be implemented (except in a hackish way) as a thin wrapper over existing WebCore functionality because webkit supports a write only API for whitelists.
The WebCore/page/OriginAccessEntry and SecurityOrigin classes will need to be enriched with code implementing the supporting functionality for this API, or otherwise duplicate copies need to be kept for the lists associated with every origin until they are removed.
Also, the crossOriginAccessList function involves returning by value a structure of significant complexity, which usually involves a lot of copying and reallocating memory, not a brilliant idea in my opinion, even when the list is usually small and the function is rarely called.
I suggest changing the crossOriginAccessList function to return a smart pointer of some kind to the whiteList and adding a constructor to the AccessEntry that would construct it based on parts retrieved from the WebCore OriginAccessEntry: scheme, domain, allow subdomains.
If we are to make changes to WebCore, I would argue for adding a port field to the access entry structure since the SecurityOrigin is also defined by the port in addition to the host and security.
You realize that in your example with allowSubDomains=false www.google.com would not be included, as what is called domain, when allowSubDomains is false, really means virtual host at list for HTTP, that is why I am not sure how to call this field that has dual meaning.

[Richard Moore]

I had some discussions with torarne yesterday, suggesting that we extend the AccessEntry class slightly
in order to support future extensions (eg. the ability to add both Allow and Deny rules, or rules about
the allowed methods). It makes sense to consider the http://www.w3.org/TR/access-control/ specification
when considering the naming policy as this is likely to be supported in other areas of webkit. Initially
I'm just suggesting we have an enum specifying what kind of ACL rule the AccessEntry represents eg.
enum AccessEntryPolicy
{
   AllowAccess,
   DenyAccess
}
and a corresponding accessor.

[Simon Hausmann]

Created attachment 47601 [details]
Initial patch of proposed API

[Adam Barth]

Comment on attachment 47601 [details]
Initial patch of proposed API

This patch is really confusing.  createIfNotExisting doesn't seem to be used for anything.

Also, it's unclear why the names for things change in the Qt API.  Shouldn't use the same names at least at the beginning?

You might want to compare with how this works in the Chromium WebKit API because that's another place this functionality is exposed to clients.

[Carol Szabo]

(In reply to comment #14)
> Created an attachment (id=47601) [details]
> Initial patch of proposed API

QWebSecurityOrigin::setAccessListForOrigin does not do what the name leads me to believe, that is replace the current access list for that origin if any, with the one passed as argument. It merely adds to the existing list if that list already exists.
In order to do that, a further extension of WebCore functionality is needed in order to wipe out the old list.
Adam noticed correctly that there is a bug in the securityListForOrigin implementation, that is basically that the createIfNeeded flag is not checked before creating a new list, but that is a minor issue.
With the risk of repeating my self, I cannot avoid noticing that the access list is returned by value from the QWebSecurityOrigin::originAcceessList() method which seems inefficient, I would prefer a smart pointer to a list.
I will look at the Chromium implementation as Adam suggested, but given what support I see on the WebKit trunk, it can't be pretty unless they have a private version of page/SecurityOrigin.

[Simon Hausmann]

Created attachment 47717 [details]
Updated patch with createIfNotExisting bug fixed

[Simon Hausmann]

(In reply to comment #15)
> (From update of attachment 47601 [details])
> This patch is really confusing.  createIfNotExisting doesn't seem to be used
> for anything.

Ooops, the use of it was missing in whiteListForOrigin(). The idea is to create the white list only if the caller wants that.

> Also, it's unclear why the names for things change in the Qt API.  Shouldn't
> use the same names at least at the beginning?

Scheme at least is a term that is used instead of protocol in the Qt APIs. The other names

If host is a better name then I'm all in favour of using it. I just noticed the use of domain in the original static fuction in SecurityOrigin.
 
> You might want to compare with how this works in the Chromium WebKit API
> because that's another place this functionality is exposed to clients.

The chromium API seems to map the SecurityOrigin function very closely. We would like to have something slightly more elaborate than just the one function to add an entry to the list.

[Simon Hausmann]

(In reply to comment #16)
> (In reply to comment #14)
> > Created an attachment (id=47601) [details] [details]
> > Initial patch of proposed API
> 
> QWebSecurityOrigin::setAccessListForOrigin does not do what the name leads me
> to believe, that is replace the current access list for that origin if any,
> with the one passed as argument. It merely adds to the existing list if that
> list already exists.

Before it adds it clears the existing list, so it is a replacement as the name suggests.

> In order to do that, a further extension of WebCore functionality is needed in
> order to wipe out the old list.

That's not needed, it's a wtf::Vector and we all clear() on it already. Am I missing something there?

> Adam noticed correctly that there is a bug in the securityListForOrigin
> implementation, that is basically that the createIfNeeded flag is not checked
> before creating a new list, but that is a minor issue.

Right :)

> With the risk of repeating my self, I cannot avoid noticing that the access
> list is returned by value from the QWebSecurityOrigin::originAcceessList()
> method which seems inefficient, I would prefer a smart pointer to a list.
> I will look at the Chromium implementation as Adam suggested, but given what
> support I see on the WebKit trunk, it can't be pretty unless they have a
> private version of page/SecurityOrigin.

I understand that a smart pointer referencing the internal data structure is more efficient, but that's not how we try to design the Qt APIs. In this particular case I'm less worried about performance given that setting up the white lists is I would assume a one-in-a-while task that may mostly happen at the startup of say the web runtime?

[Carol Szabo]

(In reply to comment #19)
> (In reply to comment #16)
> > (In reply to comment #14)
> > > Created an attachment (id=47601) [details] [details] [details]
> > > Initial patch of proposed API
> > 
> > QWebSecurityOrigin::setAccessListForOrigin does not do what the name leads me
> > to believe, that is replace the current access list for that origin if any,
> > with the one passed as argument. It merely adds to the existing list if that
> > list already exists.
> 
> Before it adds it clears the existing list, so it is a replacement as the name
> suggests.
> 
> > In order to do that, a further extension of WebCore functionality is needed in
> > order to wipe out the old list.
> 
> That's not needed, it's a wtf::Vector and we all clear() on it already. Am I
> missing something there?


Sorry, I missed the call to clear.



> I understand that a smart pointer referencing the internal data structure is
> more efficient, but that's not how we try to design the Qt APIs. In this
> particular case I'm less worried about performance given that setting up the
> white lists is I would assume a one-in-a-while task that may mostly happen at
> the startup of say the web runtime?

Actually in the current use case this needs to happen before the first page from every special origin is loaded (which may be multiple times per web runtime initialization, as for example each local app and javascript library may be in its own security domain).
This observation leads me to spot another potential issue in the current implementation: When setAccessListForOrigin is called with an empty list, the list is cleared, but the list itself remains. If one implementation caches the process and loads multiple apps in it, this may lead to leaking of some memory.
This is likely a small issue, but I thought I'd mention it.

[Adam Barth]

Note that host and domain are different concepts in SecurityOrigin.  Host refers to host name of the server in question.  Domain refers to the value of document.domain, which might be set to something wacky.  In this case, you're interested in the "host" concept.

(In reply to comment #1)
> Working on this. Patch to come soon

Has the patch been published?

Why the bug is still opened?

[Allan Sandfeld Jensen]

*** This bug has been marked as a duplicate of bug 117823 ***