create account on purchase #1966
Conversation
WalkthroughAdds token-based login UI and flows: new AccountModal, AccountButton, TokenLoginModal, tokenLogin API, schema updates for Discord user and player, Main/index.html wiring changes, Stripe header/hostname tweaks, translations, and simplifications to TerritoryPatternsModal and some UI positions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Frontend UI
participant API
participant TokenModal as TokenLoginModal
User->>UI: Click AccountButton
UI->>API: GET /user/me (getUserMe)
API-->>UI: userMeResponse
alt Email magic-link
User->>UI: Enter email & Submit
UI->>API: POST /magic-link (email, redirectDomain)
API-->>UI: 200 / error
UI->>User: show confirmation or error
end
opt Token login via URL/hash
User->>TokenModal: open(token)
TokenModal->>API: GET /login/token?login-token=...
API-->>TokenModal: { jwt, email } or error
TokenModal->>User: show progress / success
TokenModal->>UI: reload page on success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/client/Cosmetics.ts (1)
40-72: Harden checkout request with try/catch and a timeout.Network failures will currently throw unhandled; add AbortController and clear messaging.
- const response = await fetch( - `${getApiBase()}/stripe/create-checkout-session`, - { - method: "POST", - headers: { - "Content-Type": "application/json", - authorization: getAuthHeader(), - "X-Persistent-Id": getPersistentID(), - }, - body: JSON.stringify({ - priceId: pattern.product.priceId, - hostname: window.location.origin, - }), - }, - ); - - if (!response.ok) { + const controller = new AbortController(); + const t = setTimeout(() => controller.abort(), 15000); + let response: Response; + try { + response = await fetch(`${getApiBase()}/stripe/create-checkout-session`, { + method: "POST", + headers: { + "Content-Type": "application/json", + authorization: getAuthHeader(), + "X-Persistent-Id": getPersistentID(), + }, + body: JSON.stringify({ + priceId: pattern.product.priceId, + hostname: window.location.origin, + }), + signal: controller.signal, + }); + } catch (err) { + console.error("Checkout request failed:", err); + alert("Network error. Please check your connection and try again."); + return; + } finally { + clearTimeout(t); + } + + if (!response.ok) { console.error( `Error purchasing pattern:${response.status} ${response.statusText}`, ); if (response.status === 401) { alert("You are not logged in. Please log in to purchase a pattern."); } else { alert("Something went wrong. Please try again later."); } return; } const { url } = await response.json(); // Redirect to Stripe checkout window.location.href = url;src/client/jwt.ts (1)
26-44: Do not auto-accept #token from URL (account takeover risk)Any page with XSS or a malicious link can stuff a JWT into localStorage. Let Main.ts handle token-login explicitly; remove this block.
- // Check window hash - const { hash } = window.location; - if (hash.startsWith("#")) { - const params = new URLSearchParams(hash.slice(1)); - const token = params.get("token"); - if (token) { - localStorage.setItem("token", token); - params.delete("token"); - params.toString(); - } - // Clean the URL - history.replaceState( - null, - "", - window.location.pathname + - window.location.search + - (params.size > 0 ? "#" + params.toString() : ""), - ); - }src/client/Main.ts (1)
483-489: Fix regression: #join=ID no longer parsedPrevious code pushes #join=. Support both formats.
- if (decodedHash.startsWith("#join")) { - const lobbyId = params.get("lobby"); + if (decodedHash.startsWith("#join")) { + let lobbyId = params.get("lobby"); + if (!lobbyId && decodedHash.startsWith("#join=")) { + lobbyId = decodedHash.slice("#join=".length); + } if (lobbyId && ID.safeParse(lobbyId).success) { this.joinModal.open(lobbyId); console.log(`joining lobby ${lobbyId}`); } }
🧹 Nitpick comments (8)
src/client/DarkModeButton.ts (1)
38-42: Move to top-left: OK; add a11y hints.Position change is fine. Consider adding type and aria-pressed for better accessibility.
- <button + <button title="Toggle Dark Mode" - class="absolute top-0 left-0 md:top-[10px] md:left-[10px] border-none bg-none cursor-pointer text-2xl" + class="absolute top-0 left-0 md:top-[10px] md:left-[10px] border-none bg-none cursor-pointer text-2xl" + type="button" + aria-pressed="${this.darkMode}" @click=${() => this.toggleDarkMode()} >src/client/LangSelector.ts (1)
262-265: Replace all placeholders, not just the first.If a key appears multiple times in a string, replaceAll is clearer.
- text = text.replace(`{${param}}`, String(value)); + text = text.replaceAll(`{${param}}`, String(value));src/client/AccountModal.ts (1)
270-299: Minor: avoid page reload for logout UX.Optional: instead of reload, dispatch userMeResponse with empty detail so the UI updates without a full refresh.
src/core/ApiSchemas.ts (1)
34-41: Optional: tighten Discord fieldsIf you want stricter inputs:
- id: digits only (snowflake), e.g. z.string().regex(/^\d+$/)
- locale: validate BCP‑47 (or restrict to a known set)
- consider .strict() on objects to drop unknown props.
src/client/jwt.ts (1)
86-88: JWT in localStorage (XSS exposure)Prefer server-set, HttpOnly, Secure, SameSite cookies and fetch(..., { credentials: "include" }). Keep localStorage only as a short-term fallback.
src/client/TokenLoginModal.ts (1)
63-69: Cleanup interval reference on close/successAvoid dangling handles.
public close() { this.token = null; clearInterval(this.retryInterval); + this.retryInterval = undefined; this.attemptCount = 0; this.modalEl?.close(); this.isAttemptingLogin = false; }this.email = email ?? null; - clearInterval(this.retryInterval); + clearInterval(this.retryInterval); + this.retryInterval = undefined;Also applies to: 94-99
src/client/Main.ts (2)
458-461: Guard modal open() callsIf token-login element is missing, this will throw.
- this.tokenLoginModal.open(token, patternName); + this.tokenLoginModal?.open(token, patternName);- this.tokenLoginModal.open(token, null); + this.tokenLoginModal?.open(token, null);Also applies to: 478-480
246-286: Prefer class toggles over rewriting body HTMLPer codebase convention, hide/show via classList to keep listeners and SPA state intact.
Also applies to: 288-315
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
resources/images/DiscordIcon.svgis excluded by!**/*.svgresources/images/DiscordLogo.svgis excluded by!**/*.svg
📒 Files selected for processing (10)
src/client/AccountModal.ts(1 hunks)src/client/Cosmetics.ts(2 hunks)src/client/DarkModeButton.ts(1 hunks)src/client/LangSelector.ts(2 hunks)src/client/Main.ts(7 hunks)src/client/TerritoryPatternsModal.ts(0 hunks)src/client/TokenLoginModal.ts(1 hunks)src/client/index.html(2 hunks)src/client/jwt.ts(2 hunks)src/core/ApiSchemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/client/TerritoryPatternsModal.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/index.htmlsrc/client/Main.ts
🧬 Code graph analysis (5)
src/client/Cosmetics.ts (1)
src/client/Main.ts (1)
getPersistentID(613-617)
src/client/TokenLoginModal.ts (2)
src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)src/client/jwt.ts (2)
tokenLogin(73-93)getUserMe(236-263)
src/client/Main.ts (2)
src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)src/client/TerritoryPatternsModal.ts (1)
onUserMe(34-45)
src/client/jwt.ts (1)
src/core/ApiSchemas.ts (1)
TokenPayloadSchema(9-31)
src/client/AccountModal.ts (2)
src/client/jwt.ts (4)
getApiBase(18-23)discordLogin(69-71)getUserMe(236-263)logOut(101-121)src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)
🪛 ast-grep (0.38.6)
src/client/jwt.ts
[warning] 86-86: Detected potential storage of sensitive information in browser localStorage. Sensitive data like email addresses, personal information, or authentication tokens should not be stored in localStorage as it's accessible to any script.
Context: localStorage.setItem("token", jwt)
Note: [CWE-312] Cleartext Storage of Sensitive Information [REFERENCES]
- https://owasp.org/www-community/vulnerabilities/HTML5_Security_Cheat_Sheet
- https://cwe.mitre.org/data/definitions/312.html
(browser-storage-sensitive-data)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (7)
src/client/LangSelector.ts (1)
90-94: Safer key handling: LGTM.Using optional chaining on e.key avoids rare undefined cases.
src/client/Cosmetics.ts (1)
47-52: Verify backend supports newhostnamefield andX-Persistent-Idheader
No server-side matches found for these parameters—confirm the/stripe/create-checkout-sessionendpoint accepts the new header and body property.src/client/index.html (2)
198-199: Token login placement: LGTM.Component is in the main input row with sensible widths.
379-379: Possible overlap with top-right overlays.account-button (fixed top-4 right-4 z-[9999]) may cover options-menu/replay-panel cluster (top/right). Please verify on small screens and adjust spacing (e.g., add top offset or move into the existing stack).
src/core/ApiSchemas.ts (1)
43-47: All consumers updated foruser.discordnesting No directuser.id/username/etc. usages remain; change is safe.src/client/jwt.ts (1)
189-196: No action needed—z.prettifyError is official Zod v4+ API
z.prettifyError was added in Zod v4 and is available without any local augmentation.src/client/Main.ts (1)
233-241: Event dispatch on userMeResponse looks goodClean handoff to interested components.
| alertAndStrip("purchase failed"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Translate alerts and avoid echoing sensitive URLs
Never include the full URL (may contain tokens) in user-visible text; also route through translateText().
- alertAndStrip("purchase failed");
+ alertAndStrip(translateText("purchase.failed"));- alert("Something went wrong. Please contact support.");
+ alert(translateText("purchase.error_generic"));- alertAndStrip(`purchase succeeded: ${patternName}`);
+ alertAndStrip(`${translateText("purchase.succeeded")}: ${patternName}`);- alertAndStrip(
- `login failed! got url ${window.location.href} Please try again.`,
- );
+ alertAndStrip(translateText("token_login.failed"));Also applies to: 450-454, 462-464, 471-475
🤖 Prompt for AI Agents
In src/client/Main.ts around lines 444-446, the alertAndStrip call is echoing a
full URL (which may contain tokens) and is not routed through translateText();
replace the direct alert text with a translated, user-facing message via
translateText() and remove or redact the full URL (e.g. replace with a safe
placeholder like "[link]" or only include a non-sensitive domain), and apply the
same change to the other occurrences at 450-454, 462-464, and 471-475 so no
alerts display raw URLs or tokens and all user-visible strings go through
translateText().
src/client/TokenLoginModal.ts
Outdated
| public onRefresh: ( | ||
| userMe: UserMeResponse | false, | ||
| patternName: string, | ||
| ) => void; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allow null patternName through onRefresh
Token login may not have a pattern. Make the type explicit and guard consumers.
- public onRefresh: (
- userMe: UserMeResponse | false,
- patternName: string,
- ) => void;
+ public onRefresh: (
+ userMe: UserMeResponse | false,
+ patternName: string | null,
+ ) => void;- this.onRefresh(userMe, this.patternName!);
+ this.onRefresh(userMe, this.patternName);Also applies to: 91-94
🤖 Prompt for AI Agents
In src/client/TokenLoginModal.ts around lines 18-21 (and similarly at lines
91-94), the onRefresh signature assumes patternName is a string but token logins
may not have a pattern; update the type to allow null (e.g., patternName: string
| null) and update any call sites inside this file to pass null when no pattern
exists and to guard against null before using the pattern (e.g., conditional
checks or early returns). Ensure any downstream consumers of onRefresh are
updated to accept and handle null patternName to avoid runtime errors.
src/client/TokenLoginModal.ts
Outdated
| return html` <p>Logging in...</p> `; | ||
| } | ||
|
|
||
| private loginSuccess(email: string) { | ||
| return html` <p>Successfully logged in as ${email}!</p> `; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
i18n + retry logic + UX nits
- Translate user-facing strings.
- Fix off-by-one (attempts become 4).
- Kick off first try immediately on open.
private loggingIn() {
- return html` <p>Logging in...</p> `;
+ return html` <p>${translateText("token_login.logging_in")}</p> `;
}
private loginSuccess(email: string) {
- return html` <p>Successfully logged in as ${email}!</p> `;
+ return html` <p>${translateText("token_login.success")} ${email}</p> `;
} public async open(token: string, patternName: string | null) {
this.token = token;
this.patternName = patternName;
this.modalEl?.open();
- this.retryInterval = setInterval(() => this.tryLogin(), 3000);
+ this.retryInterval = setInterval(() => this.tryLogin(), 3000);
+ this.tryLogin();
}- if (this.attemptCount > 3) {
+ if (this.attemptCount >= 3) {
this.close();
- alert("Login failed. Please try again later.");
+ alert(translateText("token_login.failed_retry"));
return;
}
this.attemptCount++;Also applies to: 75-81, 56-61, 77-79
🤖 Prompt for AI Agents
In src/client/TokenLoginModal.ts around lines 49-54 (also apply same fixes to
56-61, 75-81, 77-79): user-facing strings must be wrapped with the
translation/i18n function instead of hard-coded English, the retry-attempt logic
has an off-by-one so the maximum attempts should be decremented or the counter
initialized so total attempts = 3 (not 4), and the first login attempt should be
triggered immediately when the modal opens (call the attempt function on
mount/open rather than waiting for the first interval). Update all
success/working/error messages to use i18n, correct the attempt
counter/initialization, and ensure the open handler starts the first try right
away; apply the same changes consistently to the other listed line ranges.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
evan-create-account
branch
from
40205e4 to
ba74f3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/jwt.ts (1)
127-187: Make isLoggedIn() pure; avoid logout/refresh side-effects inside a sync checkerCalling logOut() and firing postRefresh() inside a sync predicate creates hidden network effects and races. Extract an async ensureFreshToken() for refresh, and make isLoggedIn() only decode/validate and return the typed union.
Also applies to: 148-165
src/client/TokenLoginModal.ts (1)
1-95: Add missing i18n keys for token-login modal states
Add the following entries undertoken_logininresources/lang/en.json:"token_login": { "title": "Token Login", "logging_in": "Logging in…", "success": "Successfully logged in", "failed_retry": "Login failed. Please try again later." }
♻️ Duplicate comments (7)
src/core/ApiSchemas.ts (1)
45-46: Validate email format (repeat of prior feedback)Use z.string().email() to reject bad emails at the edge.
- email: z.string().optional(), + email: z.string().email().optional(),src/client/jwt.ts (2)
73-76: Do not put login tokens in the URL; use POST with JSON bodyQuery params leak via logs/referrers. Switch to POST.
-export async function tokenLogin(token: string): Promise<string | null> { - const response = await fetch( - `${getApiBase()}/login/token?login-token=${token}`, - ); +export async function tokenLogin(token: string): Promise<string | null> { + const response = await fetch(`${getApiBase()}/login/token`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ loginToken: token }), + });
78-88: Validate response, verify iss/aud, and await clearToken() before setting new tokenAdds Zod parsing, explicit iss/aud checks, and awaits clearToken to avoid races. Also tolerate missing email.
- if (response.status === 200) { - const json = await response.json(); - const { jwt, email } = json; - const payload = decodeJwt(jwt); - const result = TokenPayloadSchema.safeParse(payload); - if (!result.success) { - console.error("Invalid token", result.error, result.error.message); - return null; - } - clearToken(); - localStorage.setItem("token", jwt); - return email; + if (response.status === 200) { + const TokenLoginResponse = z.object({ + jwt: z.string(), + email: z.string().email().optional(), + }); + const json = await response.json(); + const parsed = TokenLoginResponse.safeParse(json); + if (!parsed.success) { + console.error("Invalid token login response", parsed.error); + return null; + } + const { jwt, email } = parsed.data; + const payload = decodeJwt(jwt); + const result = TokenPayloadSchema.safeParse(payload); + if (!result.success) { + console.error("Invalid token", result.error, result.error.message); + return null; + } + if (payload.iss !== getApiBase() || payload.aud !== getAudience()) { + console.error('unexpected "iss"/"aud" claim values'); + return null; + } + await clearToken(); + localStorage.setItem("token", jwt); + return email ?? null;src/client/TokenLoginModal.ts (4)
40-46: Localize user-facing stringsWrap strings with translateText and use dedicated keys.
- private loggingIn() { - return html` <p>Logging in...</p> `; - } + private loggingIn() { + return html` <p>${translateText("token_login.logging_in")}</p> `; + } - private loginSuccess(email: string) { - return html` <p>Successfully logged in as ${email}!</p> `; - } + private loginSuccess(email: string) { + return html` <p>${translateText("token_login.success")} ${email}</p> `; + }
48-52: Kick off first login attempt immediately on open()Do not wait 3s before the first try.
public async open(token: string) { this.token = token; this.modalEl?.open(); this.retryInterval = setInterval(() => this.tryLogin(), 3000); + this.tryLogin(); }
17-17: Fix timer type for browser buildsNodeJS.Timeout breaks in DOM projects without @types/node. Use ReturnType.
- private retryInterval: NodeJS.Timeout | undefined = undefined; + private retryInterval: ReturnType<typeof setInterval> | undefined;
62-72: Fix off-by-one in retry limit and localize alert3 allows 4 attempts. Use >=3 and translate the message.
private async tryLogin() { if (this.isAttemptingLogin) { return; } - if (this.attemptCount > 3) { + if (this.attemptCount >= 3) { this.close(); - alert("Login failed. Please try again later."); + alert(translateText("token_login.failed_retry")); return; } this.attemptCount++;Also applies to: 68-71
🧹 Nitpick comments (4)
src/core/ApiSchemas.ts (1)
40-40: Optional: validate locale format (simple BCP‑47 check)Keeps values predictable without extra deps.
- locale: z.string().optional(), + locale: z.string().regex(/^[a-z]{2,3}(?:-[A-Z]{2})?$/i, "Use locale like en or en-US").optional(),src/client/Cosmetics.ts (2)
44-48: Send X-Persistent-Id only when present; confirm exact header name expected by APIAvoid sending an empty/undefined identifier and verify the server expects this exact casing.
headers: { "Content-Type": "application/json", authorization: getAuthHeader(), - "X-Persistent-Id": getPersistentID(), + ...(getPersistentID() + ? { "X-Persistent-Id": getPersistentID() } + : {}), },
50-52: Align request body field name with server contract (hostname vs redirectDomain)This differs from the magic-link flow which uses redirectDomain. Please confirm the API expects hostname here or standardize the field name across endpoints.
body: JSON.stringify({ priceId: pattern.product.priceId, - hostname: window.location.origin, + redirectDomain: window.location.origin, }),src/client/TokenLoginModal.ts (1)
48-59: Optional: emit an event instead of reloading the pagePrefer composition: notify the host via a custom event (typed union payload) and let it refresh UI; avoid full reload.
Also applies to: 62-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
resources/images/DiscordIcon.svgis excluded by!**/*.svgresources/images/DiscordLogo.svgis excluded by!**/*.svgresources/images/EmailIcon.svgis excluded by!**/*.svgresources/images/LoggedOutIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
resources/lang/en.json(1 hunks)src/client/AccountModal.ts(1 hunks)src/client/Cosmetics.ts(2 hunks)src/client/DarkModeButton.ts(1 hunks)src/client/LangSelector.ts(2 hunks)src/client/Main.ts(7 hunks)src/client/TerritoryPatternsModal.ts(0 hunks)src/client/TokenLoginModal.ts(1 hunks)src/client/index.html(2 hunks)src/client/jwt.ts(2 hunks)src/core/ApiSchemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/client/TerritoryPatternsModal.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/DarkModeButton.ts
- src/client/LangSelector.ts
- src/client/index.html
- src/client/AccountModal.ts
- src/client/Main.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/client/TokenLoginModal.ts (2)
src/client/AccountModal.ts (2)
customElement(9-232)customElement(234-317)src/client/jwt.ts (1)
tokenLogin(73-93)
src/client/Cosmetics.ts (1)
src/client/Main.ts (1)
getPersistentID(604-608)
src/client/jwt.ts (1)
src/core/ApiSchemas.ts (1)
TokenPayloadSchema(9-31)
🪛 ast-grep (0.38.6)
src/client/jwt.ts
[warning] 86-86: Detected potential storage of sensitive information in browser localStorage. Sensitive data like email addresses, personal information, or authentication tokens should not be stored in localStorage as it's accessible to any script.
Context: localStorage.setItem("token", jwt)
Note: [CWE-312] Cleartext Storage of Sensitive Information [REFERENCES]
- https://owasp.org/www-community/vulnerabilities/HTML5_Security_Cheat_Sheet
- https://cwe.mitre.org/data/definitions/312.html
(browser-storage-sensitive-data)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (2)
src/core/ApiSchemas.ts (2)
50-51: Use.optional()for roles and flares in ApiSchemas.ts
Undefinedroles/flaressignals an unauthenticated client; defaulting to[]removes that semantic and breaks existing auth checks.Likely an incorrect or invalid review comment.
43-47: Verify impact of requiring discord or email in UserMeResponseSchema
No imports or.parsecalls forUserMeResponseSchemawere found—manually confirm that adding thesuperRefinerule to enforce at least one ofdiscordor
evanpelle
force-pushed
the
evan-create-account
branch
from
ba74f3d to
1067063
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Main.ts (1)
479-485: Fix join hash parsing regression (supports #join=ID and #join?lobby=ID)Current code only reads query-style; deep-linking via “#join=ID” breaks.
Apply:
- if (decodedHash.startsWith("#join")) { - const lobbyId = params.get("lobby"); + if (decodedHash.startsWith("#join")) { + // Accept both styles: #join=ID and #join?lobby=ID + const eqMatch = decodedHash.match(/^#join=([^?]+)/); + const lobbyId = eqMatch?.[1] ?? params.get("lobby"); if (lobbyId && ID.safeParse(lobbyId).success) { this.joinModal.open(lobbyId); console.log(`joining lobby ${lobbyId}`); } }
♻️ Duplicate comments (7)
src/client/jwt.ts (3)
74-77: Stop leaking token in URL; use POST with JSON bodyQuery params end up in logs/referrers. Switch to POST and put the token in the body.
Apply:
-export async function tokenLogin(token: string): Promise<string | null> { - const response = await fetch( - `${getApiBase()}/login/token?login-token=${token}`, - ); +export async function tokenLogin(token: string): Promise<string | null> { + const response = await fetch(`${getApiBase()}/login/token`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ loginToken: token }), + credentials: "include", + });
78-89: Validate response shape, verify iss/aud now, and await clearToken()Add Zod parsing for the response, double-check iss/aud immediately, and await clearToken() to avoid races.
Apply:
- if (response.status === 200) { - const json = await response.json(); - const { jwt, email } = json; - const payload = decodeJwt(jwt); - const result = TokenPayloadSchema.safeParse(payload); + if (response.status === 200) { + const TokenLoginResponse = z.object({ + jwt: z.string(), + email: z.string().email().optional(), + }); + const json = await response.json(); + const parsed = TokenLoginResponse.safeParse(json); + if (!parsed.success) { + console.error("Invalid token login response", parsed.error); + return null; + } + const { jwt, email } = parsed.data; + const payload = decodeJwt(jwt); + const result = TokenPayloadSchema.safeParse(payload); if (!result.success) { console.error("Invalid token", result.error, result.error.message); return null; } - clearToken(); - localStorage.setItem("token", jwt); + if (payload.iss !== getApiBase() || payload.aud !== getAudience()) { + console.error('unexpected "iss"/"aud" claim values'); + return null; + } + await clearToken(); + // See next comment about storage; prefer cookie or sessionStorage. + sessionStorage.setItem("token", jwt); return email;
88-88: Switch token storage to sessionStorage
localStorage is vulnerable to XSS—persist JWTs in sessionStorage instead and update reads to prefer it.
- In src/client/jwt.ts, line 57 (getItem):
- return localStorage.getItem("token"); + return sessionStorage.getItem("token") ?? localStorage.getItem("token");- In src/client/jwt.ts, lines 32, 88, and 235 (setItem):
- localStorage.setItem("token", /* token */); + sessionStorage.setItem("token", /* token */);- Remove the “hash token” import path to avoid ever persisting URL-sourced tokens.
src/core/ApiSchemas.ts (2)
34-41: Harden DiscordUser schema (snowflake, optional avatar/global_name, discriminator variants)Aligns with Discord’s updated username model and improves validation.
Apply:
-export const DiscordUserSchema = z.object({ - id: z.string(), - avatar: z.string().nullable(), - username: z.string(), - global_name: z.string().nullable(), - discriminator: z.string(), - locale: z.string().optional(), -}); +export const DiscordUserSchema = z + .object({ + id: z.string().regex(/^\d+$/, "Discord snowflake must be digits only"), + avatar: z.string().nullish(), + username: z.string(), + global_name: z.string().nullish(), + discriminator: z.union([z.literal("0"), z.string().regex(/^\d{4}$/)]), + locale: z.string().optional(), + }) + .strict();
45-47: Validate email formatEnsure bad emails are rejected at the edge.
Apply:
- email: z.string().optional(), + email: z.string().email().optional(),src/client/Main.ts (2)
219-223: Null-check the token-login element before useAvoid NPE when the element is missing.
Apply:
- this.tokenLoginModal = document.querySelector( - "token-login", - ) as TokenLoginModal; - this.tokenLoginModal instanceof TokenLoginModal; + const tl = document.querySelector("token-login") as TokenLoginModal | null; + if (!tl) { + console.warn("Token login element not found"); + } else { + this.tokenLoginModal = tl; + }
434-461: Translate alerts and don’t show raw URLs/tokensReplace hardcoded alerts with translateText() and avoid echoing sensitive URLs.
Apply:
- if (status !== "true") { - alertAndStrip("purchase failed"); + if (status !== "true") { + alertAndStrip(translateText("purchase.failed")); return; } @@ - if (!patternName) { - alert("Something went wrong. Please contact support."); + if (!patternName) { + alert(translateText("purchase.error_generic")); console.error("purchase-completed but no pattern name"); return; } @@ - } else { - alertAndStrip(`purchase succeeded: ${patternName}`); + } else { + alertAndStrip(`${translateText("purchase.succeeded")}: ${patternName}`); this.patternsModal.refresh(); }
🧹 Nitpick comments (2)
src/client/Main.ts (2)
446-457: Guard tokenLoginModal usage to avoid NPEMaintain stability when the component isn’t present.
Apply:
- this.tokenLoginModal.open(token); + this.tokenLoginModal?.open(token);
411-421: Make alert helper translate-aware (optional)If you prefer centralizing, wrap translateText inside alertAndStrip callers instead of repeating per call.
Apply:
- const alertAndStrip = (message: string) => { - alert(message); + const alertAndStrip = (message: string) => { + alert(message); strip(); };Then pass already-translated strings as done above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
resources/images/DiscordIcon.svgis excluded by!**/*.svgresources/images/DiscordLogo.svgis excluded by!**/*.svgresources/images/EmailIcon.svgis excluded by!**/*.svgresources/images/LoggedOutIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
resources/lang/en.json(1 hunks)src/client/AccountModal.ts(1 hunks)src/client/Cosmetics.ts(2 hunks)src/client/DarkModeButton.ts(1 hunks)src/client/LangSelector.ts(2 hunks)src/client/Main.ts(7 hunks)src/client/TerritoryPatternsModal.ts(0 hunks)src/client/TokenLoginModal.ts(1 hunks)src/client/index.html(2 hunks)src/client/jwt.ts(1 hunks)src/core/ApiSchemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/client/TerritoryPatternsModal.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/client/DarkModeButton.ts
- src/client/Cosmetics.ts
- src/client/AccountModal.ts
- src/client/index.html
- src/client/TokenLoginModal.ts
- src/client/LangSelector.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (2)
src/client/jwt.ts (1)
src/core/ApiSchemas.ts (1)
TokenPayloadSchema(9-31)
src/client/Main.ts (2)
src/client/TerritoryPatternsModal.ts (1)
onUserMe(34-45)src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)
🪛 ast-grep (0.38.6)
src/client/jwt.ts
[warning] 87-87: Detected potential storage of sensitive information in browser localStorage. Sensitive data like email addresses, personal information, or authentication tokens should not be stored in localStorage as it's accessible to any script.
Context: localStorage.setItem("token", jwt)
Note: [CWE-312] Cleartext Storage of Sensitive Information [REFERENCES]
- https://owasp.org/www-community/vulnerabilities/HTML5_Security_Cheat_Sheet
- https://cwe.mitre.org/data/definitions/312.html
(browser-storage-sensitive-data)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
evanpelle
force-pushed
the
evan-create-account
branch
from
1067063 to
feb9908
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/client/jwt.ts (3)
74-77: Do not send login token in URL — switch to POST JSON body.Query params leak via logs and referrers. Use POST with JSON.
Apply:
export async function tokenLogin(token: string): Promise<string | null> { - const response = await fetch( - `${getApiBase()}/login/token?login-token=${token}`, - ); + const response = await fetch(`${getApiBase()}/login/token`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ loginToken: token }), + });
79-89: Validate response shape, check iss/aud, and await clearToken().Prevents bad data usage and race; aligns with existing payload validation elsewhere.
Apply:
- if (response.status === 200) { - const json = await response.json(); - const { jwt, email } = json; - const payload = decodeJwt(jwt); - const result = TokenPayloadSchema.safeParse(payload); + if (response.status === 200) { + const TokenLoginResponse = z.object({ + jwt: z.string(), + email: z.string().email().optional(), + }); + const parsed = TokenLoginResponse.safeParse(await response.json()); + if (!parsed.success) { + console.error("Invalid token login response", parsed.error); + return null; + } + const { jwt, email } = parsed.data; + const payload = decodeJwt(jwt); + const result = TokenPayloadSchema.safeParse(payload); if (!result.success) { console.error("Invalid token", result.error, result.error.message); return null; } - clearToken(); + if (payload.iss !== getApiBase() || payload.aud !== getAudience()) { + console.error('unexpected "iss"/"aud" claim values'); + return null; + } + await clearToken(); localStorage.setItem("token", jwt); return email;
87-89: Avoid storing JWTs in localStorage (XSS-accessible). Prefer HttpOnly cookies.Have the server set Secure, HttpOnly, SameSite=Strict cookies on /login/token and /refresh; read from cookie (your getToken already supports cookie-first). If immediate migration isn’t possible, consider a short-lived in-memory/session strategy.
src/client/Main.ts (2)
434-436: Translate user messages and avoid leaking details in alerts.Route strings through translateText; keep messages generic.
Apply:
- if (status !== "true") { - alertAndStrip("purchase failed"); + if (status !== "true") { + alertAndStrip(translateText("purchase.failed")); return; }- if (!patternName) { - alert("Something went wrong. Please contact support."); + if (!patternName) { + alert(translateText("purchase.error_generic")); console.error("purchase-completed but no pattern name"); return; }- this.tokenLoginModal.open(token); + this.tokenLoginModal?.open(token) ?? + console.warn("token-login modal missing");- alertAndStrip(`purchase succeeded: ${patternName}`); + alertAndStrip(`${translateText("purchase.succeeded")}: ${patternName}`);Also applies to: 441-443, 456-456, 458-460
467-476: Do not echo full URL on login failure; translate and guard modal.Avoid exposing tokens; improve UX.
Apply:
- if (!token) { - alertAndStrip( - `login failed! got url ${window.location.href} Please try again.`, - ); + if (!token) { + alertAndStrip(translateText("token_login.failed")); return; } strip(); - this.tokenLoginModal.open(token); + this.tokenLoginModal?.open(token) ?? + console.warn("token-login modal missing");
🧹 Nitpick comments (2)
src/client/jwt.ts (1)
74-94: Handle network/parse errors with try/catch.Prevent unhandled rejections on fetch/JSON failures; return null gracefully.
Apply:
-export async function tokenLogin(token: string): Promise<string | null> { - const response = await fetch(`${getApiBase()}/login/token`, { +export async function tokenLogin(token: string): Promise<string | null> { + try { + const response = await fetch(`${getApiBase()}/login/token`, { method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify({ loginToken: token }), - }); + }); if (response.status === 200) { // ... (as updated above) return email; } else { console.error("Token login failed", response); return null; } -} + } catch (e) { + console.error("Token login error", e); + return null; + } +}src/client/Main.ts (1)
425-428: Guard decodeURIComponent to avoid URIError on malformed hashes.Small hardening; fall back to raw hash.
Apply:
- const decodedHash = decodeURIComponent(hash); + let decodedHash = hash; + try { + decodedHash = decodeURIComponent(hash); + } catch { + console.warn("Invalid encoded hash; using raw hash"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
resources/images/DiscordIcon.svgis excluded by!**/*.svgresources/images/DiscordLogo.svgis excluded by!**/*.svgresources/images/EmailIcon.svgis excluded by!**/*.svgresources/images/LoggedOutIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
resources/lang/en.json(1 hunks)src/client/AccountModal.ts(1 hunks)src/client/Cosmetics.ts(2 hunks)src/client/DarkModeButton.ts(1 hunks)src/client/LangSelector.ts(2 hunks)src/client/Main.ts(6 hunks)src/client/TerritoryPatternsModal.ts(0 hunks)src/client/TokenLoginModal.ts(1 hunks)src/client/index.html(2 hunks)src/client/jwt.ts(1 hunks)src/core/ApiSchemas.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/client/TerritoryPatternsModal.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/client/DarkModeButton.ts
- src/client/LangSelector.ts
- resources/lang/en.json
- src/core/ApiSchemas.ts
- src/client/TokenLoginModal.ts
- src/client/AccountModal.ts
- src/client/index.html
- src/client/Cosmetics.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (2)
src/client/Main.ts (2)
src/client/TerritoryPatternsModal.ts (1)
onUserMe(34-45)src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)
src/client/jwt.ts (1)
src/core/ApiSchemas.ts (1)
TokenPayloadSchema(9-31)
🪛 ast-grep (0.38.6)
src/client/jwt.ts
[warning] 87-87: Detected potential storage of sensitive information in browser localStorage. Sensitive data like email addresses, personal information, or authentication tokens should not be stored in localStorage as it's accessible to any script.
Context: localStorage.setItem("token", jwt)
Note: [CWE-312] Cleartext Storage of Sensitive Information [REFERENCES]
- https://owasp.org/www-community/vulnerabilities/HTML5_Security_Cheat_Sheet
- https://cwe.mitre.org/data/definitions/312.html
(browser-storage-sensitive-data)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (1)
src/client/Main.ts (1)
225-231: Event-based update is good.Dispatching a typed CustomEvent keeps modules decoupled. Nice.
| private userSettings: UserSettings = new UserSettings(); | ||
| private patternsModal: TerritoryPatternsModal; | ||
| private tokenLoginModal: TokenLoginModal; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make TokenLoginModal nullable and guard usage.
Prevents runtime errors if the element is missing; prefer typed unions.
Apply:
- private tokenLoginModal: TokenLoginModal;
+ private tokenLoginModal: TokenLoginModal | null = null;- this.tokenLoginModal = document.querySelector(
- "token-login",
- ) as TokenLoginModal;
- this.tokenLoginModal instanceof TokenLoginModal;
+ const tl = document.querySelector("token-login") as TokenLoginModal | null;
+ if (!tl) {
+ console.warn("Token login element not found");
+ } else {
+ this.tokenLoginModal = tl;
+ }- this.tokenLoginModal.open(token);
+ this.tokenLoginModal?.open(token) ??
+ console.warn("token-login modal missing");- this.tokenLoginModal.open(token);
+ this.tokenLoginModal?.open(token) ??
+ console.warn("token-login modal missing");Also applies to: 219-223, 456-456, 475-476
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
src/client/jwt.ts (3)
90-92: Avoid localStorage for JWTs; prefer HttpOnly cookieslocalStorage is XSS-accessible; move to server-set HttpOnly, Secure, SameSite cookies. If migration can’t happen now, prefer sessionStorage and short-lived access tokens as an interim.
74-77: Do not send login token in URL; switch to POST with JSON bodyQuery params leak via logs/referrers and browser history. Send the token in the body.
-export async function tokenLogin(token: string): Promise<string | null> { - const response = await fetch( - `${getApiBase()}/login/token?login-token=${token}`, - ); +export async function tokenLogin(token: string): Promise<string | null> { + const response = await fetch(`${getApiBase()}/login/token`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ loginToken: token }), + });
82-93: Validate response shape, verify iss/aud, and await clearToken() before storingPrevents bad payloads, mismatched issuer/audience, and race conditions.
- const json = await response.json(); - const { jwt, email } = json; - const payload = decodeJwt(jwt); - const result = TokenPayloadSchema.safeParse(payload); + const TokenLoginResponse = z.object({ + jwt: z.string(), + email: z.string().email().optional(), + }); + const json = await response.json(); + const parsed = TokenLoginResponse.safeParse(json); + if (!parsed.success) { + console.error("Invalid token login response", parsed.error); + return null; + } + const { jwt, email } = parsed.data; + const payload = decodeJwt(jwt); + const result = TokenPayloadSchema.safeParse(payload); if (!result.success) { console.error("Invalid token", result.error, result.error.message); return null; } - clearToken(); - localStorage.setItem("token", jwt); - return email; + if (payload.iss !== getApiBase() || payload.aud !== getAudience()) { + console.error('unexpected "iss"/"aud" claim values'); + return null; + } + await clearToken(); + localStorage.setItem("token", jwt); + __isLoggedIn = undefined; + return email ?? null;src/client/Main.ts (3)
435-445: Translate alerts and avoid raw stringsRoute through translateText() and keep messages generic.
- if (status !== "true") { - alertAndStrip("purchase failed"); + if (status !== "true") { + alertAndStrip(translateText("purchase.failed")); return; }- if (!patternName) { - alert("Something went wrong. Please contact support."); + if (!patternName) { + alert(translateText("purchase.error_generic")); console.error("purchase-completed but no pattern name"); return; }- } else { - alertAndStrip(`purchase succeeded: ${patternName}`); + } else { + alertAndStrip(`${translateText("purchase.succeeded")}: ${patternName}`); this.patternsModal.refresh();- if (!token) { - alertAndStrip( - `login failed! Please try again later or contact support.`, - ); + if (!token) { + alertAndStrip(translateText("token_login.failed")); return; }Also applies to: 458-458, 468-471
94-94: Make TokenLoginModal nullable and guard DOM lookupPrevents NPEs if the element is missing.
- private tokenLoginModal: TokenLoginModal; + private tokenLoginModal: TokenLoginModal | null = null;- this.tokenLoginModal = document.querySelector( - "token-login", - ) as TokenLoginModal; - this.tokenLoginModal instanceof TokenLoginModal; + const tl = document.querySelector("token-login") as TokenLoginModal | null; + if (!tl) { + console.warn("Token login element not found"); + } else { + this.tokenLoginModal = tl; + }Also applies to: 219-223
446-457: Guard modal usage when opening token-loginAvoids runtime errors if modal is absent.
- this.tokenLoginModal.open(token); + this.tokenLoginModal?.open(token) ?? + console.warn("token-login modal missing");- this.tokenLoginModal.open(token); + this.tokenLoginModal?.open(token) ?? + console.warn("token-login modal missing");Also applies to: 474-476
src/client/AccountModal.ts (5)
55-56: Internationalize all user-facing stringsUse translateText() and existing common keys; avoid hardcoded English.
- Logged in with Discord as ${this.loggedInDiscord} + ${translateText("account.logged_in_discord", { name: this.loggedInDiscord ?? "" }) || + `Logged in with Discord as ${this.loggedInDiscord ?? ""}`}- Logged in as ${this.loggedInEmail} + ${translateText("account.logged_in_email", { email: this.loggedInEmail ?? "" }) || + `Logged in as ${this.loggedInEmail ?? ""}`}- Log Out + ${translateText("main.log_out") || "Log Out"}- Choose your login method + ${translateText("account.choose_login_method") || "Choose your login method"}- <span class="px-2 bg-gray-800 text-gray-300">or</span> + <span class="px-2 bg-gray-800 text-gray-300">${translateText("common.or") || "or"}</span>- Recover account by email + ${translateText("account.recover_by_email") || "Recover account by email"}- placeholder="Enter your email address" + placeholder="${translateText("account.email_placeholder") || "Enter your email address"}"- Cancel + ${translateText("common.cancel") || "Cancel"}- Submit + ${translateText("common.submit") || "Submit"}- if (!this.email) { - alert("Please enter an email address"); + if (!this.email) { + alert(translateText("account.enter_email") || "Please enter an email address"); return; }- alert("Failed to send recovery email. Please try again."); + alert(translateText("account.recovery_failed") || "Failed to send recovery email. Please try again.");- alert("Error sending recovery email. Please try again."); + alert(translateText("account.recovery_error") || "Error sending recovery email. Please try again.");Also applies to: 68-69, 82-83, 91-94, 119-121, 129-131, 137-139, 149-156, 168-171, 199-204
151-156: Prevent double submits; show progressDisable the Submit button while sending; always clear in finally.
- <button + <button @click="${this.handleSubmit}" - class="px-4 py-2 text-sm font-medium text-white bg-blue-600 border border-transparent rounded-md hover:bg-blue-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500" + class="px-4 py-2 text-sm font-medium text-white bg-blue-600 disabled:bg-blue-400 disabled:cursor-not-allowed border border-transparent rounded-md hover:bg-blue-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500" + ?disabled=${this.submitting} > - ${translateText("common.submit") || "Submit"} + ${this.submitting + ? (translateText("common.sending") || "Sending...") + : (translateText("common.submit") || "Submit")} </button>private async handleSubmit() { if (!this.email) { alert(translateText("account.enter_email") || "Please enter an email address"); return; } try { + this.submitting = true; const apiBase = getApiBase(); const response = await fetch(`${apiBase}/magic-link`, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ redirectDomain: window.location.origin, email: this.email, }), }); if (response.ok) { alert( translateText("account_modal.recovery_email_sent", { email: this.email, }), ); this.close(); } else { console.error( "Failed to send recovery email:", response.status, response.statusText, ); alert(translateText("account.recovery_failed") || "Failed to send recovery email. Please try again."); } } catch (error) { console.error("Error sending recovery email:", error); - alert("Error sending recovery email. Please try again."); + alert(translateText("account.recovery_error") || "Error sending recovery email. Please try again."); + } finally { + this.submitting = false; } }Add near other @State fields:
@state() private submitting = false;Also applies to: 167-205
243-262: Make userMeResponse listener removable; use display name, not Discord idBind a class handler, remove it in disconnectedCallback, and normalize fields.
- document.addEventListener("userMeResponse", (event: Event) => { - const customEvent = event as CustomEvent; - - if (customEvent.detail) { - const userMeResponse = customEvent.detail as UserMeResponse; - if (userMeResponse.user.email) { - this.loggedInEmail = userMeResponse.user.email; - this.requestUpdate(); - } else if (userMeResponse.user.discord) { - this.loggedInDiscord = userMeResponse.user.discord.id; - this.requestUpdate(); - } - } else { - // Clear the logged in states when user logs out - this.loggedInEmail = null; - this.loggedInDiscord = null; - this.requestUpdate(); - } - }); + document.addEventListener("userMeResponse", this.onUserMeResponse as EventListener);Additions outside this range:
private onUserMeResponse = (event: Event) => { const { detail } = event as CustomEvent<UserMeResponse | false>; if (detail && detail !== false) { this.loggedInEmail = detail.user.email ?? null; this.loggedInDiscord = detail.user.discord?.global_name ?? detail.user.discord?.username ?? null; } else { this.loggedInEmail = null; this.loggedInDiscord = null; } this.requestUpdate(); }; disconnectedCallback() { super.disconnectedCallback(); document.removeEventListener("userMeResponse", this.onUserMeResponse as EventListener); }
225-230: Only reload after successful logout; translate errorRespect logOut() result and avoid unnecessary reloads.
- private async handleLogout() { - await logOut(); - this.close(); - // Refresh the page after logout to update the UI state - window.location.reload(); - } + private async handleLogout() { + try { + const ok = await logOut(); + if (ok) { + this.close(); + window.location.reload(); + } else { + alert(translateText("account.logout_failed") || "Logout failed. Please try again."); + } + } catch (error) { + console.error("Error during logout:", error); + alert(translateText("account.logout_error") || "Error during logout. Please try again."); + } + }
238-239: Fix @query definite assignmentAvoid strict TS errors by asserting non-null or making it nullable.
- @query("account-modal") private recoveryModal: AccountModal; + @query("account-modal") private recoveryModal!: AccountModal;
🧹 Nitpick comments (2)
src/client/Main.ts (1)
225-231: Dispatch composed custom event for cross-boundary listenersEnsures listeners outside shadow roots receive the event.
- document.dispatchEvent( - new CustomEvent("userMeResponse", { - detail: userMeResponse, - bubbles: true, - cancelable: true, - }), - ); + document.dispatchEvent( + new CustomEvent("userMeResponse", { + detail: userMeResponse, + bubbles: true, + cancelable: true, + composed: true, + }), + );src/client/AccountModal.ts (1)
211-219: Clear stale state when not logged inIf getUserMe() returns false, reset logged-in labels.
public async open() { const userMe = await getUserMe(); - if (userMe) { + if (userMe) { this.loggedInEmail = userMe.user.email ?? null; this.loggedInDiscord = userMe.user.discord?.global_name ?? null; - } + } else { + this.loggedInEmail = null; + this.loggedInDiscord = null; + } this.modalEl?.open(); this.requestUpdate(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/AccountModal.ts(1 hunks)src/client/Main.ts(6 hunks)src/client/jwt.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (3)
src/client/AccountModal.ts (2)
src/client/jwt.ts (4)
getApiBase(18-23)discordLogin(70-72)getUserMe(242-270)logOut(101-121)src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)
src/client/Main.ts (2)
src/client/TerritoryPatternsModal.ts (1)
onUserMe(34-45)src/core/ApiSchemas.ts (1)
UserMeResponse(54-54)
src/client/jwt.ts (1)
src/core/ApiSchemas.ts (1)
TokenPayloadSchema(9-31)
🪛 ast-grep (0.38.6)
src/client/jwt.ts
[warning] 90-90: Detected potential storage of sensitive information in browser localStorage. Sensitive data like email addresses, personal information, or authentication tokens should not be stored in localStorage as it's accessible to any script.
Context: localStorage.setItem("token", jwt)
Note: [CWE-312] Cleartext Storage of Sensitive Information [REFERENCES]
- https://owasp.org/www-community/vulnerabilities/HTML5_Security_Cheat_Sheet
- https://cwe.mitre.org/data/definitions/312.html
(browser-storage-sensitive-data)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Cosmetics.ts (1)
37-37: Wrap hard-coded alerts with translateText and add missing keys
ImporttranslateTextfrom./Utils, replace in Cosmetics.ts (lines 37 and 62–65):- alert("This pattern is not available for purchase."); + alert(translateText("purchase.patternNotAvailable")); @@ - alert("You are not logged in. Please log in to purchase a pattern."); + alert(translateText("purchase.loginRequired")); @@ - alert("Something went wrong. Please try again later."); + alert(translateText("errors.genericTryAgainLater"));Add these keys to
resources/lang/en.json(and mirror in other locales):"purchase": { "patternNotAvailable": "This pattern is not available for purchase.", "loginRequired": "You are not logged in. Please log in to purchase a pattern." }, "errors": { "genericTryAgainLater": "Something went wrong. Please try again later." }
🧹 Nitpick comments (3)
src/client/Cosmetics.ts (3)
52-52: Field name and path correctness: send “origin”, and consider subpath apps.
window.location.originis an origin (scheme+host+port), not a hostname. Also, if the app runs under a subpath (e.g.,/app), server-side URL building from only the origin can misroute.- hostname: window.location.origin, + origin: window.location.origin, + // Optionally include basePath if app isn’t at `/` + // basePath: window.location.pathname.replace(/\/[^/]*$/, '') || '/',Please confirm the server expects
origin(or adjust accordingly) and that redirects are correct when deployed under a subpath. Also validate against an allowlist to avoid open-redirects.
41-55: Handle network errors and JSON parse errors in purchase flow.A thrown fetch/JSON error currently bubbles and yields a silent failure for users.
- const response = await fetch( + let response: Response; + try { + response = await fetch( `${getApiBase()}/stripe/create-checkout-session`, { method: "POST", headers: { "Content-Type": "application/json", authorization: getAuthHeader(), "X-Persistent-Id": getPersistentID(), }, body: JSON.stringify({ priceId: pattern.product.priceId, - hostname: window.location.origin, + origin: window.location.origin, // see naming note above }), }, - ); + ); + } catch (err) { + console.error("Network error purchasing pattern:", err); + alert("Network error. Please try again."); + return; + } @@ - const { url } = await response.json(); + let url: string; + try { + ({ url } = await response.json()); + } catch (err) { + console.error("Invalid JSON from checkout-session:", err); + alert("Something went wrong. Please try again later."); + return; + }Also applies to: 69-73
23-26: Avoid mutating shared cosmetics data when granting access.Setting
patternData.product = nullmutates the original object. Prefer a copy to keep source data immutable.- // Remove product info because player already has access. - patternData.product = null; - patterns.set(name, patternData); + // Remove product info because player already has access, without mutating source. + patterns.set(name, { ...patternData, product: null });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/client/Cosmetics.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/Cosmetics.ts (1)
src/client/Main.ts (1)
getPersistentID(609-613)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (1)
src/client/Cosmetics.ts (1)
4-4: Good reuse: single source of truth for persistent ID.Importing
getPersistentIDkeeps this logic centralized and avoids duplication.
| "X-Persistent-Id": getPersistentID(), | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
CORS allowlist and header handling for X-Persistent-Id.
Browsers will preflight this custom header. Ensure the API allows it and actually reads it.
Run:
- Don’t log this header in access logs/analytics.
- If behind a proxy (Cloudflare/Nginx), confirm the header isn’t stripped.
🏁 Script executed:
#!/bin/bash
# 1) CORS allowlist should include X-Persistent-Id
rg -nP -C3 '(Access-Control-Allow-Headers|cors|allowedHeaders)'
# 2) Server should read the header
rg -nP -C3 'X-Persistent-Id|x-persistent-id|persistent[ _-]?id'
# 3) Checkout session handler should connect the dots
rg -nP -C3 'create-checkout-session|checkout.*session'Length of output: 61232
Ensure API CORS and header handling
- Add “X-Persistent-Id” to your server’s Access-Control-Allow-Headers in the CORS setup so browsers can send it.
- In the
/stripe/create-checkout-sessionhandler, readX-Persistent-Idfromreq.headersand attach it to the session context.
Also confirm any proxy (Cloudflare/Nginx) forwards this header and avoid logging it in analytics.
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
Description:
When purchasing an item, user will be logged in as their email automatically.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan