[Sanitizer] Ensure content is removed when an exception is thrown. [chromium/src : main]
Daniel Vogelheim (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
Does the test cover the Document::parseHTML() change?
Related details
- Daniel Vogelheim
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Are any spec changes needed?
Related details
- Daniel Vogelheim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 2 comments![]( "Open in Gerrit")
Are any spec changes needed?
No. The spec said to throw an exception. We did. But we *also* executed the actual operation that we were supposed to be raising the exception from. The spec doesn't support that.
SetHTML & friends work by first parsing, and then sanitizing. From a spec perspective, the exception is thrown for the entire operation. So when an exception is thrown anywhere in that process, none of those things should have happened. What actually happened here is that the parsing completes successfully; setting up the Sanitizer throws; and then we just leave the parse result in.
Does the test cover the Document::parseHTML() change?
- Joey Arhar
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Code-Review+1![]( "Open in Gerrit")
- Daniel Vogelheim
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 2 comments![]( "Open in Gerrit")
Daniel VogelheimDoes the test cover the Document::parseHTML() change?
Good catch, it doesn't. I'll fix that.
Added a unittest.
I couldn't come up with a WPT test that observes this. When I execute `doc = Document::parseHTML(...)` and `parseHTML` throws, then the assignment will never execute. `setHTML` is difference, because it operates on an object that already exists and that I can then access.
I guess one could argue that a fix for `parseHTML`/`parseHTMLUnsafe` is thus unneccessary. I still think it's the right fix, so I added the unit test on the C++ side, which can observe the return value, regardless of exception.
Ptal.
jarhar: This needs another owners' review, because I added the unit test. :)
Related details
- Joey Arhar
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
Daniel VogelheimDoes the test cover the Document::parseHTML() change?
Daniel VogelheimGood catch, it doesn't. I'll fix that.
Added a unittest.
I couldn't come up with a WPT test that observes this. When I execute `doc = Document::parseHTML(...)` and `parseHTML` throws, then the assignment will never execute. `setHTML` is difference, because it operates on an object that already exists and that I can then access.
I guess one could argue that a fix for `parseHTML`/`parseHTMLUnsafe` is thus unneccessary. I still think it's the right fix, so I added the unit test on the C++ side, which can observe the return value, regardless of exception.
lgtm, but if Noam has an idea about how to make this into a wpt then we should consider it
Related details
- Daniel Vogelheim
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
Daniel VogelheimDoes the test cover the Document::parseHTML() change?
Daniel VogelheimGood catch, it doesn't. I'll fix that.
Joey ArharAdded a unittest.
I couldn't come up with a WPT test that observes this. When I execute `doc = Document::parseHTML(...)` and `parseHTML` throws, then the assignment will never execute. `setHTML` is difference, because it operates on an object that already exists and that I can then access.
I guess one could argue that a fix for `parseHTML`/`parseHTMLUnsafe` is thus unneccessary. I still think it's the right fix, so I added the unit test on the C++ side, which can observe the return value, regardless of exception.
lgtm, but if Noam has an idea about how to make this into a wpt then we should consider it
@voge...@chromium.org you mean the assignment doesn't execute because the binding layer checks for exceptions? If so, I am ok with testing this as a unit-test.
Related details
- Daniel Vogelheim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 1 comment![]( "Open in Gerrit")
Daniel VogelheimDoes the test cover the Document::parseHTML() change?
Daniel VogelheimGood catch, it doesn't. I'll fix that.
Joey ArharAdded a unittest.
I couldn't come up with a WPT test that observes this. When I execute `doc = Document::parseHTML(...)` and `parseHTML` throws, then the assignment will never execute. `setHTML` is difference, because it operates on an object that already exists and that I can then access.
I guess one could argue that a fix for `parseHTML`/`parseHTMLUnsafe` is thus unneccessary. I still think it's the right fix, so I added the unit test on the C++ side, which can observe the return value, regardless of exception.
Noam Rosenthallgtm, but if Noam has an idea about how to make this into a wpt then we should consider it
@voge...@chromium.org you mean the assignment doesn't execute because the binding layer checks for exceptions? If so, I am ok with testing this as a unit-test.
Right. If you do:
```
let x = "old value";
x = Document.parseHTMLUnsafe("ssdfjsldkfj", {sanitizer: {elements:["div"], removeElements:["div"]}});
// Console reports an exception
x // Console reports "old value"
```
x is neither undefined, nor is it "ssdfjsldkfj", it retains its original value.
It's technically JavaScript/V8 that does this, rather than the bindings, but the result is the same. I'm not seeing any way how I could get at the result of function that throws an exception, because JS will continue execution in the catch handler which doesn't see the function result.
Related details
- Joey Arhar
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
Daniel VogelheimDoes the test cover the Document::parseHTML() change?
Daniel VogelheimGood catch, it doesn't. I'll fix that.
Joey ArharAdded a unittest.
I couldn't come up with a WPT test that observes this. When I execute `doc = Document::parseHTML(...)` and `parseHTML` throws, then the assignment will never execute. `setHTML` is difference, because it operates on an object that already exists and that I can then access.
I guess one could argue that a fix for `parseHTML`/`parseHTMLUnsafe` is thus unneccessary. I still think it's the right fix, so I added the unit test on the C++ side, which can observe the return value, regardless of exception.
Noam Rosenthallgtm, but if Noam has an idea about how to make this into a wpt then we should consider it
Daniel Vogelheim@voge...@chromium.org you mean the assignment doesn't execute because the binding layer checks for exceptions? If so, I am ok with testing this as a unit-test.
Right. If you do:
```
let x = "old value";
x = Document.parseHTMLUnsafe("ssdfjsldkfj", {sanitizer: {elements:["div"], removeElements:["div"]}});
// Console reports an exception
x // Console reports "old value"
```x is neither undefined, nor is it "ssdfjsldkfj", it retains its original value.
It's technically JavaScript/V8 that does this, rather than the bindings, but the result is the same. I'm not seeing any way how I could get at the result of function that throws an exception, because JS will continue execution in the catch handler which doesn't see the function result.
- Daniel Vogelheim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Daniel Vogelheim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/58943.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Daniel Vogelheim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Sanitizer] Ensure content is removed when an exception is thrown.
When the sanitize operation throws an exception, we don't clear the
parse result. Also, the callers will pass through the exception, but
will return the incomplete result to the callers.
- M third_party/blink/renderer/core/dom/document.cc
- M third_party/blink/renderer/core/dom/document_test.cc
- M third_party/blink/renderer/core/html/parser/fragment_parser.cc
- M third_party/blink/renderer/core/sanitizer/sanitizer_api.cc
- M third_party/blink/web_tests/external/wpt/sanitizer-api/sethtml-tree-construction.tentative.html
Code-Review: +1 by Noam Rosenthal, +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/58943
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |