Bug 45436

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,

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.

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

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

(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?

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.

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...

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.

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.

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?

Created attachment 70386 [details] Fixed comment#10

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

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

(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.

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.

(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.

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.

Created attachment 74803 [details] Patch that fixes flag position and documentation In this patch the setting name is "EnforceSameOrigin" but comment #17 is fixed.

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.

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

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.

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

When this could merge to main line? Maybe need to update patch, but seems that it merged without errors. Thanks.

Created attachment 204802 [details] Patch Rebased on trunk

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.

Comment on attachment 204802 [details] Patch Qt has been removed, clearing review flags.

=== 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.