Fix quirks mode regression #110

zm-cttae · 2023-01-30T17:10:11Z

We have a significant regression so we need to ~~revert~~ handle Bypass Trusted Types policy requirement #100.
The only other method of fixing this one is srcdoc and that's included in the Trusted Types policy.
The raster image methods were self-cleaning since Remove default styles #71.
Per Cannot take screenshot of more complex webpages #70 comments, let's run it in toSvg to make sure sandbox is killed in all cases.

The other methods were self-cleaning since 1904labs#71

Fetch cached doc headers to prevent 1904labs#100 regression.

src/dom-to-image-more.js

IDisposable

Is it possible to create test cases for each possibility?

IDisposable · 2023-01-30T21:57:46Z

src/dom-to-image-more.js

+                    const sandboxDoctype = document.doctype ? '<!DOCTYPE html>' : '';
+                    const sandboxHTML = `${sandboxDoctype}${sandboxDocument.documentElement.outerHTML}`;
+                    sandbox.setAttribute('srcdoc', sandboxHTML);
+                }


So, the question I have is what does this actually do versus the original code? Seems like the path where we don't need trusted types is handled now, but what happens if we do need them?

If the page would have thrown an error because of Trusted Types, it'll have the iframe with the #100 regression present!

If it doesn't have a Trusted Types policy that throws errors on a raw HTML setter (most pages thankfully don't), we do go ahead and coerce in the doctype. Setting srcdoc via setAttribute isn't perfect, but it is marginally better than document.write.

Admittedly document.write's destructive behavior is more suited to the use case but it's not implemented as an API well enough to be reliable.

Seems icky to be doing the DOCTYPE handling only in one case, but I guess because we're creating the IFRAME from the main document, it gets the right one inside?

Yep. HTML is the correct doctype for our default style query seeing as we embed inside a HTML foreignObject

My thought (so poorly expressed, sorry), is that the net effect of this code when the isTrustedTypesRequired === true is that we're not actually setting the DOCTYPE of the new IFRAME at all. I am wondering if we can/should... seems that the DOM .doctype is read-only, so I doubt we can change anything since we're already synthesized the iframe up at line 1198. I guess this PR (and the one that led to it) has gone through enough churn that I am a bit gunshy about this whole code segment. Sorry.

I might be a bit less worried if we were doing anything in the (non-existent) else clause for this test... in that my mind could settle on what's supposed to be happening in either case, but I'm too close.

Refs: 1904labs#110

src/dom-to-image-more.js

joswhite · 2023-02-01T00:15:41Z

src/dom-to-image-more.js

+                    request.open('HEAD', document.location.href, false);
+                    request.send();
+                    const csp = request.getResponseHeader('content-security-policy');
+                    isTrustedTypesRequired = csp.indexOf('require-trusted-types-for') !== -1;


To check for Trusted Type API error, instead of creating a network request, what if we try the original sandbox.contentWindow.document.write('<!DOCTYPE html>...') in the try block? Then in the catch block, we execute the fallback code.

~~Another idea - would document.write succeed if we used DOMPurity.sanitize first? See an example here Never mind - DOMPurify strips out the entire input to write() so it wouldn't work.~~

One more thing - using synchronous XMLHttpRequest is deprecated. I get the following warning in my console - "Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user’s experience. For more help http://xhr.spec.whatwg.org/". If we do stay with a network request maybe we should consider using an asynchronous one.

Deep into this rabbit-hole... stumbled upon this comment https://github.com/w3c/trusted-types/blob/main/src/trustedtypes.js#L337 which worried me a bit, but I think that's only Edge 12-18 (e.g. the not-Chromium-based version) https://caniuse.com/iframe-srcdoc

Noting this here so I don't head down the same rabbit hole.

Looks like we could just look to see if there's a CSP on the currently executing script like https://github.com/w3c/trusted-types/blob/main/src/polyfill/full.js#L27

😄 seeing as we are in the rabbit hole ...

try-finally doesn't work for some strange reason. ~~However we can actually use the Trusted Types API to check if a policy is needed here -~~ ~~spec~~

Sometimes require-trusted-types-for only appears in the content header for the response. If that's the case, the only way we can+should get what we want is either Trusted Types API or async XHR

Bummer...

doesn't look much like

await domtoimage.toSvg(document.body) VM183:1221 This document requires 'TrustedHTML' assignment. tryTechniques @ VM183:1221 ensureSandboxWindow @ VM183:1212 getDefaultStyle @ VM183:1151 copyUserComputedStyleFast @ VM183:1071 copyStyle @ VM183:367 cloneStyle @ VM183:342 Promise.then (async) processClone @ VM183:333 (anonymous) @ VM183:266 Promise.then (async) cloneNode @ VM183:265 (anonymous) @ VM183:294 Promise.then (async) (anonymous) @ VM183:293 cloneChildren @ VM183:292 (anonymous) @ VM183:263 Promise.then (async) cloneNode @ VM183:262 (anonymous) @ VM183:294 Promise.then (async) (anonymous) @ VM183:293 cloneChildren @ VM183:292 (anonymous) @ VM183:263 Promise.then (async) cloneNode @ VM183:262 (anonymous) @ VM183:294 Promise.then (async) (anonymous) @ VM183:293 cloneChildren @ VM183:292 (anonymous) @ VM183:263 Promise.then (async) cloneNode @ VM183:262 (anonymous) @ VM183:76 Promise.then (async) toSvg @ VM183:75 (anonymous) @ VM188:1 VM183:1236 This document requires 'TrustedHTML' assignment. tryTechniques @ VM183:1236 ensureSandboxWindow @ VM183:1212 getDefaultStyle @ VM183:1151 copyUserComputedStyleFast @ VM183:1071 copyStyle @ VM183:367 cloneStyle @ VM183:342 Promise.then (async) processClone @ VM183:333 (anonymous) @ VM183:266 Promise.then (async) cloneNode @ VM183:265 (anonymous) @ VM183:294 Promise.then (async) (anonymous) @ VM183:293 cloneChildren @ VM183:292 (anonymous) @ VM183:263 Promise.then (async) cloneNode @ VM183:262 (anonymous) @ VM183:294 Promise.then (async) (anonymous) @ VM183:293 cloneChildren @ VM183:292 (anonymous) @ VM183:263 Promise.then (async) cloneNode @ VM183:262 (anonymous) @ VM183:294 Promise.then (async) (anonymous) @ VM183:293 cloneChildren @ VM183:292 (anonymous) @ VM183:263 Promise.then (async) cloneNode @ VM183:262 (anonymous) @ VM183:76 Promise.then (async) toSvg @ VM183:75 (anonymous) @ VM188:1 callout:1 Access to XMLHttpRequest at 'https://www.google.com/images/hpp/Chrome_Owned_96x96.png' from origin 'https://ogs.google.com' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. VM183:764 cannot fetch resource: https://www.google.com/images/hpp/Chrome_Owned_96x96.png, status: 0 fail @ VM183:764 done @ VM183:738 XMLHttpRequest.send (async) (anonymous) @ VM183:719 getAndEncode @ VM183:707 Promise.then (async) inline @ VM183:997 (anonymous) @ VM183:1016 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 (anonymous) @ VM183:1020 (anonymous) @ VM183:1019 Promise.then (async) inlineAll @ VM183:1014 inlineImages @ VM183:483 Promise.then (async) toSvg @ VM183:79 (anonymous) @ VM188:1 VM183:719 GET https://www.google.com/images/hpp/Chrome_Owned_96x96.png net::ERR_FAILED 200

The Trusted Types API fn only tells us if we could be hitting the problem, it isn't a requirement check.
Looks like async XHR with HEAD method is the only sane check we could use here.

The API detectPolicy snippet doesn't handle trusted type policy that is set as response header on server-side. :(
Async XHR with HEAD method it is..

Sorry, this has been so painful. Thanks for the patience.

I've pushed that as quirks-regression branch https://github.com/1904labs/dom-to-image-more/tree/quirks-regression and it runs all the unit tests correctly

Can you validate @joswhite as I don't have a fiddle or unit test that shows this.

I haven't been able to prove that the old code actually activated Quirks mode. For other <iframe> instances without DOCTYPE, I get this. Not sure why I couldn't get the old code to show me the same warning. It might depend on the environment or browser as well.

Anyways, I didn't see any obvious issues. Thanks so much for your work on this!

This superceeds #110, trying to fix a regression introduced by #100

* Fix the quirks mode regression This supersedes #110, trying to fix a regression introduced by #100 Made the ensure sandbox a bit cleaner Also bumped to 3.0.3-rc.0 Fixed the prettier issues.

zm-cttae added 3 commits January 30, 2023 16:43

[nit] Remove sandbox iframe for toSvg

878f66c

The other methods were self-cleaning since 1904labs#71

Coerce doctype in sandbox iframe

42a9d1d

Fetch cached doc headers to prevent 1904labs#100 regression.

Tweak comments for standard mode in sandbox iframe

0df4a1e

zm-cttae changed the title ~~Revert #100 to fix quirks mode regression~~ Fix quirks mode regression Jan 30, 2023

zm-cttae commented Jan 30, 2023

View reviewed changes

src/dom-to-image-more.js Outdated Show resolved Hide resolved

zm-cttae marked this pull request as ready for review January 30, 2023 18:14

IDisposable reviewed Jan 30, 2023

View reviewed changes

src/dom-to-image-more.js Show resolved Hide resolved

Restore charset when coercing doctype in sandbox iframe

1ac5c78

IDisposable reviewed Jan 30, 2023

View reviewed changes

src/dom-to-image-more.js Outdated Show resolved Hide resolved

IDisposable reviewed Jan 30, 2023

View reviewed changes

Limit document request < ensureSandboxWindow to HEAD

6b609d3

Refs: 1904labs#110

IDisposable reviewed Jan 31, 2023

View reviewed changes

src/dom-to-image-more.js Outdated Show resolved Hide resolved

Add uid suffix to generated sandbox iframe

fd264d9

IDisposable reviewed Jan 31, 2023

View reviewed changes

src/dom-to-image-more.js Show resolved Hide resolved

IDisposable reviewed Jan 31, 2023

View reviewed changes

src/dom-to-image-more.js Show resolved Hide resolved

joswhite reviewed Feb 1, 2023

View reviewed changes

IDisposable added a commit that referenced this pull request Feb 1, 2023

Fix the quirks mode regression

ad8dca9

This superceeds #110, trying to fix a regression introduced by #100

IDisposable added a commit that referenced this pull request Feb 1, 2023

Fix the quirks mode regression

af0d722

This superceeds #110, trying to fix a regression introduced by #100

This was referenced Feb 1, 2023

Quirks regression fix tsayen/dom-to-image#446

Closed

Quirks mode regression fix #113

Merged

zm-cttae closed this Feb 1, 2023

zm-cttae deleted the hotfix-tosvg-remove-sandbox branch February 2, 2023 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix quirks mode regression #110

Fix quirks mode regression #110

zm-cttae commented Jan 30, 2023 •

edited

IDisposable left a comment

IDisposable Jan 30, 2023

zm-cttae Jan 30, 2023 •

edited

IDisposable Jan 31, 2023

zm-cttae Jan 31, 2023

IDisposable Jan 31, 2023

joswhite Feb 1, 2023 •

edited

joswhite Feb 1, 2023 •

edited

IDisposable Feb 1, 2023

IDisposable Feb 1, 2023

zm-cttae Feb 1, 2023 •

edited

IDisposable Feb 1, 2023

zm-cttae Feb 1, 2023

zm-cttae Feb 1, 2023

IDisposable Feb 1, 2023

joswhite Feb 2, 2023

Fix quirks mode regression #110

Fix quirks mode regression #110

Conversation

zm-cttae commented Jan 30, 2023 • edited

IDisposable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zm-cttae Jan 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joswhite Feb 1, 2023 • edited

Choose a reason for hiding this comment

joswhite Feb 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zm-cttae Feb 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zm-cttae commented Jan 30, 2023 •

edited

zm-cttae Jan 30, 2023 •

edited

joswhite Feb 1, 2023 •

edited

joswhite Feb 1, 2023 •

edited

zm-cttae Feb 1, 2023 •

edited