Skip to content

Commit

Permalink
Copied rdar:// links lose their href after pasting into Mail compose
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=248765
rdar://101878956

Reviewed by Ryosuke Niwa.

When sanitizing pasted markup, we currently strip out all URL attributes that are not either HTTP-
family, or data URLs. Done in https://commits.webkit.org/197779@main, the intent was to prevent the
user from inadvertently leaking private information — in particular, local file paths — when pasting
HTML. However, this means that in many other scenarios (e.g. when pasting app-specific URL schemes,
or even when pasting other well-known non-HTTP-family schemes such as `mailto:`), these pasted links
end up being no longer useful. To fix this, we (slightly) relax this constraint to allow non-file
URLs to remain in the sanitized markup, such that we'll allow pasted custom URL schemes (such as
`rdar://` in the original bug report), but still avoid exposing pasted file paths.

I also considered two alternatives, below:

(1) Avoid sanitizing away URL attributes on links, if the text content of the link contains the URL
    string. While this provides a nice balance between compatability and privacy (since the user-
    visible pasted text already contains the URL), it isn't sufficient to fix the bug as originally
    reported, which contains a `rdar://` link with user-visible text that does not match.

(2) Introduce SPI for Mail to adopt in compose, which would prevent us from sanitizing away non-file
    URLs (or provide a list of safe URL schemes to preserve). I decided to not go with this approach
    since this issue isn't limited to Mail compose, but rather any WebKit client, and even web apps
    in the browser. I also didn't want to introduce another behavioral difference between Mail and
    Safari (or other WebKit clients).

* Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:
(WebCore::WebContentReader::readHTML):
(WebCore::WebContentMarkupReader::readHTML):

See above for more details.

* Source/WebCore/editing/markup.cpp:
(WebCore::removeSubresourceURLAttributes):

Use destructuring syntax here to make the code a tiny bit easier to read.

* Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteHTML.mm:

Augment and rename an existing API test to cover this scenario.

Canonical link: https://commits.webkit.org/257410@main
  • Loading branch information
whsieh committed Dec 6, 2022
1 parent 30f0d48 commit 34606a5
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 8 deletions.
8 changes: 4 additions & 4 deletions Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm
Expand Up @@ -582,8 +582,8 @@ static String stripMicrosoftPrefix(const String& string)
String markup;
if (DeprecatedGlobalSettings::customPasteboardDataEnabled() && shouldSanitize()) {
markup = sanitizeMarkup(stringOmittingMicrosoftPrefix, msoListQuirksForMarkup(), WTF::Function<void (DocumentFragment&)> { [] (DocumentFragment& fragment) {
removeSubresourceURLAttributes(fragment, [] (const URL& url) {
return shouldReplaceSubresourceURLWithBlobDuringSanitization(url);
removeSubresourceURLAttributes(fragment, [](auto& url) {
return url.isLocalFile();
});
} });
} else
Expand All @@ -601,8 +601,8 @@ static String stripMicrosoftPrefix(const String& string)
String rawHTML = stripMicrosoftPrefix(string);
if (shouldSanitize()) {
markup = sanitizeMarkup(rawHTML, msoListQuirksForMarkup(), WTF::Function<void (DocumentFragment&)> { [] (DocumentFragment& fragment) {
removeSubresourceURLAttributes(fragment, [] (const URL& url) {
return shouldReplaceSubresourceURLWithBlobDuringSanitization(url);
removeSubresourceURLAttributes(fragment, [](auto& url) {
return url.isLocalFile();
});
} });
} else
Expand Down
4 changes: 2 additions & 2 deletions Source/WebCore/editing/markup.cpp
Expand Up @@ -175,8 +175,8 @@ void removeSubresourceURLAttributes(Ref<DocumentFragment>&& fragment, Function<b
}
}
}
for (auto& item : attributesToRemove)
item.element->removeAttribute(item.attributeName);
for (auto& [element, attribute] : attributesToRemove)
element->removeAttribute(attribute);
}

std::unique_ptr<Page> createPageForSanitizingWebContent()
Expand Down
10 changes: 8 additions & 2 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteHTML.mm
Expand Up @@ -120,18 +120,24 @@ void writeHTMLToPasteboard(NSString *html)
EXPECT_TRUE([webView stringByEvaluatingJavaScript:@"clipboardData.values[0].includes('dangerousCode')"].boolValue);
}

TEST(PasteHTML, StripsFileURLs)
TEST(PasteHTML, StripsFileAndJavaScriptURLs)
{
auto webView = createWebViewWithCustomPasteboardDataSetting(true);
[webView synchronouslyLoadTestPageNamed:@"paste-rtfd"];

writeHTMLToPasteboard(@"<!DOCTYPE html><html><body><a alt='hello' href='file:///private/var/folders/secret/files/'>world</a>");
writeHTMLToPasteboard(@"<!DOCTYPE html><html><body>"
"<a alt='hello' href='file:///private/var/folders/secret/files/'>world</a>"
"<a href='rdar://101878956'>Radar Link</a>"
"<a href='javascript:runCode()'>JavaScript Link</a>");
[webView paste:nil];

EXPECT_TRUE([webView stringByEvaluatingJavaScript:@"clipboardData.types.includes('text/html')"].boolValue);
EXPECT_TRUE([webView stringByEvaluatingJavaScript:@"clipboardData.values[0].includes('hello')"].boolValue);
EXPECT_TRUE([webView stringByEvaluatingJavaScript:@"clipboardData.values[0].includes('world')"].boolValue);
EXPECT_FALSE([webView stringByEvaluatingJavaScript:@"clipboardData.values[0].includes('secret')"].boolValue);
EXPECT_TRUE([webView stringByEvaluatingJavaScript:@"clipboardData.values[0].includes('rdar://101878956')"].boolValue);
EXPECT_TRUE([webView stringByEvaluatingJavaScript:@"clipboardData.values[0].includes('Radar Link')"].boolValue);
EXPECT_FALSE([webView stringByEvaluatingJavaScript:@"clipboardData.values[0].includes('runCode()')"].boolValue);
}

TEST(PasteHTML, DoesNotStripFileURLsWhenCustomPasteboardDataIsDisabled)
Expand Down

0 comments on commit 34606a5

Please sign in to comment.