Bug 117823 – [Qt] Add interface API for origin whitelisting

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Jose Lejin PJ]

The QtWebkit API set in Qt 4.8 does not contain any intefraces which could allow a QtWebkit based browser to add/remove specific domains to be whitelisted for cross origin requests.  I understand that previously these API’s were exposed through the interfaces in “QWebSecurityOrigin” which invoked the  API’s in WebCore’s SecurityOrigin class. But currently the API’s to add/remove a whitelist entry are exposed through WebCore’s SecurityPolicy class and there are no interfaces in QtWebkit API to invoke the same.

This bug is raised to introduce new API in QWebSecurityOrigin for whiteListing (add/remove).

[Deepjyoti Saha]

I will be submitting a patch for this.

[Deepjyoti Saha]

Created attachment 205064 [details]
Patch

proposed patch

[Allan Sandfeld Jensen]

Sounds like a duplicate of bug #31875 which also has some suggested patches with flexible API.

[Deepjyoti Saha]

Hi Allan,

To some extent yes - it is the same issue because currently a QtWebKit based browser has no API's exposed to specify "origins" which can be white-listed for cross origin XMLHttpRequest requests.

The last attached patch in bug #31875 has the following:

1) Support required from WebCore, i.e changes related to WebCore's SecurityOrigin.

2) The above functionality in WebCore exposed through QtWebkit API's in QWebSecurityOrigin.


But in current WebCore code: Point 1) is already addressed and implemented in WebCore's SecurityPolicy class. But the same functionality is not exposed through QtWebKit APIs for the browser to use. 

Considering the same, I have submitted this patch.

Also sometime back I raised the same query in the mailing list and Jocelyn had suggested the same as well.

Please correct me If my understanding is wrong.

Regards,
Deep

[Allan Sandfeld Jensen]

Comment on attachment 205064 [details]
Patch

I don't like creating new securityOrigins for the purpose of just calling the SecurityPolicy methods. Would it be possible to instead make the calls methods called on the QWebSecurityOrigin? In that case you already have a WebCore::SecurityOrigin in the private data.

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (From update of attachment 205064 [details])
> I don't like creating new securityOrigins for the purpose of just calling the SecurityPolicy methods. Would it be possible to instead make the calls methods called on the QWebSecurityOrigin? In that case you already have a WebCore::SecurityOrigin in the private data.

Or to match the WebPolicy API. Just make the methods take QWebSecurityOrigin as arguments.

[Deepjyoti Saha]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 205064 [details] [details])
> > I don't like creating new securityOrigins for the purpose of just calling the SecurityPolicy methods. Would it be possible to instead make the calls methods called on the QWebSecurityOrigin? In that case you already have a WebCore::SecurityOrigin in the private data.
> 
> Or to match the WebPolicy API. Just make the methods take QWebSecurityOrigin as arguments.

Hi Allen,

Currently a public constructor to create a QWebSecurityOrigin is not available. Such a constructor can be exposed. But if we are to use the existing WebCore::SecurityOrigin in QWebSecurityOriginPrivate then let us consider the following scenario: 

Suppose there are N origins that the browser wants to white-list, every time it wants to specify a new origin: the exiting WebCore::SecurityOrigin in QWebSecurityOriginPrivate needs to be deleted and replaced with a new SecurityOrigin and then call the SecurityPolicy API using that. (As we cannot modify an existing WebCore::SecurityOrigin once created)

I am not sure if this would be a nice way to do it.

Please suggest/correct I am missing something. 

Regards,
Deep



Please

[Allan Sandfeld Jensen]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 205064 [details] [details] [details])
> > > I don't like creating new securityOrigins for the purpose of just calling the SecurityPolicy methods. Would it be possible to instead make the calls methods called on the QWebSecurityOrigin? In that case you already have a WebCore::SecurityOrigin in the private data.
> > 
> > Or to match the WebPolicy API. Just make the methods take QWebSecurityOrigin as arguments.
> 
> Hi Allen,
> 
> Currently a public constructor to create a QWebSecurityOrigin is not available. Such a constructor can be exposed. But if we are to use the existing WebCore::SecurityOrigin in QWebSecurityOriginPrivate then let us consider the following scenario: 
> 
> Suppose there are N origins that the browser wants to white-list, every time it wants to specify a new origin: the exiting WebCore::SecurityOrigin in QWebSecurityOriginPrivate needs to be deleted and replaced with a new SecurityOrigin and then call the SecurityPolicy API using that. (As we cannot modify an existing WebCore::SecurityOrigin once created)
> 
> I am not sure if this would be a nice way to do it.
> 
The end result is essentially the same, but I can see your point about the lack of QWebSecurityOrigin constructors.

I still don't like the temporary smart pointers though. I think it would be nicer to keep the SecurityOrigin objects in proper RefPtr variables. 

Also I know from a Qt API point of view we discourage the use of boolean arguments. While it does match the WebCore API. I know my colleagues would prefer if you used an enum.

[Allan Sandfeld Jensen]

*** Bug 31875 has been marked as a duplicate of this bug. ***

[Deepjyoti Saha]

Created attachment 205684 [details]
Patch

proposed patch

[Deepjyoti Saha]

Created attachment 207235 [details]
Modified the patch as per Allen's suggestion in IRC.

[Deepjyoti Saha]

Created attachment 207236 [details]
Modified the patch as per Allen's suggestion in IRC.

[Jocelyn Turcotte]

Comment on attachment 205684 [details]
Patch

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

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:271
> +    Allows application/platform to whilelist an origin to have access to specific desitnations beyond same-origin-policy.

- The documentation should explain a bit what each parameter is about, also using the "\a" switch.
- No need to say "application/platform", who else would call this method?

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:273
> +void QWebSecurityOrigin::addOriginAccessWhitelistEntry(const QUrl& sourceOrigin, const QString& destinationProtocol, const QString& destinationHost, SubdomainSetting subdomainSetting)

The rest of the class names is "scheme" rather than "protocol", it would be nice to keep it consistent. It's also a bit clearer.

> Source/WebKit/qt/Api/qwebsecurityorigin.h:30
> +    class SecurityPolicy;

I think you don't need this here.

> Source/WebKit/qt/Api/qwebsecurityorigin.h:52
> +    static void addOriginAccessWhitelistEntry(const QUrl&, const QString&, const QString&, SubdomainSetting);
> +    static void removeOriginAccessWhitelistEntry(const QUrl&, const QString&, const QString&, SubdomainSetting);
> +    static void resetOriginAccessWhitelists();

The public header should always include parameter names.

I also think that having a QWebSecurityOrigin(const QUrl&) public constructor and have add/remove as member methods would be a cleaner API.
QWebSecurityOrigin basically wraps WebCore::SecurityOrigin and I don't see much reason to prevent its construction if SecurityOrigin allows it.

[Deepjyoti Saha]

Comment on attachment 205684 [details]
Patch

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

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:271
>> +    Allows application/platform to whilelist an origin to have access to specific desitnations beyond same-origin-policy.
> 
> - The documentation should explain a bit what each parameter is about, also using the "\a" switch.
> - No need to say "application/platform", who else would call this method?

Sure, I will modify the documentation part to explain the parameters

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:273
>> +void QWebSecurityOrigin::addOriginAccessWhitelistEntry(const QUrl& sourceOrigin, const QString& destinationProtocol, const QString& destinationHost, SubdomainSetting subdomainSetting)
> 
> The rest of the class names is "scheme" rather than "protocol", it would be nice to keep it consistent. It's also a bit clearer.

I will modify and use standard names like "scheme" and "host".

>> Source/WebKit/qt/Api/qwebsecurityorigin.h:30
>> +    class SecurityPolicy;
> 
> I think you don't need this here.

Yes, we actually don't need it here :-). I will remove it.

>> Source/WebKit/qt/Api/qwebsecurityorigin.h:52
>> +    static void resetOriginAccessWhitelists();
> 
> The public header should always include parameter names.
> 
> I also think that having a QWebSecurityOrigin(const QUrl&) public constructor and have add/remove as member methods would be a cleaner API.
> QWebSecurityOrigin basically wraps WebCore::SecurityOrigin and I don't see much reason to prevent its construction if SecurityOrigin allows it.

I will modify the headers and include the constructor as suggested. Thanks!

[Deepjyoti Saha]

Created attachment 207318 [details]
Patch

updated patch as per comments

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebsecurityorigin.cpp', u'Source/WebKit/qt/Api/qwebsecurityorigin.h', u'Source/WebKit/qt/Api/qwebsecurityorigin_p.h', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/tests/qwebsecurityorigin/qwebsecurityorigin.pro', u'Source/WebKit/qt/tests/qwebsecurityorigin/resources/test.html', u'Source/WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp', u'Source/WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.qrc', u'Source/tests.pri']" exit_code: 1
Source/WebKit/qt/Api/qwebsecurityorigin.h:49:  The parameter name "subdomainSetting" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebsecurityorigin.h:50:  The parameter name "subdomainSetting" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebsecurityorigin.h:53:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jocelyn Turcotte]

Comment on attachment 207318 [details]
Patch

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

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:278
> +    if (origin)
> +        d = new QWebSecurityOriginPrivate(origin);

It doesn't seem like SecurityOrigin::create can ever return null.
Plus most classes are always expecting to have a d-pointer.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:284
> +    The subdomain access is based on the \a subdomainSetting.

This isn't telling much to the behavior a different value would trigger, could you instead explain here in one sentence what is the differentce between AllowSubdomains and DisallowSubdomains? I can't tell precisely myself just with this information.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:288
> +void QWebSecurityOrigin::addOriginAccessWhitelistEntry(const QString& scheme, const QString& host, SubdomainSetting subdomainSetting)

Origin is already in the name of the class, addAccessWhitelistEntry would be more concise.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:291
> +    if (!d)
> +        return;

Not useful.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:303
> +    if (!d)
> +        return;

Ditto

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:314
> +void QWebSecurityOrigin::resetOriginAccessWhitelists()
> +{
> +    SecurityPolicy::resetOriginAccessWhitelists();
> +}

This one isn't consistent with the two others, it should either:
- Be static and be named "All" something (e.g. resetAllAccessWhitelists).
- A proper per-origin resetAccessWhitelist should be implemented in SecurityPolicy
- Be removed

Since the API should last as long as possible, removing it would be the easiest and safest way. Most people will probably only add entries and can remove them through the remove method. Do you need it specifically in your case?

> Source/WebKit/qt/Api/qwebsecurityorigin.h:53
> +    QWebSecurityOrigin(const QUrl& url);

Please mark the constructor explicit.

[Deepjyoti Saha]

Comment on attachment 207318 [details]
Patch

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

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:278
>> +        d = new QWebSecurityOriginPrivate(origin);
> 
> It doesn't seem like SecurityOrigin::create can ever return null.
> Plus most classes are always expecting to have a d-pointer.

Ok, I will modify this.

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:284
>> +    The subdomain access is based on the \a subdomainSetting.
> 
> This isn't telling much to the behavior a different value would trigger, could you instead explain here in one sentence what is the differentce between AllowSubdomains and DisallowSubdomains? I can't tell precisely myself just with this information.

Sorry, I will add an explanation for AllowSubdomains and DisallowSubdomains.

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:288
>> +void QWebSecurityOrigin::addOriginAccessWhitelistEntry(const QString& scheme, const QString& host, SubdomainSetting subdomainSetting)
> 
> Origin is already in the name of the class, addAccessWhitelistEntry would be more concise.

Yes, it sounds more concise :)

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:291
>> +        return;
> 
> Not useful.

OK, I will remove this.

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:303
>> +        return;
> 
> Ditto

I will remove this as well.

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:314
>> +}
> 
> This one isn't consistent with the two others, it should either:
> - Be static and be named "All" something (e.g. resetAllAccessWhitelists).
> - A proper per-origin resetAccessWhitelist should be implemented in SecurityPolicy
> - Be removed
> 
> Since the API should last as long as possible, removing it would be the easiest and safest way. Most people will probably only add entries and can remove them through the remove method. Do you need it specifically in your case?

Yes, this API should not be attached with a particular origin considering the current implementation in SecurityPolicy. But I would suggest to retain this and be implemented as a static API. It will come handy in a case where application wants to flush an entire set in one go instead of iterating though a list of origins. Please let me know your opinion.

>>> Source/WebKit/qt/Api/qwebsecurityorigin.h:53
>>> +    QWebSecurityOrigin(const QUrl& url);
>> 
>> The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> Please mark the constructor explicit.

Sure, I will do that.

[Deepjyoti Saha]

Created attachment 207709 [details]
Modified the patch as per suggestions.

Modified the patch as per suggestions.

[Jocelyn Turcotte]

(In reply to comment #18)
> Yes, this API should not be attached with a particular origin considering the current implementation in SecurityPolicy. But I would suggest to retain this and be implemented as a static API. It will come handy in a case where application wants to flush an entire set in one go instead of iterating though a list of origins. Please let me know your opinion.

I would prefer that we do it either the ideal way (fix SecurityPolicy to support clearing for a specific security origin) or avoid it (not add the API).

If you need the functionality in your application now we can do a compromise and add the static API, but otherwise I think that the risk isn't worth it. If we add the static API and that the code changes underneat later (because it's not ideal), we'll have to suffer consequences that might not be worth the candle.

[Jocelyn Turcotte]

Comment on attachment 207709 [details]
Modified the patch as per suggestions.

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

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:295
> +    Passing AllowSubdomains in \a subdomainSetting will allow the source origin to access
> +    the \a host's subdomains as well, whereas passing DisallowSubdomains would prevent this.

In the case of removing, this documentation is misleading. Something like "using the specified \a scheme *and \a subdomainSetting.*" would be fine here.

[Deepjyoti Saha]

Created attachment 207810 [details]
Patch

updated the patch as per comments

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebsecurityorigin.cpp', u'Source/WebKit/qt/Api/qwebsecurityorigin.h', u'Source/WebKit/qt/Api/qwebsecurityorigin_p.h', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/tests/qwebsecurityorigin/qwebsecurityorigin.pro', u'Source/WebKit/qt/tests/qwebsecurityorigin/resources/test.html', u'Source/WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp', u'Source/WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.qrc', u'Source/tests.pri']" exit_code: 1
Source/WebKit/qt/Api/qwebsecurityorigin.h:49:  The parameter name "subdomainSetting" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebsecurityorigin.h:50:  The parameter name "subdomainSetting" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebsecurityorigin.h:52:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Deepjyoti Saha]

(In reply to comment #20)
> (In reply to comment #18)
> > Yes, this API should not be attached with a particular origin considering the current implementation in SecurityPolicy. But I would suggest to retain this and be implemented as a static API. It will come handy in a case where application wants to flush an entire set in one go instead of iterating though a list of origins. Please let me know your opinion.
> 
> I would prefer that we do it either the ideal way (fix SecurityPolicy to support clearing for a specific security origin) or avoid it (not add the API).
> 
> If you need the functionality in your application now we can do a compromise and add the static API, but otherwise I think that the risk isn't worth it. If we add the static API and that the code changes underneat later (because it's not ideal), we'll have to suffer consequences that might not be worth the candle.

I understood the concern you explained. I will remove this API as of now and later if required I will take it up with proper fix in SecurityPolicy to support clearing entries for a specific origin.

[Deepjyoti Saha]

(In reply to comment #21)
> (From update of attachment 207709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207709&action=review
> 
> > Source/WebKit/qt/Api/qwebsecurityorigin.cpp:295
> > +    Passing AllowSubdomains in \a subdomainSetting will allow the source origin to access
> > +    the \a host's subdomains as well, whereas passing DisallowSubdomains would prevent this.
> 
> In the case of removing, this documentation is misleading. Something like "using the specified \a scheme *and \a subdomainSetting.*" would be fine here.

Oops.. :(  Modified the comments in the updated patch

[Jocelyn Turcotte]

Comment on attachment 207810 [details]
Patch

lgtm, thanks :)

[WebKit Commit Bot]

Comment on attachment 207810 [details]
Patch

Clearing flags on attachment: 207810

Committed r153516: <http://trac.webkit.org/changeset/153516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Deepjyoti Saha]

(In reply to comment #26)
> (From update of attachment 207810 [details])
> lgtm, thanks :)

Thanks !! :-)