Repository navigation
[No QA] Split the QA auth callback into capture and exchange (part 3/5) - #100730
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
44e9bca to
bbeaf6a
Compare
bbeaf6a to
f10d067
Compare
|
@codex review |
|
@MelvinBot review |
|
@MelvinBot find regressions |
ReviewThe capture/exchange split looks correct, and CI is green. I found one behavior gap worth fixing before merge, plus two small items. 1. A failed boot-time code exchange is now invisible, and Run loops back to Cloudflare When
With Access SSO, this is a silent loop: the tab bounces straight back, and the failure appears only in the log. The description says a press "joins it and reports the failure." That holds only if the user presses Run within the exchange's round trip. A misconfigured Suggested fix: Keep the last exchange error in 2. Minor: signing out during a refresh now gives an HTTP 401 instead of Before this PR, signing out bumped 3. Nits
What I checked and found sound
|
|
|
|
Replying to this comment. Fixed in |
|
@MelvinBot review |
Re-review after
|
|
@MelvinBot review, the PR description has been changed too |
Re-review after
|
|
@MelvinBot review this |
Review of
|
| return {outcome: 'no-pending-flow', errorMessage: 'No pending QA auth flow in this tab. Start the sign-in again'}; | ||
| } | ||
|
|
||
| if (params.get('state') !== flow.state) { |
There was a problem hiding this comment.
Screen.Recording.2026-10-02.at.12.49.22.mov
@sharabai After my first successful login, I cleared the session. Then I ran the probe once more and clicked Allow, and this time it showed OAuth callback state mismatch, as in the video above. I think the problem is in how the flow state is handled in storage. Can you help me investigate this?
There was a problem hiding this comment.
@huult I'll get back to you later today.
|
Replying to this comment. Fixed in |
@huult, great find, thanks for digging into this! Going through your steps as written, I couldn't get it to happen, but with some less typical back-and-forth between the App and Cloudflare I did hit the same error. Your find also pointed me at a few things in this PR worth improving, and I'm working on them now. |
Screen.Recording.2026-10-06.at.11.05.40.mov@sharabai You can see in my video that I’m still able to reproduce it on my side. |
|
Tag me when it's ready for another look. |
Screen.Recording.2026-10-08.at.20.21.03.movIt works. |
|
@sharabai Could you check the failed Jest tests? |
|
Yeah, this test is failing because of main. Please sync with main to fix it. |
8bf1b32 to
d8295db
Compare
|
@huult luck's on our side this time. It passed 🎊 |
|
@huult a quick update on the I reported it to Chromium: https://issues.chromium.org/issues/571157144 The fix here doesn't wait on Chrome: the pending sign-in now lives in |
|
🚧 iwiznia has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/iwiznia in version: 9.5.6-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The boot-time callback handling splits in two.
captureAuthCallbackURLreads the authorization code offwindow.location, consumes the pending flow record itsstatenames, and rewrites the URL back to where the roundtrip started;
finishSignInFromURLspends the code capture approved. Capture has to run before anything canresolve a route from the callback path, which no app route serves, and the exchange cannot run until Onyx is
initialized to persist its session.
index.jsimportssrc/setup/captureCloudflareAuthCallbackURLfor its sideeffect rather than calling it, because a statement in
index.jsruns only after every import in it has beenevaluated, which is already too late.
The bearer allowlist becomes a list:
isQAServerRequesttests membership in every configured QA origin,while
getQAResourcestays the single RFC 8707 resource indicator thetoken is bound to, since authorize and exchange must send byte identical values.
QA_SECURE_EXPENSIFY_URL,shipped unread in part 1, is the second entry, and a malformed one disables the feature rather than dropping
out: a half populated allowlist would send the
shouldUseSecurecommands out bearer-less into anunrecoverable 401. One token covers both hosts only while they stay on a single multi-domain Access
application.
CHECK_PATHleavesisQAAuthConfigured(), being the probe's business rather than thefeature's, and the probe checks it itself: an empty one would otherwise POST at the bare API root.
refreshCloudflareSessionnow requires the token it refreshes from, so no caller can skip the rotation-racecheck, and a rotation whose Onyx write fails resolves
refreshedanyway, the old token being spent and thecached pair the only usable credential. The
exchange-failedoutcome is gone, and a callback that passesevery check records
code-captured, which says only that capture handed the code on. A rejected exchangereaches the log, as does every refused callback with its outcome and reason. Run probe also reports a
rejection before it redirects, so a failing setup cannot loop the tab through Cloudflare unseen. Rows that
mount after the rejection show it at once, so their first press starts a fresh round trip. Otherwise the
first press to learn of it reports it, joining the exchange if it is still in flight, and the press after
that starts the round trip. Clear session and sign-out both drop the stored rejection, and a rejection
arriving after either is not recorded.
Important
The pending flow record lives in
localStorage, one record per OAuthstate. Chrome can hand thecallback page an earlier page's
sessionStoragecopy, which failed the round trip withOAuth callback state mismatch. Each record is single-use, expires after ten minutes, is swept on boot, andis cleared by sign-out and Clear session. Pressing Allow on an older Authorize screen that has not expired
now completes the sign-in.
Fixed Issues
$ #91419
PROPOSAL:
Tests
Preconditions:
.env, set up by followingthe setup guide on #98433 with
these changes:
https://dev.new.expensify.com:8082wherever the guide says<your-dev-origin>: the Worker'sAccess-Control-Allow-Origin, the allowed redirect URI, and the client registration.QA_SECURE_EXPENSIFY_URL=with an empty value. The setup has one host.npm run webafter every.envedit: dotenv is read once at startup.Ctrl+D elsewhere. They render a "QA auth (Cloudflare)" row with a Run probe button, a "QA auth session" row with a Clear
session button, and a status line below both once there is a result.
Setting those values also makes a QA row appear in Settings > Troubleshoot > Server. Leave it alone: the
sign-out on a QA crossing and the bearer on the request path both arrive in part 4, so selecting QA here
sends unauthenticated requests to the QA origin.
A build with no QA auth configured, a blank check path and a rejected code exchange are left to the unit
suites, which drive each of those branches directly.
A full round trip produces a session
navigates to the Cloudflare Access login page on your team domain.
to Settings > Troubleshoot and the URL bar reads
/settings/troubleshoot, not/oauth/callback."Probe succeeded (authenticatedVia: )" followed by a timestamp. The value is whatever the Worker
echoes, and
nullwhen it sends none.trip to Cloudflare.
A callback URL is rewritten even when it is refused
https://dev.new.expensify.com:8082/oauth/callback?code=fake-code&state=fake-statein the URL bar./oauth/callback, and that the appshows the screen you were last on, Settings > Troubleshoot, rather than a not-found page.
by
(No pending QA auth flow matches this callback. Start the sign-in again). A reload clears theresult.
A malformed secure root switches the whole feature off
QA_SECURE_EXPENSIFY_URL=http://qa-secure.example.com/and restart the dev server.absent.
QA_SECURE_EXPENSIFY_URL=to an empty value, restart, and verify that both QA rows are back andthe Server list offers QA again.
Sign-out drops the Cloudflare credential
reporting "Probe succeeded", since the session did not survive the sign-out.
The bearer never reaches the secure origin on this branch: the probe fires only at the primary API root, and
nothing attaches the bearer to an app request until part 5, so the allowlist cases in
CloudflareAccessTestare the only proof of that half.Offline tests
N/A: every path in this PR is an OAuth round trip against Cloudflare's edge, which has no offline behaviour
to specify. The one persisted key,
CLOUDFLARE_SESSION, is written throughOnyx.setwith no optimisticdata and no queued write, and the app has no consumer of it yet.
QA Steps
N/A: the only observable surface is the QA auth test tool, which renders solely when a Cloudflare Access
application and its
.envvalues are configured. Neither staging nor production carries them, sonothing here is reachable on a build QA runs. Nothing user-facing changes on either environment.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A: QA auth is hard-off on native.
Config/index.native.ts,captureAuthCallbackURL/index.native.tsandfinishSignInFromURL/index.native.tsare stubs, so there is nothing to render.Android: mWeb Chrome
N/A: the flow navigates the whole tab to Cloudflare and back, and the redirect URI is bound to the dev
origin, so a mobile browser cannot receive the callback.
iOS: Native
N/A: same stubs as Android native.
iOS: mWeb Safari
N/A: same redirect URI binding as Android mWeb.
MacOS: Chrome / Safari
Test 1: A full round trip produces a session
pr-100730-test-1-full-round-trip-produces-session.mp4
Test 2: A callback URL is rewritten even when it is refused
pr-100730-test-2-refused-callback-url-is-rewritten.mp4
Test 3: A malformed secure root switches the whole feature off, with
QA_SECURE_EXPENSIFY_URL=http://qa-secure.example.com/pr-100730-test-3-malformed-secure-root-switches-feature-off-part-1.mp4
Test 3, continued, with
QA_SECURE_EXPENSIFY_URL=empty againpr-100730-test-3-malformed-secure-root-switches-feature-off-part-2.mp4
Test 4: Sign-out drops the Cloudflare credential
pr-100730-test-4-sign-out-drops-cloudflare-credential.mp4