Commit f6953a5e authored by Owen Min's avatar Owen Min Committed by Commit Bot

Revert "Sanitize style elements in clipboard markup"

This reverts commit d96236b5.

Reason for revert: This may cause "WebKit Linux Leak" failure
First failure: https://ci.chromium.org/p/chromium/builders/ci/WebKit%20Linux%20Leak/7276

Original change's description:
> Sanitize style elements in clipboard markup
> 
> This patch sanitizes clipboard markup before pasting it into document
> by removing all pasted style elements and serializing them onto
> elements as inline style. In this way, we stop stylesheets in clipboard
> markup from being applied to the original elements in the document.
> 
> This patch follows the same approach as in WebKit [1]:
> - First create a dummy document to insert the markup
> - Then computes style and layout in the dummy document
> - Re-serialize the dummy document as the markup to be inserted. This
>   reuses the code path that we serialize a selection range into
>   clipboard, where we need to serialize element computed style into
>   inline styles so that the element styles are preserved.
> - Make sure all style elements are removed before inserting markup
>   into document
> 
> This patch also adds a complete test to ensure that content pasted from
> Excel is still properly styled, which is the main reason we used to
> preserve style elements in clipboard markup [2].
> 
> [1] https://trac.webkit.org/changeset/223440
> [2] http://crbug.com/121163
> 
> Bug: 1017871
> Change-Id: I3bb5a4ae7530a3fdef5ba251975e004857c06f1e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1922919
> Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
> Reviewed-by: Kent Tamura <tkent@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#718281}

TBR=yosin@chromium.org,tkent@chromium.org,xiaochengh@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1017871, 1027386
Change-Id: I1d500647d6227c9be3ae14d9604ba702e9c29834
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1933452Reviewed-by: default avatarOwen Min <zmin@chromium.org>
Reviewed-by: default avatarXiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718778}
parent 219d095d
......@@ -359,13 +359,11 @@ ClipboardCommands::GetFragmentFromClipboard(LocalFrame& frame) {
KURL url;
const String markup = SystemClipboard::GetInstance().ReadHTML(
url, fragment_start, fragment_end);
const String sanitized_markup =
SanitizeMarkupWithContext(markup, fragment_start, fragment_end);
if (!sanitized_markup.IsEmpty()) {
if (!markup.IsEmpty()) {
DCHECK(frame.GetDocument());
fragment =
CreateFragmentFromMarkup(*frame.GetDocument(), sanitized_markup, url,
kDisallowScriptingAndPluginContent);
fragment = CreateFragmentFromMarkupWithContext(
*frame.GetDocument(), markup, fragment_start, fragment_end, url,
kDisallowScriptingAndPluginContent);
}
}
if (fragment)
......
......@@ -855,8 +855,7 @@ static void RemoveHeadContents(ReplacementFragment& fragment) {
Node* next = nullptr;
for (Node* node = fragment.FirstChild(); node; node = next) {
if (IsA<HTMLBaseElement>(*node) || IsHTMLLinkElement(*node) ||
IsA<HTMLMetaElement>(*node) || IsA<HTMLStyleElement>(*node) ||
IsA<HTMLTitleElement>(*node)) {
IsA<HTMLMetaElement>(*node) || IsA<HTMLTitleElement>(*node)) {
next = NodeTraversal::NextSkippingChildren(*node);
fragment.RemoveNode(node);
} else {
......
......@@ -83,6 +83,35 @@ TEST_F(ReplaceSelectionCommandTest, pasteSpanInText) {
<< "'bar' should have been inserted";
}
// This is a regression test for https://crbug.com/121163
TEST_F(ReplaceSelectionCommandTest, styleTagsInPastedHeadIncludedInContent) {
GetDocument().setDesignMode("on");
UpdateAllLifecyclePhasesForTest();
GetDummyPageHolder().GetFrame().Selection().SetSelection(
SelectionInDOMTree::Builder()
.Collapse(Position(GetDocument().body(), 0))
.Build(),
SetSelectionOptions());
DocumentFragment* fragment = GetDocument().createDocumentFragment();
fragment->ParseHTML(
"<head><style>foo { bar: baz; }</style></head>"
"<body><p>Text</p></body>",
GetDocument().documentElement(), kDisallowScriptingAndPluginContent);
ReplaceSelectionCommand::CommandOptions options = 0;
auto* command = MakeGarbageCollected<ReplaceSelectionCommand>(
GetDocument(), fragment, options);
EXPECT_TRUE(command->Apply()) << "the replace command should have succeeded";
EXPECT_EQ(
"<head><style>foo { bar: baz; }</style></head>"
"<body><p>Text</p></body>",
GetDocument().body()->InnerHTMLAsString())
<< "the STYLE and P elements should have been pasted into the body "
<< "of the document";
}
// Helper function to set autosizing multipliers on a document.
bool SetTextAutosizingMultiplier(Document* document, float multiplier) {
bool multiplier_set = false;
......
......@@ -236,15 +236,4 @@ const CSSValue* EditingStyleUtilities::BackgroundColorValueInEffect(
return nullptr;
}
void EditingStyleUtilities::StripUAStyleRulesForMarkupSanitization(
EditingStyle* style) {
// This is a hacky approach to avoid 'font-family: ""' appearing in
// sanitized markup.
// TODO(editing-dev): Implement a non-hacky fix up for all properties
String font_family =
style->Style()->GetPropertyValue(CSSPropertyID::kFontFamily);
if (font_family == "\"\"")
style->Style()->RemoveProperty(CSSPropertyID::kFontFamily);
}
} // namespace blink
......@@ -64,8 +64,6 @@ class EditingStyleUtilities {
unicode_bidi == CSSValueID::kEmbed;
}
static void StripUAStyleRulesForMarkupSanitization(EditingStyle* style);
static bool IsTransparentColorValue(const CSSValue*);
static bool HasTransparentBackgroundColor(CSSStyleDeclaration*);
static bool HasTransparentBackgroundColor(CSSPropertyValueSet*);
......
......@@ -33,11 +33,4 @@ CreateMarkupOptions::Builder::SetShouldConvertBlocksToInlines(
return *this;
}
CreateMarkupOptions::Builder&
CreateMarkupOptions::Builder::SetIsForMarkupSanitization(
bool is_for_sanitization) {
data_.is_for_markup_sanitization_ = is_for_sanitization;
return *this;
}
} // namespace blink
......@@ -31,14 +31,12 @@ class CORE_EXPORT CreateMarkupOptions final {
bool ShouldConvertBlocksToInlines() const {
return should_convert_blocks_to_inlines_;
}
bool IsForMarkupSanitization() const { return is_for_markup_sanitization_; }
private:
Member<const Node> constraining_ancestor_;
AbsoluteURLs should_resolve_urls_ = kDoNotResolveURLs;
bool should_annotate_for_interchange_ = false;
bool should_convert_blocks_to_inlines_ = false;
bool is_for_markup_sanitization_ = false;
};
class CORE_EXPORT CreateMarkupOptions::Builder final {
......@@ -54,7 +52,6 @@ class CORE_EXPORT CreateMarkupOptions::Builder final {
Builder& SetShouldResolveURLs(AbsoluteURLs absolute_urls);
Builder& SetShouldAnnotateForInterchange(bool annotate_for_interchange);
Builder& SetShouldConvertBlocksToInlines(bool convert_blocks_for_inlines);
Builder& SetIsForMarkupSanitization(bool is_for_sanitization);
private:
CreateMarkupOptions data_;
......
......@@ -51,7 +51,6 @@
#include "third_party/blink/renderer/core/editing/visible_selection.h"
#include "third_party/blink/renderer/core/editing/visible_units.h"
#include "third_party/blink/renderer/core/frame/local_frame.h"
#include "third_party/blink/renderer/core/frame/settings.h"
#include "third_party/blink/renderer/core/html/html_anchor_element.h"
#include "third_party/blink/renderer/core/html/html_body_element.h"
#include "third_party/blink/renderer/core/html/html_br_element.h"
......@@ -63,8 +62,6 @@
#include "third_party/blink/renderer/core/html/html_table_element.h"
#include "third_party/blink/renderer/core/html_names.h"
#include "third_party/blink/renderer/core/layout/layout_object.h"
#include "third_party/blink/renderer/core/loader/empty_clients.h"
#include "third_party/blink/renderer/core/page/page.h"
#include "third_party/blink/renderer/platform/bindings/exception_state.h"
#include "third_party/blink/renderer/platform/bindings/runtime_call_stats.h"
#include "third_party/blink/renderer/platform/bindings/v8_per_isolate_data.h"
......@@ -757,57 +754,6 @@ void MergeWithNextTextNode(Text* text_node, ExceptionState& exception_state) {
text_next->remove(exception_state);
}
static Document* CreateStagingDocumentForMarkupSanitization() {
Page::PageClients page_clients;
FillWithEmptyClients(page_clients);
Page* page = Page::CreateNonOrdinary(page_clients);
page->GetSettings().SetScriptEnabled(false);
page->GetSettings().SetPluginsEnabled(false);
page->GetSettings().SetAcceleratedCompositingEnabled(false);
LocalFrame* frame = MakeGarbageCollected<LocalFrame>(
MakeGarbageCollected<EmptyLocalFrameClient>(), *page,
nullptr, // FrameOwner*
nullptr, // WindowAgentFactory*
nullptr // InterfaceRegistry*
);
// Don't leak the actual viewport size to unsanitized markup
LocalFrameView* frame_view =
MakeGarbageCollected<LocalFrameView>(*frame, IntSize(800, 600));
frame->SetView(frame_view);
frame->Init();
Document* document = frame->GetDocument();
DCHECK(document);
DCHECK(document->IsHTMLDocument());
DCHECK(document->body());
return document;
}
String SanitizeMarkupWithContext(const String& raw_markup,
unsigned fragment_start,
unsigned fragment_end) {
Document* staging_document = CreateStagingDocumentForMarkupSanitization();
Element* body = staging_document->body();
DocumentFragment* fragment = CreateFragmentFromMarkupWithContext(
*staging_document, raw_markup, fragment_start, fragment_end, KURL(),
kDisallowScriptingAndPluginContent);
body->appendChild(fragment);
staging_document->UpdateStyleAndLayout();
// This sanitizes stylesheets in the markup into element inline styles
return CreateMarkup(Position::FirstPositionInNode(*body),
Position::LastPositionInNode(*body),
CreateMarkupOptions::Builder()
.SetShouldAnnotateForInterchange(true)
.SetIsForMarkupSanitization(true)
.Build());
}
template class CORE_TEMPLATE_EXPORT CreateMarkupAlgorithm<EditingStrategy>;
template class CORE_TEMPLATE_EXPORT
CreateMarkupAlgorithm<EditingInFlatTreeStrategy>;
......
......@@ -96,10 +96,6 @@ CreateMarkup(const PositionInFlatTree& start,
const PositionInFlatTree& end,
const CreateMarkupOptions& options = CreateMarkupOptions());
String SanitizeMarkupWithContext(const String& raw_markup,
unsigned fragment_start,
unsigned fragment_end);
void MergeWithNextTextNode(Text*, ExceptionState&);
bool PropertyMissingOrEqualToNone(CSSPropertyValueSet*, CSSPropertyID);
......
......@@ -120,12 +120,7 @@ void StyledMarkupAccumulator::AppendTextWithInlineStyle(
StringBuilder buffer;
MarkupFormatter::AppendCharactersReplacingEntities(
buffer, content, 0, content.length(), kEntityMaskInPCDATA);
// Keep collapsible white spaces as is during markup sanitization.
const String text_to_append =
IsForMarkupSanitization()
? buffer.ToString()
: ConvertHTMLTextToInterchangeFormat(buffer.ToString(), text);
result_.Append(text_to_append);
result_.Append(ConvertHTMLTextToInterchangeFormat(buffer.ToString(), text));
}
if (inline_style)
result_.Append("</span>");
......
......@@ -76,9 +76,6 @@ class StyledMarkupAccumulator final {
bool ShouldConvertBlocksToInlines() const {
return options_.ShouldConvertBlocksToInlines();
}
bool IsForMarkupSanitization() const {
return options_.IsForMarkupSanitization();
}
private:
String RenderedText(Text&);
......
......@@ -94,14 +94,12 @@ class StyledMarkupTraverser {
private:
bool ShouldAnnotate() const;
bool ShouldConvertBlocksToInlines() const;
bool IsForMarkupSanitization() const;
void AppendStartMarkup(Node&);
void AppendEndMarkup(Node&);
EditingStyle* CreateInlineStyle(Element&);
bool NeedsInlineStyle(const Element&);
bool ShouldApplyWrappingStyle(const Node&) const;
bool ContainsOnlyBRElement(const Element&) const;
bool ShouldSerializeUnrenderedElement(const Node&) const;
StyledMarkupAccumulator* accumulator_;
Member<Node> last_closed_;
......@@ -114,11 +112,6 @@ bool StyledMarkupTraverser<Strategy>::ShouldAnnotate() const {
return accumulator_->ShouldAnnotate();
}
template <typename Strategy>
bool StyledMarkupTraverser<Strategy>::IsForMarkupSanitization() const {
return accumulator_ && accumulator_->IsForMarkupSanitization();
}
template <typename Strategy>
bool StyledMarkupTraverser<Strategy>::ShouldConvertBlocksToInlines() const {
return accumulator_->ShouldConvertBlocksToInlines();
......@@ -371,7 +364,10 @@ Node* StyledMarkupTraverser<Strategy>::Traverse(Node* start_node,
}
auto* element = DynamicTo<Element>(n);
if (n->GetLayoutObject() || ShouldSerializeUnrenderedElement(*n)) {
if (n->GetLayoutObject() ||
(element && element->HasDisplayContentsStyle()) ||
EnclosingElementWithTag(FirstPositionInOrBeforeNode(*n),
html_names::kSelectTag)) {
// Add the node to the markup if we're not skipping the descendants
AppendStartMarkup(*n);
......@@ -517,11 +513,6 @@ void StyledMarkupTraverser<Strategy>::AppendStartMarkup(Node& node) {
// FIXME: Should this be included in forceInline?
inline_style->Style()->SetProperty(CSSPropertyID::kFloat,
CSSValueID::kNone);
if (IsForMarkupSanitization()) {
EditingStyleUtilities::StripUAStyleRulesForMarkupSanitization(
inline_style);
}
}
accumulator_->AppendTextWithInlineStyle(text, inline_style);
break;
......@@ -580,9 +571,6 @@ EditingStyle* StyledMarkupTraverser<Strategy>::CreateInlineStyle(
inline_style->MergeStyleFromRulesForSerialization(html_element);
}
if (IsForMarkupSanitization())
EditingStyleUtilities::StripUAStyleRulesForMarkupSanitization(inline_style);
return inline_style;
}
......@@ -595,25 +583,6 @@ bool StyledMarkupTraverser<Strategy>::ContainsOnlyBRElement(
return IsA<HTMLBRElement>(first_child) && first_child == element.lastChild();
}
template <typename Strategy>
bool StyledMarkupTraverser<Strategy>::ShouldSerializeUnrenderedElement(
const Node& node) const {
DCHECK(!node.GetLayoutObject());
if (node.IsElementNode() && To<Element>(node).HasDisplayContentsStyle())
return true;
if (EnclosingElementWithTag(FirstPositionInOrBeforeNode(node),
html_names::kSelectTag)) {
return true;
}
if (IsForMarkupSanitization()) {
// During sanitization, iframes in the staging document haven't loaded and
// are hence not rendered. They should still be serialized.
if (IsA<HTMLIFrameElement>(node))
return true;
}
return false;
}
template class StyledMarkupSerializer<EditingStrategy>;
template class StyledMarkupSerializer<EditingInFlatTreeStrategy>;
......
<!DOCTYPE html>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script>
// Snapshot of real HTML written by Microsoft Excel to clipboard.
const kExcelMarkup = `
<html xmlns:v="urn:schemas-microsoft-com:vml"
xmlns:o="urn:schemas-microsoft-com:office:office"
xmlns:x="urn:schemas-microsoft-com:office:excel"
xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv=Content-Type content="text/html; charset=utf-8">
<meta name=ProgId content=Excel.Sheet>
<meta name=Generator content="Microsoft Excel 15">
<link id=Main-File rel=Main-File
href="file:///C:/Users/mockuser/AppData/Local/Temp/msohtmlclip1/01/clip.htm">
<link rel=File-List
href="file:///C:/Users/mockuser/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml">
<style>
<!--table
{mso-displayed-decimal-separator:"\.";
mso-displayed-thousand-separator:"\,";}
@page
{margin:.75in .7in .75in .7in;
mso-header-margin:.3in;
mso-footer-margin:.3in;}
tr
{mso-height-source:auto;}
col
{mso-width-source:auto;}
br
{mso-data-placement:same-cell;}
td
{padding-top:1px;
padding-right:1px;
padding-left:1px;
mso-ignore:padding;
color:black;
font-size:11.0pt;
font-weight:400;
font-style:normal;
text-decoration:none;
font-family:Calibri, sans-serif;
mso-font-charset:0;
mso-number-format:General;
text-align:general;
vertical-align:bottom;
border:none;
mso-background-source:auto;
mso-pattern:auto;
mso-protection:locked visible;
white-space:nowrap;
mso-rotate:0;}
.xl63
{font-size:20.0pt;}
.xl64
{font-weight:700;}
.xl65
{font-style:italic;}
.xl66
{text-decoration:underline;
text-underline-style:single;}
.xl67
{color:red;}
.xl68
{background:yellow;
mso-pattern:black none;}
-->
</style>
</head>
<body link="#0563C1" vlink="#954F72">
<table border=0 cellpadding=0 cellspacing=0 width=128 style='border-collapse:
collapse;width:96pt'>
<!--StartFragment-->
<col width=64 span=2 style='width:48pt'>
<tr height=35 style='height:26.0pt'>
<td height=35 class=xl63 width=64 style='height:26.0pt;width:48pt'>large</td>
<td class=xl64 width=64 style='width:48pt'>bold</td>
</tr>
<tr height=19 style='height:14.5pt'>
<td height=19 class=xl65 style='height:14.5pt'>italic</td>
<td class=xl66>underline</td>
</tr>
<tr height=19 style='height:14.5pt'>
<td height=19 class=xl67 style='height:14.5pt'>color</td>
<td class=xl68>highlight</td>
</tr>
<!--EndFragment-->
</table>
</body>
</html>
`;
document.oncopy = event => {
event.clipboardData.setData('text/html', kExcelMarkup);
event.preventDefault();
}
function parsePixelValue(str) {
const parsed = CSSNumericValue.parse(str);
assert_equals(parsed.unit, 'px');
return parsed.value;
}
</script>
<p>Test passes if we can paste content from Excel with style preserved.</p>
<div id="target" contenteditable></div>
<script>
test(() => {
target.focus();
document.execCommand('copy');
document.execCommand('paste');
// Stylesheets in clipboard should have been stripped or turned into inline style
assert_equals(document.styleSheets.length, 0);
// large bold
// italic underline
// color highlight
const cells = document.querySelectorAll('td');
assert_approx_equals(
parsePixelValue(getComputedStyle(cells[0]).fontSize),
20 * 1.3333, // 20pt to pixels
0.01);
assert_equals(getComputedStyle(cells[1]).fontWeight, '700');
assert_equals(getComputedStyle(cells[2]).fontStyle, 'italic');
assert_equals(getComputedStyle(cells[3]).textDecorationLine, 'underline');
assert_equals(getComputedStyle(cells[4]).color, 'rgb(255, 0, 0)');
assert_equals(getComputedStyle(cells[5]).backgroundColor, 'rgb(255, 255, 0)');
}, 'Style on content pasted from Excel is preserved');
</script>
This test ensures WebKit strips away base, link, meta, style and title elements before inserting HTML.
This test ensures WebKit strips away base, link, meta and title elements before inserting HTML.
PASS
......@@ -64,7 +64,7 @@ td
</body>
</html>
</script>
<p>This test ensures WebKit strips away base, link, meta, style and title elements before inserting HTML.</p>
<p>This test ensures WebKit strips away base, link, meta and title elements before inserting HTML.</p>
<div id="test" contenteditable></div>
<pre><script type="text/javascript">
......@@ -99,9 +99,10 @@ if (window.testRunner)
expectNoInstanceOf('base');
expectNoInstanceOf('meta');
expectNoInstanceOf('link');
expectNoInstanceOf('style');
expectNoInstanceOf('title');
expectInstancesOf('style');
if (passed)
document.writeln('PASS');
......
......@@ -9,6 +9,6 @@ selection_test(
selection.setClipboardData('<math><xss style=display:block>t<style>X<a title="</style><img src onerror=alert(1)>">.<a>.');
selection.document.execCommand('paste');
},
'<div contenteditable>te<br>t<img src>">.<a>.|</a>st</div>',
'<div contenteditable>te<br>t<style>X<a title="</style><img src>">.<a>.|</a>st</div>',
'Paste blocks script injection');
</script>
This test for a bug copy/pasting underlined text. The color of the underline should be the color of the element that has the text-decoration property.
| <span>
| style="text-decoration-line: underline; color: rgb(255, 0, 0);"
| style="color: rgb(255, 0, 0); text-decoration-line: underline;"
| "This should be underlined.<#selection-caret>"
| <br>
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment