PLIA-4026: Read the Prepublish DOM from the WordPress origin - #64
Conversation
getDom() built its hidden iframe from the url it was handed, which has the "Public URL" setting applied to it. On a site whose Public URL points at a different host than WordPress itself (a separate delivery domain, say), the frame was therefore cross-origin and reading contentWindow.document threw SecurityError. The throw happened inside the load listener and the promise had no reject path, so the await never settled: Prepublish appeared to do nothing, with no useful console output. The frame was also only torn down on success, so the failure left a height:100vh iframe covering the page, which is what made it look like a new page had loaded. Derive the fetch URL from window.location instead, via the URL API so the nonce survives a fragment, and build the iframe as an element rather than by interpolating into innerHTML. The Public URL remains what we report to Siteimprove, so results still land on the crawled URL. Also, so that any future failure here is visible rather than silent: wrap the document read in try/catch, add a 30s timeout, remove the frame on both paths, and have the click handler clear the spinner and log. Separately, the overlay-driven path called getDom(url) with no nonce at all, sending si_preview_nonce=undefined. Nonce verification then failed and the plugin's own scripts were enqueued into the very DOM being scraped, for every overlay-latest user rather than only split-domain ones. Capture and pass the nonce. Removing getDom's url argument left common(url) unused, so drop that parameter and its call sites. Bump the plugin to 2.1.4 with changelog entries.
mostergaard
left a comment
mostergaard
left a comment
There was a problem hiding this comment.
I can't make out if this will work in all cases. What if you have a "draft" post that's not public yet - does that work? If you load the "public" url in the iframe, isn't that wrong? I see you use the window.location.href as the source, so maybe that's actually showing the draft then? I think I read all the text to say that it's the "public" url here - but it should probably be the "CMS url" then?
I've been staring at this for a while, and I'm not sure. We need a LOT of testing around this, since we've had quite a few problems with getting DOM in the right way in the past - e.g. with missing styles etc.
If you can confirm that this is - in fact - the draft content that will be used for prepublish check, then I guess it's ok but we need a lot of testing and test that the draft state works and that the styles are getting picked up (e.g. by looking at the rerender in the page report or S2).
|
The automated test saying it is good (https://github.com/Siteimprove/CMS-plugin-Wordpress/actions/runs/34602652471). I will do some smoke test on our wp demo server now. |
|
Ran test on this version for: They all worked. @rdom-si - let's merge? |
|
@MortenFriisSiteImprove as soon as someone does approve the PR, i will merge it |
4 participants
Fixes PLIA-4026, raised from CST-5609 (City of Mississauga).
The bug
getDom()built its hidden iframe from the url it was handed, and that url has the Public URL setting applied to it. Mississauga edit oncontrib-dev.cms.mississauga.caand publish todelivery-dev.cms.mississauga.ca, with Public URL set to the delivery host, so the frame was cross-origin andiframe.contentWindow.documentthrewSecurityError.The throw happened inside the
loadlistener and the promise had no reject path, so theawaitnever settled. Prepublish appeared to do nothing and produced no useful console output. The frame was also only removed on success, so the failure left aheight:100vhiframe covering the page, which is why the report reads as "it loads a new page (our delivery version)".Their own evidence rules out the role theory that this ticket chased for months: CST-5609 comment 474447 reproduces it "with a user who is an administrator of the site (but not superadmin)", which is what you would expect from a cross-origin fault.
The change
window.locationrather than the Public URL, using theURLAPI so the nonce survives a fragment, and build the iframe as an element instead of interpolating intoinnerHTML. The Public URL is still what goes tocontentcheck_flatdom, so results land on the crawled URL.try/catcharound the document read, a 30s timeout, frame teardown on both paths, and the click handler clears the spinner and logs instead of leaving a dead overlay.getDom(url)with no nonce and so sentsi_preview_nonce=undefined. Nonce verification failed and the plugin's own scripts were enqueued into the DOM being scraped. That affected every overlay-latest user, not only split-domain ones.urlparameter ofcommon()and its call sites.Verification
Reproduced before fixing. Chrome refused local URLs under a managed-browser policy, so the check runs in Node: it extracts the real
getDomfrom the source file by brace matching, so it cannot drift from a copy, and drives it with an iframe stub that enforces the same-origin policy against whatever src the code actually builds.Not verified here, needs a real instance
There is no test framework in this repo, and Prepublish needs a live API key, site token and both prepublish options enabled. Specifically unproven:
si_preview_noncerenders the draft. The nonce check is session bound and the request is now same-origin so it carries cookies, so it should, but that is reasoning rather than a result. Worth confirming by putting a flaggable element in a draft revision only and checking that Prepublish reports it.registerPrepublishCallbackreacts to a rejected promise, now that rejection is possible. The overlay is not in this repo.Out of scope
The other symptom in CST-5609, where Prepublish only appends
#, is the plugin scripts not loading at all. That is CST-5888 / PLIA-3972, already shipped in 2.1.3. Worth keeping separate so the customer retest does not conflate the two. CST-5888 itself is still open at "Selected for Development" even though its dev ticket is Done, so it probably wants closing rather than developing.