feat(steam-link): serve /link, and expire the pending-link stash - #4880
Open
Celant wants to merge 6 commits into
Open
feat(steam-link): serve /link, and expire the pending-link stash#4880Celant wants to merge 6 commits into
Celant wants to merge 6 commits into
Conversation
…nginx devGameHtmlBypass is only wired as the bypass: option on the /w0 and /w1 proxy entries, so it's never invoked for a bare /link request -- the /link branch added in the previous commit was dead code. Vite's dev server already falls back to serving index.html for unmatched paths, so /link gets 200 in dev with no change needed; the earlier curl check couldn't tell the difference (a spot-check on made-up paths later confirmed the same 200, which is what exposed this). Reverting devGameHtmlBypass to its pre-existing state avoids config that implies behavior it doesn't have. Also add location = /link/ alongside the existing exact-match /link redirect: `location =` doesn't match a trailing slash, and this URL exists specifically to survive being hand-typed (or autocompleted) into an address bar on an unfamiliar device.
The value is duplicated across three deployables (server, this client, the desktop gate's gateFlow.js) with no shared source to import. The comment explained why 10 minutes matters but not that changing it means changing three files -- which is how the gate's own startup deadline got mis-sized twice on this feature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRMRzHbZp2VhxbxqDMw4Zr
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds production and development ChangesSteam-link flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
A dev-only stand-in for nginx's `location = /link` blocks, in the same spirit as the existing random-worker-create-proxy plugin. Without it the dev server falls through to Vite's SPA fallback and serves the home page for /link -- a 200, so it does not look broken, but the URL the desktop app prints silently would not work locally. Redirecting (rather than serving index.html directly) keeps dev and production identical, so siteUrlForAudience emits one URL shape for every environment instead of special-casing localhost. Matches on `req.originalUrl`, not `req.url`, and that is load-bearing: Vite's SPA fallback runs before this middleware and has already rewritten `req.url` to "/index.html". A check against `req.url` compiles, runs, and silently never matches. Found by logging both. Verified against a running dev server: /link and /link/ both 302 to /#steam-link; /linkxyz and / still 200, so the match is exact rather than a prefix swallowing everything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRMRzHbZp2VhxbxqDMw4Zr
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vite.config.ts (1)
71-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the middleware-order explanation.
Vite runs
configureServermiddleware before its internal middleware. The SPA fallback does not normally rewritereq.urlbefore this handler. Update this comment to describe the actual dependency. KeeporiginalUrlonly if an earlier repository middleware rewritesreq.url. (main.vite.dev)Suggested comment correction
- // Vite's SPA fallback runs BEFORE this middleware ... - // rewritten req.url to "/index.html" ... + // Use originalUrl if an earlier development middleware rewrites req.url.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vite.config.ts` around lines 71 - 78, Correct the middleware-order explanation in the comment above the URL check: remove the claim that Vite’s SPA fallback runs before this handler and rewrites req.url. Retain the use of originalUrl only if an earlier repository middleware actually performs that rewrite; otherwise update the surrounding implementation and comment to use the appropriate request URL.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vite.config.ts`:
- Around line 82-85: Update the Node middleware’s exact-route check around
URL.pathname to normalize the incoming URI using the same percent-encoded path
rules as Nginx before comparing against "/link" and "/link/". Ensure encoded
variants such as "/link%2F" resolve to the corresponding normalized route while
preserving the existing next() behavior for non-matching paths.
---
Nitpick comments:
In `@vite.config.ts`:
- Around line 71-78: Correct the middleware-order explanation in the comment
above the URL check: remove the claim that Vite’s SPA fallback runs before this
handler and rewrites req.url. Retain the use of originalUrl only if an earlier
repository middleware actually performs that rewrite; otherwise update the
surrounding implementation and comment to use the appropriate request URL.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 876b93e5-6b1b-4203-8812-999ce7235c80
📒 Files selected for processing (1)
vite.config.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Two points from review. nginx resolves percent-encoded bytes before exact `location =` matching, but URL.pathname does not. So /link%2F redirected in production and fell straight through in dev -- the exact dev/prod divergence this plugin exists to remove. Decode before comparing, and treat malformed encoding as not-our-route rather than throwing. Also corrected the comment above it. It asserted that Vite's SPA fallback is what rewrites req.url before this handler runs. The rewrite is real and measured (url="/index.html", originalUrl="/link"), but which middleware performs it was never established -- that attribution was inference presented as fact. The comment now states only what was observed, and keeps the warning that matters: switching to req.url type-checks, lints, runs, and silently never matches. Verified against a running dev server: /link, /link/ and /link%2F all 302 to /#steam-link; /linkxyz, / and /link%2Fextra all still 200. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRMRzHbZp2VhxbxqDMw4Zr
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.
Two small follow-ups to #4849 (Steam account linking), for the upcoming Steam release.
1.
openfront.io/linkserves the linking routenginx.confgains two exact-match blocks redirecting/linkand/link/to/#steam-link. The app is unchanged —#steam-linkstays the single internal route, and the client never needs to know/linkexists.Why it's worth a URL alias. When the desktop app can't hand the player's browser a URL, it shows an 8-character code and tells them where to type it. That is the one URL in the product a human types by hand — and they type it in a degraded situation, because the fallback fires precisely when this machine couldn't open a browser. The browser they end up using may be on another device; a phone is realistic, and
#sits behind a symbol layer on a phone keyboard./link./linkand/link/—location =is exact-match, and browsers and address-bar autocomplete both append slashes. For a URL whose whole job is surviving being hand-typed, that variant isn't one to lose.nginx.confrather than at the CDN edge — this is load-bearing for account linking, so it belongs somewhere version-controlled, reviewed and deployed with the app.No dev-server change was needed: Vite's SPA fallback already serves
/link.2. The pending-link stash expires with its ticket
Logging in diverts the linking flow, so the intent is stashed in
localStorageand resumed fromonUserMe. It had no expiry, but the link ticket dies after 10 minutes server-side.Without one, a player who starts linking, is sent to log in, and wanders off leaves an entry that fires weeks later — opening a confirmation modal for a long-dead token, which then errors. That's worse than doing nothing: the confirmation dialog is the security step of the whole feature (it's what stops someone on a shared machine linking the wrong account), and an unexplained prompt that always fails teaches players to dismiss it unread.
stashedAtand are discarded past the ticket TTL.PendingLinktype is unchanged — the timestamp is storage detail, invisible to callers.The TTL comment now names the other two copies of that value (the server's, and the desktop gate's own), since there's no shared source to import across three deployables.
Verification
npx vitest run tests/clientplus the translation gates → 879 tests passing.tsc --noEmit, ESLint and Prettier clean.nginx -tpasses on the modified config (nginx:alpine, via Docker).Note on the PR gate
No linked
approvedissue — internal Steam-release work rather than a community contribution, so it should clear on repo permission. Happy to file an issue and relink if you'd prefer.