Repository navigation
test(playground): the gate failed on the user agent, not on headless - #315
Merged
Merged
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 40 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. WalkthroughIl PR aggiorna il test end-to-end di compatibilità con Playground e le istruzioni per eseguirlo. I test di lifecycle salvano e ripristinano alcune opzioni GVL/GCM. ChangesCompatibilità con Playground
Ripristino delle opzioni nei test lifecycle
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Merge Risk: ⚪ Minimal · up to The changes update E2E test setup and cleanup plus release instructions. The investigated state-restoration concern does not occur, and the reported test gates passed; no merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two rough edges worth a look inline.
Reviewed changes
Reviewed the single commit: the Playground gate's user-agent fix, the GVL/GCM backup around the destructive lifecycle tests, and the release script's closing checklist.
- Real-version user agent for Playground —
playground-compat.spec.tsopens its own context with a UA built frombrowser.version(), so the bot filter no longer sees the staleChrome/145fromdevices['Desktop Chrome']. - Named bot-wall failure — the polling loop now detects the "confirm you are human" interstitial and fails with a message that clears the plugin, instead of the misleading
no frame ever rendered #wpadminbar. - GVL / GCM survive the uninstall test —
plugin-lifecycle.spec.tscopies the five non-rebuildable options toe2ebak_*inbeforeAlland restores any that are missing inafterAll. I checked that nothing in the describe recreatesfaz_gcm_settingsbefore the restore:Gcm::load_gcm_default()only runs when admin modules load, and underwp evalthey wait forrest_api_init. - Release checklist —
publish-release.shdrops the routine SVN password rotation and prints the Playground command instead.
ℹ️ Nitpicks
tests/e2e/specs/playground-compat.spec.ts:130-132— the comment says Playwright applies the device UA "when it LAUNCHES the browser". In Playwright 1.58 the cause is different: the test runner'srunBeforeCreateBrowserContexthook copies everyuseoption into anybrowser.newContext()call that does not set that key. So the observation is right but the explanation is wrong. That also makes "Everything else inuseis irrelevant" misleading: this context still inheritsbaseURL,viewport, video, etc. The explicitignoreHTTPSErrors: trueis redundant too, because the context would inherit it anyway.
claude-opus-5-5 ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (7)
tests/e2e/specs/plugin-lifecycle.spec.ts (3)
318-318: LGTM!
337-349: LGTM!
421-437: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Il ripristino ha un caso limite non coperto:
falsecome valore valido.Il controllo
false === get_option( $name, false )tratta come "assente" anche un'opzione esistente con valorefalse. In questo caso il teardown sovrascrive il valore con la copia di backup. Questo caso è improbabile per le cinque opzioni salvate, perché sono array o dati complessi. Se un test impostafaz_gcm_settingsafalse, il valore viene perso.Usa un valore sentinella unico per distinguere "assente" da
false.Correzione proposta
- if ( null !== $backup && false === get_option( $name, false ) ) { + $missing = new stdClass(); + if ( null !== $backup && $missing === get_option( $name, $missing ) ) {tests/e2e/specs/playground-compat.spec.ts (3)
186-200: LGTM!
264-266: LGTM!
138-138: 🎯 Functional CorrectnessLa stringa dichiara
Macintoshanche quando il browser gira su un altro sistema operativo, ma il commento non dimostra che Playground blocchi richieste per questa discrepanza. Inoltre, il test è opt-in e i workflow ispezionati non lo eseguono su Ubuntu. L’eventuale rifiuto da parte del filtro anti-bot resta quindi un’ipotesi, non un difetto supportato dalle evidenze disponibili.scripts/publish-release.sh (1)
245-253: LGTM!
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: fabiodalez-dev/FAZ-Cookie-Manager/.coderabbit.yaml
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0dcd46fd-6067-4b1d-8f05-9f152e1e907d
📒 Files selected for processing (3)
scripts/publish-release.shtests/e2e/specs/playground-compat.spec.tstests/e2e/specs/plugin-lifecycle.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
There was a problem hiding this comment.
✅ No new issues found. Both earlier inline points are fixed.
Reviewed changes
This run covers the one commit added since the previous Pullfrog review.
- Chromium-only UA rebuild —
playground-compat.spec.tsnow builds theChrome/${browser.version()}string only whenbrowserName === 'chromium'. Other engines get an explicituserAgent: undefined. I checked this against Playwright 1.58'srunBeforeCreateBrowserContext. It only copies auseoption when that key is absent (!(key in options)), so Firefox and WebKit send their own native UA rather than a fake Chrome one. - Playground command in the release script —
publish-release.shnow addscd ${PLUGIN_SRC} &&before the command. It also says the local e2e stack must be up, becauseglobalSetuplogs in before any browser opens.
claude-opus-5-5 ｜ 𝕏
Two review findings that were left open on PRs already merged and shipped in 1.34.0. Both are valid against the current code. strip_non_markup() found the end of a <script>, <style>, <title>, <textarea> and friends with a plain substring search for "</" plus the name. That also matches a longer name: `</titlex>` closes <title>, `</stylex>` closes <style>. Everything after the false end is then handed back as live markup, so a marker sitting in inert text — text a browser renders as the title, never as an element — passes verification. The footer route is believed, this plugin stops rendering its own revisit widget, and the visitor is left with no way to withdraw consent. That is fail-open, in the one class whose stated contract is to fail closed, and it is the same outcome the whole verification exists to prevent. HTML5 ends a raw-text element only when the name is followed by whitespace, `/` or `>`. The match is now anchored to that boundary. Four cases cover it, one per raw-text element. Each one puts a REAL element carrying the marker after the fake closing tag, and that detail is the point: my first draft left the marker in loose text, where the DOM parser finds no attribute and the assertion passes whatever strip_non_markup() does — green and worthless. Control run against the old substring search: all four go red, and the case asserting that a genuine closing tag still ends the region stays green, so the fix cannot have made the region unbounded instead. Second finding, in the compact-layout test: `strpos( $dns_body, 'width:100%' )` also matches `min-width:100%` and `max-width:100%`, neither of which makes the Do-Not-Sell row span the wrapper — so the assertion could not catch the regression it was written for. It is anchored to a declaration boundary now. Demonstrated: against a body carrying `min-width:100%` the old check passes and the new one fails. Verified: 198 of 198 unit suites, 97/97 in the withdrawal runner and 97/97 in the compact-CSS runner, php -l clean, and the two E2E specs that exercise these paths green (withdrawal-path-verification 2 passed, mobile-compact-layout 6 passed).
…tion Both reviewers found the same hole independently, and they are right. PCRE's `\s` matches the vertical tab; HTML's whitespace set does not. So `</title\x0B>` closed the raw-text region in my fix while a browser keeps reading title text — the same fail-open the change set out to close, reached with a less likely input. The delimiter set is now written out as the spec defines it: tab, LF, FF, CR, space, `/`, `>`. A case with \x0B joins the false closers, and the control run confirms it: back on `\s` that case goes red. The CSS assertion was only bounded at the front, so `width:100%junk` satisfied it — a value a browser discards, meaning the test could pass on a rule that sets no usable width. It now requires `;` or end of string after the value. Demonstrated: against `width:100%junk` the unbounded regex passes and the bounded one fails.
The Playground gate had to be run by hand for two releases because playground-compat.spec.ts failed with "no frame ever rendered #wpadminbar". The note I had written said it needed --headed. That was a correlation, and this release disproved it: it failed in --headed too, twice. The cause is the user agent. playwright.config.ts applies devices['Desktop Chrome'], which pins a FIXED string: Playwright 1.5x names Chrome 145 while the Chromium it ships reports 148, and the bot filter in front of playground.wordpress.net reads that as a stale or forged Chrome. Measured on one machine, same URL, seconds apart, and repeated with the order reversed so the result cannot be an ordering effect: real UA (HeadlessChrome/148.0.7778.96) -> no interstitial devices['Desktop Chrome'] (Chrome/145.0.7632) -> interstitial, every time Headless is irrelevant: both modes pass with the real UA and both fail with the pinned one. The test now opens its own context and rebuilds the user agent around browser.version(), stating the version actually running. That override has to be explicit: Playwright applies the device user agent when it LAUNCHES the browser, so a plain browser.newContext() inherits Chrome/145 as well — verified by printing navigator.userAgent from both. The gate now passes unattended and headless in about 12 seconds. Proven both ways: removing the override turns it red, restoring it turns it green. When the wall does appear, the test now names it instead of implicating the plugin — "wp.org served a bot interstitial, so this run says NOTHING about the plugin". The check sits inside the polling loop rather than straight after the goto, because the interstitial is painted after DOMContentLoaded and an early check sees an empty page; also verified in both directions. Second fix, same theme: the suite was sabotaging its own next run. plugin-lifecycle runs uninstall.php with remove_data_on_uninstall=true, which deletes every faz_* option — including the Global Vendor List and the GCM settings, which Activator::install() cannot rebuild because one is a network download and the other defaults to off. The next full run then started without a GVL and gcm-tcf skipped itself with "the Global Vendor List is not downloaded on this site": a skip the suite inflicted on itself, which reads like a missing prerequisite and costs a manual recovery. It now copies those options aside before the uninstall and restores any that are missing afterwards. The copies use an `e2ebak_` prefix on purpose, since uninstall.php sweeps `faz_%` and a backup inside the blast radius is not a backup; the values never leave the database, because the GVL is megabytes and would not survive wp eval's argv. Control run: with the restore disabled the GVL is gone after the spec, with it in place the GVL survives and no e2ebak_ row is left behind. Last, publish-release.sh no longer tells me to rotate the SVN Application Password after every release. The commit goes through the macOS Keychain: no password in argv, in the environment or in any log, so nothing was exposed and there is nothing to sanitise. release.md §6a has carried the evidence for that since 1.33.0 and only the script's closing message had not caught up. It now prints the Playground command instead, which is a step that still needs running.
…eeds a stack Both findings are correct. The user-agent override was unconditional, and playwright.config.ts honours FAZ_E2E_BROWSERS. Under the firefox or webkit project browser.version() returns a Gecko or WebKit version, and pouring that into a "Chrome/…" string would have forged exactly the kind of user agent this change exists to stop sending — the fix would have reintroduced the defect on two engines out of three. The override is now Chromium-only. The explicit `undefined` on the other engines is not a no-op: it still overrides the project's device user agent, so firefox and webkit send their own native one. The printed Playground command claimed to be unattended but is not self-contained: the config's globalSetup logs into the local test site and asserts its prerequisites before any browser opens, so the local stack has to be up, and `-c tests/e2e/playwright.config.ts` is relative to the repo. Run from elsewhere, or with nginx down, it fails at the login step with an error that reads like Playground's fault — the same misdiagnosis this PR is about. The line now cds into the plugin directory and states the prerequisite. Verified: the Playground gate still passes headless, 1 passed in 13.4s.
fabiodalez-dev
force-pushed
the
test/playground-gate-user-agent
branch
from
October 7, 2026 12:56
f0d696e to
8cf546a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Playground gate had to be run by hand for two releases because
playground-compat.spec.tsfailed withno frame ever rendered #wpadminbar. My note said it needed--headed. That was a correlation, and 1.34.0 disproved it: it failed in--headedtoo, twice.The cause is the user agent
playwright.config.tsappliesdevices['Desktop Chrome'], which pins a FIXED string: Playwright 1.5x names Chrome 145 while the Chromium it ships reports 148, and the bot filter in front of playground.wordpress.net reads that as a stale or forged Chrome. Measured on one machine, same URL, seconds apart, then repeated with the order reversed so the result cannot be an ordering or rate-limit effect:HeadlessChrome/148.0.7778.96)devices['Desktop Chrome'](Chrome/145.0.7632.6)Headless is irrelevant: both modes pass with the real UA and both fail with the pinned one. It is the same shape as the Imunify360 WAF false positive, where the threshold was also
Chrome/145.The test now opens its own context and rebuilds the user agent around
browser.version(), stating the version actually running. That override has to be explicit: Playwright applies the device user agent when it LAUNCHES the browser, so a plainbrowser.newContext()inherits Chrome/145 as well — verified by printingnavigator.userAgentfrom both.The gate now passes unattended and headless in about 12 seconds. Proven both ways: removing the override turns it red, restoring it turns it green.
When the wall does appear, the test names it instead of implicating the plugin — "wp.org served a bot interstitial, so this run says NOTHING about the plugin". The check sits inside the polling loop rather than straight after the
goto, because the interstitial is painted afterDOMContentLoadedand an early check sees an empty page. Also verified in both directions.The suite was sabotaging its own next run
plugin-lifecyclerunsuninstall.phpwithremove_data_on_uninstall=true, which deletes everyfaz_*option — including the Global Vendor List and the GCM settings, whichActivator::install()cannot rebuild because one is a network download and the other defaults to off. The next full run then started without a GVL andgcm-tcfskipped itself with "the Global Vendor List is not downloaded on this site": a skip the suite inflicted on itself, which reads like a missing prerequisite and costs a manual recovery.It now copies those options aside before the uninstall and restores any that are missing afterwards. The copies use an
e2ebak_prefix on purpose, sinceuninstall.phpsweepsfaz_%and a backup inside the blast radius is not a backup; the values never leave the database, because the GVL is megabytes and would not survivewp eval's argv.Control run: with the restore disabled the GVL is gone after the spec; with it in place the GVL survives and no
e2ebak_row is left behind.No more routine SVN password rotation
publish-release.shno longer tells me to rotate the SVN Application Password after every release. The commit goes through the macOS Keychain: no password in argv, in the environment or in any log, so nothing was exposed and there is nothing to sanitise.release.md§6a has carried the evidence for that since 1.33.0; only the script's closing message had not caught up. It now prints the Playground command instead, which is a step that still needs running.Scope
Test infrastructure and release scripts only. No plugin file changes, so the published 1.34.0 is unaffected and does not need re-releasing.
Verification
Each spec run the way the batch runner runs it, one spec per process:
playground-compatplugin-lifecyclegcm-tcfSummary by CodeRabbit
Test
Documentazione