Hhh#3978
Closed
MeowMeowMeow123412345 wants to merge 22 commits into
Closed
Conversation
Refactor getApiBase function to prioritize API_DOMAIN environment variable for API base URL.
Add magic credential handling for token login
Refactor JWT audience and issuer methods to improve handling of environment variables and API domain parsing.
Added registration of authentication routes to the server.
Add condition to handle root subdomain case
Add condition to handle root subdomain case
Added SUBDOMAIN variable for deployment configuration.
|
|
Contributor
WalkthroughThis PR adds Magic SDK-based email authentication to the application. It configures the client to send magic links directly via Magic SDK, implements a complete server-side auth system with JWT signing and refresh tokens, refactors configuration to support flexible API domains and JWT audience/issuer settings, and updates deployment scripts to handle root-domain and subdomain routing. ChangesDeployment Subdomain Normalization
Environment & Configuration Setup
Client-Side Magic Authentication
Server-Side Auth Routes & JWT System
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant MagicSDK
participant Server as Server<br/>(AuthRoutes)
participant MagicAdmin as Magic<br/>(Admin)
User->>Client: Enter email in login form
Client->>MagicSDK: sendMagicLink(email)
MagicSDK->>User: Send email with magic link
User->>User: Click magic link<br/>(magic_credential in URL)
User->>Client: Navigate to app with<br/>magic_credential param
Client->>Client: handleUrl() extracts credential
Client->>Client: openWithToken(credential)
Client->>Server: POST /auth/login/token<br/>(login-token querystring)
Server->>MagicAdmin: Verify login token
MagicAdmin-->>Server: email + metadata
Server->>Server: Create/find user
Server->>Server: Generate JWT + refresh token
Server->>Server: Set refresh cookie
Server-->>Client: { email, id, ... }
Client->>Client: Store JWT in state
Client->>Server: GET /users/@me<br/>(Authorization: Bearer JWT)
Server->>Server: Verify token
Server-->>Client: { email, id, ... }
Client->>User: Render authenticated UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/client/Auth.ts (1)
182-189: 💤 Low valueConsider caching the Magic client instance.
Creating a new
Magicinstance on every call togetMagicClient()is wasteful. The Magic SDK recommends reusing the client. A simple singleton pattern would avoid repeated initialization overhead.♻️ Proposed singleton pattern
+let __magicClient: Magic | null = null; + function getMagicClient(): Magic | null { + if (__magicClient) { + return __magicClient; + } const publishableKey = process.env.MAGIC_PUBLISHABLE_KEY; if (!publishableKey) { console.error("MAGIC_PUBLISHABLE_KEY is not configured"); return null; } - return new Magic(publishableKey); + __magicClient = new Magic(publishableKey); + return __magicClient; }🤖 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 `@src/client/Auth.ts` around lines 182 - 189, The getMagicClient function currently constructs a new Magic(...) on every call; change it to implement a simple singleton by storing the created Magic instance in a module-level variable (e.g., let magicClient: Magic | null = null) and returning that cached instance on subsequent calls; keep the same publishableKey check (initialize magicClient only if publishableKey exists) and ensure getMagicClient returns the cached magicClient (or null) instead of creating a new Magic each time.src/server/AuthRoutes.ts (3)
198-200: ⚡ Quick winLinear scan to find user by
persistentIdis inefficient.
Array.from(usersByEmail.values()).find(...)iterates all users. Consider adding a second map keyed bypersistentIdfor O(1) lookups.Proposed fix - add a second map
+const usersByPersistentId = new Map< + string, + { email: string; persistentId: string; publicId: string } +>(); + function createUser(email: string) { const existing = usersByEmail.get(email); if (existing) return existing; const user = { email, persistentId: createPersistentId(), publicId: createPublicId(), }; usersByEmail.set(email, user); + usersByPersistentId.set(user.persistentId, user); return user; }Then replace
Array.from(usersByEmail.values()).find(...)withusersByPersistentId.get(persistentId).🤖 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 `@src/server/AuthRoutes.ts` around lines 198 - 200, The current lookup uses Array.from(usersByEmail.values()).find(...) which does a linear scan to match entry.persistentId === result.persistentId; introduce a second Map (e.g., usersByPersistentId) keyed by persistentId, populate/update it wherever users are created/updated/removed, and replace the linear search with a constant-time lookup like usersByPersistentId.get(result.persistentId) (ensure consistency between usersByEmail and usersByPersistentId on all user lifecycle operations).
57-68: ⚡ Quick win
getAllowedOrigins()is called on every request, re-parsing the env var each time.For better performance, parse and cache allowed origins once at startup.
Proposed fix
+const allowedOriginsCache = new Set<string>(); + +function initAllowedOrigins(): void { + const env = process.env.CORS_ORIGINS; + if (env) { + env.split(",").forEach((value) => { + const trimmed = value.trim(); + if (trimmed.length > 0) { + allowedOriginsCache.add(trimmed); + } + }); + } +} + function getAllowedOrigins(): Set<string> { - const origins = new Set<string>(); - const env = process.env.CORS_ORIGINS; - if (env) { - env.split(",").forEach((value) => { - const trimmed = value.trim(); - if (trimmed.length > 0) { - origins.add(trimmed); - } - }); - } - return origins; + return allowedOriginsCache; } + +// Call at module load +initAllowedOrigins();🤖 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 `@src/server/AuthRoutes.ts` around lines 57 - 68, The getAllowedOrigins() function re-parses process.env.CORS_ORIGINS on every request; change it to parse once and return a cached Set: create a top-level cachedOrigins (or lazy-initialized via an initAllowedOrigins function) that splits, trims and populates the Set at module load (or on first call) and have getAllowedOrigins() simply return that cached Set; update any call sites to use the new cached value (keep function name getAllowedOrigins if used widely) so runtime requests no longer re-parse the environment variable.
104-112: 💤 Low valueCookie with
sameSite: "none"requires careful consideration.Using
sameSite: "none"allows cross-site cookie sending, which is needed for cross-origin auth flows but increases CSRF exposure. This is fine if intentional for your architecture (client on different domain than API). Just verify this matches your deployment model.Also, note that
clearCookieon Line 271-275 should use matching options (includingpathif you set one) for reliable cookie removal.🤖 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 `@src/server/AuthRoutes.ts` around lines 104 - 112, The current setRefreshCookie function sets sameSite: "none" which permits cross-site cookies—confirm this is intentional for your cross-origin auth flow or make it configurable (e.g., derive sameSite from an env/config flag) in setRefreshCookie (and document it); also ensure cookie clearing uses identical options so clearCookie reliably removes the cookie—update the clearCookie call to include the same secure, sameSite, httpOnly, maxAge/path options (and include path if you set one) so the refreshCookieName is cleared consistently.
🤖 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 `@example.env`:
- Line 10: The inline comment after the SUBDOMAIN assignment can break dotenv
parsing (your export $(grep ... | xargs) loader) — move the comment off the
assignment so the line contains only the key/value (SUBDOMAIN=www) and put the
explanatory text on its own line prefixed with #; update the SUBDOMAIN line in
the file (referencing the SUBDOMAIN variable) so it contains no inline comment.
In `@package.json`:
- Around line 74-76: The packages "cookie-parser" and "`@magic-sdk/admin`" are
used at runtime (imported in server code) but currently listed under
devDependencies; move both "cookie-parser" and "`@magic-sdk/admin`" entries from
devDependencies into dependencies in package.json so production installs include
them and server files that import those packages (e.g., the server Worker and
AuthRoutes modules) won't fail at runtime.
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 109-154: Add unit tests covering all jwtAudience()/jwtIssuer()
derivation branches: test explicit overrides using Env.JWT_AUDIENCE and
Env.JWT_ISSUER, scheme stripping when parseApiDomain() returns a URL (use
Env.API_DOMAIN), audience values that already include http/https, localhost and
IP forms (e.g., "localhost", "localhost:3000", "127.0.0.1", "[::1]"), audiences
starting with "api.", and the default fallback; exercise
DefaultConfig.jwtAudience, DefaultConfig.jwtIssuer and parseApiDomain behavior
by stubbing Env variables and asserting expected outputs for each branch.
In `@src/server/AuthRoutes.ts`:
- Around line 14-29: The in-memory Maps refreshTokens and usersByEmail in
AuthRoutes.ts are process-local and must be replaced with a persistent/shared
store to survive restarts and scale horizontally; implement a small storage
abstraction (e.g., TokenStore/UserStore interface) and replace direct Map
accesses in functions that read/write refreshTokens and usersByEmail with async
calls to a Redis-backed or DB-backed implementation that supports TTL for tokens
and atomic operations for lookups/invalidations, migrate existing code paths to
await the new store methods, and provide a default in-memory fallback for local
dev only.
- Line 6: Remove the unused import TokenPayloadSchema from the import list in
AuthRoutes.ts so only used symbols (e.g., UserMeResponse) are imported; update
the import line that currently reads import { TokenPayloadSchema, UserMeResponse
} from "../core/ApiSchemas" to omit TokenPayloadSchema to satisfy ESLint and
avoid confusion.
- Line 31: The code instantiates the Magic SDK unconditionally (const magic =
new Magic(magicSecretKey)) which will cause confusing runtime failures when
magicSecretKey (MAGIC_SECRET_KEY) is empty; update the startup logic to validate
magicSecretKey before creating the Magic instance and fail fast with a clear
error (throw or process.exit) if it's missing, or guard all uses of magic (e.g.,
magic.users.getMetadataByToken) behind a conditional that checks the key so
initialization only occurs when a valid key is present.
In `@src/server/Worker.ts`:
- Line 2: Remove the unused cookie-parser middleware from Worker.ts: delete the
import cookieParser and remove the app.use(cookieParser()) call so Worker no
longer loads cookie parsing on every request; search for the symbols
cookieParser and the app.use(cookieParser()) invocation in Worker.ts (and ensure
no other modules rely on req.cookies) before committing the removal.
---
Nitpick comments:
In `@src/client/Auth.ts`:
- Around line 182-189: The getMagicClient function currently constructs a new
Magic(...) on every call; change it to implement a simple singleton by storing
the created Magic instance in a module-level variable (e.g., let magicClient:
Magic | null = null) and returning that cached instance on subsequent calls;
keep the same publishableKey check (initialize magicClient only if
publishableKey exists) and ensure getMagicClient returns the cached magicClient
(or null) instead of creating a new Magic each time.
In `@src/server/AuthRoutes.ts`:
- Around line 198-200: The current lookup uses
Array.from(usersByEmail.values()).find(...) which does a linear scan to match
entry.persistentId === result.persistentId; introduce a second Map (e.g.,
usersByPersistentId) keyed by persistentId, populate/update it wherever users
are created/updated/removed, and replace the linear search with a constant-time
lookup like usersByPersistentId.get(result.persistentId) (ensure consistency
between usersByEmail and usersByPersistentId on all user lifecycle operations).
- Around line 57-68: The getAllowedOrigins() function re-parses
process.env.CORS_ORIGINS on every request; change it to parse once and return a
cached Set: create a top-level cachedOrigins (or lazy-initialized via an
initAllowedOrigins function) that splits, trims and populates the Set at module
load (or on first call) and have getAllowedOrigins() simply return that cached
Set; update any call sites to use the new cached value (keep function name
getAllowedOrigins if used widely) so runtime requests no longer re-parse the
environment variable.
- Around line 104-112: The current setRefreshCookie function sets sameSite:
"none" which permits cross-site cookies—confirm this is intentional for your
cross-origin auth flow or make it configurable (e.g., derive sameSite from an
env/config flag) in setRefreshCookie (and document it); also ensure cookie
clearing uses identical options so clearCookie reliably removes the
cookie—update the clearCookie call to include the same secure, sameSite,
httpOnly, maxAge/path options (and include path if you set one) so the
refreshCookieName is cleared consistently.
🪄 Autofix (Beta)
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
Run ID: 2a3acf04-0e7a-4f0d-89a1-dc6dbbb8a7cc
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonresources/flags/Quebec.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
build-deploy.shdeploy.shexample.envpackage.jsonsrc/client/Api.tssrc/client/Auth.tssrc/client/Main.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Env.tssrc/server/AuthRoutes.tssrc/server/Master.tssrc/server/Worker.tstsconfig.jsonupdate.shvite.config.ts
| GHCR_TOKEN=your_docker_token_here | ||
|
|
||
| DOMAIN=your-domain.com | ||
| SUBDOMAIN=www # Set to blank for root domain deployment, or use 'root' when calling deploy scripts. |
Contributor
There was a problem hiding this comment.
Move the comment to its own line to avoid .env parse breakage.
On Line 10, inline comments can break your current loader (export $(grep ... | xargs)) by turning comment words into invalid export tokens.
Suggested fix
-SUBDOMAIN=www # Set to blank for root domain deployment, or use 'root' when calling deploy scripts.
+# Set to blank for root domain deployment, or use 'root' when calling deploy scripts.
+SUBDOMAIN=www📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SUBDOMAIN=www # Set to blank for root domain deployment, or use 'root' when calling deploy scripts. | |
| # Set to blank for root domain deployment, or use 'root' when calling deploy scripts. | |
| SUBDOMAIN=www |
🤖 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 `@example.env` at line 10, The inline comment after the SUBDOMAIN assignment
can break dotenv parsing (your export $(grep ... | xargs) loader) — move the
comment off the assignment so the line contains only the key/value
(SUBDOMAIN=www) and put the explanatory text on its own line prefixed with #;
update the SUBDOMAIN line in the file (referencing the SUBDOMAIN variable) so it
contains no inline comment.
Comment on lines
+74
to
+76
| "cookie-parser": "^1.4.7", | ||
| "@magic-sdk/admin": "^2.0.0", | ||
| "magic-sdk": "^6.0.0", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether these packages are imported by runtime server code
rg -n -C2 '`@magic-sdk/admin`|cookie-parser|magic-sdk' src/server src/client
# Show where they are currently declared
python - <<'PY'
import json
from pathlib import Path
pkg = json.loads(Path("package.json").read_text())
for name in ["cookie-parser", "`@magic-sdk/admin`", "magic-sdk"]:
loc = "dependencies" if name in pkg.get("dependencies", {}) else "devDependencies" if name in pkg.get("devDependencies", {}) else "missing"
print(f"{name}: {loc}")
PYRepository: openfrontio/OpenFrontIO
Length of output: 1000
Move server runtime packages to dependencies.
cookie-parser and @magic-sdk/admin are imported in server code (src/server/Worker.ts and src/server/AuthRoutes.ts). When production deploys with npm install --production, packages in devDependencies are skipped, causing runtime errors. Move them to dependencies.
🤖 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 `@package.json` around lines 74 - 76, The packages "cookie-parser" and
"`@magic-sdk/admin`" are used at runtime (imported in server code) but currently
listed under devDependencies; move both "cookie-parser" and "`@magic-sdk/admin`"
entries from devDependencies into dependencies in package.json so production
installs include them and server files that import those packages (e.g., the
server Worker and AuthRoutes modules) won't fail at runtime.
Comment on lines
+109
to
+154
|
|
||
| private parseApiDomain(): string | undefined { | ||
| const apiDomain = Env.API_DOMAIN?.trim(); | ||
| if (!apiDomain) return undefined; | ||
| return apiDomain.replace(/\/+$/, ""); | ||
| } | ||
|
|
||
| jwtAudience(): string { | ||
| if (Env.JWT_AUDIENCE?.trim()) { | ||
| return Env.JWT_AUDIENCE.trim(); | ||
| } | ||
|
|
||
| const apiDomain = this.parseApiDomain(); | ||
| if (apiDomain) { | ||
| return apiDomain.replace(/^https?:\/\//, ""); | ||
| } | ||
|
|
||
| return "openfront.io"; | ||
| } | ||
|
|
||
| jwtIssuer(): string { | ||
| if (Env.JWT_ISSUER?.trim()) { | ||
| return Env.JWT_ISSUER.trim().replace(/\/+$/, ""); | ||
| } | ||
|
|
||
| const audience = this.jwtAudience().trim(); | ||
| if (audience.startsWith("http://") || audience.startsWith("https://")) { | ||
| return audience.replace(/\/+$/, ""); | ||
| } | ||
|
|
||
| if ( | ||
| audience === "localhost" || | ||
| audience.startsWith("localhost:") || | ||
| audience.startsWith("127.") || | ||
| audience.startsWith("0.") || | ||
| audience.startsWith("[::1]") | ||
| ) { | ||
| return `http://${audience}`; | ||
| } | ||
|
|
||
| if (audience.startsWith("api.")) { | ||
| return `https://${audience}`; | ||
| } | ||
|
|
||
| return `https://api.${audience}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Add focused tests for JWT audience/issuer derivation paths.
This core config change rewires auth-critical jwtAudience() / jwtIssuer() behavior, but no tests are included for these new branches (explicit env override, scheme stripping, localhost/IP handling, api.*, fallback).
As per coding guidelines: “All changes to src/core/ must include tests”.
🤖 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 `@src/core/configuration/DefaultConfig.ts` around lines 109 - 154, Add unit
tests covering all jwtAudience()/jwtIssuer() derivation branches: test explicit
overrides using Env.JWT_AUDIENCE and Env.JWT_ISSUER, scheme stripping when
parseApiDomain() returns a URL (use Env.API_DOMAIN), audience values that
already include http/https, localhost and IP forms (e.g., "localhost",
"localhost:3000", "127.0.0.1", "[::1]"), audiences starting with "api.", and the
default fallback; exercise DefaultConfig.jwtAudience, DefaultConfig.jwtIssuer
and parseApiDomain behavior by stubbing Env variables and asserting expected
outputs for each branch.
| import { randomBytes, randomUUID } from "crypto"; | ||
| import { exportJWK, generateKeyPair, SignJWT } from "jose"; | ||
| import { getServerConfigFromServer } from "../core/configuration/ConfigLoader"; | ||
| import { TokenPayloadSchema, UserMeResponse } from "../core/ApiSchemas"; |
Contributor
There was a problem hiding this comment.
Remove unused import TokenPayloadSchema.
ESLint flags this as unused. Clean it up to avoid confusion.
Proposed fix
-import { TokenPayloadSchema, UserMeResponse } from "../core/ApiSchemas";
+import { UserMeResponse } from "../core/ApiSchemas";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { TokenPayloadSchema, UserMeResponse } from "../core/ApiSchemas"; | |
| import { UserMeResponse } from "../core/ApiSchemas"; |
🧰 Tools
🪛 ESLint
[error] 6-6: 'TokenPayloadSchema' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 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 `@src/server/AuthRoutes.ts` at line 6, Remove the unused import
TokenPayloadSchema from the import list in AuthRoutes.ts so only used symbols
(e.g., UserMeResponse) are imported; update the import line that currently reads
import { TokenPayloadSchema, UserMeResponse } from "../core/ApiSchemas" to omit
TokenPayloadSchema to satisfy ESLint and avoid confusion.
Comment on lines
+14
to
+29
| const refreshTokens = new Map< | ||
| string, | ||
| { | ||
| persistentId: string; | ||
| email: string; | ||
| expiresAt: number; | ||
| } | ||
| >(); | ||
| const usersByEmail = new Map< | ||
| string, | ||
| { | ||
| email: string; | ||
| persistentId: string; | ||
| publicId: string; | ||
| } | ||
| >(); |
Contributor
There was a problem hiding this comment.
In-memory token/user storage will not survive restarts and breaks horizontal scaling.
Both refreshTokens and usersByEmail maps are process-local. When the server restarts, all sessions are lost. If you run multiple master instances behind a load balancer, users will get "invalid token" errors when requests hit a different instance.
Consider using Redis or a database for production deployments.
🤖 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 `@src/server/AuthRoutes.ts` around lines 14 - 29, The in-memory Maps
refreshTokens and usersByEmail in AuthRoutes.ts are process-local and must be
replaced with a persistent/shared store to survive restarts and scale
horizontally; implement a small storage abstraction (e.g., TokenStore/UserStore
interface) and replace direct Map accesses in functions that read/write
refreshTokens and usersByEmail with async calls to a Redis-backed or DB-backed
implementation that supports TTL for tokens and atomic operations for
lookups/invalidations, migrate existing code paths to await the new store
methods, and provide a default in-memory fallback for local dev only.
| } | ||
| >(); | ||
|
|
||
| const magic = new Magic(magicSecretKey); |
Contributor
There was a problem hiding this comment.
Magic SDK is initialized even when MAGIC_SECRET_KEY is empty.
If the key is missing, magic.users.getMetadataByToken() will fail at runtime with a confusing error. Consider failing fast at startup if this is a required dependency.
Proposed fix
const magicSecretKey = process.env.MAGIC_SECRET_KEY ?? "";
+if (!magicSecretKey) {
+ console.warn("MAGIC_SECRET_KEY not set; auth routes will reject logins");
+}
const magic = new Magic(magicSecretKey);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const magic = new Magic(magicSecretKey); | |
| const magicSecretKey = process.env.MAGIC_SECRET_KEY ?? ""; | |
| if (!magicSecretKey) { | |
| console.warn("MAGIC_SECRET_KEY not set; auth routes will reject logins"); | |
| } | |
| const magic = new Magic(magicSecretKey); |
🤖 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 `@src/server/AuthRoutes.ts` at line 31, The code instantiates the Magic SDK
unconditionally (const magic = new Magic(magicSecretKey)) which will cause
confusing runtime failures when magicSecretKey (MAGIC_SECRET_KEY) is empty;
update the startup logic to validate magicSecretKey before creating the Magic
instance and fail fast with a clear error (throw or process.exit) if it's
missing, or guard all uses of magic (e.g., magic.users.getMetadataByToken)
behind a conditional that checks the key so initialization only occurs when a
valid key is present.
| @@ -1,4 +1,5 @@ | |||
| import compression from "compression"; | |||
| import cookieParser from "cookie-parser"; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Worker.ts or its imported modules access req.cookies
rg -n 'req\.cookies' src/server/Worker.ts
rg -n 'req\.cookies' src/server/GameManager.ts src/server/Client.ts src/server/WorkerLobbyService.ts 2>/dev/null || echo "Files not found or no matches"Repository: openfrontio/OpenFrontIO
Length of output: 97
🏁 Script executed:
# 1. Verify line 54 and the middleware setup
sed -n '50,60p' src/server/Worker.ts
# 2. Check if cookie-parser is used elsewhere or just added
rg -n 'cookie-parser' src/server/
# 3. Verify Master.ts has auth routes that Worker.ts lacks
echo "=== Checking Master.ts for auth routes ==="
rg -n 'router.post.*auth|router.get.*auth|router.post.*login|router.get.*login' src/server/Master.ts | head -5
echo "=== Checking Worker.ts for auth routes ==="
rg -n 'router.post.*auth|router.get.*auth|router.post.*login|router.get.*login' src/server/Worker.ts
# 4. Search for any TODO or comments about future cookie usage
rg -n -i 'TODO|FIXME|future.*cookie|cookie.*future' src/server/Worker.ts src/server/GameManager.tsRepository: openfrontio/OpenFrontIO
Length of output: 587
🏁 Script executed:
# Check if auth is handled differently in Master.ts
rg -n "app.use|router\." src/server/Master.ts | head -20
# Check for any cookie usage in Worker WebSocket handling
rg -n 'wss\.|socket\.handshake|cookie' src/server/Worker.ts | head -10
# Search for any usage of express app in Worker that might need cookies
rg -n 'authenticate|auth|login|session' src/server/Worker.tsRepository: openfrontio/OpenFrontIO
Length of output: 1138
🏁 Script executed:
# Check Master.ts for cookie-parser usage and auth routes
rg -n 'cookie-parser|cookieParser' src/server/Master.ts
# Check Master.ts middleware setup more completely
head -100 src/server/Master.ts | grep -n "app.use"
# Confirm Worker.ts authentication method
rg -n 'authorization|Bearer' src/server/Worker.ts | head -5Repository: openfrontio/OpenFrontIO
Length of output: 420
Remove unused cookie-parser middleware from Worker.
Worker.ts uses Bearer token authentication (reading from req.headers.authorization), not cookies. The cookie-parser middleware is added at line 54 but req.cookies is never accessed in Worker or its imported modules. This adds unnecessary overhead to every request in the Worker process. Master.ts doesn't even use this middleware.
Remove the import on line 2 and the app.use(cookieParser()) call on line 54.
🤖 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 `@src/server/Worker.ts` at line 2, Remove the unused cookie-parser middleware
from Worker.ts: delete the import cookieParser and remove the
app.use(cookieParser()) call so Worker no longer loads cookie parsing on every
request; search for the symbols cookieParser and the app.use(cookieParser())
invocation in Worker.ts (and ensure no other modules rely on req.cookies) before
committing the removal.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
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
3 participants
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.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME