Refactor authentication handling and improve token management - #1
Refactor authentication handling and improve token management#1yunusakin wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the mock authentication system with JWT signature verification and makes several infrastructure improvements. The changes migrate token storage from localStorage to sessionStorage and add proper environment-specific build configurations.
- Implements HMAC-SHA256 signature verification for mock JWT tokens using crypto-js
- Migrates authentication token storage from localStorage to sessionStorage
- Adds success validation checks before updating state in store mutation operations
- Introduces environment-specific build scripts and Docker build arguments
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.app.json | Adds "node" types to support Node.js type definitions |
| src/utils/auth.ts | Implements JWT signature creation and verification with timing-safe comparison |
| src/store/novaStore.ts | Adds success checks before updating state in all mutation operations |
| src/store/authStore.ts | Updates token validation to include signature verification and migrates to sessionStorage |
| src/pages/ProfilePage.tsx | Updates UI text to reflect sessionStorage usage |
| src/config/env.ts | Adds authTokenSecret configuration variable |
| src/components/layout/AppLayout.tsx | Adds environment label display in mobile navigation |
| src/components/auth/ProtectedRoute.tsx | Improves type definition from JSX.Element to ReactNode |
| src/api/authApi.ts | Updates token creation to include signature |
| package.json | Adds crypto-js dependency and new environment-specific build scripts |
| package-lock.json | Updates lock file with crypto-js and type definitions |
| environments/*/docker-compose.yml | Adds BUILD_SCRIPT argument support for environment-specific builds |
| environments//.env. | Adds VITE_AUTH_TOKEN_SECRET to all environment configurations |
| Dockerfile | Makes build script configurable via ARG for multi-environment support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const payloadData = decodeToken(token) | ||
| if (!payloadData) return false |
There was a problem hiding this comment.
The token is being split twice in the isTokenValid function - once at line 93 and again inside decodeToken at line 96. This is redundant and could be optimized by passing the already-extracted payload string to a modified version of decodeToken, or by decoding the payload inline instead of calling decodeToken.
| const payloadData = decodeToken(token) | |
| if (!payloadData) return false | |
| const decoded = base64Decode(payload) | |
| if (!decoded) return false | |
| let payloadData: AuthTokenPayload | |
| try { | |
| payloadData = JSON.parse(decoded) as AuthTokenPayload | |
| } catch { | |
| return false | |
| } |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const base64Encode = (value: string) => { | ||
| if (typeof window !== 'undefined' && typeof window.btoa === 'function') { | ||
| return window.btoa(value) | ||
| } | ||
|
|
||
| const BufferCtor = getBuffer() | ||
| if (BufferCtor) { | ||
| return BufferCtor.from(value, 'utf-8').toString('base64') | ||
| } | ||
|
|
||
| throw new Error('Unable to encode value as base64.') | ||
| const words = encUtf8.parse(value) | ||
| return encBase64.stringify(words) | ||
| } |
There was a problem hiding this comment.
JWT tokens should use base64url encoding (RFC 4648 §5) instead of standard base64 encoding. The current implementation uses standard base64 which includes +, /, and = characters that are not URL-safe and can cause issues when tokens are passed in URLs or headers.
Consider using a base64url encoding library or manually replacing characters: + → -, / → _, and removing = padding.
| const timingSafeEqual = (a: string, b: string) => { | ||
| if (a.length !== b.length) return false | ||
| let mismatch = 0 | ||
| for (let index = 0; index < a.length; index += 1) { | ||
| mismatch |= a.charCodeAt(index) ^ b.charCodeAt(index) |
There was a problem hiding this comment.
The timingSafeEqual function has a critical flaw: it returns false early when lengths don't match, which is not timing-safe. This early return allows timing attacks to determine the expected signature length.
For true timing safety, the function should always iterate through the same number of operations regardless of input. However, since this is client-side code and the secret is already exposed in the bundle (see security concern), this may be acceptable. Consider adding a comment explaining this is client-side only.
| const timingSafeEqual = (a: string, b: string) => { | |
| if (a.length !== b.length) return false | |
| let mismatch = 0 | |
| for (let index = 0; index < a.length; index += 1) { | |
| mismatch |= a.charCodeAt(index) ^ b.charCodeAt(index) | |
| /** | |
| * Timing-safe string comparison. | |
| * Always iterates over the maximum length of the inputs, and includes length difference in mismatch. | |
| * Note: This is client-side code and the secret is exposed in the bundle, so timing attacks are not a practical concern. | |
| */ | |
| const timingSafeEqual = (a: string, b: string) => { | |
| const len = Math.max(a.length, b.length) | |
| let mismatch = a.length === b.length ? 0 : 1 | |
| for (let index = 0; index < len; index += 1) { | |
| const ac = a.charCodeAt(index) || 0 | |
| const bc = b.charCodeAt(index) || 0 | |
| mismatch |= ac ^ bc |
| orderApi: readEnv('VITE_ORDER_API'), | ||
| authApi: readEnv('VITE_AUTH_API'), | ||
| authTokenKey: readEnv('VITE_AUTH_TOKEN_KEY'), | ||
| authTokenSecret: readEnv('VITE_AUTH_TOKEN_SECRET'), |
There was a problem hiding this comment.
Critical security issue: The authTokenSecret is being read from environment variables that are bundled into the client-side application. In Vite, all VITE_* variables are exposed to the client bundle, making this secret publicly visible to anyone who inspects the JavaScript.
JWT signature verification in the browser provides no security since attackers can read the secret from the bundle and forge valid tokens. Consider either:
- Moving token validation to the backend only
- Using this as a mock/development-only feature with clear documentation
- Implementing a different client-side authentication approach
If this is intentional for mock/development purposes, add clear comments explaining the security limitations.
| authTokenSecret: readEnv('VITE_AUTH_TOKEN_SECRET'), | |
| // SECURITY WARNING: | |
| // Never expose authentication secrets to the client-side bundle in production. | |
| // JWT signature verification in the browser provides no security since attackers can read the secret from the bundle and forge valid tokens. | |
| // If you need to use authTokenSecret for mock/development purposes, ensure this is only included in development mode and never in production. | |
| // Example (uncomment for development only): | |
| // ...(import.meta.env.MODE === 'development' && { | |
| // authTokenSecret: readEnv('VITE_AUTH_TOKEN_SECRET'), | |
| // }), |
| # Basic CSP - can be tightened based on specific needs | ||
| add_header Content-Security-Policy "default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self' data:; connect-src 'self' http: https:;"; |
There was a problem hiding this comment.
The Content-Security-Policy allows 'unsafe-inline' and 'unsafe-eval' for scripts, which significantly weakens the CSP protection. While these may be necessary for React development builds, consider:
- Using nonces or hashes for inline scripts instead of
'unsafe-inline' - Avoiding
'unsafe-eval'if possible, or at least documenting why it's necessary - Creating separate CSP configurations for development vs production environments
The overly permissive connect-src 'self' http: https: also allows connections to any HTTP/HTTPS endpoint, which could be tightened to specific API domains.
| # Basic CSP - can be tightened based on specific needs | |
| add_header Content-Security-Policy "default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self' data:; connect-src 'self' http: https:;"; | |
| # Production CSP: tightened for security. For development, consider relaxing as needed and document why. | |
| add_header Content-Security-Policy "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self' data:; connect-src 'self' https://api.example.com;"; |
| VITE_ORDER_API=https://nova.example.com/api/orders | ||
| VITE_AUTH_API=https://nova.example.com/api/auth | ||
| VITE_AUTH_TOKEN_KEY=nova_token | ||
| VITE_AUTH_TOKEN_SECRET=prod-mock-secret |
There was a problem hiding this comment.
The secrets in environment files (test-mock-secret, prod-mock-secret, etc.) are hardcoded and committed to the repository. While these appear to be for mock/development purposes, consider:
- Using environment-specific secret generation or injection during deployment
- Adding a comment clarifying these are for mock authentication only
- Ensuring production deployments use proper secret management (e.g., Kubernetes secrets, AWS Secrets Manager)
If these are truly just for local development/testing, the naming could be more explicit like mock-test-secret to make it clear they shouldn't be used in production.
| VITE_AUTH_TOKEN_SECRET=prod-mock-secret | |
| # VITE_AUTH_TOKEN_SECRET should be injected securely during deployment (do not commit secrets) | |
| VITE_AUTH_TOKEN_SECRET= |
| type ProtectedRouteProps = { | ||
| roles?: AuthRole[] | ||
| children: JSX.Element | ||
| children: ReactNode |
There was a problem hiding this comment.
[nitpick] Inconsistent type usage: The children prop type changed from JSX.Element to ReactNode. While ReactNode is more flexible and generally preferred, this change should be consistent across the codebase.
The more flexible ReactNode type allows strings, numbers, fragments, and arrays of children, whereas JSX.Element only allows single React elements. This is actually an improvement, but verify that all components expecting children are updated similarly for consistency.
No description provided.