feat: Google OAuth 기반 학교 이메일 로그인 시스템 추가#8
Open
dudrjs125-byte wants to merge 13 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughAdds a Vite+React client and a TypeScript Express server implementing Google OAuth flows, JWT access/refresh token handling, in-memory repositories, auth routes and middleware, structured error types, and two SQL migrations (initial schema and case-insensitive uniqueness). Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as React Client
participant Server as Express Server
participant Google as Google OAuth
participant Repo as In-Memory Repo
User->>Client: Click "Login"
Client->>Server: POST /api/auth/login
Server->>Repo: save OAuth state (state, rememberMe, expiresAt)
Server-->>Client: { authUrl, state }
Client->>Google: Redirect to authUrl
Google->>User: User authenticates / consents
Google-->>Client: Redirect back with code & state
Client->>Server: GET /api/auth/callback?code&state
Server->>Repo: validate & delete state
Server->>Google: POST token exchange (code)
Google-->>Server: return tokens (access_token, id_token?)
alt id_token contains user info
Server->>Server: extract user info from id_token
else
Server->>Google: GET userinfo (access_token)
Google-->>Server: return user info
end
Server->>Server: validate email/domain, upsert user
Server->>Server: generate access & refresh tokens
Server->>Repo: store hashed refresh token
Server-->>Client: set cookies & redirect ?login=success
Client->>Server: GET /api/auth/me (with accessToken cookie)
Server->>Server: verify access token -> return user info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
server/src/errors/error-codes.ts (1)
11-11: Prefer503 Service Unavailablefor upstream Google outages.Line 11 currently uses
500, but this case represents external provider unavailability.503communicates retryable upstream dependency failure more accurately for clients/monitoring.Suggested patch
- GOOGLE_UNAVAILABLE: { httpStatus: 500, code: 'GOOGLE_UNAVAILABLE', message: 'Google 서버에 연결할 수 없습니다' }, + GOOGLE_UNAVAILABLE: { httpStatus: 503, code: 'GOOGLE_UNAVAILABLE', message: 'Google 서버에 연결할 수 없습니다' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/errors/error-codes.ts` at line 11, The GOOGLE_UNAVAILABLE entry in error-codes.ts is using httpStatus: 500 but should represent an upstream provider outage; change the httpStatus value for the GOOGLE_UNAVAILABLE constant/object from 500 to 503 so the error accurately signals a retryable Service Unavailable response to clients/monitoring; update any places that construct responses from error codes (e.g., where error-codes.GOOGLE_UNAVAILABLE is read) if they rely on the numeric status.client/.env.example (1)
1-1: Consider a neutral example URL to avoid accidental localhost propagation.Line 1 is fine for local setup, but
.env.exampleis often copied into non-local environments. A neutral placeholder can reduce misconfiguration risk.Suggested patch
-VITE_API_URL=http://localhost:3000 +VITE_API_URL=https://api.example.com🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/.env.example` at line 1, Replace the current local URL in the .env.example by using a neutral placeholder for VITE_API_URL (e.g., an example or api.example.com style URL) so consumers copying the file don't accidentally propagate localhost; update the VITE_API_URL entry in the example file to a non-localhost placeholder and add a brief comment if needed to indicate it's a placeholder for the real API URL.server/src/repositories/refresh-token-repository.ts (1)
3-8: Plan a shared backing store before deploying this auth flow.This repository is fine for local/dev use, but refresh tokens stored only in memory will be invalidated on restart and won't work correctly once the app is replicated behind a load balancer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/repositories/refresh-token-repository.ts` around lines 3 - 8, The InMemoryRefreshTokenRepository currently keeps tokens only in a Map (tokens) which will be lost on process restart and doesn't work for multi-instance deployments; replace it with or add a production-backed implementation by making InMemoryRefreshTokenRepository accept an injected persistent store client (e.g., Redis or a DB) in its constructor and implement save, findByTokenHash, and deleteByUserId to read/write to that store (or add a new PersistentRefreshTokenRepository class that implements RefreshTokenRepository and uses the persistent client), and ensure the DI/creation site selects the persistent implementation in non-dev environments; update code paths that construct InMemoryRefreshTokenRepository to supply the persistent client or swap to the new PersistentRefreshTokenRepository.server/src/repositories/user-repository.ts (1)
5-6: Return a copy fromfindByEmail()too.
create()already clones on write, butfindByEmail()returns the storedUserinstance. Any caller mutation will silently change repository state outside the repository API. Clone on read so this in-memory repository behaves like a real persistence boundary.♻️ Possible fix
- async findByEmail(email: string) { return this.users.get(email.toLowerCase()) ?? null; } + async findByEmail(email: string) { + const user = this.users.get(email.toLowerCase()); + return user ? { ...user } : null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/repositories/user-repository.ts` around lines 5 - 6, findByEmail currently returns the stored User instance from the users Map, allowing callers to mutate repository state; update the findByEmail method to return a cloned copy (e.g., shallow copy via object spread) of the stored user (like how create clones on write) and still return null when not found so the in-memory repository enforces a persistence boundary.server/migrations/001_create_tables.sql (1)
6-8: Drop the indexes already implied byUNIQUE.
"user"(email)andrefresh_token(token)already get btree indexes from the unique constraints on Lines 3 and 5. Keeping Lines 6 and 8 adds duplicate indexes and extra write cost for no benefit; onlyrefresh_token(user_id)needs a separate index here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migrations/001_create_tables.sql` around lines 6 - 8, Remove the redundant index creations for user.email and refresh_token.token: the UNIQUE constraints on "user"(email) and refresh_token(token) already create btree indexes, so delete the CREATE INDEX statements that define idx_user_email and idx_refresh_token_token; keep only the CREATE INDEX for refresh_token(user_id) (idx_refresh_token_user_id) to preserve the necessary lookup index.server/src/services/auth-service.ts (1)
45-45: Keep token hashing behind the injected abstraction.
AuthServiceImplusesTokenServiceeverywhere else, but Lines 45 and 54 reach back intoTokenServiceImpl. That makes the service harder to swap or test with another token implementation. Prefer exposing hashing on the interface or moving it to a separate injected helper.Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/services/auth-service.ts` at line 45, AuthServiceImpl is directly calling TokenServiceImpl.hashToken (at the refresh token hash sites) which breaks the TokenService abstraction; add a hashToken method to the TokenService interface (or introduce a new injected TokenHasher helper) and use the injected instance inside AuthServiceImpl instead of calling TokenServiceImpl.hashToken directly—update the TokenService definition, its implementations to expose hashToken, and replace the direct TokenServiceImpl.hashToken calls in AuthServiceImpl (the places handling refreshToken hashing) to call the injected tokenService.hashToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/app/App.tsx`:
- Around line 9-28: The current useEffect in App.tsx reads accessToken (and
email/name) from URL query params and treats that as authentication (setting
setUser and calling replaceState), which leaks tokens and allows spoofing;
instead, stop parsing tokens from URL in useEffect and have authentication
handled via Secure, HttpOnly cookies or server-side sessions so the client only
receives a short success/error flag; update the callback flow (server callback
handler that currently appends accessToken/refreshToken to the redirect URL) to
set Secure HttpOnly cookies or create a backend session and redirect with a
minimal opaque indicator (e.g., ?auth=success or ?error=...) so App.tsx’s
useEffect only reads that indicator and calls setUser after fetching user info
from a protected endpoint (e.g., call /api/me) rather than trusting URL params,
and remove any logic that relies on accessToken in the query (references:
useEffect, setUser, setError, window.history.replaceState).
In `@server/.env.example`:
- Around line 4-5: The constructor in server/src/services/token-service.ts
currently lets empty JWT secrets slip through because it checks
(!accessTokenSecret && !process.env.ACCESS_TOKEN_SECRET); update the validation
for both accessTokenSecret and refreshTokenSecret (in the TokenService
constructor) to reject empty strings by either reusing the requireEnv() pattern
from server/src/app.ts or by explicitly checking if a value is falsy or blank
(e.g., if (!value || value.trim() === '') ) and throw an Error with a clear
message; then assign using the non-empty value (accessTokenSecret ??
process.env.ACCESS_TOKEN_SECRET) / (refreshTokenSecret ??
process.env.REFRESH_TOKEN_SECRET).
In `@server/migrations/001_create_tables.sql`:
- Around line 2-3: The UNIQUE constraints on allowed_domain.domain and
"user".email/"user".school_domain are case-sensitive; add case-insensitive
uniqueness by enabling the citext extension or creating functional unique
indexes using lower() — e.g., enable citext and change columns to citext or
create UNIQUE INDEX on lower(domain), lower(email), and lower(school_domain) for
the allowed_domain and "user" tables (target symbols: allowed_domain.domain,
"user".email, "user".school_domain) so logical duplicates differing only by case
are prevented.
In `@server/package.json`:
- Around line 13-18: The devDependency `@types/express` is at major v5 while
runtime "express" is v4; update the `@types/express` entry in package.json to a v4
major release (e.g., change "@types/express": "^5.0.0" to a compatible "^4.x"
version) so types match the runtime, then run your package manager to update
lockfile/node_modules (npm/yarn install) and re-run TypeScript compilation to
confirm route/middleware signature errors are resolved.
In `@server/src/app.ts`:
- Around line 30-34: The current wiring uses process-local/auth-in-memory state
(EmailValidatorImpl, GoogleOAuthClientImpl, TokenServiceImpl, AuthServiceImpl,
createAuthRouter) which will fail across restarts or multi-instance deployments;
change the wiring to inject and use the persistent repository implementations
added in the PR (e.g., OAuthStateRepository, RefreshTokenRepository or
AuthRepository) into AuthServiceImpl (or create a DB-backed TokenServiceImpl and
OAuthState store) and ensure createAuthRouter receives the AuthServiceImpl that
uses those repositories so OAuth state and refresh-token validation are
persisted and shared across processes.
- Around line 11-14: The default sampleAllowedDomains array in server/src/app.ts
(symbol: sampleAllowedDomains) makes createApp() permissive by allowing
'school.ac.kr' and 'kookmin.ac.kr'; remove or replace that hard-coded sample
list with a safe default (e.g., an empty array) and ensure createApp(...)
requires or validates the provided allowedDomains parameter (symbol: createApp)
so no placeholder domains are accepted silently—if allowedDomains is missing or
empty, either fail fast or require explicit opt-in via configuration and
document the change.
In `@server/src/routes/callback.ts`:
- Around line 13-15: Do not send tokens/PII in the query string: change the
callback flow in the handler that calls authService.handleCallback and currently
does res.redirect(`${FRONTEND_URL}?${params}`) so that you do not append
result.accessToken, result.refreshToken, userId, email or name to
URLSearchParams; instead set a secure, SameSite HttpOnly cookie (or a
short-lived one-time exchange code tied to server session) with the access token
and only redirect to FRONTEND_URL without sensitive query params, or return the
one-time code and have the frontend fetch tokens from a server endpoint; ensure
refreshToken is never exposed in redirects and remove use of URLSearchParams for
secrets.
In `@server/src/routes/login.ts`:
- Around line 6-8: The handler destructures rememberMe directly from req.body
which can throw if body is undefined; update the returned async function (the
login route handler) to first guard that req.body exists and then derive
rememberMe with a safe default (e.g., const rememberMe = Boolean(req.body &&
req.body.rememberMe) or using optional chaining and defaulting to false) before
calling authService.initiateLogin(rememberMe); ensure the guard happens
outside/above the try block so no property access occurs on undefined and
preserve the existing try/catch around the authService call.
In `@server/src/routes/refresh.ts`:
- Around line 6-9: The route currently destructures refreshToken from req.body
which can throw if req.body is undefined or not an object; update the handler
that reads req.body/refreshToken to first verify req.body is a non-null object
(e.g., typeof req.body === 'object' && req.body !== null) before accessing
refreshToken, then only destructure or read refreshToken and validate it's a
string; on invalid input return the same 400 JSON using createErrorResponse(new
AppError('MISSING_REQUIRED_FIELD')) and keep using
authService.refreshToken(refreshToken) when valid.
In `@server/src/services/auth-service.ts`:
- Around line 16-18: The in-memory Maps oauthStates and refreshTokens in
AuthServiceImpl are unsafe across processes and never expire; replace them with
a shared, TTL-backed repository (e.g., Redis or a persistent store) and inject
that repository into AuthServiceImpl instead of using new Map()s; implement
repository methods used by the service (createState/getState/deleteState and
createRefreshToken/getRefreshToken/deleteRefreshToken) with automatic expiration
and use those in the code paths that currently reference oauthStates and
refreshTokens (the state validation and refresh-token logic), and update
server/src/app.ts to wire a single shared repository instance into
AuthServiceImpl.
In `@server/src/services/email-validator.ts`:
- Around line 18-22: The current extractDomain implementation incorrectly allows
multiple '@' and spaces in the local part; update extractDomain to ensure
exactly one '@' (e.g., count occurrences or compare indexOf and lastIndexOf) and
validate the local part (the substring before atIndex on the trimmed string) is
non-empty and contains no whitespace characters; keep the existing domain checks
(variable domain) and return domain.toLowerCase() after these additional
validations.
In `@server/src/services/google-oauth-client.ts`:
- Around line 24-33: The current exchangeCodeForToken and getUserInfo functions
treat all non-2xx responses as permanent AUTH_FAILED; change them to treat
transient upstream errors (HTTP 429 and any 5xx) as temporary Google failures by
throwing AppError('GOOGLE_UNAVAILABLE') so callers can retry with backoff, while
preserving AppError('AUTH_FAILED') for client/auth-related 4xx responses (e.g.,
400/401/403). Locate exchangeCodeForToken and getUserInfo in
google-oauth-client.ts, inspect the response.ok branch and replace the single
AUTH_FAILED throw with a status-based decision: if response.status === 429 ||
(response.status >= 500 && response.status < 600) throw
AppError('GOOGLE_UNAVAILABLE'), else throw AppError('AUTH_FAILED'); keep
existing network catch behavior intact.
In `@server/src/services/token-service.ts`:
- Around line 19-20: The current verifyAccessToken and verifyRefreshToken
methods force-cast jwt.verify results and use non-null assertions, risking
undefined claims; implement a shared helper (e.g., private verifyToken(token:
string, secret: string): TokenPayload | null) that calls jwt.verify, guards
against string payloads, extracts userId, email, iat, exp and validates their
types (userId/email are strings, iat/exp are numbers), returning null if
validation fails or on exception, and then have verifyAccessToken and
verifyRefreshToken delegate to this helper to return a validated TokenPayload or
null.
In `@server/src/types/user-info.ts`:
- Around line 1-4: Extend the UserInfo model to include hosted-domain and
email-verification claims by adding a method (e.g., decodeAndVerifyIdToken or
verifyIdTokenClaims) that takes the raw id_token, decodes and validates its
signature against the provider JWKS, checks standard claims (exp, aud, iss), and
extracts hd and email_verified into the UserInfo object; update the UserInfo
interface (UserInfo) to include hd?: string and email_verified?: boolean and
ensure the token-verification method is used wherever tokens are exchanged so
domain enforcement uses the verified hd claim instead of parsing the email
string.
In `@server/tsconfig.json`:
- Around line 4-5: The tsconfig currently uses "moduleResolution": "bundler",
which allows extensionless relative imports that will fail under Node's ESM
runtime; change the "moduleResolution" compiler option value from "bundler" to
"NodeNext" in tsconfig.json so TypeScript enforces Node-style ESM resolution
(catching missing .js extensions and mismatched imports at compile time) and
ensure the "module" remains ES2022 to match runtime ESM semantics.
---
Nitpick comments:
In `@client/.env.example`:
- Line 1: Replace the current local URL in the .env.example by using a neutral
placeholder for VITE_API_URL (e.g., an example or api.example.com style URL) so
consumers copying the file don't accidentally propagate localhost; update the
VITE_API_URL entry in the example file to a non-localhost placeholder and add a
brief comment if needed to indicate it's a placeholder for the real API URL.
In `@server/migrations/001_create_tables.sql`:
- Around line 6-8: Remove the redundant index creations for user.email and
refresh_token.token: the UNIQUE constraints on "user"(email) and
refresh_token(token) already create btree indexes, so delete the CREATE INDEX
statements that define idx_user_email and idx_refresh_token_token; keep only the
CREATE INDEX for refresh_token(user_id) (idx_refresh_token_user_id) to preserve
the necessary lookup index.
In `@server/src/errors/error-codes.ts`:
- Line 11: The GOOGLE_UNAVAILABLE entry in error-codes.ts is using httpStatus:
500 but should represent an upstream provider outage; change the httpStatus
value for the GOOGLE_UNAVAILABLE constant/object from 500 to 503 so the error
accurately signals a retryable Service Unavailable response to
clients/monitoring; update any places that construct responses from error codes
(e.g., where error-codes.GOOGLE_UNAVAILABLE is read) if they rely on the numeric
status.
In `@server/src/repositories/refresh-token-repository.ts`:
- Around line 3-8: The InMemoryRefreshTokenRepository currently keeps tokens
only in a Map (tokens) which will be lost on process restart and doesn't work
for multi-instance deployments; replace it with or add a production-backed
implementation by making InMemoryRefreshTokenRepository accept an injected
persistent store client (e.g., Redis or a DB) in its constructor and implement
save, findByTokenHash, and deleteByUserId to read/write to that store (or add a
new PersistentRefreshTokenRepository class that implements
RefreshTokenRepository and uses the persistent client), and ensure the
DI/creation site selects the persistent implementation in non-dev environments;
update code paths that construct InMemoryRefreshTokenRepository to supply the
persistent client or swap to the new PersistentRefreshTokenRepository.
In `@server/src/repositories/user-repository.ts`:
- Around line 5-6: findByEmail currently returns the stored User instance from
the users Map, allowing callers to mutate repository state; update the
findByEmail method to return a cloned copy (e.g., shallow copy via object
spread) of the stored user (like how create clones on write) and still return
null when not found so the in-memory repository enforces a persistence boundary.
In `@server/src/services/auth-service.ts`:
- Line 45: AuthServiceImpl is directly calling TokenServiceImpl.hashToken (at
the refresh token hash sites) which breaks the TokenService abstraction; add a
hashToken method to the TokenService interface (or introduce a new injected
TokenHasher helper) and use the injected instance inside AuthServiceImpl instead
of calling TokenServiceImpl.hashToken directly—update the TokenService
definition, its implementations to expose hashToken, and replace the direct
TokenServiceImpl.hashToken calls in AuthServiceImpl (the places handling
refreshToken hashing) to call the injected tokenService.hashToken.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59b5a363-e7d6-4ab0-9363-0206e69a722a
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonserver/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
client/.env.exampleclient/index.htmlclient/package.jsonclient/postcss.config.mjsclient/src/app/App.tsxclient/src/main.tsxclient/src/styles/fonts.cssclient/src/styles/index.cssclient/src/styles/tailwind.cssclient/src/styles/theme.cssclient/vite.config.tsserver/.env.exampleserver/migrations/001_create_tables.sqlserver/package.jsonserver/src/app.tsserver/src/errors/app-error.tsserver/src/errors/error-codes.tsserver/src/errors/error-response.tsserver/src/errors/index.tsserver/src/interfaces/auth-service.tsserver/src/interfaces/email-validator.tsserver/src/interfaces/google-oauth-client.tsserver/src/interfaces/index.tsserver/src/interfaces/token-service.tsserver/src/middleware/error-handler.tsserver/src/repositories/allowed-domain-repository.tsserver/src/repositories/index.tsserver/src/repositories/oauth-state-repository.tsserver/src/repositories/refresh-token-repository.tsserver/src/repositories/user-repository.tsserver/src/routes/callback.tsserver/src/routes/index.tsserver/src/routes/login.tsserver/src/routes/refresh.tsserver/src/server.tsserver/src/services/auth-service.tsserver/src/services/email-validator.tsserver/src/services/google-oauth-client.tsserver/src/services/index.tsserver/src/services/token-service.tsserver/src/types/allowed-domain.tsserver/src/types/auth-result.tsserver/src/types/index.tsserver/src/types/oauth-state.tsserver/src/types/oauth-token-response.tsserver/src/types/refresh-token.tsserver/src/types/token-payload.tsserver/src/types/user-info.tsserver/src/types/user.tsserver/tsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/routes/me.ts`:
- Around line 8-9: The current check treats any falsy payload from
tokenService.verifyAccessToken(token) as TOKEN_EXPIRED; update the logic to
avoid misclassifying all verification failures by either: 1) changing the
response to a neutral authentication error (e.g., AUTH_INVALID or AUTH_FAILED)
when payload is null, or 2) modify TokenService.verifyAccessToken to return an
object with a failure reason (e.g., { ok: false, reason: 'EXPIRED' | 'MALFORMED'
| 'INVALID_SIGNATURE' }) and then map those reasons to appropriate responses
(only return TOKEN_EXPIRED for explicit expiration reason). Locate the call site
tokenService.verifyAccessToken and the TOKEN_EXPIRED response in the me route
and implement one of the two fixes so clients receive an accurate error type.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bebf3ea-1cb9-4231-9a00-4497556cd347
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
client/src/app/App.tsxserver/package.jsonserver/src/app.tsserver/src/routes/callback.tsserver/src/routes/index.tsserver/src/routes/me.ts
✅ Files skipped from review due to trivial changes (1)
- server/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- server/src/routes/index.ts
- server/src/app.ts
- client/src/app/App.tsx
- server/src/routes/callback.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
server/src/routes/login.ts (1)
7-8:⚠️ Potential issue | 🟠 MajorGuard
req.bodybefore destructuring to prevent pre-tryruntime failure.At Line 7,
const { rememberMe } = req.body;can throw ifbodyis missing, and that happens before yourtry/catch. Safely default body first, then deriverememberMe.Proposed fix
export function createLoginHandler(authService: AuthService) { return async (req: { body: Record<string, unknown> }, res: { status: (code: number) => { json: (body: unknown) => void }; json: (body: unknown) => void }): Promise<void> => { - const { rememberMe } = req.body; - try { res.json(await authService.initiateLogin(rememberMe === true)); } + const body = req.body ?? {}; + const rememberMe = body.rememberMe === true; + try { res.json(await authService.initiateLogin(rememberMe)); } catch (error) { if (error instanceof AppError) res.status(error.httpStatus).json(createErrorResponse(error)); else res.status(500).json(createErrorResponse(new AppError('INTERNAL_ERROR'))); } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/routes/login.ts` around lines 7 - 8, Guard against missing req.body before destructuring: replace the top-level destructure of rememberMe from req.body with a safe default (e.g., set const body = req.body ?? {} or use (req.body || {}) when deriving rememberMe) or move the destructuring inside the existing try block so a missing body doesn't throw before the try/catch; ensure you still call authService.initiateLogin(rememberMe === true) with the guarded rememberMe value.server/src/app.ts (1)
31-35:⚠️ Potential issue | 🟠 MajorAuth state remains process-local and can break in multi-instance deployments.
Line 34 wires
AuthServiceImpl, andserver/src/services/auth-service.ts:12-22still keeps OAuth state and refresh-token data in in-memoryMaps. This will fail across restarts or when login/callback/refresh hit different instances. Please wire persistent repository-backed storage for OAuth state and refresh tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/app.ts` around lines 31 - 35, Auth state and refresh tokens are stored in in-memory Maps inside AuthServiceImpl (and TokenServiceImpl), which breaks in multi-instance or restart scenarios; introduce repository interfaces (e.g., RefreshTokenRepository, OAuthStateRepository) and concrete persistent implementations (DB-backed) and change AuthServiceImpl and TokenServiceImpl to accept these repositories via their constructors; then update server/src/app.ts to instantiate the persistent repository implementations and pass them into new AuthServiceImpl(...) and new TokenServiceImpl(...) (and ensure GoogleOAuthClientImpl still uses the OAuthStateRepository), and update createAuthRouter(authService, tokenService) wiring to use the repository-backed services so state and refresh tokens are persisted across instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/migrations/002_case_insensitive_unique.sql`:
- Around line 8-21: The migration's UPDATE that lowercases "user".email and
allowed_domain.domain can fail if case-variant duplicates exist (e.g.,
User@example.com vs user@example.com); add a pre-check before the normalization
step that queries for duplicates using LOWER(email) and LOWER(domain) (group by
the lowercased value and HAVING count(*) > 1) for the "user" and allowed_domain
tables, and abort the migration with a clear error listing offending row
identifiers (or instruct maintainers how to resolve/merge them) if any
duplicates are found; place this check before the UPDATE statements that modify
email and domain and before dropping constraints/indexes so the migration only
proceeds when duplicates are resolved.
In `@server/src/repositories/refresh-token-repository.ts`:
- Around line 5-6: The save method currently accepts any token.token value which
breaks the invariant expected by findByTokenHash; update
RefreshTokenRepository.save(token: RefreshToken) to validate that token.token is
a hashed value before storing (e.g., add an isTokenHash helper that checks the
format/length/charset or required prefix for your chosen hash scheme) and throw
a clear error if validation fails so plaintext tokens cannot be persisted; keep
the storage key as token.token to stay compatible with findByTokenHash and
add/update tests to cover both valid-hash saves and attempted plaintext saves.
---
Duplicate comments:
In `@server/src/app.ts`:
- Around line 31-35: Auth state and refresh tokens are stored in in-memory Maps
inside AuthServiceImpl (and TokenServiceImpl), which breaks in multi-instance or
restart scenarios; introduce repository interfaces (e.g.,
RefreshTokenRepository, OAuthStateRepository) and concrete persistent
implementations (DB-backed) and change AuthServiceImpl and TokenServiceImpl to
accept these repositories via their constructors; then update server/src/app.ts
to instantiate the persistent repository implementations and pass them into new
AuthServiceImpl(...) and new TokenServiceImpl(...) (and ensure
GoogleOAuthClientImpl still uses the OAuthStateRepository), and update
createAuthRouter(authService, tokenService) wiring to use the repository-backed
services so state and refresh tokens are persisted across instances.
In `@server/src/routes/login.ts`:
- Around line 7-8: Guard against missing req.body before destructuring: replace
the top-level destructure of rememberMe from req.body with a safe default (e.g.,
set const body = req.body ?? {} or use (req.body || {}) when deriving
rememberMe) or move the destructuring inside the existing try block so a missing
body doesn't throw before the try/catch; ensure you still call
authService.initiateLogin(rememberMe === true) with the guarded rememberMe
value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c968c02-acf2-4024-8d5d-1298e14bb25c
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
server/migrations/002_case_insensitive_unique.sqlserver/package.jsonserver/src/app.tsserver/src/errors/app-error.tsserver/src/errors/error-response.tsserver/src/errors/index.tsserver/src/interfaces/auth-service.tsserver/src/interfaces/google-oauth-client.tsserver/src/interfaces/index.tsserver/src/interfaces/token-service.tsserver/src/middleware/error-handler.tsserver/src/repositories/allowed-domain-repository.tsserver/src/repositories/index.tsserver/src/repositories/oauth-state-repository.tsserver/src/repositories/refresh-token-repository.tsserver/src/repositories/user-repository.tsserver/src/routes/callback.tsserver/src/routes/index.tsserver/src/routes/login.tsserver/src/routes/me.tsserver/src/routes/refresh.tsserver/src/server.tsserver/src/services/auth-service.tsserver/src/services/email-validator.tsserver/src/services/google-oauth-client.tsserver/src/services/index.tsserver/src/services/token-service.tsserver/src/types/index.tsserver/src/types/token-verify-result.tsserver/src/types/user-info.tsserver/tsconfig.json
✅ Files skipped from review due to trivial changes (12)
- server/tsconfig.json
- server/src/types/token-verify-result.ts
- server/package.json
- server/src/interfaces/index.ts
- server/src/interfaces/auth-service.ts
- server/src/routes/refresh.ts
- server/src/errors/index.ts
- server/src/services/index.ts
- server/src/errors/app-error.ts
- server/src/errors/error-response.ts
- server/src/repositories/user-repository.ts
- server/src/repositories/index.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- server/src/types/user-info.ts
- server/src/server.ts
- server/src/routes/index.ts
- server/src/middleware/error-handler.ts
- server/src/interfaces/google-oauth-client.ts
- server/src/interfaces/token-service.ts
- server/src/services/email-validator.ts
- server/src/routes/callback.ts
- server/src/repositories/allowed-domain-repository.ts
- server/src/repositories/oauth-state-repository.ts
- server/src/types/index.ts
- server/src/services/token-service.ts
- server/src/services/auth-service.ts
- server/src/services/google-oauth-client.ts
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
server/.env.example (1)
4-5:⚠️ Potential issue | 🔴 Critical빈 JWT 시크릿 값이 런타임에서 통과될 수 있어 서명 보안이 깨질 수 있습니다.
server/src/services/token-service.ts의 현재 검증(if (!accessTokenSecret && !process.env.ACCESS_TOKEN_SECRET))은 빈 문자열("")을 막지 못합니다. 이.env.example구조(빈 값 템플릿)와 결합되면 매우 취약한 시크릿으로 서비스가 뜰 수 있습니다.
trim()기반 비어있음 검증으로 강제해 주세요.제안 diff (token-service)
- if (!accessTokenSecret && !process.env.ACCESS_TOKEN_SECRET) throw new Error('ACCESS_TOKEN_SECRET 미설정'); - if (!refreshTokenSecret && !process.env.REFRESH_TOKEN_SECRET) throw new Error('REFRESH_TOKEN_SECRET 미설정'); - this.accessTokenSecret = accessTokenSecret ?? process.env.ACCESS_TOKEN_SECRET!; - this.refreshTokenSecret = refreshTokenSecret ?? process.env.REFRESH_TOKEN_SECRET!; + const resolvedAccess = accessTokenSecret ?? process.env.ACCESS_TOKEN_SECRET; + const resolvedRefresh = refreshTokenSecret ?? process.env.REFRESH_TOKEN_SECRET; + if (!resolvedAccess || resolvedAccess.trim() === '') throw new Error('ACCESS_TOKEN_SECRET 미설정'); + if (!resolvedRefresh || resolvedRefresh.trim() === '') throw new Error('REFRESH_TOKEN_SECRET 미설정'); + this.accessTokenSecret = resolvedAccess; + this.refreshTokenSecret = resolvedRefresh;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/.env.example` around lines 4 - 5, The JWT secret validation in server/src/services/token-service.ts currently allows empty strings because it checks (!accessTokenSecret && !process.env.ACCESS_TOKEN_SECRET); change the checks to validate trimmed values (e.g., use accessTokenSecret?.trim() and process.env.ACCESS_TOKEN_SECRET?.trim()) and reject if the resulting string is empty or missing, and do the same for the refresh secret; ensure you derive the effective secret (accessTokenSecret ?? process.env.ACCESS_TOKEN_SECRET) or validate both sources individually, then throw/log and abort when the trimmed value is falsy so blank strings cannot pass as valid secrets.server/src/routes/login.ts (1)
6-8:⚠️ Potential issue | 🟠 MajorGuard
req.bodybefore readingrememberMe.
const { rememberMe } = req.bodycan still throw before thetry/catchruns, so a request without a parsed body bypasses this route’s intended JSON error handling.Suggested fix
export function createLoginHandler(authService: AuthService) { return async (req: { body: Record<string, unknown> }, res: { status: (code: number) => { json: (body: unknown) => void }; json: (body: unknown) => void }): Promise<void> => { - const { rememberMe } = req.body; - try { res.json(await authService.initiateLogin(rememberMe === true)); } + const body = typeof req.body === 'object' && req.body !== null + ? (req.body as Record<string, unknown>) + : {}; + const rememberMe = body.rememberMe === true; + try { res.json(await authService.initiateLogin(rememberMe)); } catch (error) { if (error instanceof AppError) res.status(error.httpStatus).json(createErrorResponse(error)); else res.status(500).json(createErrorResponse(new AppError('INTERNAL_ERROR'))); } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/routes/login.ts` around lines 6 - 8, The route handler currently destructures const { rememberMe } = req.body outside the try/catch which can throw if req.body is undefined; change the handler to guard req.body before reading it (e.g., verify req.body is present and an object or default to an empty object) or move the destructuring into the try block so any missing body is handled by the existing JSON error response; ensure the value passed into authService.initiateLogin is a boolean check (rememberMe === true) after the guard.server/src/routes/refresh.ts (1)
6-9:⚠️ Potential issue | 🟠 MajorValidate the body safely before reading
refreshToken.This still dereferences
req.bodytoo early. If the body is absent or not an object, the route throws before it can return the intended 400 JSON response.Suggested fix
export function createRefreshHandler(authService: AuthService) { return async (req: { body: Record<string, unknown> }, res: { status: (code: number) => { json: (body: unknown) => void }; json: (body: unknown) => void }): Promise<void> => { - const { refreshToken } = req.body; - if (!refreshToken || typeof refreshToken !== 'string') { res.status(400).json(createErrorResponse(new AppError('MISSING_REQUIRED_FIELD'))); return; } + const body = typeof req.body === 'object' && req.body !== null + ? (req.body as Record<string, unknown>) + : {}; + const refreshToken = typeof body.refreshToken === 'string' ? body.refreshToken.trim() : ''; + if (!refreshToken) { res.status(400).json(createErrorResponse(new AppError('MISSING_REQUIRED_FIELD'))); return; } try { res.json(await authService.refreshToken(refreshToken)); } catch (error) { if (error instanceof AppError) res.status(error.httpStatus).json(createErrorResponse(error)); else res.status(500).json(createErrorResponse(new AppError('INTERNAL_ERROR'))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/routes/refresh.ts` around lines 6 - 9, The route handler in refresh.ts reads req.body directly and can throw if body is missing or not an object; update the anonymous async handler so it first verifies req.body is present and is an object (e.g., if (!req.body || typeof req.body !== 'object') return res.status(400).json(createErrorResponse(new AppError('MISSING_REQUIRED_FIELD')))), then safely extract const { refreshToken } = req.body as Record<string, unknown> and continue with the existing typeof refreshToken === 'string' check before calling authService.refreshToken; keep using createErrorResponse and AppError for the 400 response.server/src/app.ts (1)
31-35:⚠️ Potential issue | 🟠 MajorAuth/session state is still process-local in app wiring.
This keeps OAuth state and refresh-token/session behavior tied to a single process. Restarts or multi-instance deployments can still break login/refresh flows.
Use the repository abstractions already introduced in this PR (or DB-backed implementations) when constructing
AuthServiceImpl, so auth state is shared and durable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/app.ts` around lines 31 - 35, The wiring currently creates process-local auth state by instantiating TokenServiceImpl (and passing only in-memory components) when constructing AuthServiceImpl; replace the in-memory/session-local pieces with the repo-backed implementations introduced in this PR so state is durable across processes. Concretely, instantiate the DB-backed Token/Session repository (the repository abstraction added in this PR) and pass that repository into the TokenServiceImpl (or construct a TokenService that wraps the repo-backed implementation) and into AuthServiceImpl instead of the current TokenServiceImpl, while keeping EmailValidatorImpl, GoogleOAuthClientImpl and createAuthRouter usage unchanged.server/src/services/email-validator.ts (1)
21-22:⚠️ Potential issue | 🟡 MinorWhitespace validation is still too narrow in
extractDomain().
includes(' ')only blocks ASCII spaces. Inputs with tabs/newlines in local/domain still pass format validation.🔧 Proposed fix
- if (!local || local.includes(' ')) throw new AppError('INVALID_EMAIL_FORMAT'); - if (!domain || domain.includes(' ')) throw new AppError('INVALID_EMAIL_FORMAT'); + if (!local || /\s/.test(local)) throw new AppError('INVALID_EMAIL_FORMAT'); + if (!domain || /\s/.test(domain)) throw new AppError('INVALID_EMAIL_FORMAT');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/services/email-validator.ts` around lines 21 - 22, The validation in extractDomain() currently only checks for ASCII spaces using includes(' '), so update the checks on local and domain (the variables validated in extractDomain()) to reject any Unicode whitespace by testing for general whitespace (e.g., using a \s check or equivalent) or by comparing trimmed vs original values; ensure both the local and domain branches use this broader whitespace detection and still throw AppError('INVALID_EMAIL_FORMAT') when whitespace is found.
🧹 Nitpick comments (10)
server/src/interfaces/email-validator.ts (1)
4-4:getAllowedDomains는 불변 컬렉션 반환을 고려해도 좋겠습니다.외부 호출자가 배열을 변경하지 못하도록
ReadonlyArray<string>반환 타입으로 고정하면 계약 안정성이 올라갑니다.제안 diff
export interface EmailValidator { isAllowedDomain(email: string): boolean; extractDomain(email: string): string; - getAllowedDomains(): string[]; + getAllowedDomains(): ReadonlyArray<string>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/interfaces/email-validator.ts` at line 4, getAllowedDomains currently returns a mutable string[]; change its signature to return ReadonlyArray<string> (i.e. getAllowedDomains(): ReadonlyArray<string>) and update every implementation of that interface (any classes or functions implementing getAllowedDomains) to match the new return type so callers cannot mutate the returned collection.client/src/styles/fonts.css (1)
1-1: 현재는 플레이스홀더 파일이라 의도를 주석으로 조금 더 명확히 남기면 좋겠습니다.예: “향후 커스텀 웹폰트 선언 위치” 같은 TODO를 추가하면 유지보수자가 빈 파일로 오해하지 않습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/styles/fonts.css` at line 1, Update the placeholder comment in fonts.css to clarify intent for future maintainers by replacing or augmenting "/* 폰트 설정 */" with a more descriptive TODO comment such as indicating this is the location for future custom webfont declarations (e.g., "TODO: 향후 커스텀 웹폰트 선언 위치 — 여기에 `@font-face` 규칙 및 웹폰트 로드 코드를 추가하세요") so the file isn't mistaken for an accidental empty file; keep the description short and in Korean to match the repo language.client/postcss.config.mjs (1)
1-1: 빈 PostCSS 설정은 의도 주석(또는 파일 제거) 중 하나로 명확화하는 편이 좋습니다.현재 설정 자체는 동작 가능하지만, 팀원이 “누락”으로 오해할 여지가 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/postcss.config.mjs` at line 1, The PostCSS config currently exports an empty object (export default {}), which can be mistaken for a missing config; either add a one-line explanatory comment above the export stating this file is intentionally empty/disabled (e.g., "Intentionally empty — PostCSS not used in this project") or remove the file entirely if unused; update the export/default block (export default {}) accordingly so reviewers clearly understand the intent.client/src/styles/theme.css (1)
1-1: 테마 파일은 최소한의:root토큰 스캐폴드를 두면 확장성이 좋아집니다.지금도 문제는 없지만, 기본 변수 골격을 두면 이후 테마 확장 시 충돌이 줄어듭니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/styles/theme.css` at line 1, 현재 theme.css에 전역 루트 토큰 스캐폴드를 추가해 확장성을 높이세요: theme.css의 :root 블록을 만들고 --color-bg, --color-text, --color-primary, --color-muted, --radius-base, --space-1, --space-2, --font-sans 같은 기본 CSS 커스텀 프로퍼티를 최소한으로 정의(기본값 포함)하여 테마를 확장하거나 오버라이드할 때 충돌을 줄이도록 합니다; 해당 변경은 기존 클래스/유틸리티와 호환되게 현재 파일의 주석 스타일을 유지하면서 적용하세요.client/index.html (1)
3-3: Move inline CSS out ofindex.htmlto support stricter CSP and maintainability.Embedding styles inline makes future CSP hardening (
style-srcwithout'unsafe-inline') harder. Prefer placing these rules in your existing stylesheet pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/index.html` at line 3, The inline <style> block in index.html (inside the <head> element) should be removed and its rules (html, body { height: 100%; margin: 0; } and `#root` { height: 100%; }) moved into your project's CSS pipeline (e.g., main stylesheet or app CSS module). Update index.html to reference that stylesheet (or ensure the bundler imports it) and remove the inline <style> tag so future CSP policies can disallow 'unsafe-inline' while preserving the layout behavior for the `#root` element.server/src/routes/index.ts (1)
11-14: Consider route-level throttling on auth endpoints.
/login,/callback, and/refreshare abuse-prone paths. Adding rate limiting here will improve resilience against brute-force/replay pressure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/routes/index.ts` around lines 11 - 14, Add route-level rate limiting middleware to the auth endpoints to mitigate brute-force/replay attacks: create one or more express-compatible rate limiter instances (e.g., a strict limiter for POST /login and POST /refresh and a slightly looser one for GET /callback and GET /me, or use a shared config) and apply them as middleware before the existing handlers (attach the limiter to the routes that call createLoginHandler, createCallbackHandler, createRefreshHandler, and optionally createMeHandler). Ensure the limiter is imported/constructed in the same module (or a shared middleware file) and configured to return proper 429 responses and to respect IP/client identifier logic used in the app.server/src/types/oauth-token-response.ts (1)
3-3: Optional: narrowtoken_typefor stronger provider-specific typing.If this interface is strictly for Google responses, you can tighten
token_typefromstringto'Bearer'to catch invalid assumptions earlier.Suggested patch
export interface OAuthTokenResponse { access_token: string; - token_type: string; + token_type: 'Bearer'; expires_in: number; id_token?: string; refresh_token?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/types/oauth-token-response.ts` at line 3, The token_type property in the OAuth token response interface is currently typed as string; if this type is intended to model Google responses only, narrow token_type to the literal 'Bearer' in the interface (e.g., change token_type: string to token_type: 'Bearer') and update any call sites or tests that assume a generic string accordingly so they accept the literal type; target the interface declaration (the OAuth token response type in server/src/types/oauth-token-response.ts) when making this change.server/src/middleware/error-handler.ts (1)
5-8: Consider logging unexpected errors before fallback 500.Current fallback is correct, but without logging, production incident triage will be harder.
🔎 Minimal refactor
export function errorHandler(err: unknown, _req: Request, res: Response, _next: NextFunction): void { if (err instanceof AppError) { res.status(err.httpStatus).json(createErrorResponse(err)); return; } + console.error('[errorHandler] Unhandled error', err); res.status(500).json(createErrorResponse(new AppError('INTERNAL_ERROR'))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/middleware/error-handler.ts` around lines 5 - 8, The global errorHandler currently returns a 500 for non-AppError instances without logging; update errorHandler to log unexpected errors (the err value) before sending the fallback response by calling your project's logger (e.g., processLogger.error or the configured logger) and include context like the route/handler if available; preserve the existing branches that handle AppError and use createErrorResponse(new AppError('INTERNAL_ERROR')) after logging so production triage has the original error details accessible.server/src/services/auth-service.ts (2)
49-49: Clarify the hosted domain (hd) validation logic.Line 49 checks if
userInfo.hd(Google Workspace domain) is an allowed domain, but this seems redundant since line 48 already validates the actual email domain. Thehdclaim represents the organization's domain, which may legitimately differ from the email's domain suffix.If the intent is to ensure both match, this should be explicit. If
hdvalidation is unnecessary when email is already validated, consider removing it to avoid rejecting valid users whose Workspace domain differs from their email domain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/services/auth-service.ts` at line 49, The hosted-domain (userInfo.hd) check currently duplicates email domain validation and can incorrectly reject users; either remove the hd check or make the intent explicit: if you require the hd to match the email domain, replace the call to this.emailValidator.isAllowedDomain(`user@${userInfo.hd}`) with an explicit comparison between the parsed email domain and userInfo.hd (and throw AppError('INVALID_EMAIL_DOMAIN') only when they differ), otherwise just delete the hd branch so only the validated email domain is enforced; locate this logic near userInfo.hd, emailValidator.isAllowedDomain and the AppError('INVALID_EMAIL_DOMAIN') throw to apply the change.
70-83: Consider adding a secondary index for user lookups by ID.
findUserByIdperforms a linear scan (O(n)) through all users. While acceptable for in-memory storage with small user counts, this could become a bottleneck. If this remains in-memory, consider maintaining a secondaryMap<string, User>keyed byid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/services/auth-service.ts` around lines 70 - 83, findUserById currently scans this.users which is O(n); add a secondary Map keyed by id (e.g., private usersById: Map<string, User>) and keep it in sync: when creating a user in findOrCreateUser add usersById.set(user.id, user), when returning an existing user ensure usersById.set(existing.id, existing) (or verify presence), and change findUserById to return this.usersById.get(userId). Also update any user-delete or update paths to maintain both maps in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/styles/tailwind.css`:
- Around line 1-3: Move the two `@import` rules (including "@import
'tw-animate-css'") before the "@source '../**/*.{js,ts,jsx,tsx}'" at-rule to
satisfy Stylelint's import ordering and prevent the tw-animate-css import from
being dropped; also update your Stylelint config to recognize Tailwind v4
"@source" at-rules (e.g., allow the "source" at-rule in rules like
at-rule-no-unknown or add it to the at-rule list) so the "@source" line is
permitted.
In `@server/src/repositories/user-repository.ts`:
- Around line 11-12: The create() method currently stores the user under a
lowercased key but leaves User.email as-is and returns the original/mutable
object; change create(user: User) to create and store a shallow copy with email
normalized (e.g., const normalized = { ...user, email: user.email.toLowerCase()
}), set this.users.set(normalized.email, normalized), and return a copy (or the
normalized object) so callers don't get a reference to the internal map;
similarly update findByEmail(email: string) to read from this.users using
email.toLowerCase() and return a shallow copy (or null) instead of the internal
object to avoid exposing mutable state.
In `@server/src/routes/callback.ts`:
- Around line 18-20: The cookie maxAge is hardcoded to 30 days causing stale
cookies; change the handler so handleCallback returns the refresh token expiry
(e.g., result.refreshTokenExpiresIn or result.refreshTokenExpiryTimestamp) and
use that value to set res.cookie('refreshToken', result.refreshToken, {
httpOnly: true, secure: IS_PRODUCTION, sameSite: 'lax', maxAge:
computedExpiryInMs }) with a safe fallback (keep 30 days) when the expiry is not
provided; update handleCallback's return type and the call site in callback.ts
to read that expiry and convert seconds/timestamp to milliseconds for maxAge.
In `@server/src/server.ts`:
- Around line 2-4: The PORT value should be validated and normalized before
calling app.listen: parse process.env.PORT (e.g., using parseInt or Number) and
ensure it is a positive integer; if the parsed value is NaN or <= 0, log a clear
error and exit (or fallback to the default 3000 explicitly) so you don’t
accidentally pass a non-numeric string as a Unix socket path. Update the code
that defines PORT and the call site (the PORT constant and the app.listen(...)
invocation in server.ts where createApp() is used) to perform this validation
and pass a numeric port to app.listen.
In `@server/src/services/auth-service.ts`:
- Around line 60-68: The refreshToken method is missing validation of the stored
token's expiry: after computing tokenHash with TokenServiceImpl.hashToken and
before issuing a new access token in refreshToken, fetch the stored token object
from this.refreshTokens.get(tokenHash) and verify its expiresAt timestamp is in
the future; if expired, remove it from this.refreshTokens and throw
AppError('INVALID_REFRESH_TOKEN'); only then proceed to
findUserById(payload.userId) and generate the access token.
In `@server/src/services/google-oauth-client.ts`:
- Around line 49-63: The extractUserInfoFromIdToken function currently decodes
the JWT without signature validation; replace the manual parsing with
google-auth-library verification: add google-auth-library as a dependency,
construct an OAuth2Client using this.clientId, call OAuth2Client.verifyIdToken({
idToken, audience: this.clientId }) and use ticket.getPayload() to obtain the
claims, then validate aud/iss/exp as appropriate and extract
email/name/picture/hd/email_verified; ensure any errors return null (or
propagate as before) and remove the base64url JSON decode flow.
---
Duplicate comments:
In `@server/.env.example`:
- Around line 4-5: The JWT secret validation in
server/src/services/token-service.ts currently allows empty strings because it
checks (!accessTokenSecret && !process.env.ACCESS_TOKEN_SECRET); change the
checks to validate trimmed values (e.g., use accessTokenSecret?.trim() and
process.env.ACCESS_TOKEN_SECRET?.trim()) and reject if the resulting string is
empty or missing, and do the same for the refresh secret; ensure you derive the
effective secret (accessTokenSecret ?? process.env.ACCESS_TOKEN_SECRET) or
validate both sources individually, then throw/log and abort when the trimmed
value is falsy so blank strings cannot pass as valid secrets.
In `@server/src/app.ts`:
- Around line 31-35: The wiring currently creates process-local auth state by
instantiating TokenServiceImpl (and passing only in-memory components) when
constructing AuthServiceImpl; replace the in-memory/session-local pieces with
the repo-backed implementations introduced in this PR so state is durable across
processes. Concretely, instantiate the DB-backed Token/Session repository (the
repository abstraction added in this PR) and pass that repository into the
TokenServiceImpl (or construct a TokenService that wraps the repo-backed
implementation) and into AuthServiceImpl instead of the current
TokenServiceImpl, while keeping EmailValidatorImpl, GoogleOAuthClientImpl and
createAuthRouter usage unchanged.
In `@server/src/routes/login.ts`:
- Around line 6-8: The route handler currently destructures const { rememberMe }
= req.body outside the try/catch which can throw if req.body is undefined;
change the handler to guard req.body before reading it (e.g., verify req.body is
present and an object or default to an empty object) or move the destructuring
into the try block so any missing body is handled by the existing JSON error
response; ensure the value passed into authService.initiateLogin is a boolean
check (rememberMe === true) after the guard.
In `@server/src/routes/refresh.ts`:
- Around line 6-9: The route handler in refresh.ts reads req.body directly and
can throw if body is missing or not an object; update the anonymous async
handler so it first verifies req.body is present and is an object (e.g., if
(!req.body || typeof req.body !== 'object') return
res.status(400).json(createErrorResponse(new
AppError('MISSING_REQUIRED_FIELD')))), then safely extract const { refreshToken
} = req.body as Record<string, unknown> and continue with the existing typeof
refreshToken === 'string' check before calling authService.refreshToken; keep
using createErrorResponse and AppError for the 400 response.
In `@server/src/services/email-validator.ts`:
- Around line 21-22: The validation in extractDomain() currently only checks for
ASCII spaces using includes(' '), so update the checks on local and domain (the
variables validated in extractDomain()) to reject any Unicode whitespace by
testing for general whitespace (e.g., using a \s check or equivalent) or by
comparing trimmed vs original values; ensure both the local and domain branches
use this broader whitespace detection and still throw
AppError('INVALID_EMAIL_FORMAT') when whitespace is found.
---
Nitpick comments:
In `@client/index.html`:
- Line 3: The inline <style> block in index.html (inside the <head> element)
should be removed and its rules (html, body { height: 100%; margin: 0; } and
`#root` { height: 100%; }) moved into your project's CSS pipeline (e.g., main
stylesheet or app CSS module). Update index.html to reference that stylesheet
(or ensure the bundler imports it) and remove the inline <style> tag so future
CSP policies can disallow 'unsafe-inline' while preserving the layout behavior
for the `#root` element.
In `@client/postcss.config.mjs`:
- Line 1: The PostCSS config currently exports an empty object (export default
{}), which can be mistaken for a missing config; either add a one-line
explanatory comment above the export stating this file is intentionally
empty/disabled (e.g., "Intentionally empty — PostCSS not used in this project")
or remove the file entirely if unused; update the export/default block (export
default {}) accordingly so reviewers clearly understand the intent.
In `@client/src/styles/fonts.css`:
- Line 1: Update the placeholder comment in fonts.css to clarify intent for
future maintainers by replacing or augmenting "/* 폰트 설정 */" with a more
descriptive TODO comment such as indicating this is the location for future
custom webfont declarations (e.g., "TODO: 향후 커스텀 웹폰트 선언 위치 — 여기에 `@font-face` 규칙 및
웹폰트 로드 코드를 추가하세요") so the file isn't mistaken for an accidental empty file; keep
the description short and in Korean to match the repo language.
In `@client/src/styles/theme.css`:
- Line 1: 현재 theme.css에 전역 루트 토큰 스캐폴드를 추가해 확장성을 높이세요: theme.css의 :root 블록을 만들고
--color-bg, --color-text, --color-primary, --color-muted, --radius-base,
--space-1, --space-2, --font-sans 같은 기본 CSS 커스텀 프로퍼티를 최소한으로 정의(기본값 포함)하여 테마를
확장하거나 오버라이드할 때 충돌을 줄이도록 합니다; 해당 변경은 기존 클래스/유틸리티와 호환되게 현재 파일의 주석 스타일을 유지하면서
적용하세요.
In `@server/src/interfaces/email-validator.ts`:
- Line 4: getAllowedDomains currently returns a mutable string[]; change its
signature to return ReadonlyArray<string> (i.e. getAllowedDomains():
ReadonlyArray<string>) and update every implementation of that interface (any
classes or functions implementing getAllowedDomains) to match the new return
type so callers cannot mutate the returned collection.
In `@server/src/middleware/error-handler.ts`:
- Around line 5-8: The global errorHandler currently returns a 500 for
non-AppError instances without logging; update errorHandler to log unexpected
errors (the err value) before sending the fallback response by calling your
project's logger (e.g., processLogger.error or the configured logger) and
include context like the route/handler if available; preserve the existing
branches that handle AppError and use createErrorResponse(new
AppError('INTERNAL_ERROR')) after logging so production triage has the original
error details accessible.
In `@server/src/routes/index.ts`:
- Around line 11-14: Add route-level rate limiting middleware to the auth
endpoints to mitigate brute-force/replay attacks: create one or more
express-compatible rate limiter instances (e.g., a strict limiter for POST
/login and POST /refresh and a slightly looser one for GET /callback and GET
/me, or use a shared config) and apply them as middleware before the existing
handlers (attach the limiter to the routes that call createLoginHandler,
createCallbackHandler, createRefreshHandler, and optionally createMeHandler).
Ensure the limiter is imported/constructed in the same module (or a shared
middleware file) and configured to return proper 429 responses and to respect
IP/client identifier logic used in the app.
In `@server/src/services/auth-service.ts`:
- Line 49: The hosted-domain (userInfo.hd) check currently duplicates email
domain validation and can incorrectly reject users; either remove the hd check
or make the intent explicit: if you require the hd to match the email domain,
replace the call to this.emailValidator.isAllowedDomain(`user@${userInfo.hd}`)
with an explicit comparison between the parsed email domain and userInfo.hd (and
throw AppError('INVALID_EMAIL_DOMAIN') only when they differ), otherwise just
delete the hd branch so only the validated email domain is enforced; locate this
logic near userInfo.hd, emailValidator.isAllowedDomain and the
AppError('INVALID_EMAIL_DOMAIN') throw to apply the change.
- Around line 70-83: findUserById currently scans this.users which is O(n); add
a secondary Map keyed by id (e.g., private usersById: Map<string, User>) and
keep it in sync: when creating a user in findOrCreateUser add
usersById.set(user.id, user), when returning an existing user ensure
usersById.set(existing.id, existing) (or verify presence), and change
findUserById to return this.usersById.get(userId). Also update any user-delete
or update paths to maintain both maps in sync.
In `@server/src/types/oauth-token-response.ts`:
- Line 3: The token_type property in the OAuth token response interface is
currently typed as string; if this type is intended to model Google responses
only, narrow token_type to the literal 'Bearer' in the interface (e.g., change
token_type: string to token_type: 'Bearer') and update any call sites or tests
that assume a generic string accordingly so they accept the literal type; target
the interface declaration (the OAuth token response type in
server/src/types/oauth-token-response.ts) when making this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bb6621c-6d54-4b15-bc5e-1a1fd4117ad6
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonserver/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
client/.env.exampleclient/index.htmlclient/package.jsonclient/postcss.config.mjsclient/src/app/App.tsxclient/src/main.tsxclient/src/styles/fonts.cssclient/src/styles/index.cssclient/src/styles/tailwind.cssclient/src/styles/theme.cssclient/vite.config.tsserver/.env.exampleserver/migrations/001_create_tables.sqlserver/migrations/002_case_insensitive_unique.sqlserver/package.jsonserver/src/app.tsserver/src/errors/app-error.tsserver/src/errors/error-codes.tsserver/src/errors/error-response.tsserver/src/errors/index.tsserver/src/interfaces/auth-service.tsserver/src/interfaces/email-validator.tsserver/src/interfaces/google-oauth-client.tsserver/src/interfaces/index.tsserver/src/interfaces/token-service.tsserver/src/middleware/error-handler.tsserver/src/repositories/allowed-domain-repository.tsserver/src/repositories/index.tsserver/src/repositories/oauth-state-repository.tsserver/src/repositories/refresh-token-repository.tsserver/src/repositories/user-repository.tsserver/src/routes/callback.tsserver/src/routes/index.tsserver/src/routes/login.tsserver/src/routes/me.tsserver/src/routes/refresh.tsserver/src/server.tsserver/src/services/auth-service.tsserver/src/services/email-validator.tsserver/src/services/google-oauth-client.tsserver/src/services/index.tsserver/src/services/token-service.tsserver/src/types/allowed-domain.tsserver/src/types/auth-result.tsserver/src/types/index.tsserver/src/types/oauth-state.tsserver/src/types/oauth-token-response.tsserver/src/types/refresh-token.tsserver/src/types/token-payload.tsserver/src/types/token-verify-result.tsserver/src/types/user-info.tsserver/src/types/user.tsserver/tsconfig.json
Comment on lines
+1
to
+3
| @import 'tailwindcss' source(none); | ||
| @source '../**/*.{js,ts,jsx,tsx}'; | ||
| @import 'tw-animate-css'; |
There was a problem hiding this comment.
Fix @import ordering to avoid dropping tw-animate-css.
Line 3 places @import 'tw-animate-css' after the non-import at-rule on Line 2, which violates import ordering and is already flagged by Stylelint. Move all @import rules before @source. Also, since Line 2 uses Tailwind v4 syntax, ensure Stylelint is configured to allow @source.
Proposed change
`@import` 'tailwindcss' source(none);
-@source '../**/*.{js,ts,jsx,tsx}';
`@import` 'tw-animate-css';
+@source '../**/*.{js,ts,jsx,tsx}';📝 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 'tailwindcss' source(none); | |
| @source '../**/*.{js,ts,jsx,tsx}'; | |
| @import 'tw-animate-css'; | |
| `@import` 'tailwindcss' source(none); | |
| `@import` 'tw-animate-css'; | |
| `@source` '../**/*.{js,ts,jsx,tsx}'; |
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 3-3: Invalid position for @import rule (no-invalid-position-at-import-rule)
(no-invalid-position-at-import-rule)
[error] 2-2: Unexpected unknown at-rule "@source" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/styles/tailwind.css` around lines 1 - 3, Move the two `@import`
rules (including "@import 'tw-animate-css'") before the "@source
'../**/*.{js,ts,jsx,tsx}'" at-rule to satisfy Stylelint's import ordering and
prevent the tw-animate-css import from being dropped; also update your Stylelint
config to recognize Tailwind v4 "@source" at-rules (e.g., allow the "source"
at-rule in rules like at-rule-no-unknown or add it to the at-rule list) so the
"@source" line is permitted.
Comment on lines
+11
to
+12
| async create(user: User) { this.users.set(user.email.toLowerCase(), { ...user }); return { ...user }; } | ||
| async findByEmail(email: string) { return this.users.get(email.toLowerCase()) ?? null; } |
There was a problem hiding this comment.
create() does not fully enforce the lowercase email invariant.
The key is normalized, but the stored/returned User.email can remain mixed-case. Also, findByEmail() currently returns a mutable internal reference.
🔧 Proposed fix
- async create(user: User) { this.users.set(user.email.toLowerCase(), { ...user }); return { ...user }; }
- async findByEmail(email: string) { return this.users.get(email.toLowerCase()) ?? null; }
+ async create(user: User) {
+ const normalizedEmail = user.email.toLowerCase();
+ const stored = { ...user, email: normalizedEmail };
+ this.users.set(normalizedEmail, stored);
+ return { ...stored };
+ }
+ async findByEmail(email: string) {
+ const found = this.users.get(email.toLowerCase());
+ return found ? { ...found } : null;
+ }📝 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
| async create(user: User) { this.users.set(user.email.toLowerCase(), { ...user }); return { ...user }; } | |
| async findByEmail(email: string) { return this.users.get(email.toLowerCase()) ?? null; } | |
| async create(user: User) { | |
| const normalizedEmail = user.email.toLowerCase(); | |
| const stored = { ...user, email: normalizedEmail }; | |
| this.users.set(normalizedEmail, stored); | |
| return { ...stored }; | |
| } | |
| async findByEmail(email: string) { | |
| const found = this.users.get(email.toLowerCase()); | |
| return found ? { ...found } : null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/repositories/user-repository.ts` around lines 11 - 12, The
create() method currently stores the user under a lowercased key but leaves
User.email as-is and returns the original/mutable object; change create(user:
User) to create and store a shallow copy with email normalized (e.g., const
normalized = { ...user, email: user.email.toLowerCase() }), set
this.users.set(normalized.email, normalized), and return a copy (or the
normalized object) so callers don't get a reference to the internal map;
similarly update findByEmail(email: string) to read from this.users using
email.toLowerCase() and return a shallow copy (or null) instead of the internal
object to avoid exposing mutable state.
Comment on lines
+18
to
+20
| res.cookie('refreshToken', result.refreshToken, { | ||
| httpOnly: true, secure: IS_PRODUCTION, sameSite: 'lax', maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| }); |
There was a problem hiding this comment.
Refresh-token cookie TTL is hardcoded and can diverge from JWT expiry.
maxAge is always 30 days, but refresh token lifetime can be shorter (e.g., non-remember flows). This leaves stale cookies and inconsistent session behavior.
🔧 Proposed fix (requires `handleCallback` to expose refresh expiry)
+ const refreshTokenMaxAgeMs = Math.max(0, result.refreshTokenExpiresAt.getTime() - Date.now());
res.cookie('refreshToken', result.refreshToken, {
httpOnly: true, secure: IS_PRODUCTION, sameSite: 'lax', maxAge: 30 * 24 * 60 * 60 * 1000,
+ maxAge: refreshTokenMaxAgeMs,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/routes/callback.ts` around lines 18 - 20, The cookie maxAge is
hardcoded to 30 days causing stale cookies; change the handler so handleCallback
returns the refresh token expiry (e.g., result.refreshTokenExpiresIn or
result.refreshTokenExpiryTimestamp) and use that value to set
res.cookie('refreshToken', result.refreshToken, { httpOnly: true, secure:
IS_PRODUCTION, sameSite: 'lax', maxAge: computedExpiryInMs }) with a safe
fallback (keep 30 days) when the expiry is not provided; update handleCallback's
return type and the call site in callback.ts to read that expiry and convert
seconds/timestamp to milliseconds for maxAge.
Comment on lines
+2
to
+4
| const PORT = process.env.PORT ?? 3000; | ||
| const app = createApp(); | ||
| app.listen(PORT, () => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n server/src/server.tsRepository: AWS-PORJECT-2/AWS_PROJECT_2
Length of output: 310
🌐 Web query:
Node.js http.Server.listen() port parameter behavior string vs number
💡 Result:
In Node.js, the http.Server.listen method inherits its behavior from net.Server.listen. The first parameter can be either a number (for TCP port) or a string (for Unix domain socket path or Windows named pipe). When a number is passed (e.g., server.listen(8080)), it binds to the specified TCP port on all interfaces (or with host if specified). When a string is passed (e.g., server.listen('/tmp.sock')), it binds to a Unix domain socket at that filesystem path (on Unix-like systems) or a named pipe (on Windows). Strings that represent valid port numbers (e.g., '8080') are coerced to numbers internally via normalization logic in lib/net.js. The normalizeArgs (or historically normalizeConnectArgs) function parses the string with +port or similar coercion, validates it as a legal port (integer 0-65535, not NaN), and treats it as a TCP port. For example: - server.listen('8080') binds to TCP port 8080. - server.listen or server.listen(0) binds to a random available TCP port. - Invalid port strings like 'fake' or '99999' result in a Unix socket/pipe at that path, or an error if invalid for binding. This dual behavior allows flexibility for both TCP and IPC without separate methods. server.address returns the bound address: { port: 8080, family: 'IPv4', address: '0.0.0.0' } for TCP, or the string path for sockets/pipes. Examples: const http = require('node:http'); const server = http.createServer; // TCP port (number) server.listen(8080, => console.log(server.address)); // { port: 8080, ... } // TCP port (string coerced) server.close( => server.listen('8080', => console.log(server.address))); // Same as above // Unix socket (string path) server.close( => server.listen('/tmp/my.sock', => console.log(server.address))); // '/tmp/my.sock' This behavior is consistent across Node.js v0.10+ to latest v25 (as of 2026), per source code evolution in net.js PRs #1116, #4039.
Citations:
- 1: https://nodejs.org/api/net.html
- 2: https://stackoverflow.com/questions/76205903/what-happens-if-i-pass-server-listen-a-string-for-a-port
- 3: App.listen(). Should accept strings? nodejs/node#43356
- 4: lib: allow server.listen({ port: "1234" }) nodejs/node#1116
- 5: net: refactor Server.prototype.listen nodejs/node#4039
- 6: https://nodejs.org/docs/latest/api/net.html
- 7: nodejs/node@fd6af98c2d
- 8: nodejs/node@480b48244f
Validate and normalize PORT before calling listen.
process.env.PORT is a string if set. When passed directly to app.listen(), non-numeric values are silently treated as Unix socket paths rather than failing with a clear validation error. This prevents fast-failing on misconfigured environment variables and can produce confusing startup errors.
Suggested patch
-const PORT = process.env.PORT ?? 3000;
+const parsedPort = Number.parseInt(process.env.PORT ?? '', 10);
+const PORT = Number.isInteger(parsedPort) && parsedPort > 0 ? parsedPort : 3000;
const app = createApp();
app.listen(PORT, () => {
console.log(`doothing 서버가 http://localhost:${PORT} 에서 실행 중입니다`);
});📝 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 PORT = process.env.PORT ?? 3000; | |
| const app = createApp(); | |
| app.listen(PORT, () => { | |
| const parsedPort = Number.parseInt(process.env.PORT ?? '', 10); | |
| const PORT = Number.isInteger(parsedPort) && parsedPort > 0 ? parsedPort : 3000; | |
| const app = createApp(); | |
| app.listen(PORT, () => { | |
| console.log(`doothing 서버가 http://localhost:${PORT} 에서 실행 중입니다`); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/server.ts` around lines 2 - 4, The PORT value should be validated
and normalized before calling app.listen: parse process.env.PORT (e.g., using
parseInt or Number) and ensure it is a positive integer; if the parsed value is
NaN or <= 0, log a clear error and exit (or fallback to the default 3000
explicitly) so you don’t accidentally pass a non-numeric string as a Unix socket
path. Update the code that defines PORT and the call site (the PORT constant and
the app.listen(...) invocation in server.ts where createApp() is used) to
perform this validation and pass a numeric port to app.listen.
Comment on lines
+60
to
+68
| async refreshToken(refreshToken: string): Promise<{ accessToken: string }> { | ||
| const payload = this.tokenService.verifyRefreshToken(refreshToken); | ||
| if (!payload) throw new AppError('INVALID_REFRESH_TOKEN'); | ||
| const tokenHash = TokenServiceImpl.hashToken(refreshToken); | ||
| if (!this.refreshTokens.get(tokenHash)) throw new AppError('INVALID_REFRESH_TOKEN'); | ||
| const user = this.findUserById(payload.userId); | ||
| if (!user) throw new AppError('INVALID_REFRESH_TOKEN'); | ||
| return { accessToken: this.tokenService.generateAccessToken(user) }; | ||
| } |
There was a problem hiding this comment.
Missing expiry check for stored refresh token.
The refreshToken method verifies the JWT signature (line 61) and checks the token exists in storage (line 64), but never validates the stored expiresAt field. Expired refresh tokens will still issue new access tokens.
🐛 Proposed fix to add expiry validation
async refreshToken(refreshToken: string): Promise<{ accessToken: string }> {
const payload = this.tokenService.verifyRefreshToken(refreshToken);
if (!payload) throw new AppError('INVALID_REFRESH_TOKEN');
const tokenHash = TokenServiceImpl.hashToken(refreshToken);
- if (!this.refreshTokens.get(tokenHash)) throw new AppError('INVALID_REFRESH_TOKEN');
+ const storedToken = this.refreshTokens.get(tokenHash);
+ if (!storedToken) throw new AppError('INVALID_REFRESH_TOKEN');
+ if (new Date() > storedToken.expiresAt) {
+ this.refreshTokens.delete(tokenHash);
+ throw new AppError('INVALID_REFRESH_TOKEN');
+ }
const user = this.findUserById(payload.userId);
if (!user) throw new AppError('INVALID_REFRESH_TOKEN');
return { accessToken: this.tokenService.generateAccessToken(user) };
}📝 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
| async refreshToken(refreshToken: string): Promise<{ accessToken: string }> { | |
| const payload = this.tokenService.verifyRefreshToken(refreshToken); | |
| if (!payload) throw new AppError('INVALID_REFRESH_TOKEN'); | |
| const tokenHash = TokenServiceImpl.hashToken(refreshToken); | |
| if (!this.refreshTokens.get(tokenHash)) throw new AppError('INVALID_REFRESH_TOKEN'); | |
| const user = this.findUserById(payload.userId); | |
| if (!user) throw new AppError('INVALID_REFRESH_TOKEN'); | |
| return { accessToken: this.tokenService.generateAccessToken(user) }; | |
| } | |
| async refreshToken(refreshToken: string): Promise<{ accessToken: string }> { | |
| const payload = this.tokenService.verifyRefreshToken(refreshToken); | |
| if (!payload) throw new AppError('INVALID_REFRESH_TOKEN'); | |
| const tokenHash = TokenServiceImpl.hashToken(refreshToken); | |
| const storedToken = this.refreshTokens.get(tokenHash); | |
| if (!storedToken) throw new AppError('INVALID_REFRESH_TOKEN'); | |
| if (new Date() > storedToken.expiresAt) { | |
| this.refreshTokens.delete(tokenHash); | |
| throw new AppError('INVALID_REFRESH_TOKEN'); | |
| } | |
| const user = this.findUserById(payload.userId); | |
| if (!user) throw new AppError('INVALID_REFRESH_TOKEN'); | |
| return { accessToken: this.tokenService.generateAccessToken(user) }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/auth-service.ts` around lines 60 - 68, The refreshToken
method is missing validation of the stored token's expiry: after computing
tokenHash with TokenServiceImpl.hashToken and before issuing a new access token
in refreshToken, fetch the stored token object from
this.refreshTokens.get(tokenHash) and verify its expiresAt timestamp is in the
future; if expired, remove it from this.refreshTokens and throw
AppError('INVALID_REFRESH_TOKEN'); only then proceed to
findUserById(payload.userId) and generate the access token.
Comment on lines
+49
to
+63
| extractUserInfoFromIdToken(idToken: string): UserInfo | null { | ||
| try { | ||
| const parts = idToken.split('.'); | ||
| if (parts.length !== 3) return null; | ||
| const payload = JSON.parse(Buffer.from(parts[1], 'base64url').toString('utf-8')); | ||
| if (typeof payload.email !== 'string' || typeof payload.name !== 'string') return null; | ||
| return { | ||
| email: payload.email, | ||
| name: payload.name, | ||
| picture: payload.picture, | ||
| hd: payload.hd, | ||
| email_verified: payload.email_verified, | ||
| }; | ||
| } catch { return null; } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How should Google OAuth ID tokens be verified in Node.js server-side applications?
💡 Result:
To verify Google OAuth ID tokens server-side in Node.js applications, use the official google-auth-library package. This is the recommended approach from Google documentation. 1. Install the library: npm install google-auth-library 2. Use the OAuth2Client to verify the ID token: const {OAuth2Client} = require('google-auth-library'); const client = new OAuth2Client(YOUR_WEB_CLIENT_ID); // Replace with your client ID from Google Cloud Console async function verifyIdToken(idToken) { try { const ticket = await client.verifyIdToken({ idToken: idToken, audience: YOUR_WEB_CLIENT_ID, // The client ID or array of client IDs }); const payload = ticket.getPayload; const userid = payload['sub']; // Unique Google user ID // Additional checks: payload['email'], payload['email_verified'], payload['hd'] for domain return payload; } catch (error) { console.error('Token verification failed:', error); throw error; } } Key verification steps performed by verifyIdToken: - JWT signature validation using Google's public certificates - 'aud' claim matches your client ID - 'exp' claim (not expired) - 'iss' claim is accounts.google.com or googleapis.com Send the ID token from client to server via HTTPS POST (e.g., in request body as 'credential'). For multiple clients, pass an array of client IDs to audience. For Google Workspace domains, verify payload['hd']. Avoid manual JWT parsing; use the library for security. This applies to Google Identity Services (GIS), Google Sign-In, and OpenID Connect flows.
Citations:
- 1: https://developers.google.com/identity/gsi/web/guides/verify-google-id-token
- 2: https://developers.google.com/identity/sign-in/web/backend-auth
- 3: https://developers.google.com/identity/sign-in/web/backend-auth?hl=en
🏁 Script executed:
# Check if google-auth-library is already a dependency
if [ -f package.json ]; then
grep -E '"google-auth-library"|"jose"|"jsonwebtoken"' package.json
fiRepository: AWS-PORJECT-2/AWS_PROJECT_2
Length of output: 53
🏁 Script executed:
# Get the full context of the google-oauth-client.ts file
if [ -f server/src/services/google-oauth-client.ts ]; then
wc -l server/src/services/google-oauth-client.ts
fiRepository: AWS-PORJECT-2/AWS_PROJECT_2
Length of output: 116
🏁 Script executed:
# Check the imports and full implementation
if [ -f server/src/services/google-oauth-client.ts ]; then
cat -n server/src/services/google-oauth-client.ts | head -80
fiRepository: AWS-PORJECT-2/AWS_PROJECT_2
Length of output: 3388
🏁 Script executed:
# Search for where extractUserInfoFromIdToken is called
rg "extractUserInfoFromIdToken" --type ts -A 3 -B 3Repository: AWS-PORJECT-2/AWS_PROJECT_2
Length of output: 1613
ID token parsed without signature verification.
extractUserInfoFromIdToken decodes the JWT payload without verifying the cryptographic signature. Although the token originates from Google's OAuth2 token endpoint (authenticated with client credentials), relying on manual JWT parsing without signature verification does not follow security best practices and leaves the implementation vulnerable to exploitation if the token transmission is compromised.
Google's official documentation recommends verifying ID tokens using the google-auth-library package. Use OAuth2Client.verifyIdToken() to cryptographically validate the token signature, check the aud claim matches your client ID, and validate expiration and issuer claims.
Current approach (optional extraction with fallback to getUserInfo) provides some defense-in-depth, but proper JWT verification should be mandatory rather than optional.
Add google-auth-library as a dependency and replace the manual parsing with:
const { OAuth2Client } = require('google-auth-library');
const client = new OAuth2Client(this.clientId);
const ticket = await client.verifyIdToken({ idToken, audience: this.clientId });
const payload = ticket.getPayload();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/google-oauth-client.ts` around lines 49 - 63, The
extractUserInfoFromIdToken function currently decodes the JWT without signature
validation; replace the manual parsing with google-auth-library verification:
add google-auth-library as a dependency, construct an OAuth2Client using
this.clientId, call OAuth2Client.verifyIdToken({ idToken, audience:
this.clientId }) and use ticket.getPayload() to obtain the claims, then validate
aud/iss/exp as appropriate and extract email/name/picture/hd/email_verified;
ensure any errors return null (or propagate as before) and remove the base64url
JSON decode flow.
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
2 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.
변경 사항
새로 추가된 파일
기존 파일 수정 여부
보안 점검
Summary by CodeRabbit
New Features
Chores