Stop Playwright tracing during E2E login to prevent credential exposure#7191
Stop Playwright tracing during E2E login to prevent credential exposure#7191ryancbahan wants to merge 5 commits intomainfrom
Conversation
Playwright traces capture page.fill() values verbatim, and traces are uploaded as publicly downloadable CI artifacts. This stops tracing before entering credentials and restarts it after login completes, navigating to about:blank before restart so the first snapshot doesn't capture residual form state. Also removes page HTML dump from login error messages since filled input values could appear in the DOM. Ref: shopify/bugbounty#3638393 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous approach (context.tracing.stop()) did not work — Playwright's test runner instruments API calls at a level above context.tracing, so fill() values are logged in traces regardless. Using evaluate() to set input values via the DOM bypasses the Playwright action log entirely, so credentials never appear in trace files or HTML reports. Ref: shopify/bugbounty#3638393 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix prettier formatting for error string concatenation - Avoid HTMLInputElement (not in tsconfig types: ["node"]) by casting through unknown Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Drop explicit Element type annotation (not in node-only tsconfig) - Inline string concatenation to fix no-useless-concat Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dmerand
left a comment
dmerand
left a comment
There was a problem hiding this comment.
I'm no expert in Playwright, but after a pretty extensive web search, the robots found a few things
| */ | ||
| async function fillSensitive(page: Page, selector: string, value: string): Promise<void> { | ||
| const locator = page.locator(selector).first() | ||
| await locator.evaluate((el, val) => { |
There was a problem hiding this comment.
🐛 Bug: Worth noting that this helper now bypasses two reliability guarantees that fill() previously provided: actionability waiting and proven input-state propagation. waitForSelector() only guarantees presence, so evaluate() can run before the field is actually visible or editable, and direct input.value = val assignment can be ignored by framework-managed inputs that expect the native setter path.
Suggestion: Consider hardening fillSensitive() so it preserves the important behavior that fill() used to provide: wait for the field to be actionable before writing, and set the value through the native input setter before dispatching events. If that is too brittle, another tested non-logging approach may be safer than raw DOM assignment.
There was a problem hiding this comment.
i think we should leave this for now and see if there's flakiness. I don't want to add waits unless we have to.
| await page.waitForSelector('input[name="account[password]"], input[type="password"]', {timeout: 60_000}) | ||
| await page.locator('input[name="account[password]"], input[type="password"]').first().fill(password) | ||
| await fillSensitive(page, 'input[name="account[password]"], input[type="password"]', password) | ||
| await page.locator('button[type="submit"]').first().click() |
There was a problem hiding this comment.
🔒 Security: Worth reviewing whether this change actually delivers the intended credential protection. Replacing fill() with evaluate() avoids Playwright's action log, but this diff also removes the earlier tracing pause/resume flow. The next traced actions still happen on the login page after the password field has been populated, so CI trace snapshots can still capture the filled form.
Suggestion: Consider restoring a trace-suppression strategy around credential entry rather than relying on DOM writes alone. A practical approach is to stop or pause trace capture before the first credential field is populated, then resume only after navigation has moved away from the login form or after explicitly loading about:blank.
There was a problem hiding this comment.
The trace suppression didn't actually work -- it didn't filter the results report, which is why I removed it. I did verify the output logs and result report and this approach does work.
Navigate to about:blank in the catch block before rethrowing so that failure artifacts (screenshots, trace snapshots) do not capture the login form with credentials still populated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
about:blankbefore restarting tracing so the first snapshot doesn't capture residual form stateRef: shopify/bugbounty#3638393
Test plan
playwright-reportandplaywright-resultsartifacts after CI runs[e2e] Tracing paused for credential entryand[e2e] Tracing resumed after credential entryappear in CI logs