Perform critical app review and assessment #8
Conversation
Week 1 Critical Security: - Fix JWT test mode bypass (now requires TEST_JWT_SECRET with proper signature) - Add CSRF protection plugin (origin-based validation) - Fix CORS configuration (no more fallback to wildcard *) - Enable rate limiting in all environments (with multipliers) - Implement proper RBAC for admin routes (actual role checks) Week 2-3 High Priority: - Add input length limits to TypeBox models (prevent memory exhaustion) - Disable production sourcemaps (don't expose source code) - Add security headers (XSS protection, HSTS, Referrer-Policy, etc.) - Fix schema-migration drift (drop unused admin_whitelist table) - Create .env.example with deployment checklist Month 1 Medium Priority: - Add JWT invalidation via token blocklist table and service - Add token blocklist schema and migration This addresses the critical security vulnerabilities identified in the app review.
- Remove mockAssetService that was not even being used (createAssetRoutes signature changed to only take rootDir) - Convert tests to use real database with test data isolation - Fix CDN URL Handling tests to work with real assetDatabaseService - Add cleanup jobs for activity_log and token_blocklist tables Per CLAUDE.md: "NO MOCKS for internal code (database, HTTP handlers, business logic)" - only mock external APIs (Meshy, OpenAI, Privy).
Document the recommended approach for breaking down the 1500+ line ThreeViewer.tsx into smaller, maintainable components. This was identified during the critical security review as a code quality issue. The plan outlines: - Component extraction strategy (SceneManager, ModelLoader, etc.) - Implementation phases - Testing requirements
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds JWT token blocklist support (DB/schema/service/migration), CSRF plugin, environment-aware rate limiting, security header changes, auth plugin updates (blocklist check, admin guard, test signing), cron cleanup jobs, real-DB asset tests, test helpers, config/example env, and assorted docs/refactor plan. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API / Middleware
participant Auth as Auth Plugin
participant Blocklist as TokenBlocklistService
participant DB
participant Handler
Client->>API: Request with Authorization (JWT or Bearer af_ API key)
API->>Auth: optionalAuth()
Auth->>Auth: Extract token
alt API key (starts "af_")
Auth->>Handler: Authenticate as API key (skip blocklist)
else JWT
Auth->>Blocklist: isTokenBlocklisted(token)
Blocklist->>DB: SELECT hashed token
DB-->>Blocklist: Found / Not found
alt Found
Auth-->>Client: 401/403 (token revoked)
else Not found
Auth->>Auth: Verify JWT (Privy or HMAC in test mode)
alt Verified
Auth->>Handler: Inject authenticated user
else Verification failed
Auth-->>Client: 401 Unauthorized
end
end
end
Handler-->>Client: Response
sequenceDiagram
participant Client
participant API as API / CSRF Plugin
participant Allowed as AllowedOriginsBuilder
Client->>API: POST/PUT/DELETE (Origin / Referer)
API->>Allowed: Build allowed origins from FRONTEND_URL, CORS_ALLOWED_ORIGINS, dev localhost
alt Authorization is API key (af_)
API->>API: Skip CSRF check
API-->>Client: Continue
else Regular request
API->>API: Determine origin from Origin header or Referer
API->>Allowed: Check origin in allowed list
alt Match
API-->>Client: Continue
else No match
API-->>Client: 403 CSRF_VALIDATION_FAILED
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Dexploarer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's security posture, improves the reliability of its testing suite, and lays groundwork for better maintainability. Key changes include the introduction of CSRF protection, a robust JWT token blocklist, and more stringent security headers. The asset routes now undergo more realistic integration testing against a live database, and environment configurations are clearer. Furthermore, rate limiting has been made more flexible and always-on, and a strategic plan for refactoring a major frontend component has been documented. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Compliance Guide 🔍(Compliance updated until commit d57bfd3)Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label Previous compliance checksCompliance check up to commit 51a22c5
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
CI Feedback 🧐(Feedback updated until commit 51a22c5)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Greptile OverviewGreptile SummaryThis PR implements critical security hardening and refactoring improvements for the Asset-Forge application. The changes focus on JWT token invalidation infrastructure, authentication security enhancements, security headers, CSRF protection, improved input validation, and source map security. Key architectural improvements include adding a token blocklist system for proper logout functionality, fixing authentication test vulnerabilities, implementing comprehensive security headers, and enhancing CORS configuration. The PR also addresses code quality concerns by creating a refactoring plan for a massive 6,340-line React component and improving test reliability by removing mock services in favor of real database integration. Important Files Changed
Confidence score: 4/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (3)
-
apps/core/__tests__/helpers/auth.ts, line 96-109 (link)logic: Mock verification function doesn't validate the JWT signature it just signed, defeating the security improvement. Should verify the HMAC signature or throw if invalid.
Should the mock verification function validate the JWT signature for more realistic testing?
-
apps/core/__tests__/helpers/auth.ts, line 102 (link)syntax: Using base64 instead of base64url for decoding - inconsistent with encoding on line 31.
-
apps/core/server/plugins/rate-limiting.plugin.ts, line 112 (link)syntax: Comment says 'Automatically disabled in development mode' but this is no longer accurate with the new environment-aware approach
18 files reviewed, 12 comments
| // This is safe because browsers always send Origin on cross-origin requests | ||
| if (!effectiveOrigin) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
style: The comment about skipping requests without origin being safe needs verification - some legitimate same-origin requests might not include Origin header, potentially creating a bypass. Have you verified that all legitimate same-origin requests in your application will include the Origin header, or should there be additional validation for requests without Origin?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/server/plugins/csrf.plugin.ts
Line: 98:101
Comment:
**style:** The comment about skipping requests without origin being safe needs verification - some legitimate same-origin requests might not include Origin header, potentially creating a bypass. Have you verified that all legitimate same-origin requests in your application will include the Origin header, or should there be additional validation for requests without Origin?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| async blockUserTokens(userId: string, reason?: string): Promise<void> { | ||
| // We can't block tokens we haven't seen | ||
| // For a proper "logout everywhere", the user should change their password | ||
| // or we should track active sessions | ||
| logger.info( | ||
| { userId, reason, context: "auth" }, | ||
| "User token blocklist requested (note: only blocks known tokens)", | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: The blockUserTokens method only logs but doesn't actually block any tokens, which could be misleading. Should this method query existing sessions or tokens for the user and block them, or is the current no-op implementation intentional?
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/server/services/TokenBlocklistService.ts
Line: 95:103
Comment:
**logic:** The blockUserTokens method only logs but doesn't actually block any tokens, which could be misleading. Should this method query existing sessions or tokens for the user and block them, or is the current no-op implementation intentional?
How can I resolve this? If you propose a fix, please make it concise.| ); | ||
| } catch (error: any) { | ||
| // Ignore duplicate key errors (token already blocklisted) | ||
| if (error.code === "23505") { |
There was a problem hiding this comment.
style: PostgreSQL-specific error code hardcoded - consider making this more database-agnostic or documenting the assumption
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/server/services/TokenBlocklistService.ts
Line: 63:63
Comment:
**style:** PostgreSQL-specific error code hardcoded - consider making this more database-agnostic or documenting the assumption
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| afterEach(async () => { | ||
| // Reset mock assets with CDN URLs | ||
| mockAssets.length = 0; | ||
| mockAssets.push( | ||
| { | ||
| // Re-create test assets if they were deleted during tests | ||
| // Check if assets still exist | ||
| const asset1Exists = await db.query.assets.findFirst({ | ||
| where: eq(assets.id, "test-asset-1"), | ||
| }); | ||
| const asset2Exists = await db.query.assets.findFirst({ | ||
| where: eq(assets.id, "test-asset-2"), | ||
| }); | ||
|
|
||
| if (!asset1Exists) { | ||
| testAsset1 = await createTestAsset(testUser1.user.id, { | ||
| id: "test-asset-1", | ||
| name: "Test Sword", | ||
| type: "weapon", | ||
| tier: 1, | ||
| category: "melee", | ||
| filePath: "test-asset-1/model.glb", | ||
| status: "completed", | ||
| visibility: "public", | ||
| cdnUrl: "https://cdn.asset-forge.com/models/test-asset-1/model.glb", | ||
| cdnThumbnailUrl: | ||
| "https://cdn.asset-forge.com/models/test-asset-1/thumbnail.png", | ||
| hasSpriteSheet: false, | ||
| createdBy: "user-123", | ||
| walletAddress: "0xABC", | ||
| isPublic: true, | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| }, | ||
| { | ||
| }); | ||
| } | ||
|
|
||
| if (!asset2Exists) { | ||
| testAsset2 = await createTestAsset(testAdmin.user.id, { | ||
| id: "test-asset-2", | ||
| name: "Admin Asset", | ||
| type: "armor", | ||
| tier: 2, | ||
| category: "heavy", | ||
| filePath: "test-asset-2/model.glb", | ||
| status: "completed", | ||
| visibility: "public", | ||
| cdnUrl: "https://cdn.asset-forge.com/models/test-asset-2/model.glb", | ||
| createdBy: "admin-456", | ||
| isPublic: true, | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
style: the afterEach recreation logic may cause issues if tests run in parallel - consider using unique asset IDs per test instead of shared ones
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/__tests__/integration/api/routes/assets.test.ts
Line: 103:139
Comment:
**style:** the afterEach recreation logic may cause issues if tests run in parallel - consider using unique asset IDs per test instead of shared ones
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| it("should return empty array when no assets exist", async () => { | ||
| mockAssets.length = 0; | ||
| // Delete all test assets from real database | ||
| await db.delete(assets).execute(); |
There was a problem hiding this comment.
logic: deleting all assets in the test database could affect parallel tests - consider scoping deletion to specific test assets only
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/__tests__/integration/api/routes/assets.test.ts
Line: 191:191
Comment:
**logic:** deleting all assets in the test database could affect parallel tests - consider scoping deletion to specific test assets only
How can I resolve this? If you propose a fix, please make it concise.| const { db } = await import("../db/db"); | ||
| const { activityLog } = await import("../db/schema"); | ||
| const { lt } = await import("drizzle-orm"); |
There was a problem hiding this comment.
style: inconsistent with other cleanup jobs that use service methods - consider creating an ActivityLogService with a cleanupOld() method for better separation of concerns
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/server/plugins/cron.plugin.ts
Line: 107:109
Comment:
**style:** inconsistent with other cleanup jobs that use service methods - consider creating an ActivityLogService with a `cleanupOld()` method for better separation of concerns
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| set.headers["X-Frame-Options"] = "DENY"; | ||
|
|
||
| // Legacy XSS protection (for older browsers) | ||
| set.headers["X-XSS-Protection"] = "1; mode=block"; |
There was a problem hiding this comment.
style: X-XSS-Protection is deprecated and can actually introduce vulnerabilities in some browsers. Consider removing this header and relying on CSP instead.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/server/plugins/security-headers.ts
Line: 38:38
Comment:
**style:** `X-XSS-Protection` is deprecated and can actually introduce vulnerabilities in some browsers. Consider removing this header and relying on CSP instead.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| */ | ||
| const RATE_LIMITING_ENABLED = | ||
| env.NODE_ENV === "production" || env.ENABLE_RATE_LIMITING === true; | ||
| const RATE_LIMITING_ENABLED = process.env.DISABLE_RATE_LIMITING !== "true"; |
There was a problem hiding this comment.
style: Using process.env directly bypasses the env validation system - should use env.DISABLE_RATE_LIMITING for consistency
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/server/plugins/rate-limiting.plugin.ts
Line: 43:43
Comment:
**style:** Using `process.env` directly bypasses the env validation system - should use `env.DISABLE_RATE_LIMITING` for consistency
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // Continue the chain | ||
| app |
There was a problem hiding this comment.
style: method chaining after conditional block creates confusing code structure - the app variable continues the chain but may not include the debug plugin. Should this be restructured to avoid the confusing method chaining pattern after the conditional debug plugin registration?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/core/server/api-elysia.ts
Line: 336:337
Comment:
**style:** method chaining after conditional block creates confusing code structure - the `app` variable continues the chain but may not include the debug plugin. Should this be restructured to avoid the confusing method chaining pattern after the conditional debug plugin registration?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| }); | ||
|
|
||
| afterEach(async () => { | ||
| // Reset mock assets with CDN URLs | ||
| mockAssets.length = 0; | ||
| mockAssets.push( | ||
| { | ||
| // Re-create test assets if they were deleted during tests | ||
| // Check if assets still exist | ||
| const asset1Exists = await db.query.assets.findFirst({ | ||
| where: eq(assets.id, "test-asset-1"), | ||
| }); | ||
| const asset2Exists = await db.query.assets.findFirst({ | ||
| where: eq(assets.id, "test-asset-2"), | ||
| }); | ||
|
|
||
| if (!asset1Exists) { | ||
| testAsset1 = await createTestAsset(testUser1.user.id, { | ||
| id: "test-asset-1", | ||
| name: "Test Sword", | ||
| type: "weapon", | ||
| tier: 1, | ||
| category: "melee", | ||
| filePath: "test-asset-1/model.glb", | ||
| status: "completed", | ||
| visibility: "public", | ||
| cdnUrl: "https://cdn.asset-forge.com/models/test-asset-1/model.glb", | ||
| cdnThumbnailUrl: | ||
| "https://cdn.asset-forge.com/models/test-asset-1/thumbnail.png", | ||
| hasSpriteSheet: false, | ||
| createdBy: "user-123", | ||
| walletAddress: "0xABC", | ||
| isPublic: true, | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| }, | ||
| { | ||
| }); | ||
| } | ||
|
|
||
| if (!asset2Exists) { | ||
| testAsset2 = await createTestAsset(testAdmin.user.id, { | ||
| id: "test-asset-2", | ||
| name: "Admin Asset", | ||
| type: "armor", | ||
| tier: 2, | ||
| category: "heavy", | ||
| filePath: "test-asset-2/model.glb", | ||
| status: "completed", | ||
| visibility: "public", | ||
| cdnUrl: "https://cdn.asset-forge.com/models/test-asset-2/model.glb", | ||
| createdBy: "admin-456", | ||
| isPublic: true, | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| ); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| describe("GET /api/assets", () => { |
There was a problem hiding this comment.
Potential Data Race in Asset Recreation
The afterEach hook attempts to recreate test assets if they were deleted during tests. If tests are executed in parallel or if asset creation is not atomic, this could lead to inconsistent test state or race conditions, resulting in flaky tests or unexpected failures.
Recommended Solution:
- Ensure tests are run sequentially, or use database transactions/locks to guarantee atomicity of asset creation and deletion.
- Consider using a dedicated test database with transaction rollback for each test to ensure isolation.
| // Plus CDN fields merged from database | ||
| expect(returnedAsset.cdnUrl).toBe( | ||
| "https://cdn.example.com/models/full-asset/model.glb", | ||
| ); |
There was a problem hiding this comment.
Insufficient Validation of Asset Metadata Fields
The CDN URL handling and migration validation tests check for the presence or absence of certain fields (e.g., cdnUrl, modelUrl, filePath) but do not comprehensively verify the integrity or correctness of all asset metadata. If the asset schema changes or the API response includes unexpected fields, these tests may not catch regressions.
Recommended Solution:
- Strengthen assertions to validate all expected asset fields and their types/values.
- Add negative tests to ensure no legacy or deprecated fields are present in the response.
| // If no origins configured in production, reject (don't fall back to wildcard) | ||
| if (allowedOrigins.length === 0) { | ||
| logger.warn( | ||
| { context: "cors" }, | ||
| "No CORS origins configured - rejecting cross-origin requests", | ||
| ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Potential for Outage Due to Strict CORS Configuration
If no allowed origins are configured (i.e., env.FRONTEND_URL and env.CORS_ALLOWED_ORIGINS are unset), the CORS middleware will reject all cross-origin requests, even in production. While this is secure, it could cause unexpected outages if environment variables are misconfigured or missing. The current implementation only logs a warning and does not prevent server startup.
Recommendation:
Consider adding a startup check that throws an error and prevents the server from starting if in production and no CORS origins are configured. This will ensure misconfiguration is detected early and avoids silent failures:
if (env.NODE_ENV === "production" && allowedOrigins.length === 0) {
throw new Error("No CORS origins configured in production. Set FRONTEND_URL or CORS_ALLOWED_ORIGINS.");
}| CREATE TABLE "token_blocklist" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | ||
| "token_id" varchar(255) NOT NULL, | ||
| "user_id" uuid, | ||
| "reason" varchar(100), | ||
| "created_at" timestamp with time zone DEFAULT now() NOT NULL, | ||
| "expires_at" timestamp with time zone NOT NULL, | ||
| CONSTRAINT "token_blocklist_token_id_unique" UNIQUE("token_id") | ||
| ); |
There was a problem hiding this comment.
Potential for Unbounded Table Growth
The migration does not include any mechanism to automatically remove expired tokens from the token_blocklist table. Over time, this could result in unbounded growth and degrade query performance. Consider implementing a scheduled job or database trigger to periodically delete rows where expires_at < now(). For example:
DELETE FROM token_blocklist WHERE expires_at < now();This will help maintain optimal performance and storage efficiency.
| CREATE TABLE "token_blocklist" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | ||
| "token_id" varchar(255) NOT NULL, | ||
| "user_id" uuid, |
There was a problem hiding this comment.
Missing Foreign Key Constraint on user_id
The user_id column is defined as a UUID but does not reference any users table. This could lead to orphaned records if user accounts are deleted. If possible, add a foreign key constraint to ensure referential integrity:
FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE SET NULLThis will help maintain data consistency between the blocklist and user records.
| error: "TOO_MANY_REQUESTS", | ||
| message, | ||
| } as any; | ||
| } |
There was a problem hiding this comment.
Type Safety Issue: Use of as any in Error Response
The createRateLimitError function (lines 32-37) returns an object cast as any. This undermines type safety and could lead to runtime errors or inconsistent error responses if the structure changes.
Recommendation:
Define and use a specific error response type, e.g.:
type RateLimitError = { error: string; message: string };
function createRateLimitError(message: string): RateLimitError { ... }This ensures consistent error handling and better maintainability.
| set.headers["Permissions-Policy"] = | ||
| "geolocation=(), microphone=(), camera=(), payment=()"; |
There was a problem hiding this comment.
Permissions-Policy header uses deprecated syntax.
The Permissions-Policy header is currently set as:
set.headers["Permissions-Policy"] = "geolocation=(), microphone=(), camera=(), payment=()";Modern browsers expect the new syntax, e.g.:
set.headers["Permissions-Policy"] = "geolocation=(), microphone=(), camera=(), payment=()";However, the correct format is:
set.headers["Permissions-Policy"] = "geolocation=(), microphone=(), camera=(), payment=()";But for broader compatibility, consider using the new Feature-Policy header or the updated Permissions-Policy syntax:
set.headers["Permissions-Policy"] = "geolocation=(), microphone=(), camera=(), payment=()";Review the latest MDN documentation and update the header for maximum browser support.
| set.headers["Strict-Transport-Security"] = | ||
| "max-age=31536000; includeSubDomains"; |
There was a problem hiding this comment.
Strict-Transport-Security header missing preload directive.
The HSTS header is set as:
set.headers["Strict-Transport-Security"] = "max-age=31536000; includeSubDomains";For sites intended to be included in browser preload lists, add the preload directive:
set.headers["Strict-Transport-Security"] = "max-age=31536000; includeSubDomains; preload";This increases security for first-time visitors. If you do not intend to submit your domain to the preload list, this can be omitted.
| async blockUserTokens(userId: string, reason?: string): Promise<void> { | ||
| // We can't block tokens we haven't seen | ||
| // For a proper "logout everywhere", the user should change their password | ||
| // or we should track active sessions | ||
| logger.info( | ||
| { userId, reason, context: "auth" }, | ||
| "User token blocklist requested (note: only blocks known tokens)", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Misleading Implementation of blockUserTokens
The blockUserTokens method only logs the request and does not actually block any tokens for the user. This could lead to a false sense of security for developers or users expecting all tokens to be invalidated. While the comments clarify the limitation, the method's name and signature suggest that it performs a blocking operation.
Recommendation:
- Either implement logic to block all known tokens for the user (e.g., by inserting blocklist entries for all tokens associated with the user), or rename the method and update its documentation to make its behavior explicit.
- Alternatively, throw an error or warning if the method is called, to prevent misuse.
| sourcemap: true, | ||
| // Disable source maps in production (security - don't expose source code) | ||
| // Enable in development for debugging | ||
| sourcemap: process.env.NODE_ENV !== "production", |
There was a problem hiding this comment.
Potential Security Risk: The sourcemap option is enabled based on process.env.NODE_ENV !== "production". If NODE_ENV is not set or is set incorrectly in the production environment, source maps may be unintentionally exposed, leaking source code and sensitive information.
Recommendation: Add a default fallback or stricter validation for NODE_ENV to ensure source maps are never exposed in production builds. For example:
sourcemap: process.env.NODE_ENV && process.env.NODE_ENV !== "production"Or explicitly default to false if NODE_ENV is not set:
sourcemap: process.env.NODE_ENV ? process.env.NODE_ENV !== "production" : false
PR Code Suggestions ✨
Explore these optional code suggestions:
|
||||||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of critical security and quality improvements across the application. The changes, including fixing the JWT test bypass, adding CSRF protection, hardening CORS and rate-limiting, and implementing a JWT blocklist, are excellent and significantly improve the application's security posture. The move from mocks to a real database in integration tests is also a major step forward for test reliability. My review focuses on a few areas to further strengthen these new security features, such as ensuring the token blocklist logic is fully robust and improving test coverage for API responses.
| async blockToken( | ||
| token: string, | ||
| userId?: string, | ||
| reason?: string, | ||
| expiresAt?: Date, | ||
| ): Promise<void> { | ||
| const tokenId = this.hashToken(token); | ||
|
|
||
| // Default expiry to 24 hours if not specified | ||
| const expiry = expiresAt || new Date(Date.now() + 24 * 60 * 60 * 1000); | ||
|
|
||
| try { | ||
| await db.insert(tokenBlocklist).values({ | ||
| tokenId, | ||
| userId: userId || null, | ||
| reason: reason || "logout", | ||
| expiresAt: expiry, | ||
| }); |
There was a problem hiding this comment.
The default 24-hour expiration for a blocklisted token is risky. If a JWT's original lifetime is longer than 24 hours, it could become valid again after the blocklist entry is prematurely removed. This would defeat the purpose of the logout/revocation feature. The expiration of the blocklist entry should always match the exp claim of the JWT itself. This change makes the expiresAt parameter mandatory and uses it directly, removing the unsafe default.
async blockToken(
token: string,
expiresAt: Date,
userId?: string,
reason?: string,
): Promise<void> {
const tokenId = this.hashToken(token);
try {
await db.insert(tokenBlocklist).values({
tokenId,
userId: userId || null,
reason: reason || "logout",
expiresAt,
});| // Should have all fields from database | ||
| expect(returnedAsset).toBeDefined(); | ||
| expect(returnedAsset.name).toBe("Full Asset"); | ||
| expect(returnedAsset.description).toBe("Test description"); | ||
| expect(returnedAsset.type).toBe("weapon"); | ||
| expect(returnedAsset.tier).toBe(3); | ||
| expect(returnedAsset.modelUrl).toBe("/gdd-assets/full-asset/model.glb"); | ||
|
|
||
| // Plus CDN fields merged from database | ||
| expect(returnedAsset.cdnUrl).toBe( | ||
| "https://cdn.example.com/models/full-asset/model.glb", | ||
| ); |
There was a problem hiding this comment.
This test is named should preserve all asset fields from database, but it doesn't check all the fields of the created asset. For example, the fullAsset is created with a description, but there's no assertion to verify its presence in the response. This could lead to undetected regressions if fields are accidentally removed from the API response in the future. To make the test more robust and align with its name, consider adding assertions for other important fields like description.
// Should have all fields from database
expect(returnedAsset).toBeDefined();
expect(returnedAsset.name).toBe("Full Asset");
expect(returnedAsset.description).toBe("Test description");
expect(returnedAsset.type).toBe("weapon");
expect(returnedAsset.cdnUrl).toBe(
"https://cdn.example.com/models/full-asset/model.glb",
);| } catch (error) { | ||
| // Don't block auth if blocklist check fails - log and continue | ||
| logger.warn( | ||
| { err: error, context: "auth" }, | ||
| "Failed to check token blocklist, continuing", | ||
| ); | ||
| } |
There was a problem hiding this comment.
The token blocklist check currently uses a 'fail-open' approach. If the database connection fails and isTokenBlocklisted throws an error, the check is bypassed, and authentication proceeds. This could allow a revoked token to be used during a database outage. For enhanced security, consider a 'fail-closed' strategy by rejecting the token within the catch block. This prioritizes security over availability in this specific failure scenario.
} catch (error) {
// Fail-closed: If blocklist check fails, reject the token for security.
logger.error(
{ err: error, context: "auth" },
"Failed to check token blocklist, rejecting token.",
);
return {};
}![codiumai-pr-agent-free[bot]](https://avatars.githubusercontent.com/in/354216?s=60&v=4){kind=small}
Co-authored-by: codiumai-pr-agent-free[bot] <138128286+codiumai-pr-agent-free[bot]@users.noreply.github.com>
Co-authored-by: codiumai-pr-agent-free[bot] <138128286+codiumai-pr-agent-free[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/core/server/models.ts (1)
159-159: Add maxLength constraint to notes field for consistency.The
notesfield inAssetMetadatalacks a maxLength constraint, while the same field inAssetUpdate(line 225) hasmaxLength: 5000. This inconsistency could allow unbounded notes during creation/retrieval but limit them during updates.Apply this diff to enforce consistent validation:
- notes: t.Optional(t.String()), // User notes for this asset + notes: t.Optional(t.String({ maxLength: 5000 })), // User notes for this assetapps/core/server/plugins/rate-limiting.plugin.ts (2)
109-113: Outdated comment about development mode.The comment on line 112 says "Automatically disabled in development mode" but the new logic keeps rate limiting enabled by default in all environments (with higher limits in dev/test). Update to reflect the actual behavior.
/** * Rate Limiting Plugin * Applies global rate limiting and endpoint-specific limits - * Automatically disabled in development mode + * Rate limits scale by environment (10x in dev, 100x in test) + * Can be disabled with DISABLE_RATE_LIMITING=true */
156-158: Fallback to hostname creates shared rate limit bucket.When IP detection fails, falling back to
new URL(req.url).hostnamemeans all requests to the same host share one rate limit bucket, which could either:
- Block legitimate users if one abuser hits the limit
- Be easily bypassed since all users share the quota
Consider using a unique request identifier or returning a more restrictive fallback:
- // Last resort: use request URL - return new URL(req.url).hostname || "unknown"; + // Last resort: use a unique identifier per request + // This is restrictive but prevents abuse when IP detection fails + return `unknown-${Date.now()}-${Math.random()}`;Alternatively, log a warning and apply stricter limits for "unknown" clients.
apps/core/__tests__/integration/api/routes/assets.test.ts (1)
189-201: Test deletes all assets but may affect other concurrent test runs.
db.delete(assets).execute()deletes ALL assets in the database, not just test assets. This could cause issues if:
- Tests run in parallel
- Other test suites share the same database
Consider scoping the deletion to test assets only.
- // Delete all test assets from real database - await db.delete(assets).execute(); + // Delete only test assets to avoid affecting other tests + await db.delete(assets).where( + eq(assets.id, "test-asset-1") + ).execute(); + await db.delete(assets).where( + eq(assets.id, "test-asset-2") + ).execute();
🧹 Nitpick comments (9)
apps/core/src/components/three-viewer/REFACTORING_PLAN.ts (1)
1-52: Consider using Markdown format for documentation.While this refactoring plan is well-structured and thorough, using a
.tsfile extension for pure documentation is unconventional. This file contains no executable TypeScript code—only comments.Consider these alternatives:
- Move to
REFACTORING_PLAN.mdin the same directory or adocs/folder- Create an ADR (Architecture Decision Record) if your project uses them
- Document this in a GitHub issue or project board for better visibility and tracking
Additionally, the metrics on lines 45-48 (ref/state/effect counts) are point-in-time snapshots that may become outdated if the ThreeViewer component changes before refactoring begins.
</comment_end>
apps/core/server/plugins/security-headers.ts (2)
37-38: X-XSS-Protection is deprecated in modern browsers.This header is ignored by modern browsers that have built-in XSS protections. Consider implementing a Content-Security-Policy (CSP) header instead for comprehensive XSS protection:
// Example CSP - adjust based on your needs set.headers["Content-Security-Policy"] = "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline';";Not blocking since X-XSS-Protection doesn't cause harm and provides backward compatibility.
47-51: Consider addingpreloaddirective for HSTS submission.If you plan to submit to the HSTS preload list for maximum security, the directive requires
preload:- set.headers["Strict-Transport-Security"] = - "max-age=31536000; includeSubDomains"; + set.headers["Strict-Transport-Security"] = + "max-age=31536000; includeSubDomains; preload";Only add
preloadif you're certain all subdomains support HTTPS and you intend to submit to hstspreload.org.apps/core/.env.example (1)
98-98: Clarify minimum length requirement for WEBHOOK_SECRET.The comment says "32_chars_minimum" but the placeholder value
your_webhook_secret_32_chars_minimumis 36 characters and could be confused as the actual minimum. Consider:-WEBHOOK_SECRET=your_webhook_secret_32_chars_minimum +WEBHOOK_SECRET= # Generate with: openssl rand -base64 32apps/core/server/plugins/csrf.plugin.ts (1)
82-129: CSRF validation logic is secure with minor improvement opportunity.The origin validation correctly:
- Extracts origin from Origin or Referer headers
- Skips validation for same-origin requests (when Origin is absent)
- Returns 403 for disallowed origins with comprehensive logging
Optional defensive improvement: Line 112's
new URL(request.url).pathnamecould be wrapped in a try-catch if Elysia doesn't guarantee valid URLs, though this is likely unnecessary since Elysia validates requests.apps/core/server/services/TokenBlocklistService.ts (2)
61-67: Avoid usinganytype for error handling.Per coding guidelines,
anytypes should not be used. Use a typed approach for PostgreSQL error handling.- } catch (error: any) { + } catch (error: unknown) { // Ignore duplicate key errors (token already blocklisted) - if (error.code === "23505") { + if (error instanceof Error && 'code' in error && (error as { code: string }).code === "23505") { return; } throw error;Alternatively, create a type guard for PostgreSQL errors:
function isPostgresError(error: unknown): error is { code: string } { return typeof error === 'object' && error !== null && 'code' in error; }
90-103: Clarify or implement blockUserTokens functionality.This method logs a message but performs no actual blocking. If this is intentional design (as noted in the comment), consider:
- Marking the method as deprecated or removing it until implemented
- Throwing a
NotImplementedErrorto make the limitation explicit at runtime- Adding a TODO comment with an issue reference
The current implementation could mislead callers into thinking tokens were blocked.
apps/core/server/plugins/auth.plugin.ts (1)
181-186: Consider using static import for crypto module.The dynamic
await import("crypto")on every test-mode authentication adds unnecessary overhead. Thecryptomodule is already imported at the top ofTokenBlocklistService.ts, suggesting it's available in this environment.Add to existing imports at the top of the file:
import { createHmac } from "crypto";Then simplify the verification:
- const crypto = await import("crypto"); - const signatureInput = `${parts[0]}.${parts[1]}`; - const expectedSignature = crypto - .createHmac("sha256", testSecret) + const signatureInput = `${parts[0]}.${parts[1]}`; + const expectedSignature = createHmac("sha256", testSecret) .update(signatureInput) .digest("base64url");apps/core/__tests__/integration/api/routes/assets.test.ts (1)
103-140: Test isolation approach has a potential race condition.The
afterEachre-creates assets only if they don't exist, which is good for isolation. However, if a test modifies an asset (e.g., changes itsname), subsequent tests may see stale data since re-creation only happens when the asset is deleted.Consider either:
- Always deleting and re-creating test assets in
afterEach- Or resetting asset fields to known values after each test
If full isolation is needed:
afterEach(async () => { - // Re-create test assets if they were deleted during tests - // Check if assets still exist - const asset1Exists = await db.query.assets.findFirst({ - where: eq(assets.id, "test-asset-1"), - }); + // Always reset test assets to known state + await db.delete(assets).where(eq(assets.id, "test-asset-1")).execute(); + await db.delete(assets).where(eq(assets.id, "test-asset-2")).execute(); + + testAsset1 = await createTestAsset(testUser1.user.id, { + id: "test-asset-1", + // ... original values + }); + testAsset2 = await createTestAsset(testAdmin.user.id, { + id: "test-asset-2", + // ... original values + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/core/.env.example(1 hunks)apps/core/__tests__/helpers/auth.ts(2 hunks)apps/core/__tests__/integration/api/routes/assets.test.ts(13 hunks)apps/core/server/api-elysia.ts(4 hunks)apps/core/server/db/migrations/0032_fix_schema_drift.sql(1 hunks)apps/core/server/db/migrations/0033_add_token_blocklist.sql(1 hunks)apps/core/server/db/migrations/meta/_journal.json(1 hunks)apps/core/server/db/schema/index.ts(3 hunks)apps/core/server/db/schema/token-blocklist.schema.ts(1 hunks)apps/core/server/models.ts(3 hunks)apps/core/server/plugins/auth.plugin.ts(5 hunks)apps/core/server/plugins/cron.plugin.ts(4 hunks)apps/core/server/plugins/csrf.plugin.ts(1 hunks)apps/core/server/plugins/rate-limiting.plugin.ts(2 hunks)apps/core/server/plugins/security-headers.ts(2 hunks)apps/core/server/services/TokenBlocklistService.ts(1 hunks)apps/core/src/components/three-viewer/REFACTORING_PLAN.ts(1 hunks)apps/core/vite.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>in the Asset-Forge project
Useconstoverlet, never usevar
Use template literals for strings instead of string concatenation
Files:
apps/core/vite.config.tsapps/core/server/db/schema/index.tsapps/core/server/db/schema/token-blocklist.schema.tsapps/core/server/api-elysia.tsapps/core/server/plugins/cron.plugin.tsapps/core/server/plugins/rate-limiting.plugin.tsapps/core/server/services/TokenBlocklistService.tsapps/core/server/plugins/security-headers.tsapps/core/server/models.tsapps/core/server/plugins/csrf.plugin.tsapps/core/__tests__/integration/api/routes/assets.test.tsapps/core/__tests__/helpers/auth.tsapps/core/server/plugins/auth.plugin.tsapps/core/src/components/three-viewer/REFACTORING_PLAN.ts
**/*.{ts,tsx,js,jsx,json,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Use 2-space indentation for all code
Files:
apps/core/vite.config.tsapps/core/server/db/migrations/meta/_journal.jsonapps/core/server/db/schema/index.tsapps/core/server/db/schema/token-blocklist.schema.tsapps/core/server/api-elysia.tsapps/core/server/plugins/cron.plugin.tsapps/core/server/plugins/rate-limiting.plugin.tsapps/core/server/services/TokenBlocklistService.tsapps/core/server/plugins/security-headers.tsapps/core/server/models.tsapps/core/server/plugins/csrf.plugin.tsapps/core/__tests__/integration/api/routes/assets.test.tsapps/core/__tests__/helpers/auth.tsapps/core/server/plugins/auth.plugin.tsapps/core/src/components/three-viewer/REFACTORING_PLAN.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript strict mode everywhere
Noanyorunknowntypes in TypeScript code
Use explicit return types on public methods in TypeScript
Files:
apps/core/vite.config.tsapps/core/server/db/schema/index.tsapps/core/server/db/schema/token-blocklist.schema.tsapps/core/server/api-elysia.tsapps/core/server/plugins/cron.plugin.tsapps/core/server/plugins/rate-limiting.plugin.tsapps/core/server/services/TokenBlocklistService.tsapps/core/server/plugins/security-headers.tsapps/core/server/models.tsapps/core/server/plugins/csrf.plugin.tsapps/core/__tests__/integration/api/routes/assets.test.tsapps/core/__tests__/helpers/auth.tsapps/core/server/plugins/auth.plugin.tsapps/core/src/components/three-viewer/REFACTORING_PLAN.ts
apps/core/server/db/migrations/**/*.sql
📄 CodeRabbit inference engine (CLAUDE.md)
Generate database migration with
bun run db:generateafter editing schema
Files:
apps/core/server/db/migrations/0032_fix_schema_drift.sqlapps/core/server/db/migrations/0033_add_token_blocklist.sql
apps/core/server/db/schema/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Edit TypeScript schema in
apps/core/server/db/schema/when schema changes are needed
Files:
apps/core/server/db/schema/index.tsapps/core/server/db/schema/token-blocklist.schema.ts
apps/core/server/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/core/server/**/*.ts: Research with Deepwiki before adding new endpoints in Elysia
Define API types in service file with TypeBox validation in Elysia endpoints
Implement business logic in service class for API endpoints
Create route handler with TypeBox validation for API endpoints
Add Swagger documentation to new API endpoints
Follow REST conventions for API endpoints
Files:
apps/core/server/db/schema/index.tsapps/core/server/db/schema/token-blocklist.schema.tsapps/core/server/api-elysia.tsapps/core/server/plugins/cron.plugin.tsapps/core/server/plugins/rate-limiting.plugin.tsapps/core/server/services/TokenBlocklistService.tsapps/core/server/plugins/security-headers.tsapps/core/server/models.tsapps/core/server/plugins/csrf.plugin.tsapps/core/server/plugins/auth.plugin.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Usebun testinstead ofjestorvitestfor testing
NO MOCKS for internal code (database, HTTP handlers, business logic) - use real PostgreSQL database with test data isolation
SMART MOCKS for external APIs (OpenAI, Meshy, Privy) to avoid costs and ensure reliability
Files:
apps/core/__tests__/integration/api/routes/assets.test.ts
🧬 Code graph analysis (6)
apps/core/server/plugins/cron.plugin.ts (4)
apps/core/server/services/GenerationPipelineService.ts (1)
generationPipelineService(333-333)apps/core/server/db/db.ts (1)
db(95-98)apps/core/server/db/schema/users.schema.ts (1)
activityLog(143-183)apps/core/server/services/TokenBlocklistService.ts (1)
tokenBlocklistService(107-107)
apps/core/server/plugins/rate-limiting.plugin.ts (1)
apps/core/server/config/env.ts (1)
env(320-320)
apps/core/server/services/TokenBlocklistService.ts (3)
apps/core/server/db/db.ts (1)
db(95-98)apps/core/server/db/schema/token-blocklist.schema.ts (1)
tokenBlocklist(14-35)apps/core/server/routes/content/shared.ts (1)
logger(23-23)
apps/core/server/plugins/csrf.plugin.ts (2)
apps/core/server/config/env.ts (1)
env(320-320)apps/core/server/routes/content/shared.ts (1)
logger(23-23)
apps/core/__tests__/integration/api/routes/assets.test.ts (3)
apps/core/__tests__/helpers/db.ts (2)
createTestUser(125-154)createTestAsset(168-188)apps/core/server/db/db.ts (1)
db(95-98)apps/core/server/db/schema/assets.schema.ts (1)
assets(25-145)
apps/core/server/plugins/auth.plugin.ts (3)
apps/core/server/services/TokenBlocklistService.ts (1)
tokenBlocklistService(107-107)apps/core/server/routes/content/shared.ts (3)
logger(23-23)Elysia(50-50)ForbiddenError(55-55)apps/core/server/config/env.ts (1)
env(320-320)
🪛 dotenv-linter (4.0.0)
apps/core/.env.example
[warning] 11-11: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 22-22: [UnorderedKey] The API_PORT key should go before the PORT key
(UnorderedKey)
[warning] 72-72: [UnorderedKey] The QDRANT_API_KEY key should go before the QDRANT_URL key
(UnorderedKey)
[warning] 87-87: [UnorderedKey] The CDN_API_KEY key should go before the CDN_URL key
(UnorderedKey)
[warning] 89-89: [UnorderedKey] The AUTO_PUBLISH_TO_CDN key should go before the CDN_API_KEY key
(UnorderedKey)
[warning] 99-99: [UnorderedKey] The CDN_WEBHOOK_ENABLED key should go before the WEBHOOK_SECRET key
(UnorderedKey)
[warning] 112-112: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🔇 Additional comments (30)
apps/core/src/components/three-viewer/REFACTORING_PLAN.ts (1)
54-54: Empty export is unnecessary for documentation.The
export {}pattern is typically used to force TypeScript to treat a file as a module. However, since this file is pure documentation, module scope provides no benefit. If converted to a Markdown file as suggested above, this line would be unnecessary.</comment_end>
apps/core/vite.config.ts (1)
118-120: Good security practice to disable sourcemaps in production.The conditional logic correctly enables sourcemaps for development/test debugging while hiding source code in production builds. Ensure
NODE_ENV=productionis set during CI/CD build steps to take effect.apps/core/server/plugins/security-headers.ts (1)
17-17: LGTM!Using
constwith environment check at module load time is efficient and follows the coding guidelines.apps/core/.env.example (2)
1-7: Well-structured environment configuration template.The organization with clear sections, required/optional markers, and the deployment checklist is excellent for developer onboarding and production readiness.
36-39: Good guidance on encryption secret generation.The
openssl rand -base64 32instruction is helpful. Consider adding a note that this secret must be consistent across deployments to decrypt existing API keys.apps/core/server/plugins/rate-limiting.plugin.ts (1)
54-63: LGTM - Well-defined base rate limits.The base limits are reasonable for production, and the multiplier pattern provides appropriate scaling for development and testing environments.
apps/core/server/db/migrations/0032_fix_schema_drift.sql (1)
1-12: Migration 0032 duplicates the table drop already executed in migration 0007.Migration 0007 already dropped
admin_whitelistwith CASCADE. Migration 0032 attempts to drop the same table again 25 versions later. While theIF EXISTSclauses make this harmless, it's redundant and indicates either:
- A migration history issue if 0007 wasn't applied, or
- Unnecessary duplication if 0007 was successfully applied
The original review's concerns about data loss and backups are misdirected—any data in
admin_whitelistwould have been lost in migration 0007, not 0032.Likely an incorrect or invalid review comment.
apps/core/server/db/migrations/meta/_journal.json (1)
228-242: LGTM! Migration journal entries added correctly.The new migration entries follow the established pattern and properly track the schema drift fix and token blocklist additions.
apps/core/server/db/schema/index.ts (1)
45-46: LGTM! Token blocklist schema properly integrated.The new schema is correctly exported, imported, and spread into the central schema object, following the established pattern used by other schemas in this file.
Also applies to: 62-62, 78-78
apps/core/server/plugins/cron.plugin.ts (3)
20-20: LGTM! Import follows established pattern.The TokenBlocklistService import is consistent with other service imports in the file.
99-129: LGTM! Activity log cleanup properly implemented.The cron job correctly:
- Uses dynamic imports to minimize memory footprint
- Applies a reasonable 90-day retention policy
- Handles potential null rowCount with fallback to 0
- Includes comprehensive error handling and logging
131-146: LGTM! Token blocklist cleanup properly delegated.The cron job correctly delegates to the service layer for cleanup logic, maintaining good separation of concerns with proper error handling and logging.
apps/core/__tests__/helpers/auth.ts (1)
23-46: LGTM! JWT signing correctly implemented.The test helper now generates properly signed JWTs using:
- HMAC-SHA256 matching the HS256 algorithm
- Base64url encoding (JWT spec compliant)
- Correct signature computation over
header.body- Reasonable token expiration (1 hour)
This ensures test tokens behave like real JWTs and can be validated cryptographically.
apps/core/server/api-elysia.ts (4)
42-42: LGTM! CSRF plugin import added.
255-293: LGTM! Secure CORS origin handling with no wildcard fallback.The dynamic origin function correctly:
- Requires explicit origin configuration (no wildcard)
- Rejects requests when origins are unconfigured
- Adds localhost origins only in non-production environments
- Logs warnings for rejected origins
The
NODE_ENV !== "production"check (line 270) allows localhost in staging/test environments, which is appropriate for flexibility during deployment validation.
303-306: LGTM! Middleware order is correct.The plugin registration order is appropriate:
- Rate limiting (prevent abuse)
- Auth (identify user/API key)
- CSRF (validate origin, with API key bypass)
This ensures CSRF can properly detect and skip API key requests.
325-334: Debug plugin properly restricted to non-production.The conditional registration correctly prevents debug endpoints from being exposed in production environments. The pattern temporarily breaks the fluent chain (line 337) but is functionally correct.
apps/core/server/plugins/csrf.plugin.ts (2)
46-60: LGTM! Origin matching logic is robust.The function correctly:
- Rejects null/missing origins
- Performs exact matches first (efficient)
- Handles trailing slash variations (prevents false rejections)
66-80: LGTM! Method filtering and API key bypass correctly implemented.The plugin appropriately:
- Only validates state-changing methods (POST, PUT, DELETE, PATCH)
- Skips CSRF checks for API key authentication (machine-to-machine)
- Uses defensive optional chaining for header access
apps/core/server/db/schema/token-blocklist.schema.ts (1)
37-38: LGTM! Type exports follow Drizzle ORM conventions.The select and insert types are properly inferred and clearly named for use throughout the application.
apps/core/server/services/TokenBlocklistService.ts (2)
1-18: LGTM - Secure token hashing implementation.Good security practice to hash tokens before storage using SHA-256, preventing exposure of raw tokens if the database is compromised.
20-33: LGTM - Clean blocklist lookup.The implementation correctly hashes the token before lookup and uses a limit(1) for efficiency.
apps/core/server/plugins/auth.plugin.ts (3)
73-91: LGTM - Resilient blocklist integration.Good design choices:
- Only checking blocklist for JWT tokens (not API keys)
- Graceful degradation - auth continues if blocklist check fails
- Appropriate logging for both blocked tokens and errors
449-470: LGTM - Proper admin role enforcement.The updated
requireAdminGuardnow correctly:
- Requires authentication first
- Checks
role === "admin"explicitly- Throws
ForbiddenError(403) for non-admin users- Logs access attempts for audit trail
160-194: NODE_ENV validation confirmed—test-mode JWT verification is secure.The Zod enum validation in
apps/core/server/config/env.ts(lines 20-22) strictly constrainsNODE_ENVto["development", "production", "test"], making it impossible to manipulate. The test-mode code path is reliably guarded and cannot be accidentally triggered in production. The non-constant-time string comparison in the JWT verification is acceptable for test-mode only.apps/core/__tests__/integration/api/routes/assets.test.ts (5)
4-7: LGTM - Good alignment with testing guidelines.The comment correctly references the CLAUDE.md testing policy about using real databases instead of mocks for internal services.
165-187: Test assertions for legacy fields should use strict checks.The assertions at lines 185-186 verify legacy fields don't exist, which is good for validating CDN migration. The test properly validates that
cdnUrlcontains the expected domain.
634-683: LGTM - Comprehensive CDN URL validation tests.These tests properly validate:
- CDN URL fields are populated from database records
- All CDN-related fields (
cdnUrl,cdnThumbnailUrl,cdnConceptArtUrl,cdnFiles) are correctly returned- Test data is created with real database operations
685-715: Good edge case coverage for assets without CDN URLs.Testing the null/falsy case for
cdnUrlensures the API handles both CDN-enabled and local-only assets correctly.
822-836: Smart approach to validate CDN migration completeness.Checking the entire JSON response string for legacy field names (
modelUrl,filePath,/gdd-assets/) is an effective way to catch any accidental leakage of deprecated fields.
| export const tokenBlocklist = pgTable( | ||
| "token_blocklist", | ||
| { | ||
| id: uuid("id").defaultRandom().primaryKey(), | ||
| // Token identifier (jti claim from JWT or hash of token) | ||
| tokenId: varchar("token_id", { length: 255 }).notNull().unique(), | ||
| // User who owned this token (for audit) | ||
| userId: uuid("user_id"), | ||
| // Reason for blocklisting | ||
| reason: varchar("reason", { length: 100 }), | ||
| // When the token was blocklisted | ||
| createdAt: timestamp("created_at", { withTimezone: true }) | ||
| .defaultNow() | ||
| .notNull(), | ||
| // When the original token expires (for cleanup) | ||
| expiresAt: timestamp("expires_at", { withTimezone: true }).notNull(), | ||
| }, | ||
| (table) => [ | ||
| index("idx_token_blocklist_token_id").on(table.tokenId), | ||
| index("idx_token_blocklist_expires_at").on(table.expiresAt), | ||
| ], | ||
| ); |
There was a problem hiding this comment.
Remove redundant index on tokenId (consistent with SQL migration issue).
Line 19 declares tokenId with .unique(), which automatically creates a unique index. Line 32 then explicitly creates another index on the same column, resulting in duplicate indexes.
Remove the redundant index definition:
(table) => [
- index("idx_token_blocklist_token_id").on(table.tokenId),
index("idx_token_blocklist_expires_at").on(table.expiresAt),
],
);This issue is consistent with the redundant CREATE INDEX in the SQL migration (apps/core/server/db/migrations/0033_add_token_blocklist.sql line 14).
🤖 Prompt for AI Agents
In apps/core/server/db/schema/token-blocklist.schema.ts around lines 14 to 35,
the tokenId column is declared with .unique() which already creates a unique
index, but there is an explicit index added later on the same column; remove the
redundant index definition inside the pgTable index array (delete the
index("idx_token_blocklist_token_id").on(table.tokenId) entry) and also update
the corresponding SQL migration
(apps/core/server/db/migrations/0033_add_token_blocklist.sql) to remove the
duplicate CREATE INDEX for token_id so only one index is created.
| * Rate limiting is now ALWAYS enabled by default | ||
| * Can only be disabled explicitly with DISABLE_RATE_LIMITING=true (for specific tests) | ||
| */ | ||
| const RATE_LIMITING_ENABLED = | ||
| env.NODE_ENV === "production" || env.ENABLE_RATE_LIMITING === true; | ||
| const RATE_LIMITING_ENABLED = process.env.DISABLE_RATE_LIMITING !== "true"; | ||
|
|
||
| /** | ||
| * Multiplier for rate limits based on environment | ||
| * - Production: 1x (base limits) | ||
| * - Development: 10x (relaxed for testing) | ||
| * - Test: 100x (very relaxed for automated tests) | ||
| */ | ||
| const RATE_LIMIT_MULTIPLIER = | ||
| env.NODE_ENV === "production" ? 1 : env.NODE_ENV === "test" ? 100 : 10; |
There was a problem hiding this comment.
🧩 Analysis chain
Inconsistent environment variable access pattern.
Line 43 uses process.env.DISABLE_RATE_LIMITING while line 52 uses env.NODE_ENV from the imported config. This inconsistency could cause issues if the env config applies transformations or validation.
-const RATE_LIMITING_ENABLED = process.env.DISABLE_RATE_LIMITING !== "true";
+const RATE_LIMITING_ENABLED = env.DISABLE_RATE_LIMITING !== true;Verify that DISABLE_RATE_LIMITING is defined in your env schema (apps/core/server/config/env.ts) and use the parsed env object consistently.
🏁 Script executed:
#!/bin/bash
# Check if DISABLE_RATE_LIMITING is defined in env schema
echo "=== Checking env schema for DISABLE_RATE_LIMITING ==="
rg -n "DISABLE_RATE_LIMITING" apps/core/server/config/Length of output: 53
Now I need to look at the actual env config and rate-limiting plugin files to verify the issue:
🏁 Script executed:
#!/bin/bash
# Look at the env schema file to understand its structure
echo "=== Checking env schema file ==="
cat apps/core/server/config/env.ts 2>/dev/null | head -100
echo -e "\n=== Checking imports in rate-limiting plugin ==="
head -20 apps/core/server/plugins/rate-limiting.plugin.ts
echo -e "\n=== Checking all DISABLE_RATE_LIMITING usages ==="
rg -n "DISABLE_RATE_LIMITING" --type ts --type tsxLength of output: 4097
🏁 Script executed:
#!/bin/bash
# Search for the complete env schema to see how optional flags are typed
echo "=== Checking complete env schema for optional flags ==="
cat apps/core/server/config/env.ts | grep -A 2 "\.optional\|\.default\|\.boolean" | head -40
# Look for test-related environment variables
echo -e "\n=== Searching for test-related env variables ==="
rg "test|TEST" apps/core/server/config/env.ts -n
# Check rate-limiting plugin full context around line 43 and 52
echo -e "\n=== Context around DISABLE_RATE_LIMITING and NODE_ENV usage ==="
sed -n '35,60p' apps/core/server/plugins/rate-limiting.plugin.tsLength of output: 2052
Add DISABLE_RATE_LIMITING to env schema and use consistent environment variable access.
Line 43 uses direct process.env.DISABLE_RATE_LIMITING access while line 52 uses validated env.NODE_ENV from the schema. This inconsistency violates the application's env validation pattern.
Add DISABLE_RATE_LIMITING to apps/core/server/config/env.ts:
DISABLE_RATE_LIMITING: z.string().optional().transform((val) => val === "true").default("false"),Then update line 43 to use the validated env object:
const RATE_LIMITING_ENABLED = !env.DISABLE_RATE_LIMITING;🤖 Prompt for AI Agents
In apps/core/server/plugins/rate-limiting.plugin.ts around lines 40 to 52, the
code inconsistently reads DISABLE_RATE_LIMITING from process.env while NODE_ENV
is read from the validated env schema; add DISABLE_RATE_LIMITING to
apps/core/server/config/env.ts as a validated optional boolean (transforming the
"true" string to true and defaulting to false) and then replace the process.env
access with the schema-backed value by using env.DISABLE_RATE_LIMITING (so
RATE_LIMITING_ENABLED becomes the negation of env.DISABLE_RATE_LIMITING).
Per Elysia best practices, add createTestClient() and createAuthTestClient() helpers that use Eden Treaty for zero-overhead, type-safe testing. Eden Treaty is the RECOMMENDED approach because: - Full TypeScript autocomplete for routes - Compile-time type validation - Zero network overhead (calls app.handle() internally) - No manual Request construction needed The existing app.handle() utilities remain available for low-level testing. See: https://elysiajs.com/patterns/unit-test See: https://elysiajs.com/eden/treaty/unit-test
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // In development, allow localhost | ||
| if (env.NODE_ENV === "development") { | ||
| origins.push( | ||
| "http://localhost:3000", |
There was a problem hiding this comment.
The CSRF whitelist only adds localhost origins when NODE_ENV === "development" (csrf.plugin.ts lines 30-33), while the CORS middleware still allows the same localhost origins for any non‑production environment (api-elysia.ts lines 269-276). When the server runs under the normal test/CI setting NODE_ENV=test without FRONTEND_URL/CORS_ALLOWED_ORIGINS configured, the allowedOrigins array stays empty and every browser POST/PUT/DELETE from http://localhost:3000/3004 will hit the CSRF 403 even though CORS preflight succeeds, effectively blocking state‑changing requests in test/staging unless extra env vars are supplied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/core/server/plugins/auth.plugin.ts (1)
96-96: Consider using a singleton ApiKeyService instance.This instantiation-per-request pattern was previously flagged as a potential performance issue. If thread safety and statelessness allow, consider using a shared instance.
🧹 Nitpick comments (2)
apps/core/__tests__/helpers/api.ts (1)
62-65: Handle potential nullprivyUserIdsafely.According to the
AuthUsertype definition,privyUserIdcan benull, butcreateAuthHeaderexpects astring. While authenticated test users typically have a non-nullprivyUserId, the type system doesn't enforce this.Add a runtime check or non-null assertion:
return treaty(app, { headers: { - Authorization: createAuthHeader( - user.privyUserId, - user.email || undefined, - ), + Authorization: createAuthHeader( + user.privyUserId ?? '', + user.email || undefined, + ), }, });Alternatively, throw an error if
privyUserIdis null to make the requirement explicit:+ if (!user.privyUserId) { + throw new Error('createAuthTestClient requires a user with a valid privyUserId'); + } return treaty(app, { headers: { Authorization: createAuthHeader( user.privyUserId, user.email || undefined, ), }, });apps/core/server/plugins/auth.plugin.ts (1)
182-182: Import crypto module at the top of the file.The
cryptomodule is a built-in Node.js module and doesn't need dynamic import. This adds unnecessary overhead on every request in test mode.Apply this change:
+import crypto from "crypto";Then update line 182-187:
- const crypto = await import("crypto"); const signatureInput = `${parts[0]}.${parts[1]}`; const expectedSignature = crypto .createHmac("sha256", testSecret)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/core/__tests__/helpers/api.ts(1 hunks)apps/core/server/plugins/auth.plugin.ts(5 hunks)apps/core/server/services/TokenBlocklistService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/core/server/services/TokenBlocklistService.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>in the Asset-Forge project
Useconstoverlet, never usevar
Use template literals for strings instead of string concatenation
Files:
apps/core/__tests__/helpers/api.tsapps/core/server/plugins/auth.plugin.ts
**/*.{ts,tsx,js,jsx,json,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Use 2-space indentation for all code
Files:
apps/core/__tests__/helpers/api.tsapps/core/server/plugins/auth.plugin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript strict mode everywhere
Noanyorunknowntypes in TypeScript code
Use explicit return types on public methods in TypeScript
Files:
apps/core/__tests__/helpers/api.tsapps/core/server/plugins/auth.plugin.ts
apps/core/server/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/core/server/**/*.ts: Research with Deepwiki before adding new endpoints in Elysia
Define API types in service file with TypeBox validation in Elysia endpoints
Implement business logic in service class for API endpoints
Create route handler with TypeBox validation for API endpoints
Add Swagger documentation to new API endpoints
Follow REST conventions for API endpoints
Files:
apps/core/server/plugins/auth.plugin.ts
🧬 Code graph analysis (1)
apps/core/__tests__/helpers/api.ts (2)
apps/core/server/types/auth.ts (1)
AuthUser(7-17)apps/core/__tests__/helpers/auth.ts (1)
createAuthHeader(52-55)
🔇 Additional comments (1)
apps/core/server/plugins/auth.plugin.ts (1)
460-471: LGTM! Proper admin role enforcement.The admin guard correctly implements role-based access control:
- Checks authentication first
- Validates
role === "admin"- Throws appropriate
ForbiddenError(403) when access denied- Includes helpful logging for audit trails
This aligns with the PR's security objectives to implement RBAC for admin routes.
apps/core/__tests__/helpers/api.ts
Outdated
| export function createTestClient< | ||
| T extends Elysia<any, any, any, any, any, any>, | ||
| >(app: T) { | ||
| return treaty(app); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Add explicit return type and avoid any in generic constraints.
This function violates two coding guidelines:
- Uses
anytypes in the generic constraint - Missing explicit return type
As per coding guidelines, TypeScript strict mode requires explicit return types on public methods and prohibits any types.
Consider using the actual Elysia type parameters or unknown with proper type constraints. Also add an explicit return type. Please verify the correct type signature for treaty return type:
🌐 Web query:
What is the return type of the treaty function from @elysiajs/eden package?
💡 Result:
The TypeScript return type is Treaty.Create — i.e. treaty is declared as
(domain: string | App, config?: Treaty.Config) => Treaty.Create. [1]
Sources:
[1] @elysiajs/eden dist types (index.d.ts).
🏁 Script executed:
#!/bin/bash
# Check imports and usage of Elysia and treaty in the test file
rg -n "import.*Elysia|import.*treaty" apps/core/__tests__/helpers/api.ts -B2 -A2Length of output: 206
🏁 Script executed:
#!/bin/bash
# Search for other uses of createTestClient or similar patterns in codebase
rg -n "Treaty\.Create|treaty\(" apps/core/__tests__ -C2 --type=ts | head -40Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Check if there are type definitions or other createTestClient-like functions
rg -n "extends Elysia" apps/core --type=ts -C2Length of output: 647
🏁 Script executed:
#!/bin/bash
# Check @elysiajs/eden package documentation for proper typing
echo "Checking if there's a way to properly type Elysia without 'any'..."
echo "The treaty function signature: <const App extends Elysia>(domain: string | App, config?: Treaty.Config) => Treaty.Create<App>"Length of output: 192
🏁 Script executed:
#!/bin/bash
# Verify the exact location of the code section
cat << 'EOF' | head -1
File: apps/core/__tests__/helpers/api.ts
Lines: 42-46 (function definition)
Current code uses:
- Generic constraint: T extends Elysia<any, any, any, any, any, any>
- No explicit return type
- treaty(app) call
Required fixes per coding guidelines:
1. Explicit return type: Treaty.Create<T>
2. Address use of 'any' types
EOFLength of output: 40
Add explicit return type and remove any type parameters from generic constraint.
This function violates two coding guidelines:
- Uses
anyin the generic constraint (5 instances) - Missing explicit return type on public method
Replace the generic constraint T extends Elysia<any, any, any, any, any, any> with T extends Elysia and add the explicit return type Treaty.Create<T>.
export function createTestClient<
T extends Elysia,
>(app: T): Treaty.Create<T> {
return treaty(app);
}Also add the missing import: import type { Treaty } from "@elysiajs/eden";
Note: The same issues exist in createAuthTestClient on line 57–62 and should be fixed identically.
Co-authored-by: codiumai-pr-agent-free[bot] <138128286+codiumai-pr-agent-free[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/core/server/plugins/csrf.plugin.ts (3)
30-37: NODE_ENV check inconsistency already flagged.Multiple previous reviewers have identified that line 31's
NODE_ENV === "development"check is inconsistent with the CORS configuration inapi-elysia.ts(which usesNODE_ENV !== "production"). This causes localhost origins to be blocked in test and staging environments even though CORS allows them.
83-95: Origin/Referer header format validation concern already raised.A previous reviewer noted that the origin and referer headers are used without validating their format (beyond the try-catch for URL parsing). While the try-catch on lines 89-94 prevents crashes from malformed referer URLs, it doesn't validate that the extracted origin conforms to expected patterns before using it in security decisions on line 126.
97-122: Missing origin header handling already addressed.Previous reviewers raised concerns about skipping CSRF validation for requests without an Origin/Referer header. The current implementation addresses this by enforcing stricter checks in production (lines 100-118), rejecting such requests with a 403, while allowing them in non-production environments for easier testing.
🧹 Nitpick comments (2)
apps/core/server/plugins/csrf.plugin.ts (2)
88-95: Consider logging referer parsing failures for debugging.The empty catch block silently swallows URL parsing errors when extracting the origin from the Referer header. While this is safe from a security perspective (failing to parse results in no effective origin, which triggers downstream validation), logging a debug or warning message would help troubleshoot malformed Referer headers in production.
try { const url = new URL(referer); effectiveOrigin = url.origin; - } catch { - // Invalid referer URL + } catch (error) { + logger.debug( + { context: "csrf", referer, error }, + "Failed to parse origin from Referer header" + ); }
124-126: Ensure allowed origins are configured to prevent blocking all requests.When both
FRONTEND_URLandCORS_ALLOWED_ORIGINSare unset in non-development environments,getAllowedOrigins()returns an empty array, causing all state-changing requests to be rejected with a 403. Verify that production deployments configure at least one of these environment variables, or consider adding a startup validation check to warn operators about this misconfiguration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/core/server/plugins/csrf.plugin.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>in the Asset-Forge project
Useconstoverlet, never usevar
Use template literals for strings instead of string concatenation
Files:
apps/core/server/plugins/csrf.plugin.ts
apps/core/server/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/core/server/**/*.ts: Research with Deepwiki before adding new endpoints in Elysia
Define API types in service file with TypeBox validation in Elysia endpoints
Implement business logic in service class for API endpoints
Create route handler with TypeBox validation for API endpoints
Add Swagger documentation to new API endpoints
Follow REST conventions for API endpoints
Files:
apps/core/server/plugins/csrf.plugin.ts
**/*.{ts,tsx,js,jsx,json,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Use 2-space indentation for all code
Files:
apps/core/server/plugins/csrf.plugin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript strict mode everywhere
Noanyorunknowntypes in TypeScript code
Use explicit return types on public methods in TypeScript
Files:
apps/core/server/plugins/csrf.plugin.ts
🧬 Code graph analysis (1)
apps/core/server/plugins/csrf.plugin.ts (2)
apps/core/server/config/env.ts (1)
env(320-320)apps/core/server/routes/content/shared.ts (2)
Elysia(50-50)logger(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests (Playwright) (webkit)
🔇 Additional comments (1)
apps/core/server/plugins/csrf.plugin.ts (1)
17-41: Add explicit return type to comply with coding guidelines.The
getAllowedOriginsfunction is missing an explicit return type annotation, which violates the project's TypeScript coding guideline requiring explicit return types on public methods.Apply this diff:
-function getAllowedOrigins(): string[] { +function getAllowedOrigins(): string[] {Wait, the function already has
: string[]return type annotation on line 17. Let me re-examine the code...Actually, looking at line 17 again:
function getAllowedOrigins(): string[] {- the return type IS present. No issue here.Let me reconsider - I should skip this comment.
- CSRF plugin now uses NODE_ENV !== "production" to match CORS behavior, allowing localhost in both development and test environments - Remove redundant token_blocklist index (unique constraint auto-creates one) - TEST_JWT_SECRET helper now throws if env var missing (fail-closed) - Eden Treaty helpers use AnyElysiaApp type alias instead of explicit anys - Replace all `any` types with `unknown` in test helper functions - Add documentation comment for rate-limiting DISABLE_RATE_LIMITING check
| .defaultNow() | ||
| .notNull(), | ||
| // When the original token expires (for cleanup) | ||
| expiresAt: timestamp("expires_at", { withTimezone: true }).notNull(), |
There was a problem hiding this comment.
Potential performance and data consistency issue with expired tokens
The expiresAt field is used for cleanup, but the schema does not enforce or automate the removal of expired blocklist entries. If expired tokens are not regularly purged, the table may grow unnecessarily, impacting query performance and storage.
Recommendation:
Implement a scheduled cleanup process (e.g., a cron job or database job) that periodically deletes entries where expiresAt < NOW(). This ensures the blocklist remains performant and does not accumulate obsolete data.
| if (authHeader?.startsWith("Bearer af_")) { | ||
| return; // API key requests don't need CSRF protection |
There was a problem hiding this comment.
Potential Security Issue: Overly Broad API Key CSRF Bypass
The CSRF check is skipped for any request with an Authorization header starting with Bearer af_. If this pattern is not strictly enforced and API keys are not securely managed, attackers could potentially forge such headers to bypass CSRF protection.
Recommendation:
Ensure that API key authentication is robust and that only trusted machine-to-machine clients can use this bypass. Consider validating the API key format and origin more strictly, or limiting this bypass to specific endpoints where machine-to-machine communication is required.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/core/server/plugins/rate-limiting.plugin.ts (1)
113-117: Misleading comment: rate limiting is not disabled in development.The comment on line 116 states "Automatically disabled in development mode," but this is no longer accurate. Rate limiting is now environment-aware (10x in dev, 100x in test) and only disabled when
DISABLE_RATE_LIMITING=true./** * Rate Limiting Plugin * Applies global rate limiting and endpoint-specific limits - * Automatically disabled in development mode + * Environment-aware: stricter in production, relaxed in dev/test */
♻️ Duplicate comments (3)
apps/core/server/plugins/rate-limiting.plugin.ts (2)
32-37: Type safety: Replaceas anycast with a proper type.The
as anycast undermines TypeScript's type safety. Define a specific error response type to ensure consistent error handling.This was already flagged in a previous review. Consider applying a fix like:
+type RateLimitErrorResponse = { error: string; message: string }; + -function createRateLimitError(message: string) { +function createRateLimitError(message: string): RateLimitErrorResponse { return { error: "TOO_MANY_REQUESTS", message, - } as any; + }; }If
elysia-rate-limitrequires a specific type forerrorResponse, you may need to check the library's type exports.
39-56: Environment variable access inconsistency persists.While the added comment (lines 43-45) explains the rationale for using
process.env.DISABLE_RATE_LIMITINGdirectly, this still creates an inconsistency with the validatedenv.NODE_ENVusage on line 56. Additionally, as flagged in previous reviews, this allows rate limiting to be disabled in production if the env var is set.Previous reviewers recommended adding
DISABLE_RATE_LIMITINGto the env schema with production safeguards:// In apps/core/server/config/env.ts: DISABLE_RATE_LIMITING: z.string().optional().transform((val) => val === "true").default("false"),Then enforce production safety:
-const RATE_LIMITING_ENABLED = process.env.DISABLE_RATE_LIMITING !== "true"; +const RATE_LIMITING_ENABLED = env.NODE_ENV === "production" ? true : !env.DISABLE_RATE_LIMITING;This ensures rate limiting cannot be bypassed in production while maintaining test flexibility.
apps/core/__tests__/helpers/api.ts (1)
49-51: Add explicit return types for Treaty test clients (and align with Treaty typings)These exported helpers still lack explicit return types, which violates the “explicit return types on public methods” guideline and repeats the concern from the previous review. You can tighten the typing and match Eden Treaty’s
Create<App>return type:-import { treaty } from "@elysiajs/eden"; +import { treaty, type Treaty } from "@elysiajs/eden"; -export function createTestClient<T extends AnyElysiaApp>(app: T) { - return treaty(app); -} +export function createTestClient<T extends AnyElysiaApp>( + app: T, +): Treaty.Create<T> { + return treaty(app); +} -export function createAuthTestClient<T extends AnyElysiaApp>( - app: T, - user: AuthUser, -) { - return treaty(app, { +export function createAuthTestClient<T extends AnyElysiaApp>( + app: T, + user: AuthUser, +): Treaty.Create<T> { + return treaty(app, { headers: { Authorization: createAuthHeader( - user.privyUserId, + // If AuthUser.privyUserId can be null, consider guarding or asserting here + user.privyUserId, user.email || undefined, ), }, }); }Also, if
AuthUser["privyUserId"]is typed asstring | null(as inapps/core/server/types/auth.ts), you may want to narrow or assert non‑null before passing it intocreateAuthHeaderto keep strict mode happy and avoid accidentalnullsubjects in mock JWTs. Please verify whichAuthUsertype this file actually imports.This relies on the Eden Treaty type definitions and your shared
AuthUsertype; please confirm against your installed@elysiajs/edenversion and auth typings.Also applies to: 62-74
🧹 Nitpick comments (3)
apps/core/server/plugins/rate-limiting.plugin.ts (1)
160-168: Useserver.requestIP(req)instead of URL.hostname for more reliable client IP detection.The
serverparameter is available in the generator function scope. Bun'srequestIPmethod properly returns the client's SocketAddress, while the current fallback usingnew URL(req.url).hostnamereturns the server's hostname instead of the client's address. This causes all clients without proper headers to share the same rate limit bucket.Replace the fallback logic at lines 160-161 with:
// Last resort: use request URL - return new URL(req.url).hostname || "unknown"; + const addr = server.requestIP(req); + return addr?.address || "unknown";apps/core/server/db/schema/token-blocklist.schema.ts (1)
20-29: Optionally indexuserId(and/or add FK) if user-scoped queries are hot.If
blockUserTokensor other user-scoped operations are frequent, consider adding an index onuserId(and possibly a foreign key to the users table) to keep those queries efficient and maintain referential integrity. This can be deferred until profiling shows need.Example index addition:
(table) => [ // Note: tokenId already has a unique constraint which creates an index automatically // Only add index for expiresAt which is used for cleanup queries index("idx_token_blocklist_expires_at").on(table.expiresAt), + // Optional: speed up user-scoped revocation queries + index("idx_token_blocklist_user_id").on(table.userId), ],apps/core/__tests__/helpers/api.ts (1)
18-29: Consider using a type‑only import forElysia
Elysiais only used for typing (AnyElysiaApp, function params), so you can avoid a runtime import by switching to a type‑only import:-import { Elysia } from "elysia"; +import type { Elysia } from "elysia";This keeps the helper purely type‑level and avoids pulling
elysiainto the test bundle unnecessarily.As this touches TypeScript import semantics, please confirm with your TS config / bundler behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/core/__tests__/helpers/api.ts(7 hunks)apps/core/__tests__/helpers/auth.ts(2 hunks)apps/core/server/db/schema/token-blocklist.schema.ts(1 hunks)apps/core/server/plugins/csrf.plugin.ts(1 hunks)apps/core/server/plugins/rate-limiting.plugin.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/core/tests/helpers/auth.ts
- apps/core/server/plugins/csrf.plugin.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>in the Asset-Forge project
Useconstoverlet, never usevar
Use template literals for strings instead of string concatenation
Files:
apps/core/server/db/schema/token-blocklist.schema.tsapps/core/server/plugins/rate-limiting.plugin.tsapps/core/__tests__/helpers/api.ts
apps/core/server/db/schema/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Edit TypeScript schema in
apps/core/server/db/schema/when schema changes are needed
Files:
apps/core/server/db/schema/token-blocklist.schema.ts
apps/core/server/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/core/server/**/*.ts: Research with Deepwiki before adding new endpoints in Elysia
Define API types in service file with TypeBox validation in Elysia endpoints
Implement business logic in service class for API endpoints
Create route handler with TypeBox validation for API endpoints
Add Swagger documentation to new API endpoints
Follow REST conventions for API endpoints
Files:
apps/core/server/db/schema/token-blocklist.schema.tsapps/core/server/plugins/rate-limiting.plugin.ts
**/*.{ts,tsx,js,jsx,json,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Use 2-space indentation for all code
Files:
apps/core/server/db/schema/token-blocklist.schema.tsapps/core/server/plugins/rate-limiting.plugin.tsapps/core/__tests__/helpers/api.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript strict mode everywhere
Noanyorunknowntypes in TypeScript code
Use explicit return types on public methods in TypeScript
Files:
apps/core/server/db/schema/token-blocklist.schema.tsapps/core/server/plugins/rate-limiting.plugin.tsapps/core/__tests__/helpers/api.ts
🧬 Code graph analysis (2)
apps/core/server/plugins/rate-limiting.plugin.ts (1)
apps/core/server/config/env.ts (1)
env(320-320)
apps/core/__tests__/helpers/api.ts (3)
apps/core/server/routes/content/shared.ts (1)
Elysia(50-50)apps/core/server/types/auth.ts (1)
AuthUser(7-17)apps/core/__tests__/helpers/auth.ts (1)
createAuthHeader(63-66)
🔇 Additional comments (4)
apps/core/server/plugins/rate-limiting.plugin.ts (1)
58-76: Environment-aware rate limits look good.The base limit constants with multiplier approach is a clean pattern for environment-specific rate limiting. Production gets strict limits (1x), while dev (10x) and test (100x) get progressively relaxed limits.
apps/core/server/db/schema/token-blocklist.schema.ts (1)
14-36: Schema definition looks solid and resolves prior redundant-index concern.The
token_blocklistschema is well-structured: appropriate types,tokenIduniqueness without an extra manual index (fixing the earlier redundant-index issue), and anexpiresAtindex to support cleanup queries. Exported infer types are correctly wired for select/insert use.apps/core/__tests__/helpers/api.ts (2)
4-15: Doc updates align well with Elysia testing best practicesThe added guidance around Eden Treaty vs
app.handle()is clear, follows current Elysia testing patterns, and is helpful context for test authors. No changes requested.
102-106: Replace explicitunknownusages with a JSON value type alias to satisfy guidelinesSeveral helpers now use
unknownin parameter types and generic defaults:
post,patch:body: unknownauthPost,authPatch:body: unknowntestRouteJSON<T = unknown>extractJSON<T = unknown>This conflicts with the project guideline of “No
anyorunknowntypes in TypeScript code.” You can keep strong typing withoutunknownby introducing a reusable JSON value alias and constraining usages to it. For example:type AnyElysiaApp = Elysia; +// JSON value type used for request/response bodies +type JsonValue = + | string + | number + | boolean + | null + | JsonValue[] + | { [key: string]: JsonValue }; + /** * Create a POST request with JSON body */ -export function post( - path: string, - body: unknown, - headers?: HeadersInit, -): Request { +export function post( + path: string, + body: JsonValue, + headers?: HeadersInit, +): Request { @@ /** * Create a PATCH request with JSON body */ -export function patch( - path: string, - body: unknown, - headers?: HeadersInit, -): Request { +export function patch( + path: string, + body: JsonValue, + headers?: HeadersInit, +): Request { @@ -export function authPost(path: string, body: unknown, user: AuthUser): Request { +export function authPost( + path: string, + body: JsonValue, + user: AuthUser, +): Request { @@ -export function authPatch( - path: string, - body: unknown, - user: AuthUser, -): Request { +export function authPatch( + path: string, + body: JsonValue, + user: AuthUser, +): Request { @@ -export async function testRouteJSON<T = unknown>( +export async function testRouteJSON<T extends JsonValue>( app: Elysia, request: Request, ): Promise<{ response: Response; data: T }> { @@ -export async function extractJSON<T = unknown>(response: Response): Promise<T> { +export async function extractJSON<T extends JsonValue>( + response: Response, +): Promise<T> {This keeps the helpers flexible for any JSON‑serializable payload, removes explicit
unknown, and stays within the strict TypeScript rules you’ve defined.As this relies on TypeScript’s handling of recursive JSON types and generic constraints, please confirm with your TS version and lint rules.
Also applies to: 120-124, 163-164, 180-184, 227-234, 278-283
Dexploarer
deleted the
claude/critical-app-review-019txope9jjiPCuNTQJC1Qh5
branch
PR Type
Bug fix, Enhancement, Tests
Description
Fix critical JWT test mode bypass by requiring proper HMAC-SHA256 signature verification
Add CSRF protection plugin with origin-based validation for state-changing requests
Fix CORS configuration to eliminate wildcard fallback and require explicit origin allowlisting
Enable rate limiting in all environments with environment-aware multipliers (1x prod, 10x dev, 100x test)
Implement proper RBAC for admin routes with actual role verification instead of deprecated single-team bypass
Add input length limits to TypeBox models across AssetMetadata to prevent memory exhaustion attacks
Disable production sourcemaps to prevent source code exposure in compiled bundles
Add comprehensive security headers (XSS protection, HSTS, Referrer-Policy, Permissions-Policy)
Implement JWT token blocklist service for logout and token revocation functionality
Convert asset route tests from mocks to real database with proper test data isolation
Create
.env.examplewith deployment security checklist and all configuration optionsAdd database migrations to fix schema drift and create token blocklist table
Document ThreeViewer refactoring strategy for breaking down 6,340-line component
Diagram Walkthrough
File Walkthrough
7 files
Implement proper JWT HMAC-SHA256 signature verificationAdd CSRF plugin and fix CORS origin allowlistingAdd maxLength constraints to prevent memory exhaustionFix JWT test verification and implement admin RBAC checksEnable rate limiting in all environments with multipliersDisable production sourcemaps to prevent code exposureDrop unused admin_whitelist table from schema1 files
Convert asset tests from mocks to real database7 files
Export token blocklist schema for JWT invalidationCreate token blocklist schema for logout functionalityAdd cleanup jobs for activity logs and token blocklistImplement origin-based CSRF protection for state-changing requestsAdd comprehensive security headers including HSTS and XSS protectionImplement JWT token blocklist service for revocationCreate token blocklist table for JWT invalidation2 files
Document ThreeViewer component refactoring strategyCreate comprehensive environment configuration with deploymentchecklist1 files
Update migration journal with new migrationsSummary by CodeRabbit
New Features
Security
Improvements
Tests / Dev
Docs
✏️ Tip: You can customize this high-level summary in your review settings.