Repository navigation
test(auth): replace brittle assertions with contract/property tests - #83
Conversation
- Remove cookie count assertions, keep cookie-contract assertions - Replace DUMMY_HASH pin with timing-safe security property test - Add capture-and-assert for signup user creation data - Enhance mobile-login timing-safe test with property assertions - Add auth-test-patterns.md documenting repo seam recommendations
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded@2witstudios has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughTests for auth routes were updated to assert security properties (cookie attributes, bcrypt behavior) rather than exact implementation details; signup test now captures and asserts inserted user data; new documentation describes auth testing patterns and repository/seam recommendations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/auth/__tests__/signup.test.ts (1)
205-242: Avoid implicit-anyinmockImplementationparam; type the captured insert payload.
With strict TS /noImplicitAny,mockImplementation((data) => ...)will fail type-checking and it also violates the “noanytypes” guideline. As per coding guidelines, annotate the shape you read (name,password) and assertpasswordis a string before regex matching.- let capturedUserData: Record<string, unknown> | undefined; - const mockValues = vi.fn().mockImplementation((data) => { + type CreatedUserInsert = { + email?: string; + name?: string; + password?: string; + } & Record<string, unknown>; + + let capturedUserData: CreatedUserInsert | undefined; + const mockValues = vi.fn().mockImplementation((data: CreatedUserInsert) => { // Capture the first insert (user creation) if (!capturedUserData && data.email) { capturedUserData = data; } return { returning: vi.fn().mockResolvedValue([ { id: 'new-user-id', name: data.name || 'New User', email: data.email || 'new@example.com', tokenVersion: 0, role: 'user', }, ]), }; }); (db.insert as Mock).mockReturnValue({ values: mockValues }); @@ expect(capturedUserData!.password).not.toBe('ValidPass123!'); - expect(capturedUserData!.password).toMatch(/^\$2[aby]?\$\d+\$/); // bcrypt hash format + expect(typeof capturedUserData!.password).toBe('string'); + expect(capturedUserData!.password).toMatch(/^\$2[aby]?\$\d+\$/); // bcrypt hash format
🧹 Nitpick comments (5)
apps/web/src/app/api/auth/__tests__/logout.test.ts (1)
113-137: Cookie-clearing contract is solid; consider asserting sameSite/secure too (to guarantee overwrite).
Right now you only asserthttpOnly+path+expires: new Date(0). If the real cookies are set withsameSiteand/orsecure(ordomain), clearing should usually mirror those attributes; adding them here would better protect against “cookie didn’t actually clear” regressions.apps/web/src/app/api/auth/__tests__/mobile-login.test.ts (1)
317-343: Nice security-property test; make the hash assertion slightly more type/shape-robust.
expect(hash).toMatch(...)assumeshashis a string; consider assertingtypeof hash === 'string'first, and (optionally) using a fuller bcrypt hash regex if you want stronger guarantees than the prefix.apps/web/src/app/api/auth/__tests__/login.test.ts (2)
136-160: Cookie contract assertions look good; consider whethersecureshould be part of the contract.
If production cookies are alwayssecure, capturing that here (possibly environment-gated) would prevent accidental weakening.
327-340: Timing-safe bcrypt property test is a good replacement for the pinned dummy-hash.
Minor robustness tweak: asserttypeof hash === 'string'beforetoMatch, and consider a stricter bcrypt hash regex if desired.docs/testing/auth-test-patterns.md (1)
160-179: Avoid hardcoding exact test counts in docs (will go stale).
Consider rephrasing “All 218 tests passing…” to something timeless (or point to CI) so the doc doesn’t require churn with every test add/remove.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/src/app/api/auth/__tests__/login.test.ts(2 hunks)apps/web/src/app/api/auth/__tests__/logout.test.ts(1 hunks)apps/web/src/app/api/auth/__tests__/mobile-login.test.ts(2 hunks)apps/web/src/app/api/auth/__tests__/signup.test.ts(2 hunks)docs/testing/auth-test-patterns.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/web/src/app/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/app/**/*.ts: In Next.js 15 route handlers with dynamic routes, you MUST awaitcontext.paramsbefore destructuring because params are Promise objects
Get request body usingconst body = await request.json();
Get search params usingconst { searchParams } = new URL(request.url);
Return JSON responses usingreturn Response.json(data)orreturn NextResponse.json(data)
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/logout.test.tsapps/web/src/app/api/auth/__tests__/login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
anytypes in TypeScript code - always use proper TypeScript types
**/*.{ts,tsx}: Noanytypes - always use proper TypeScript types
Use kebab-case for filenames (e.g.,image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: importgetUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions
Use Drizzle client from@pagespace/dbfor all database access
Always structure message content using the message parts structure:{ parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/logout.test.tsapps/web/src/app/api/auth/__tests__/login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/*.{ts,tsx}: Use centralized permission functions from@pagespace/lib/permissionsfor access control, such asgetUserAccessLevel()andcanUserEditPage()
Always use Drizzle client from@pagespace/dbfor database access instead of direct database connections
Always use message parts structure withpartsarray containing objects withtypeandtextfields when constructing messages for AI
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/logout.test.tsapps/web/src/app/api/auth/__tests__/login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier and lint with ESLint using the configuration at
apps/web/eslint.config.mjs
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/logout.test.tsapps/web/src/app/api/auth/__tests__/login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/logout.test.tsapps/web/src/app/api/auth/__tests__/login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
🧬 Code graph analysis (1)
apps/web/src/app/api/auth/__tests__/signup.test.ts (1)
packages/db/src/index.ts (1)
db(20-20)
⏰ 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: Unit Tests
- Type CapturedUserData interface in signup.test.ts (no implicit-any) - Add sameSite to cookie-clearing contract in logout.test.ts - Add typeof hash === 'string' guards before regex assertions - Use stricter full bcrypt hash regex pattern - Remove hardcoded test count from docs (point to CI instead)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/src/app/api/auth/__tests__/signup.test.ts (1)
204-248: Capture-and-assert pattern is well-implemented.The test effectively validates user creation data and password hashing. However, consider aligning the bcrypt regex pattern with mobile-login.test.ts for consistency and stronger validation.
The current regex at line 247 validates only the bcrypt prefix:
expect(capturedUserData!.password).toMatch(/^\$2[aby]?\$\d+\$/);For consistency with mobile-login.test.ts (line 343) and stronger hash validation, consider using the full bcrypt format pattern:
- expect(capturedUserData!.password).toMatch(/^\$2[aby]?\$\d+\$/); // bcrypt hash format + expect(capturedUserData!.password).toMatch(/^\$2[aby]?\$\d{1,2}\$[./A-Za-z0-9]{53}$/); // Full bcrypt hash formatThis validates the complete hash structure (cost factor + 53-char salt+hash) rather than just the prefix, providing better security property validation without introducing brittleness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/src/app/api/auth/__tests__/login.test.ts(2 hunks)apps/web/src/app/api/auth/__tests__/logout.test.ts(2 hunks)apps/web/src/app/api/auth/__tests__/mobile-login.test.ts(2 hunks)apps/web/src/app/api/auth/__tests__/signup.test.ts(2 hunks)docs/testing/auth-test-patterns.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/app/api/auth/tests/login.test.ts
- apps/web/src/app/api/auth/tests/logout.test.ts
- docs/testing/auth-test-patterns.md
🧰 Additional context used
📓 Path-based instructions (5)
apps/web/src/app/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/app/**/*.ts: In Next.js 15 route handlers with dynamic routes, you MUST awaitcontext.paramsbefore destructuring because params are Promise objects
Get request body usingconst body = await request.json();
Get search params usingconst { searchParams } = new URL(request.url);
Return JSON responses usingreturn Response.json(data)orreturn NextResponse.json(data)
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
anytypes in TypeScript code - always use proper TypeScript types
**/*.{ts,tsx}: Noanytypes - always use proper TypeScript types
Use kebab-case for filenames (e.g.,image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: importgetUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions
Use Drizzle client from@pagespace/dbfor all database access
Always structure message content using the message parts structure:{ parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/*.{ts,tsx}: Use centralized permission functions from@pagespace/lib/permissionsfor access control, such asgetUserAccessLevel()andcanUserEditPage()
Always use Drizzle client from@pagespace/dbfor database access instead of direct database connections
Always use message parts structure withpartsarray containing objects withtypeandtextfields when constructing messages for AI
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier and lint with ESLint using the configuration at
apps/web/eslint.config.mjs
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching
Files:
apps/web/src/app/api/auth/__tests__/mobile-login.test.tsapps/web/src/app/api/auth/__tests__/signup.test.ts
🧬 Code graph analysis (1)
apps/web/src/app/api/auth/__tests__/signup.test.ts (1)
packages/db/src/index.ts (1)
db(20-20)
⏰ 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: Unit Tests
🔇 Additional comments (1)
apps/web/src/app/api/auth/__tests__/mobile-login.test.ts (1)
317-344: Excellent security property validation!The enhanced assertions effectively validate the timing-safe comparison guarantee. The test now verifies not just that
bcrypt.compareis called, but that it's called with the correct password and a valid bcrypt hash format, preventing timing attacks that could reveal user existence.
- Use full bcrypt hash regex pattern for consistency with other tests - Update bcrypt.hash mock to return properly formatted 60-char hash
2 participants
Summary by CodeRabbit
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.