Change moves inline css classes causing CSP error for iFrameOlga Korokhinanit: usually we try to provide 1-line headings
Chris ThompsonI failed to squeeze all the context in one line, let's keep it as an exception this way, some of reviewers are not involved in bugs related discussions so we need as much context as we can have.
Olga KorokhinaSuggestion: "Move inline CSS to fix blob iframe CSP errors" (and then move this full sentence into the description perhaps)
Addressed, thank you!
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
pre.wrap-pre {Olga KorokhinaI don't think it's a good idea to have a class name in this official css file. That would affect developers who use this same class name.
Olga KorokhinaHow would you recommend to have this selector?
@abigai...@google.com I've replaced class with name and made it less like to be used by someone outside, chrome#wrap-pre . How to you think if this is a) the right direction? 2) enough unique name?
"style-src 'self';"Olga KorokhinaThis left here for the purpose of testing for one who wants to fetch and try the CL, will be reverted before merge.
Olga KorokhinaTests are failing because of this, once we revert it tests will go fine.
Set to proper value, ready to merge.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
<pre style="word-wrap: break-word; white-space: pre-wrap;"><script>This is an html file being loaded, not a text file, so why would this test be relevant for this change?
Attribute(html_names::kNameAttr, AtomicString("chrome#wrap-pre")));Would adding this to `<head>` also be stopped by the CSP?:
`<style>pre { word-wrap: break-word; white-space: pre-wrap }</style>`
If so, an alternative is to have a separate text document UA stylesheet.
pre[name="chrome#wrap-pre"] {This will affect styling of author html document that would happen to use pre name="chrome#wrap-pre".
<pre id="pre" style="word-wrap: break-word; white-space: pre-wrap;"><script>This is an html file being loaded, not a text file, so why would this test be relevant for this change?
Attribute(html_names::kNameAttr, AtomicString("chrome#wrap-pre")));Would adding this to `<head>` also be stopped by the CSP?:
`<style>pre { word-wrap: break-word; white-space: pre-wrap }</style>`
If so, an alternative is to have a separate text document UA stylesheet.
+1. We should not be modifying the common resource `html.css` for this task, but I think we can address this in other ways. Otherwise, would it be possible to addd a CSP exception? e.g.
```
source->OverrideContentSecurityPolicy(
network::mojom::CSPDirectiveName::StyleSrc,
"style-src 'self' chrome-untrusted://resources 'word-wrap';");
```
You would have to replace chrome-untrusted://resources with the correct origin.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Attribute(html_names::kNameAttr, AtomicString("chrome#wrap-pre")));Abigail KleinWould adding this to `<head>` also be stopped by the CSP?:
`<style>pre { word-wrap: break-word; white-space: pre-wrap }</style>`
If so, an alternative is to have a separate text document UA stylesheet.
+1. We should not be modifying the common resource `html.css` for this task, but I think we can address this in other ways. Otherwise, would it be possible to addd a CSP exception? e.g.
```
source->OverrideContentSecurityPolicy(
network::mojom::CSPDirectiveName::StyleSrc,
"style-src 'self' chrome-untrusted://resources 'word-wrap';");
```
You would have to replace chrome-untrusted://resources with the correct origin.
Folks I am struggling to write down the <style>...<style>, nothing works, f.i.
// Artificial <style> with styles removed from inline for <pre>.
attributes.push_back(
Attribute(html_names::kContentAttr,
AtomicString("pre { word-wrap: break-word; white-space: pre-wrap }")));
AtomicHTMLToken fake_style_start(HTMLToken::kStartTag, html_names::HTMLTag::kStyle);
TreeBuilder()->ConstructTree(&fake_style_start);
AtomicHTMLToken fake_style_content(HTMLToken::kComment, html_names::HTMLTag::kPlaintext, attributes);
TreeBuilder()->ConstructTree(&fake_style_content);// set insertion mode to kTextMode?
AtomicHTMLToken fake_style_end(HTMLToken::kEndTag, html_names::HTMLTag::kStyle);
TreeBuilder()->ConstructTree(&fake_style_end);
attributes.clear();
Anyone knows how to do it?
<pre id="pre" style="word-wrap: break-word; white-space: pre-wrap;"><script>This is an html file being loaded, not a text file, so why would this test be relevant for this change?
I believe aim of a test is to have the match with generated one so have a selector and not a direct inline style here.
Folks I am struggling to write down the <style>...<style>, nothing works, f.i.
// Artificial <style> with styles removed from inline for <pre>.
attributes.push_back(
Attribute(html_names::kContentAttr,
AtomicString("pre { word-wrap: break-word; white-space: pre-wrap }")));
AtomicHTMLToken fake_style_start(HTMLToken::kStartTag, html_names::HTMLTag::kStyle);
TreeBuilder()->ConstructTree(&fake_style_start);AtomicHTMLToken fake_style_content(HTMLToken::kComment, html_names::HTMLTag::kPlaintext, attributes);TreeBuilder()->ConstructTree(&fake_style_content);// set insertion mode to kTextMode?AtomicHTMLToken fake_style_end(HTMLToken::kEndTag, html_names::HTMLTag::kStyle);
TreeBuilder()->ConstructTree(&fake_style_end);
attributes.clear();Anyone knows how to do it?
Not familiar with the html parser, but my guess is that you need something else than an AtomicHTMLToken since you need to open/close the style element with a text node child.
<pre id="pre" style="word-wrap: break-word; white-space: pre-wrap;"><script>Olga KorokhinaThis is an html file being loaded, not a text file, so why would this test be relevant for this change?
I believe aim of a test is to have the match with generated one so have a selector and not a direct inline style here.
Looking at the CL introducing this test, the bug seems to be about loading txt files directly: https://chromium-review.googlesource.com/c/chromium/src/+/1388552
However, since this test is not a text file, you will not be able to pull in the txt file rendering style when the html.css styling is removed. I would leave these tests without any changes.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Attribute(html_names::kNameAttr, AtomicString("chrome#wrap-pre")));I am able to generate <style> </style> (first and third AtomicHTMLToken in a sample code above) but not the plain text content inside...
<pre id="pre" style="word-wrap: break-word; white-space: pre-wrap;"><script>Olga KorokhinaThis is an html file being loaded, not a text file, so why would this test be relevant for this change?
Rune LillesveenI believe aim of a test is to have the match with generated one so have a selector and not a direct inline style here.
Looking at the CL introducing this test, the bug seems to be about loading txt files directly: https://chromium-review.googlesource.com/c/chromium/src/+/1388552
However, since this test is not a text file, you will not be able to pull in the txt file rendering style when the html.css styling is removed. I would leave these tests without any changes.
Copy, thank you very much. So test is 'kinda artificial'.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Attribute(html_names::kNameAttr, AtomicString("chrome#wrap-pre")));UPD: failed to inject the plain text with the rules inside of the <style></style> block so moved this styles for <pre> to new blob.css file and adding it as
<html><head><link rel="stylesheet" crossorigin="" href="chrome://resources/css/blob.css"><link rel="stylesheet" crossorigin="" href="blob.css"><meta name="color-scheme" content="light dark"></head><body><pre>...</pre></body></html>
Is it a correct way to go, colleagues?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inserting the source text should be possible if you ask someone who knows the HTML parser, but have you considered Abigail's proposal to handle this through a CSP override for text documents? That sounds simpler if it's safe.
<pre id="pre" style="word-wrap: break-word; white-space: pre-wrap;"><script>Olga KorokhinaThis is an html file being loaded, not a text file, so why would this test be relevant for this change?
Rune LillesveenI believe aim of a test is to have the match with generated one so have a selector and not a direct inline style here.
Olga KorokhinaLooking at the CL introducing this test, the bug seems to be about loading txt files directly: https://chromium-review.googlesource.com/c/chromium/src/+/1388552
However, since this test is not a text file, you will not be able to pull in the txt file rendering style when the html.css styling is removed. I would leave these tests without any changes.
Copy, thank you very much. So test is 'kinda artificial'.
No, it's not artificial, but whether the long text is from a text file or included in an html file is not really relevant to the underlying issue.
<pre style="word-wrap: break-word; white-space: pre-wrap;"><script>Olga KorokhinaThis is an html file being loaded, not a text file, so why would this test be relevant for this change?
Reverted, thank you. Because of this has an inline style I am trying to get rid of, technically if we have new rule in html.css and use the name of selector here - style should be applied.
Yes, and this is exactly what we want to escape - we need csp headers remain strict, this is why we do all these changes, check out bug/336209144 . cth...@chromium.org please correct me if I am wrong.
pre[name="chrome#wrap-pre"] {Olga KorokhinaThis will affect styling of author html document that would happen to use pre name="chrome#wrap-pre".
This is indeed a problem so I've been trying to make name unique. Why I tend to put this new rule into this class it is it loaded and available always, and with attempt to put new rule in dedicated .css file I am getting 'can not load local resource' for IWA (where I need to fix the blob loading problem as a goal of these changes) as well as for plain html files, we can't load the chrome:// ... resource.
The content from the author is always just text, so what would be blocked by the CSP is styles injected by us. Unless it's possible to inject style from the author text document, why couldn't we override the CSP for text documents?
AtomicHTMLToken link_style(HTMLToken::kStartTag,nit: The variable name `link_style` is a bit misleading as it represents a `<style>` element, not a `<link>` element. To improve clarity, please consider renaming it to something like `style_start_tag`. (Blink Style Guide: Naming)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
AtomicHTMLToken link_style_2(HTMLToken::kEndTag,nit: The variable name `link_style_2` is not descriptive. To improve readability, please consider renaming it to something that better reflects its purpose, such as `style_end_tag`. (Blink Style Guide: Naming)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +1 |
Attribute(html_names::kNameAttr, AtomicString("chrome#wrap-pre")));I believe this wouldn't work: original set up is clients with strict CSP rules (self only) inside of Isolated Web App use the iFrame with blob text in src. Blob is rendered as text inside of a <pre> with inline style in this file so clients get CSP error. We can not add and use any of source files here (shipped with Chrome) like <link rel="style" ...> - local resources chrome:// will not be loaded (checked by adding here directly and in a header of an html file wrapping the texts). Another opportunity is clients add .css files with needed pre styling to their IWA (can access, passes through CSP) but the inline style here will override it and still cause the CSP error.
So after many experiments I can say that the only working solution is move style to html.css so it be applied by StyleResolver as a rule with either class or name with highly unique string.
As far as I understand styles applied in a order of first rules found in html.css,then client provided .css and and then some overrides in files like forced_colors.css. Potential clients affected by this (if they apparently have same name) will have an opportunity to fix it just by setting word-wrap and white-space to default value.
So, I am very much pushing forward the solution with adding a rule into html.css. @fut...@chromium.org @abigai...@google.com @sim...@chromium.org , Are you fine with this?
I believe this wouldn't work: original set up is clients with strict CSP rules (self only) inside of Isolated Web App use the iFrame with blob text in src. Blob is rendered as text inside of a <pre> with inline style in this file so clients get CSP error. We can not add and use any of source files here (shipped with Chrome) like <link rel="style" ...> - local resources chrome:// will not be loaded (checked by adding here directly and in a header of an html file wrapping the texts). Another opportunity is clients add .css files with needed pre styling to their IWA (can access, passes through CSP) but the inline style here will override it and still cause the CSP error.
So after many experiments I can say that the only working solution is move style to html.css so it be applied by StyleResolver as a rule with either class or name with highly unique string.
As far as I understand styles applied in a order of first rules found in html.css,then client provided .css and and then some overrides in files like forced_colors.css. Potential clients affected by this (if they apparently have same name) will have an opportunity to fix it just by setting word-wrap and white-space to default value.
So, I am very much pushing forward the solution with adding a rule into html.css. @fut...@chromium.org @abigai...@google.com @sim...@chromium.org , Are you fine with this?
No, and I don't understand why we cannot override the CSP (in C++) for TextDocument where we know all non-text content is generated by the browser.
If a UA stylesheet is the only way, it is possible to create a separate sheet in CSSDefaultStyleSheets that is only applied to TextDocument.
I tried with the separate css and failed to integrate it - it shall not be included as <link ...> but somehow added to the scope of StyleResolver. Do you know where I can find a valid sample of how to do it? Thank you in advance!
AtomicHTMLToken link_style(HTMLToken::kStartTag,nit: The variable name `link_style` is a bit misleading as it represents a `<style>` element, not a `<link>` element. To improve clarity, please consider renaming it to something like `style_start_tag`. (Blink Style Guide: Naming)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
will be reverted, used for illustrative purpose.
AtomicHTMLToken link_style_2(HTMLToken::kEndTag,nit: The variable name `link_style_2` is not descriptive. To improve readability, please consider renaming it to something that better reflects its purpose, such as `style_end_tag`. (Blink Style Guide: Naming)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Used for illustrative purpose, will be reverted.
I tried with the separate css and failed to integrate it - it shall not be included as <link ...> but somehow added to the scope of StyleResolver. Do you know where I can find a valid sample of how to do it? Thank you in advance!
But again, I think I'd prefer overriding the CSP. We do that for devtools, for instance:
Thank you for sharing the css_default_style_sheet, this clarifies how to add a style, how we determine the context is a text document so we can apply this style?
Allowing inline style seems to work with a local python http server that serves txt files with `style-src 'none'` (modulo nullptr checks):
```
void TextDocumentParser::InsertFakePreElement() {
GetDocument()->GetExecutionContext()->GetContentSecurityPolicy()->SetOverrideAllowInlineStyle(true);
```
removing myself from the attention set until the tests are passing
Attribute(html_names::kNameAttr, AtomicString("chrome#wrap-pre")));@cth...@chromium.org How do you think, will this solve our issue with Citrix, if we leave all as is and just override the CSP?
The proposed solution actually removes the CSP error, thank you all for advises! I believe we should go with this solution for now, let Citrix check if this works for them and if not - continue effort on moving inline styles away.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
GetContentSecurityPolicy()->SetOverrideAllowInlineStyle(true);If the execution context is available on TextDocument construction, I think that's where we should set the override, with a comment saying something like:
```
// Text documents are rendered using a UA-inserted <pre> tag with a style attribute
// This style attribute should be allowed regardless of the CSP headers sent with
// the text file. This is safe since the all non-text rendered content, that would
// have been blocked, is inserted by the UA.
```
GetContentSecurityPolicy()->SetOverrideAllowInlineStyle(true);If the execution context is available on TextDocument construction, I think that's where we should set the override, with a comment saying something like:
```
// Text documents are rendered using a UA-inserted <pre> tag with a style attribute
// This style attribute should be allowed regardless of the CSP headers sent with
// the text file. This is safe since the all non-text rendered content, that would
// have been blocked, is inserted by the UA.
```
| Code-Review | +1 |
lgtm with the typos in my proposed text fixed.
// attribute This style attribute should be allowed regardless of the CSPMissing '.'
// headers sent with the text file. This is safe since the all non-textremove "the"
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Move inline CSS to fix blob iframe CSP errorsThis is no longer accurate.
with Blob resource in existing css file. Inline styles for <pre> wrapping text in ifame moved to html.css making it available on any rendered page, this eliminates CSP security error fired if policies for styles do not include 'unsafe-inline'. Solves several known issues with blobs as iFrame source.Need to update the description to match the actual change.
Note that you should wrap the lines of the commit message at ~72 chars.