Repository navigation
fix(security): strip OAuth query params from chooser error messages - #777
Merged
Merged
Conversation
Found while live-verifying v0.73.0's release gate, in code this release did not
touch. `client._handle_account_chooser` interpolates `page.url` verbatim at three
raise sites, and Google's auth URLs carry `state`, `code_challenge`, `client_id`
and challenge tokens. That text is the artifact users are asked to paste into a
GitHub issue.
Measured on the real path, not theorised. `gflow image t2i --profile <name>`
against a profile Google had put behind a password challenge:
BEFORE exit 38, 5 secret matches -
accounts.google.com/v3/signin/challenge/pwd?TL=ACv9tzFkh8ZJ...
plus the OAuth state and client_id
AFTER exit 38, 0 secret matches, same landing still named
`safe_page_url()` joins `flow_host_kind` / `flow_landing_kind` in `_common.py` and
keeps scheme+host+path. All FOUR raise sites route through it - the three in
client.py and `raise_if_known_landing`, which had been stripping inline. One
helper rather than four copies: the council flagged this class of leak in the new
code (D3) and it applied identically to the raise site next door, which is the
tell that it belonged in a shared function.
The landing stays in the message. Knowing WHERE the session stopped is the whole
value of the diagnosis; only the credentials are gone.
Regression test asserts both halves - the URL is still named AND none of
client_id / code_challenge / state= / TL= survive - so a future edit that
re-inlines the URL goes red instead of quietly leaking again.
Verified: ruff, format, pyright 0 errors; 8 chooser tests, 86 in the affected
modules; the live A/B above.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesAuthentication URL sanitization
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to The change removes OAuth query and fragment values from authentication errors while preserving useful page context. Covered chooser and edge cases leave no actionable merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Zero issues; the gate failed purely on coverage of lines this PR added or changed. Two real gaps, both legitimate: - `safe_page_url` edge branches. I had verified None / "" / non-URL / "https://[bad" by running them in a terminal and reading the output. That is evidence for one person once; it is not a test, so nothing re-checks it and the lines were exactly as untested as the gate said. Every branch is pinned now. - `client.py`'s "no account is recorded in this profile" raise had NO test at all — every chooser test writes an ACCOUNT_FILE first, which is how the line stayed uncovered while the two raise sites beside it were exercised. It is a real path: a profile authenticated before .gflow_account existed, or one whose file was removed, has nothing to auto-select with. No coverage exclusions widened, no assertions weakened — the skill forbids both, and either would turn the gate green while leaving the lines untested.
6 tasks done
ffroliva
added a commit
that referenced
this pull request
Sep 10, 2026
* spike(776): measure whether the migrated settings trigger is ever visible-but-unclickable
0/3 on the overlay hypothesis — body{pointer-events:none} never occurred and the
click landed in 65-130ms every run, so this settles nothing about #776 and is
recorded as unmeasured, not as transience.
Three things it did measure:
- _dismiss_dialog runs before Angular mounts the composer, 3/3 (trigger appears
1479-3143ms after domcontentloaded, the check fires at ~0ms). The miss is
structural, not flaky.
- a healthy click on this control costs 65-130ms against a 5000ms budget, so
#776's expiry is a control that never became actionable.
- the locale-settle error the reporter asked about reproduced on all 3 runs while
the click still landed - it is #643, and it is not sufficient to cause #776.
Rung 1 also found #593's pointer-events measurement was taken on labs.google;
migrated_composer.py:744 calling the migrated dialog '#593's twin' is asserted,
not measured.
Refs #776
* fix(migrated): say what was true when a click never lands (#776)
video r2v on flow.google.com reached migrated.editor_ready and died 5.039s later
as a bare Playwright TimeoutError - exit 1, no locator, no cause, no MP4. By
elimination that is _open_pane's trigger.click(timeout=5000): the wait_for(visible)
one line above IS guarded and would have raised exit 23, so the control was visible
and the click expired.
#752 finding #7 predicted this at this exact function before #776 was filed. Its
count()->visibility half was fixed; the click half was not, leaving a comment that
describes the failure the next line went on producing.
It reads, it does not diagnose. Two causes were live and neither could be measured:
Flow's announcement overlay (#593, measured on labs.google, never on this host -
0/3 in today's spike) and a mid-run agent-mode flip. A guard built on either would
answer confidently and be wrong half the time (#770 is the precedent). So on a
timeout the driver reads Playwright's four actionability conditions back and reports
the ones that fired; when every reading is healthy it says so, which eliminates
three and leaves 'stable' rather than inventing a fourth.
- costs nothing when healthy: the read runs only in the except branch, the rule
raise_if_known_landing already states
- MCP gains more than the CLI: a non-GFlowError on the queued path shipped
'detail: sha256:...'; the typed error routes it to the Problem Details branch
- four sites with a named reason each, not all nineteen
- the occluder report is a closed allowlist (tag + <=3 framework class tokens);
typing the error moves the text from hashed telemetry to a message printed raw,
and a signed-in Flow page carries the account email on exactly those elements
- retryable unchanged: preserved, not measured
Closes #776
* test+docs(776): cover every reading offline, and fix the docstring the council caught
Council D9 found a Blocker I had put on my own predict list and then dropped:
UiSelectorDriftError's docstring still said 'finds no matching element', which the
new raise site makes false - it now also fires when the selector DOES match and the
element will not take the interaction.
- errors.py: the class covers two shapes, and the detail has to say which
- KNOWN_ISSUES: the lookup table documented 6 of the 8 messages the code can emit;
the missing two were 'not rendered' and 'answers no hit test'
- E2E_TESTING: a second worked example, chosen because it fails DIFFERENTLY from the
first - a client-side redirect vs Playwright's actionability gate
- tests: one offline case per reading. CI's coverage run excludes -m e2e, so a branch
proven only in the browser reads as dead code to Sonar's new-code gate - the exact
way PR #777 went red at 70%
- tests/worker: the MCP twin A/B - a bare TimeoutError reaches an agent as
'detail: sha256:...', the typed one as problem details with exit 23
Refs #776
* refactor(776): cut the prose the council measured, and merge one fact told twice
Council D14 measured 64.5% of this change's added lines as non-code prose, against
the 35% issue #759 already named as a problem in this same file. Fair hit - three of
the passages were the CHANGELOG's content retold at the call site, and one was an
exact duplicate of the comment 80 lines above it.
- _click's docstring: the #593/#752/#770 archaeology is the CHANGELOG's job
- the redaction comment above the redact_sensitive_text return restated the
module-level one verbatim
- the _open_pane call site told the #752 story a third time
- SUBMIT_BUTTON's three-line comment for a one-line constant
D1 also found a real over-report: the JS only hit-tests when the box has a nonzero
width and height, so an unrendered element ALWAYS comes back hit_testable=false and
occluder=null. Two independent ifs then said "it is not rendered" and "it answers no
hit test" about one fact. Now one chain for "can a pointer reach it", ordered most
specific first, with enabled-ness and a page-wide block kept as the separate axes
they are - chaining those would have hidden a disabled control behind whatever
covered it.
Two tests added for the invariants the real JS enforces, which the earlier
parametrized cases violated by holding hit_testable=True on an unrendered element.
Refs #776
* docs(776): correct a truncation claim that was wrong, and flag a fake that no longer works
Council D4 caught a false statement in my own docstring: it said the queued MCP path
raw-slices detail to 500 chars "while the CLI path does not". It does. redact_sensitive_text
IS redact_error_detail, which truncates at the raise site, so every surface sees the same
cap. Locator-first ordering is still right, for the simpler reason that the cut happens at
the source - fixed in both the method docstring and the test that repeated it.
Also:
- tests/worker: _fail_r2v_with enqueues a t2v task. The name described the reporter's
command, not the fixture.
- tests/api/transports/test_migrated_composer.py defines a local PlaywrightTimeoutError
that shadows the real class for that whole 2000-line file. Since #776 the driver
catches the REAL one, so a future click-timeout test written with that file's dominant
fake would silently exercise nothing and say so nowhere. Documented at the definition,
pointing at the file that does it correctly.
Refs #776
This was referenced Sep 10, 2026
Merged
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
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.
Found while running v0.73.0's release gate — in code this release did not touch.
The leak
client._handle_account_chooserinterpolatespage.urlverbatim at three raise sites. Google's auth URLs carrystate,code_challenge,client_idand challenge tokens (TL=…), and that error text is the artifact users are asked to paste into a GitHub issue.Measured on the real path, not theorised
gflow image t2i --profile <name>, against a profile Google had put behind a password challenge:Same command, same profile, before and after.
The fix
safe_page_url()joinsflow_host_kind/flow_landing_kindin_common.pyand keeps scheme+host+path. All four raise sites route through it — the three inclient.pyplusraise_if_known_landing, which had been stripping inline.One helper rather than four copies. The council flagged this leak class in the new code (D3, #775) and it applied identically to the raise site next door — that's the tell it belonged in a shared function, not a per-site strip.
The landing stays in the message. Knowing where the session stopped is the entire value of the diagnosis; only the credentials are gone.
Why not a follow-up issue
It's pre-existing (#764's raise sites), so "file it separately" would normally apply. Two things override that:
stateandTL=…would be incoherent, and would mean D3's finding got applied to my code only because I happened to write that line.Test plan
client_id/code_challenge/state=/TL=survive, so a future edit that re-inlines the URL goes red instead of quietly leakingsafe_page_urlverified on edge cases: query+fragment stripped, plain URL untouched,None/""/ non-URL /https://[badall total-by-constructionuv run ruff check·ruff format --check·uv run pyright src→ 0 errorsBlocks the v0.73.0 release until merged.
Summary by CodeRabbit
Bug Fixes
Tests