[Qt] Expose the web security setting.

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Eugene Ostroukhov]

We are developing an IDE for mobile web widgets. Those widgets will be ran on the handhelds as standalone applications. Our IDE has a built-in web server that serves both the widget itself and "simulator" and has some logic (so from the browser opens "http://" url instead of "file://"). In this case call to remote site (i.e. to fetch RSS feed) becomes a cross-domain request and is blocked by webkit security.

WebKit has a setting to disable this check (http://trac.webkit.org/changeset/40449). Qt does not expose it.

[Eugene Ostroukhov]

This is the patch against Qt Git repository:

diff --git a/src/3rdparty/webkit/WebKit/qt/Api/qwebsettings.cpp b/src/3rdparty/webkit/WebKit/qt/Api/qwebsettings.cpp
index d907d86..08bdf19 100644
--- a/src/3rdparty/webkit/WebKit/qt/Api/qwebsettings.cpp
+++ b/src/3rdparty/webkit/WebKit/qt/Api/qwebsettings.cpp
@@ -151,6 +151,10 @@ void QWebSettingsPrivate::apply()
         value = attributes.value(QWebSettings::JavascriptEnabled,
                                       global->attributes.value(QWebSettings::JavascriptEnabled));
         settings->setJavaScriptEnabled(value);
+
+        value = attributes.value(QWebSettings::WebSecurityEnabled,
+                                      global->attributes.value(QWebSettings::WebSecurityEnabled));
+        settings->setWebSecurityEnabled(value);
 #if USE(ACCELERATED_COMPOSITING)
         value = attributes.value(QWebSettings::AcceleratedCompositingEnabled,
                                       global->attributes.value(QWebSettings::AcceleratedCompositingEnabled));
@@ -469,6 +473,7 @@ QWebSettings::QWebSettings()
     d->attributes.insert(QWebSettings::TiledBackingStoreEnabled, false);
     d->attributes.insert(QWebSettings::FrameFlatteningEnabled, false);
     d->attributes.insert(QWebSettings::SiteSpecificQuirksEnabled, true);
+    d->attributes.insert(QWebSettings::WebSecurityEnabled, true);
     d->offlineStorageDefaultQuota = 5 * 1024 * 1024;
     d->defaultTextEncoding = QLatin1String("iso-8859-1");
 }
diff --git a/src/3rdparty/webkit/WebKit/qt/Api/qwebsettings.h b/src/3rdparty/webkit/WebKit/qt/Api/qwebsettings.h
index 156f633..03a8ab8 100644
--- a/src/3rdparty/webkit/WebKit/qt/Api/qwebsettings.h
+++ b/src/3rdparty/webkit/WebKit/qt/Api/qwebsettings.h
@@ -74,7 +74,8 @@ public:
         LocalContentCanAccessFileUrls,
         TiledBackingStoreEnabled,
         FrameFlatteningEnabled,
-        SiteSpecificQuirksEnabled
+        SiteSpecificQuirksEnabled,
+        WebSecurityEnabled
     };
     enum WebGraphic {
         MissingImageGraphic,

[Benjamin Poulain]

Here are the informations to submit a patch: http://trac.webkit.org/wiki/QtWebKitContrib

The WebKit teams don't take any patch from the comments. If you need help to submit your first patch, please come on IRC on the channel #qtwebkit.

Please also check if it is possible to do a unit test for this feature in the API.

[Benjamin Poulain]

Oh, and you need to update the documentation as well when you add a value in the public API.

[Eugene Ostroukhov]

Created attachment 67421 [details]
Patch against webkit Git head.

[Antonio Gomes]

(In reply to comment #4)
> Created an attachment (id=67421) [details]
> Patch against webkit Git head.

\value WebSecurityEnabled Specifies whether browser should enforce same-origin policy. Note that
+        setting this flag is strongly discouraged as it makes the browser more prone to malicious code.

I think WebSecurity is too general. Specially because in the documentation you explicitly cited about cross-domain access. Could not the setting be named along this?

[Eugene Ostroukhov]

This patch is merely adding an inderection for the similary named property from the core WebKit codebase (this setting was introduced by a Chromium team and it corresponds to "--disable-web-security" command line switch).

I do not know if this setting affects anything other then cross-domain calls. I used this name for consistency with the WebKit. I can rename the setting if needed.

[Eugene Ostroukhov]

Created attachment 67647 [details]
Alternative patch

As an option - this patch will call it "EnforceSameOrigin".

But I am not sure if it is a good practice to use different terminology from the upstream project...

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.cpp"
WebKit/qt/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


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

[Eugene Ostroukhov]

Created attachment 67680 [details]
Alternative setting name. Tab from ChangeLog was removed

I apologize - I ran WebKitTools/Scripts/check-webkit-style prior to updating the ChangeLog.

[Alexis Menard (darktears)]

Comment on attachment 67680 [details]
Alternative setting name. Tab from ChangeLog was removed

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

> WebKit/qt/Api/qwebsettings.cpp:429
> +        from remote servers. Note that setting this flag is strongly discouraged as it makes the browser more 

"Note that setting that flag *to false* ..." helps for a better understanding of the documentation I think. Could you add that it's true by default?

[Eugene Ostroukhov]

Created attachment 70386 [details]
Fixed comment#10

[Benjamin Poulain]

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

[Benjamin Poulain]

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

[Alex Karpenko]

(In reply to comment #5)
> I think WebSecurity is too general. Specially because in the documentation you explicitly cited about cross-domain access. Could not the setting be named along this?

No, please leave it as WebSecurityEnabled. This is consistent with the naming of other flags in QWebSettings, which themselves mirror the naming upstream. I've submitted a patch independently from this one, which also used WebSecurityEnabled as the proper naming for this flag: Bug 44482.

I'd like to know what's holding this patch from being merged? LocalContentCanAccessRemoteUrls and LocalContentCanAccessFileUrls are exposed, but this isn't. There are valid use cases for this setting, as evidenced by its existence upstream, and three independent bug reports requesting addition to the API.

[Adam Barth]

Speaking as the developer who added this setting, I think it make sense to expose.  I would review the patch, but I don't feel qualified to make API choices for Qt.

[Andreas Kling]

(In reply to comment #15)
> Speaking as the developer who added this setting, I think it make sense to expose.  I would review the patch, but I don't feel qualified to make API choices for Qt.

Endorsement noted! :)

All Qt API changes must go through API review, which we'll be doing in Oslo this week.

[Simon Hausmann]

Comment on attachment 70386 [details]
Fixed comment#10

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

It looks straigh-forward to me, apart from the binary compatiblity (r- for that). But I think it would be good if the API documentation would not only say that this setting is problematic, it should also explain _when_ this setting _is_ useful after all.

> WebKit/qt/Api/qwebsettings.h:72
> +        EnforceSameOrigin,

New enum values must be added at the end, otherwise the addition breaks binary compatibility.

[Eugene Ostroukhov]

Created attachment 74803 [details]
Patch that fixes flag position and documentation

In this patch the setting name is "EnforceSameOrigin" but comment #17 is fixed.

[Eugene Ostroukhov]

Created attachment 74808 [details]
Same patch as comment #18 with the flag renamed to "WebSecurityEnabled"

This is the same patch I uploaded as comment #18 but the flag name is "WebSecurityEnabled" for consistency with core WebKit.

[Alexis Menard (darktears)]

Comment on attachment 74808 [details]
Same patch as comment #18 with the flag renamed to "WebSecurityEnabled"

LGTM :)

[Adam Barth]

Comment on attachment 74803 [details]
Patch that fixes flag position and documentation

This patch has been up for review for a long time but does not appear to have Qt API review.  Folks with the power to approve Qt APIs should feel free to override this R-, but having this patch sit up for review for a long time isn't helping anyone.

[Adam Barth]

Comment on attachment 74808 [details]
Same patch as comment #18 with the flag renamed to "WebSecurityEnabled"

See above.

[Azat Khuzhin]

When this could merge to main line?

Maybe need to update patch, but seems that it merged without errors.

Thanks.

[Allan Sandfeld Jensen]

Created attachment 204802 [details]
Patch

Rebased on trunk

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebsettings.cpp', u'Source/WebKit/qt/Api/qwebsettings.h', u'Source/WebKit/qt/ChangeLog']" exit_code: 1
Source/WebKit/qt/Api/qwebsettings.cpp:276:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 3 files


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

[Anders Carlsson]

Comment on attachment 204802 [details]
Patch

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.