When putting URL into pasteboard, we should sanitize it for non-web contents and cross origin websites. Otherwise, we run the risk of letting web contents expositing vulnerabilities in URL parsers in other applications.
Created attachment 323120 [details] WIP
<rdar://problem/34874518>
Created attachment 323123 [details] WIP2
This includes the patch for https://webkit.org/b/178056.
Created attachment 323141 [details] WIP3
Comment on attachment 323141 [details] WIP3 Attachment 323141 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4795251 New failing tests: editing/pasteboard/data-transfer-get-data-on-drop-custom.html
Created attachment 323146 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 323147 [details] WIP4
Comment on attachment 323147 [details] WIP4 Attachment 323147 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4795951 New failing tests: http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html editing/pasteboard/data-transfer-set-data-ignore-copied-walformed-url-in-null-origin.html editing/pasteboard/data-transfer-set-data-sanitlize-url-when-copying-in-null-origin.html
Created attachment 323149 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Hm... the assertion failure here seems unrelated: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000106e67677 WTFCrash + 39 (Assertions.cpp:270) 1 com.apple.WebCore 0x0000000110f18c89 WebCore::RenderBlockFlow::ensureLineBoxes() + 761 (RenderBlockFlow.cpp:3661) 2 com.apple.WebCore 0x00000001111f2b90 WebCore::RenderText::ensureLineBoxes() + 80 (RenderText.cpp:1347) 3 com.apple.WebCore 0x0000000110e4e0aa WebCore::ensureLineBoxesIfNeeded(WebCore::RenderObject&) + 90 (Position.cpp:641) 4 com.apple.WebCore 0x0000000110e4c200 WebCore::Position::downstream(WebCore::EditingBoundaryCrossingRule) const + 656 (Position.cpp:825) 5 com.apple.WebCore 0x000000010f454026 WebCore::previousVisuallyDistinctCandidate(WebCore::Position const&) + 182 (Editing.cpp:273) 6 com.apple.WebCore 0x000000011196ed23 WebCore::VisiblePosition::previous(WebCore::EditingBoundaryCrossingRule, bool*) const + 147 (VisiblePosition.cpp:88) 7 com.apple.WebCore 0x000000011197c0a0 WebCore::isStartOfDocument(WebCore::VisiblePosition const&) + 64 (VisibleUnits.cpp:1485) 8 com.apple.WebCore 0x000000010f6e2e66 WebCore::FrameSelection::selectFrameElementInParentIfFullySelected() + 166 (FrameSelection.cpp:1872) 9 com.apple.WebCore 0x000000010f6e2a40 WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(WebCore::VisibleSelection const&, unsigned int, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity) + 1232 (FrameSelection.cpp:341) 10 com.apple.WebCore 0x000000010f6e1184 WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity) + 84 (FrameSelection.cpp:350) 11 com.apple.WebCore 0x000000010f6eba69 WebCore::FrameSelection::selectAll() + 1161 (FrameSelection.cpp:1950) 12 com.apple.WebCore 0x000000010f48abe8 WebCore::executeSelectAll(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 40 (EditorCommand.cpp:980) 13 com.apple.WebCore 0x000000010f4867f9 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 217 (EditorCommand.cpp:1775) 14 com.apple.WebCore 0x000000010f2c3ae9 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 89 (Document.cpp:5016) 15 com.apple.WebCore 0x000000010fed16b1 WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*, WebCore::JSDocument*, JSC::ThrowScope&) + 689 (JSDocument.cpp:4893) 16 com.apple.WebCore 0x000000010fec1697 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*, WebCore::JSDocument*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) + 615 (JSDOMOperation.h:53) 17 com.apple.WebCore 0x000000010fea9cec WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*) + 28 (JSDocument.cpp:4898) 18 ??? 0x000048ef8d201028 0 + 80193702072360 19 com.apple.JavaScriptCore 0x0000000105aaf061 llint_entry + 31259 20 com.apple.JavaScriptCore 0x0000000105aaf061 llint_entry + 31259 21 com.apple.JavaScriptCore 0x0000000105aa7427 vmEntryToJavaScript + 343
Created attachment 323263 [details] Implements the feature
Comment on attachment 323263 [details] Implements the feature Attachment 323263 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4807984 New failing tests: http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html editing/pasteboard/data-transfer-set-data-ignore-copied-walformed-url-in-null-origin.html editing/pasteboard/data-transfer-set-data-sanitlize-url-when-copying-in-null-origin.html
Created attachment 323281 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to Build Bot from comment #13) > Comment on attachment 323263 [details] > Implements the feature > > Attachment 323263 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/4807984 > > New failing tests: > http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html > editing/pasteboard/data-transfer-set-data-ignore-copied-walformed-url-in- > null-origin.html > editing/pasteboard/data-transfer-set-data-sanitlize-url-when-copying-in-null- > origin.html This is very mysterious... I can't reproduce these assertion failures at all on my local machine :(
Created attachment 323285 [details] Worked around the assertion failure in El Capitan
The unrelated assertion failure I encountered is now tracked in https://bugs.webkit.org/show_bug.cgi?id=178116.
Ping reviewers.
Comment on attachment 323285 [details] Worked around the assertion failure in El Capitan View in context: https://bugs.webkit.org/attachment.cgi?id=323285&action=review > Source/WebCore/ChangeLog:51 > + Store the original value if the sanitization resuled in any difference. resuled => resulted > Source/WebCore/ChangeLog:55 > + (WebCore::DataTransfer::createForDragStartEvent): Added Document as an arugment to compute the origin string. arugment => argument > Source/WebKit/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). Perhaps a sentence here saying that you're plumbing an `origin` through pasteboard codepaths in the client layer? > Source/WebCore/dom/DataTransfer.cpp:81 > +static String originForDocument(Document& document) In the null origin case, the String we pass back isn't an origin at all, so I think the name originForDocument is slightly misleading. Maybe originIdentifierForDocument? > Source/WebCore/dom/DataTransfer.cpp:162 > + if (is<StaticPasteboard>(*m_pasteboard)) Pasteboard has an isStatic() method, I think that would be cleaner than using the is<StaticPasteboard> check. I think this also warrants a comment describing why it's always safe to treat data on a "StaticPasteboard" as same origin data (because we only get here when starting a drag or copying, so the data the page reads here can only be data the page just wrote). > Source/WebCore/dom/DataTransfer.cpp:213 > + if (url.isValid() && !url.isEmpty()) Can an empty URL be valid? If not, we can just shorten this to url.isValid(). > Source/WebCore/dom/Document.cpp:5314 > + // happen to be identical within 1 miroseconds of one another for two documents with an identical pointer value. miroseconds => microseconds > Tools/TestWebKitAPI/Tests/WebKitCocoa/copy-url.html:20 > +editor.focus(); document.execCommand('selctAll'); "selectAll". Though, it looks like this isn't necessary anyways for the test, given that it passes anyways, since the copy event must be fired even if we don't have a range selection. I suppose focus()ing ensures that the selection is at the very least a caret, and not none, so we don't bail early in the copy?
Comment on attachment 323285 [details] Worked around the assertion failure in El Capitan View in context: https://bugs.webkit.org/attachment.cgi?id=323285&action=review >> Source/WebCore/ChangeLog:51 >> + Store the original value if the sanitization resuled in any difference. > > resuled => resulted Will fix. >> Source/WebCore/ChangeLog:55 >> + (WebCore::DataTransfer::createForDragStartEvent): Added Document as an arugment to compute the origin string. > > arugment => argument Will fix. >> Source/WebKit/ChangeLog:7 >> + Reviewed by NOBODY (OOPS!). > > Perhaps a sentence here saying that you're plumbing an `origin` through pasteboard codepaths in the client layer? Sure, will add. >> Source/WebCore/dom/DataTransfer.cpp:81 >> +static String originForDocument(Document& document) > > In the null origin case, the String we pass back isn't an origin at all, so I think the name originForDocument is slightly misleading. Maybe originIdentifierForDocument? That's a good point. originIdentifierForDocument will do. >> Source/WebCore/dom/DataTransfer.cpp:162 >> + if (is<StaticPasteboard>(*m_pasteboard)) > > Pasteboard has an isStatic() method, I think that would be cleaner than using the is<StaticPasteboard> check. I think this also warrants a comment describing why it's always safe to treat data on a "StaticPasteboard" as same origin data (because we only get here when starting a drag or copying, so the data the page reads here can only be data the page just wrote). Sure, will add a comment. is<StaticPasteboard>() is the preferred method of checking types in WebKit because we may decide to start RTTI instead of these isX() methods. >> Source/WebCore/dom/DataTransfer.cpp:213 >> + if (url.isValid() && !url.isEmpty()) > > Can an empty URL be valid? If not, we can just shorten this to url.isValid(). Oh, that's a good point. We probably don't need url.isEmpty() here. I think I added this before I had "sanitizedData != data" check below for some reason. >> Source/WebCore/dom/Document.cpp:5314 >> + // happen to be identical within 1 miroseconds of one another for two documents with an identical pointer value. > > miroseconds => microseconds Will fix. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/copy-url.html:20 >> +editor.focus(); document.execCommand('selctAll'); > > "selectAll". Though, it looks like this isn't necessary anyways for the test, given that it passes anyways, since the copy event must be fired even if we don't have a range selection. I suppose focus()ing ensures that the selection is at the very least a caret, and not none, so we don't bail early in the copy? Yeah, I guess so. Will remove.
Comment on attachment 323285 [details] Worked around the assertion failure in El Capitan View in context: https://bugs.webkit.org/attachment.cgi?id=323285&action=review > Source/WebCore/ChangeLog:15 > + the origin of a originating document into our custom pasteboard data. Note that we expose any URL which ...of "an" originating document... > Source/WebCore/ChangeLog:19 > + and a milisecond precision timestamp, and the memory address of the document. For two null-origin documents milisecond => millisecond > Source/WebCore/ChangeLog:30 > + to diverse between null origin and non-null origin documents. diverse => diverge? > Source/WebCore/ChangeLog:95 > + off the team data. Don't expose custom types that written by cross origin documents. "that written" => "that are written" > Source/WebCore/ChangeLog:96 > + (WebCore::PlatformPasteboard::write): Add the orign string with custom pasteboard types in the team data. "orign" => "origin" > Source/WebCore/dom/DataTransfer.cpp:218 > + if (sanitizedData != data) Interesting. So if we're writing a value for "text/uri-list" and the URLParser spits out a slightly different URL than what was supplied, then we'll write "text/uri-list" into both the custom data blob, as well as the platform pasteboard? It seems like we'd only need to put it in the platform pasteboard, but perhaps I'm missing something here... > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:540 > + if (![plist isKindOfClass:[NSArray class]] || [(NSArray *)plist count] < 2) We cast (NSArray *)plist a lot here :P can we make a local NSArray * variable instead, after checking -isKindOfClass:? > Tools/TestWebKitAPI/Tests/WebKitCocoa/CopyURL.mm:67 > + EXPECT_WK_STREQ(@"1", [webView stringByEvaluatingJavaScript:@"window.didCopy"]); I'd prefer using -boolValue and EXPECT_TRUE here instead of checking for the string "1", but up to you. > LayoutTests/editing/pasteboard/data-transfer-get-data-on-copying-pasting-malformed-url-in-same-document-expected.txt:8 > +FAIL JSON.stringify(Array.from(event.clipboardData.items).map((item) => {kind: item.kind, type: item.type})) should be [{"kind":"string","type":"text/uri-list"}]. Threw exception SyntaxError: Unexpected token ':' Is this test case intended to throw a syntax error? > LayoutTests/http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html:35 > +setTimeout(() => finishJSTest(), 3000); This can just be setTimeout(finishJSTest, 3000) ...speaking of which, do we know exactly what we're waiting on here? I wonder if we can do this in a way that avoids a hard timeout period :P I've had bad experiences with tests
> ...speaking of which, do we know exactly what we're waiting on here? I > wonder if we can do this in a way that avoids a hard timeout period :P I've > had bad experiences with tests ...didn't quite finish that thought. [...] bad experience with tests that try to wait for a set amount of time.
Comment on attachment 323285 [details] Worked around the assertion failure in El Capitan View in context: https://bugs.webkit.org/attachment.cgi?id=323285&action=review A few drive-by comments for now. > Source/WebCore/ChangeLog:13 > + Because the Web compatbility requires that DataTransfer exposes the original URL to any document in the typo: compatbility >>> Source/WebCore/dom/DataTransfer.cpp:162 >>> + if (is<StaticPasteboard>(*m_pasteboard)) >> >> Pasteboard has an isStatic() method, I think that would be cleaner than using the is<StaticPasteboard> check. I think this also warrants a comment describing why it's always safe to treat data on a "StaticPasteboard" as same origin data (because we only get here when starting a drag or copying, so the data the page reads here can only be data the page just wrote). > > Sure, will add a comment. > > is<StaticPasteboard>() is the preferred method of checking types in WebKit > because we may decide to start RTTI instead of these isX() methods. I agree that using is<>() is our preferred pattern. > Source/WebCore/dom/DataTransfer.cpp:212 > + auto url = URLParser(data).result(); So we need to worry about username / password in URLs? > Source/WebCore/dom/Document.cpp:5310 > +String Document::uniqueIdentifier() Can we use createCanonicalUUIDString() ?
(In reply to Wenson Hsieh from comment #21) > Comment on attachment 323285 [details] > Worked around the assertion failure in El Capitan > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323285&action=review > > > Source/WebCore/ChangeLog:15 > > + the origin of a originating document into our custom pasteboard data. Note that we expose any URL which > > ...of "an" originating document... Fixed. > > Source/WebCore/ChangeLog:19 > > + and a milisecond precision timestamp, and the memory address of the document. For two null-origin documents > > milisecond => millisecond Fixed. > > Source/WebCore/ChangeLog:30 > > + to diverse between null origin and non-null origin documents. > > diverse => diverge? Fixed. > > Source/WebCore/ChangeLog:95 > > + off the team data. Don't expose custom types that written by cross origin documents. > > "that written" => "that are written" Fixed. > > Source/WebCore/ChangeLog:96 > > + (WebCore::PlatformPasteboard::write): Add the orign string with custom pasteboard types in the team data. > > "orign" => "origin" Fixed. > > Source/WebCore/dom/DataTransfer.cpp:218 > > + if (sanitizedData != data) > > Interesting. So if we're writing a value for "text/uri-list" and the > URLParser spits out a slightly different URL than what was supplied, then > we'll write "text/uri-list" into both the custom data blob, as well as the > platform pasteboard? It seems like we'd only need to put it in the platform > pasteboard, but perhaps I'm missing something here... We'd need the original URL for the same origin content whereas the native type will be read by other applications and cross origin content. > > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:540 > > + if (![plist isKindOfClass:[NSArray class]] || [(NSArray *)plist count] < 2) > > We cast (NSArray *)plist a lot here :P can we make a local NSArray * > variable instead, after checking -isKindOfClass:? Will fix. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/CopyURL.mm:67 > > + EXPECT_WK_STREQ(@"1", [webView stringByEvaluatingJavaScript:@"window.didCopy"]); > > I'd prefer using -boolValue and EXPECT_TRUE here instead of checking for the > string "1", but up to you. Will fix. > > LayoutTests/editing/pasteboard/data-transfer-get-data-on-copying-pasting-malformed-url-in-same-document-expected.txt:8 > > +FAIL JSON.stringify(Array.from(event.clipboardData.items).map((item) => {kind: item.kind, type: item.type})) should be [{"kind":"string","type":"text/uri-list"}]. Threw exception SyntaxError: Unexpected token ':' > > Is this test case intended to throw a syntax error? Oops, clearly not. Will fix. > > LayoutTests/http/tests/security/clipboard/copy-paste-url-across-origin-sanitizes-url.html:35 > > +setTimeout(() => finishJSTest(), 3000); > > This can just be setTimeout(finishJSTest, 3000) > > ...speaking of which, do we know exactly what we're waiting on here? I > wonder if we can do this in a way that avoids a hard timeout period :P I've > had bad experiences with tests Oops, this was a debugging code. Will remove.
(In reply to Chris Dumez from comment #23) > Comment on attachment 323285 [details] > Worked around the assertion failure in El Capitan > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323285&action=review > > A few drive-by comments for now. > > > Source/WebCore/ChangeLog:13 > > + Because the Web compatbility requires that DataTransfer exposes the original URL to any document in the > > typo: compatbility Will fix. > > Source/WebCore/dom/DataTransfer.cpp:212 > > + auto url = URLParser(data).result(); > > So we need to worry about username / password in URLs? I thought we should but then noticed that this is the case of the user copying URL from the web content so I don't think we really need to strip them away. When the user is pasting URL into the web content, the user will know/see that the URL contains username & password, so again, we probably don't need to strip them away. > > Source/WebCore/dom/Document.cpp:5310 > > +String Document::uniqueIdentifier() > > Can we use createCanonicalUUIDString() ? Will do.
Created attachment 323390 [details] Addressed review comments
Created attachment 323391 [details] Addressed review comments
Created attachment 323393 [details] Rebaseline one test in WK1
Comment on attachment 323393 [details] Rebaseline one test in WK1 View in context: https://bugs.webkit.org/attachment.cgi?id=323393&action=review > Source/WebCore/dom/DataTransfer.h:128 > + String m_origin; Maybe m_originIdentifier would be a more accurate name for this member?
Committed r223195: <https://trac.webkit.org/changeset/223195>