Allow shadowing of history object
Created attachment 85088 [details] Patch
This is from the same motivation as https://bugs.webkit.org/show_bug.cgi?id=41802. See also https://trac.webkit.org/wiki/Fingerprinting#ii.Window.HistoryObject
This seems fine. My only concern is to make sure Replaceable and DoNotCheckDomainSecurity play nicely together. Can you write a test that shows that when a cross-origin iframe replaces its history object, that the parent page can't see the new object?
Note that site compatibility concerns in bug 41802 haven't been addressed (other than by deciding to wait and see if bug reports come in). So, this patch shares the same concerns.
I presume you mean https://bugs.webkit.org/show_bug.cgi?id=41802#c13 ? Perhaps we should dig into the history of why our behavior is the way it is and what the history is of other implementations. Generally, I wouldn't expect this change to cause many compat problems because we're making our behavior more flexible.
(In reply to comment #5) > Generally, I wouldn't expect this change to cause many compat problems because we're making our behavior more flexible. One kind of problem we’ve seen in the past in cases like this is websites that accidentally attempts to replace a built-in property; harmless if not allowed, but can interfere with the operation of the rest of the code on the site if allowed.
Taking a step back, maybe the best way of permitting client manipulation/overloading of this and the other javascript items listed in https://trac.webkit.org/wiki/Fingerprinting (particularly those that belong to objects that will never be replaceable such as window and document) is to follow the method used for Navigator::userAgent() - which allows FrameLoaderClient to decide.
That can also work. It's mostly a matter of who's going to opt into this mechanism. Making the option available to FrameLoaderClient means someone with access to the WebKit API will need to make that choice. Making the JS object replaceable means that someone with access to the web-facing API will need to make that choice. I don't have the bigger picture, so it's hard for me to give you advice as to what works better for your goals.
Comment on attachment 85088 [details] Patch R- for lack of a security test, but please consider the discussion above when revising your patch.
Created attachment 88300 [details] Patch
(In reply to comment #9) > (From update of attachment 85088 [details]) > R- for lack of a security test, but please consider the discussion above when revising your patch. Allowing the history object to be replaceable is still useful for my own use case. I've updated the patch and added a test to ensure a cross-origin iframe cannot affect the parent's history object. Is this what you were getting at in #3, or is there a reason a parent should not be able to see a child's history object once it has been replaced?
(In reply to comment #8) > That can also work. It's mostly a matter of who's going to opt into this mechanism. Making the option available to FrameLoaderClient means someone with access to the WebKit API will need to make that choice. Making the JS object replaceable means that someone with access to the web-facing API will need to make that choice. > > I don't have the bigger picture, so it's hard for me to give you advice as to what works better for your goals. My own use case is for a client-facing API, I've outlined what I'm trying to do at: https://lists.webkit.org/pipermail/webkit-qt/2011-March/001358.html and listed the relevant bugs there: https://bugs.webkit.org/show_bug.cgi?id=56274 (DOM) https://bugs.webkit.org/show_bug.cgi?id=56482 (CSS) I'm planning to post that mail to webkit-dev for feedback too, once I've received some from the QtWebKit team.
Comment on attachment 88300 [details] Patch Can you also test the case where the child replaces the history object with another object and see if that change is visible to a cross-domain parent frame? If it is, we're creating a security hole.
Created attachment 89625 [details] Patch
Comment on attachment 89625 [details] Patch Thanks for adding the test. This looks good, but I need to look at this when I more awake. Please feel free to harass me on #webkit for a review. There's still the compat question, but I don't really know how we can address that other than to try.
Comment on attachment 89625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89625&action=review I'm still slightly worried about this patch, but I can't think of another test to run. Please be on the lookout for compatibility regressions! > LayoutTests/http/tests/history/cross-origin-replace-history-object-child-expected.txt:6 > +CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/history/resources/cross-origin-replaces-history-object-child-iframe.html from frame with URL http://127.0.0.1:8000/history/cross-origin-replace-history-object-child.html. Domains, protocols and ports must match. Where does this message come from?
Comment on attachment 89625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89625&action=review > LayoutTests/http/tests/history/cross-origin-replace-history-object-child.html:18 > + alert("Child window's history object after attempt to clear: " + window.frames[0].history); It comes from here. It gets the same message on the first access with: alert("Child window's history object before attempt to clear: " + window.frames[0].history); Look Ok to you?
(In reply to comment #17) > (From update of attachment 89625 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89625&action=review > > > LayoutTests/http/tests/history/cross-origin-replace-history-object-child.html:18 > > + alert("Child window's history object after attempt to clear: " + window.frames[0].history); > > It comes from here. It gets the same message on the first access with: > alert("Child window's history object before attempt to clear: " + window.frames[0].history); > > Look Ok to you? It's strange that we get the message even though access is allowed. Is that behavior the same before / after your patch?
(In reply to comment #18) > > It's strange that we get the message even though access is allowed. Is that behavior the same before / after your patch? It is yes, here is the result without: CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/history/resources/cross-origin-replaces-history-object-child-iframe.html from frame with URL http://127.0.0.1:8000/history/cross-origin-replace-history-object-child.html. Domains, protocols and ports must match. ALERT: Child window's history object before attempt to clear: [object History] ALERT: About to shadow child window's history object: [object History] ALERT: Shadowed child window's history object: [object History] CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/history/resources/cross-origin-replaces-history-object-child-iframe.html from frame with URL http://127.0.0.1:8000/history/cross-origin-replace-history-object-child.html. Domains, protocols and ports must match. ALERT: Child window's history object after attempt to clear: [object History] I'll commit this weekend when the bots are quiet.
Comment on attachment 89625 [details] Patch Rejecting attachment 89625 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2 Last 500 characters of output: sts/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ............ 725.45s total testing time 23387 test cases (99%) succeeded 2 test cases (<1%) had incorrect layout 14 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8464006
Created attachment 91101 [details] Archive of layout-test-results from cr-jail-3 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: cr-jail-3 Port: Mac Platform: Mac OS X 10.6.6
Created attachment 91276 [details] Patch
I revised the patch to pass a test I missed (fast/dom/Window/get-set-properties.html) and update the expected results for another test (http/tests/security/cross-frame-access-put-expected.txt) to take account of the fact that a window property with the DoNotCheckDomainSecurityOnGet attribute (which the history object now has) generates two cross-origin JS warnings rather than just one. I observed this same behaviour in the test with window.self, which also has DoNotCheckDomainSecurityOnGet - though I did not look too deeply into why the extra warning happens.
Comment on attachment 91276 [details] Patch Clearing flags on attachment: 91276 Committed r85142: <http://trac.webkit.org/changeset/85142>
All reviewed patches have been landed. Closing bug.