Fix vulnerabilities and adds additional tests#46
Merged
Conversation
Add comprehensive tests for previously untested code paths: - Core: CsrfProtection.protect(), error classes, timingSafeEqual, generateSecureSecret, config merging, path exclusion, strategy routing - Express: getTokenFromRequest() priority chain, HTTP method coverage, signed-token strategy validation - Next.js: client utilities (getCsrfToken, csrfFetch, refreshCsrfToken), signed-token and hybrid strategy middleware tests https://claude.ai/code/session_01JPhEZP5jGsAtWYvCjC8Ek9
…ve deps Add pnpm overrides for lodash (>=4.18.0) and defu (>=6.1.5) to address GHSA-r5fr-rjxr-66jc (lodash code injection), GHSA-f23m-r3pf-42rh (lodash prototype pollution), and GHSA-737v-mqg7-c878 (defu prototype pollution).
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a changeset and pnpm overrides to force patched Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ort, function or class' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Muneeb Samuels <muneebs@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
package.json (1)
74-75: Constrain override ranges to the current major versions.Using unbounded
>=X.Y.Zranges can pull unexpected major releases in dependency updates. Add upper bounds to lock to the current major version:♻️ Suggested change
- "lodash": ">=4.18.0", - "defu": ">=6.1.5" + "lodash": ">=4.18.0 <5.0.0", + "defu": ">=6.1.5 <7.0.0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 74 - 75, Update the unbounded dependency ranges for "lodash" and "defu" in package.json to constrain them to their current major versions (e.g., change "lodash": ">=4.18.0" and "defu": ">=6.1.5" to ranges that prevent pulling major bumps such as ">=4.18.0 <5.0.0" or use caret semantics like "^4.18.0" and "^6.1.5"); locate the entries for "lodash" and "defu" in package.json and replace the version strings accordingly to lock the major version while allowing non-breaking updates.packages/nextjs/tests/middleware.test.ts (1)
447-457: UseHeadersinstead ofMapinextractRequestmocks for production fidelity.The test mocks use
Mapfor headers, but the actual adapter returnsHeaders(fromNextRequest). UsingHeadersaligns tests with production behavior. Note thatadapter.test.tsalready follows this pattern successfully.♻️ Suggested adjustment
First, add the import if not present:
import { Headers } from 'next/dist/compiled/@edge-runtime/primitives';Then update the mocks:
- headers: new Map([ - ['origin', 'http://localhost'], - ['x-csrf-token', signedToken!], - ['content-type', 'application/json'], - ]), + headers: new Headers({ + origin: 'http://localhost', + 'x-csrf-token': signedToken!, + 'content-type': 'application/json', + }),Applies to lines 447-457 and 490-499.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/tests/middleware.test.ts` around lines 447 - 457, Replace the Maps used in the mocked adapter.extractRequest calls with the same Headers type used by NextRequest to match production behavior: import Headers from 'next/dist/compiled/@edge-runtime/primitives' (if missing) and construct new Headers(...) for the headers argument in the mock implementations that return method/url/headers/cookies/body; update both mocked blocks that currently build headers with new Map([...]) to use new Headers([...]) so tests mirror adapter.extractRequest's real return shape (refer to the mocked extractRequest usages in middleware.test.ts).packages/core/tests/csrf.test.ts (3)
1-1: Remove unused importsviandafterEach.Static analysis correctly identifies that
viandafterEachare imported but never used in this file.♻️ Proposed fix
-import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { describe, expect, it, beforeEach } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/csrf.test.ts` at line 1, The test file imports unused symbols vi and afterEach; update the import statement that currently lists describe, expect, it, vi, beforeEach, afterEach to remove vi and afterEach so it only imports describe, expect, it, beforeEach (or whatever subset is actually used), ensuring no unused imports remain (adjust any references to vi/afterEach if present).
319-343: Non-null assertion on line 340 can be avoided.The
serverCookieis retrieved with!but could beundefinedif the cookie wasn't set. Consider using a guard or optional chaining.♻️ Proposed fix
// Server cookie (signed, httpOnly) expect(cookies.has('csrf-token-server')).toBe(true); - const serverCookie = cookies.get('csrf-token-server')!; + const serverCookie = cookies.get('csrf-token-server'); + expect(serverCookie).toBeDefined(); + if (!serverCookie) throw new Error('Server cookie should exist'); // Signed token has exactly one dot: {unsignedToken}.{signature} expect(serverCookie.value.split('.').length).toBe(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/csrf.test.ts` around lines 319 - 343, The test uses a non-null assertion on serverCookie which can be undefined; update the 'signed-double-submit: response cookies include both client cookie and signed server cookie' test to assert the server cookie exists before using it (e.g., expect(serverCookie).toBeDefined() or check cookies.has('csrf-token-server') and return/fail if missing) and then safely access its value (avoid using '!' by using the guarded variable or optional chaining when splitting the token). Ensure you reference csrfResponse.cookies and the serverCookie variable in your change so the assertion and subsequent split operate on a guaranteed-defined value.
85-92: Consider avoiding non-null assertion on line 91.Line 90 asserts
result.tokenis a string, but the linter doesn't recognize this guards the non-null assertion on line 91.♻️ Proposed fix
expect(result.success).toBe(true); expect(typeof result.token).toBe('string'); - expect(result.token!.length).toBeGreaterThan(0); + expect(result.token?.length).toBeGreaterThan(0);Or use a guard:
expect(result.success).toBe(true); expect(typeof result.token).toBe('string'); + if (!result.token) throw new Error('Token should exist'); - expect(result.token!.length).toBeGreaterThan(0); + expect(result.token.length).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/tests/csrf.test.ts` around lines 85 - 92, The test uses a non-null assertion on result.token (result.token!.length) which the linter rejects; modify the test to explicitly guard or assert token presence before checking length. For example, in the test around csrf.protect, add an explicit assertion like expect(result.token).toBeDefined() or assign const token = result.token as string after that assertion and then assert token.length > 0; reference csrf.protect and result.token when making the change.packages/express/tests/middleware.test.ts (2)
494-519: Duplicate helper functions can be extracted to reduce redundancy.
createMockReqandcreateMockResare defined identically in both the "HTTP method coverage" and "Signed-token strategy" describe blocks. Consider extracting them to the top-level describe scope or a shared test utilities file.♻️ Proposed refactor to extract helpers
Move the helpers to the top-level describe block (after
beforeEach):describe('CSRF Middleware', () => { const mockNext = vi.fn(); beforeEach(() => { vi.clearAllMocks(); }); + const createMockReq = (method: string, extras: Partial<Request> = {}) => + ({ + method, + url: '/api/data', + headers: {}, + cookies: {}, + get: vi.fn(), + header: vi.fn(), + accepts: vi.fn(), + acceptsCharsets: vi.fn(), + acceptsEncodings: vi.fn(), + acceptsLanguages: vi.fn(), + param: vi.fn(), + is: vi.fn(), + app: {}, + route: {}, + ...extras, + }) as unknown as Request; + + const createMockRes = () => + ({ + setHeader: vi.fn(), + cookie: vi.fn(), + }) as unknown as Response; + describe('GET requests', () => {Then remove the duplicate definitions from both "HTTP method coverage" and "Signed-token strategy" blocks.
Also applies to: 595-619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/express/tests/middleware.test.ts` around lines 494 - 519, Extract the duplicate test helpers createMockReq and createMockRes out of the two inner describe blocks and place them in the parent/top-level describe scope (or a shared test utilities module) so both "HTTP method coverage" and "Signed-token strategy" can reuse them; remove the duplicate definitions inside the inner describes and update tests to reference the single shared createMockReq and createMockRes functions (look for symbols createMockReq and createMockRes in the existing tests to locate the duplicates).
634-654: Consider restructuring to avoid non-null assertions.Static analysis flags the non-null assertions (
!) on lines 637, 644, and 647. While these are safe because line 635 assertssignedTokenis defined, you can restructure to satisfy the linter.♻️ Proposed refactor
expect(mockNext).toHaveBeenCalled(); const signedToken = getReq.csrfToken; expect(signedToken).toBeDefined(); + if (!signedToken) throw new Error('Token should be defined'); // Signed tokens have dots (exp.nonce.signature) - expect(signedToken!.split('.').length).toBe(3); + expect(signedToken.split('.').length).toBe(3); mockNext.mockClear(); // POST with valid signed token const postReq = createMockReq('POST', { headers: { - 'x-csrf-token': signedToken!, + 'x-csrf-token': signedToken, }, cookies: { - 'csrf-token': signedToken!, + 'csrf-token': signedToken, }, } as Partial<Request>);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/express/tests/middleware.test.ts` around lines 634 - 654, The test uses non-null assertions on signedToken; instead after getting signedToken from getReq.csrfToken and calling expect(signedToken).toBeDefined(), add an explicit runtime guard (e.g., if (signedToken === undefined) throw new Error('csrf token missing in test');) then use a local const token: string = signedToken and pass token into createMockReq and headers/cookies and the middleware call; update references of signedToken! to token and remove all `!` non-null assertions (symbols: signedToken, getReq.csrfToken, createMockReq, middleware, mockNext).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nextjs/tests/middleware.test.ts`:
- Line 353: Several tests use non-null assertions (e.g., headerToken!) which
break Biome's noNonNullAssertion rule; remove the `!` usages and instead assert
existence before dereferencing (for example, add explicit checks like
expect(headerToken).toBeDefined() / not.toBeNull() or guard with if
(!headerToken) throw new Error(...) ) and then call methods like split on the
now-validated value; locate occurrences by searching for the identifier
headerToken and the other failing non-null assertions in middleware.test.ts and
replace each `variable!` with an existence assertion/guard followed by normal
usage.
---
Nitpick comments:
In `@package.json`:
- Around line 74-75: Update the unbounded dependency ranges for "lodash" and
"defu" in package.json to constrain them to their current major versions (e.g.,
change "lodash": ">=4.18.0" and "defu": ">=6.1.5" to ranges that prevent pulling
major bumps such as ">=4.18.0 <5.0.0" or use caret semantics like "^4.18.0" and
"^6.1.5"); locate the entries for "lodash" and "defu" in package.json and
replace the version strings accordingly to lock the major version while allowing
non-breaking updates.
In `@packages/core/tests/csrf.test.ts`:
- Line 1: The test file imports unused symbols vi and afterEach; update the
import statement that currently lists describe, expect, it, vi, beforeEach,
afterEach to remove vi and afterEach so it only imports describe, expect, it,
beforeEach (or whatever subset is actually used), ensuring no unused imports
remain (adjust any references to vi/afterEach if present).
- Around line 319-343: The test uses a non-null assertion on serverCookie which
can be undefined; update the 'signed-double-submit: response cookies include
both client cookie and signed server cookie' test to assert the server cookie
exists before using it (e.g., expect(serverCookie).toBeDefined() or check
cookies.has('csrf-token-server') and return/fail if missing) and then safely
access its value (avoid using '!' by using the guarded variable or optional
chaining when splitting the token). Ensure you reference csrfResponse.cookies
and the serverCookie variable in your change so the assertion and subsequent
split operate on a guaranteed-defined value.
- Around line 85-92: The test uses a non-null assertion on result.token
(result.token!.length) which the linter rejects; modify the test to explicitly
guard or assert token presence before checking length. For example, in the test
around csrf.protect, add an explicit assertion like
expect(result.token).toBeDefined() or assign const token = result.token as
string after that assertion and then assert token.length > 0; reference
csrf.protect and result.token when making the change.
In `@packages/express/tests/middleware.test.ts`:
- Around line 494-519: Extract the duplicate test helpers createMockReq and
createMockRes out of the two inner describe blocks and place them in the
parent/top-level describe scope (or a shared test utilities module) so both
"HTTP method coverage" and "Signed-token strategy" can reuse them; remove the
duplicate definitions inside the inner describes and update tests to reference
the single shared createMockReq and createMockRes functions (look for symbols
createMockReq and createMockRes in the existing tests to locate the duplicates).
- Around line 634-654: The test uses non-null assertions on signedToken; instead
after getting signedToken from getReq.csrfToken and calling
expect(signedToken).toBeDefined(), add an explicit runtime guard (e.g., if
(signedToken === undefined) throw new Error('csrf token missing in test');) then
use a local const token: string = signedToken and pass token into createMockReq
and headers/cookies and the middleware call; update references of signedToken!
to token and remove all `!` non-null assertions (symbols: signedToken,
getReq.csrfToken, createMockReq, middleware, mockNext).
In `@packages/nextjs/tests/middleware.test.ts`:
- Around line 447-457: Replace the Maps used in the mocked
adapter.extractRequest calls with the same Headers type used by NextRequest to
match production behavior: import Headers from
'next/dist/compiled/@edge-runtime/primitives' (if missing) and construct new
Headers(...) for the headers argument in the mock implementations that return
method/url/headers/cookies/body; update both mocked blocks that currently build
headers with new Map([...]) to use new Headers([...]) so tests mirror
adapter.extractRequest's real return shape (refer to the mocked extractRequest
usages in middleware.test.ts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b567acdf-8859-4b06-bcc8-aa229a434c6d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/fix-vulnerable-deps.mdpackage.jsonpackages/core/tests/crypto.test.tspackages/core/tests/csrf.test.tspackages/core/tests/errors.test.tspackages/core/tests/validation.test.tspackages/express/tests/adapter.test.tspackages/express/tests/middleware.test.tspackages/nextjs/package.jsonpackages/nextjs/tests/client.test.tspackages/nextjs/tests/middleware.test.ts
| const headerToken = result.response.headers.get('x-csrf-token'); | ||
| expect(headerToken).toBeDefined(); | ||
| // Signed tokens have 3 parts: exp.nonce.signature | ||
| expect(headerToken!.split('.').length).toBe(3); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Potential non-null assertions in Next.js middleware tests:"
rg -n '\w+!\b' packages/nextjs/tests/middleware.test.tsRepository: muneebs/csrf-armor
Length of output: 120
🏁 Script executed:
# Check if the file exists and read the actual content at line 353 and surrounding lines
ls -la packages/nextjs/tests/middleware.test.ts 2>/dev/null && \
echo "=== File exists. Checking line 353 and surrounding context ===" && \
sed -n '350,360p' packages/nextjs/tests/middleware.test.tsRepository: muneebs/csrf-armor
Length of output: 651
🏁 Script executed:
# Try alternative pattern for non-null assertions
rg -n '!' packages/nextjs/tests/middleware.test.ts | grep -E '(353|375|381|435|442|452|455|486|495|497):'Repository: muneebs/csrf-armor
Length of output: 562
🏁 Script executed:
# Check all the specific line numbers mentioned in the review
for line in 353 375 381 435 442 452 455 486 495 497; do
echo "=== Line $line ===" && \
sed -n "${line}p" packages/nextjs/tests/middleware.test.ts || echo "Line not found or file not readable"
doneRepository: muneebs/csrf-armor
Length of output: 692
🏁 Script executed:
# Check for Biome configuration
find . -name "biome.json" -o -name "biome.jsonc" -o -name ".biomerc*" | head -5Repository: muneebs/csrf-armor
Length of output: 75
🏁 Script executed:
# Check if there's a linting script or CI configuration
find . -name "package.json" -path "*/nextjs/*" | head -3 | xargs -I {} sh -c "echo '=== {} ===' && cat {}"Repository: muneebs/csrf-armor
Length of output: 1914
🏁 Script executed:
# Search for any mention of noNonNullAssertion in the codebase
rg "noNonNullAssertion" --type json --type jsoncRepository: muneebs/csrf-armor
Length of output: 91
🏁 Script executed:
# Read the Biome configuration
cat biome.jsoncRepository: muneebs/csrf-armor
Length of output: 1925
🏁 Script executed:
# Search for noNonNullAssertion without type filtering
rg "noNonNullAssertion"Repository: muneebs/csrf-armor
Length of output: 1579
🏁 Script executed:
# Check the exact lines with their preceding comments to verify they have biome-ignore directives
for line in 353 375 381 435 442 452 455 486 495 497; do
echo "=== Context for line $line ===" && \
sed -n "$((line-1)),$((line))p" packages/nextjs/tests/middleware.test.ts
doneRepository: muneebs/csrf-armor
Length of output: 1159
🏁 Script executed:
# Get more context to see if biome-ignore directives exist nearby these lines
for line in 353 375 381 435 442 452 455 486 495 497; do
echo "=== Context around line $line (lines $((line-2)) to $((line+1))) ===" && \
sed -n "$((line-2)),$((line+1))p" packages/nextjs/tests/middleware.test.ts
doneRepository: muneebs/csrf-armor
Length of output: 1852
🏁 Script executed:
# Find where the biome-ignore directives actually are located
rg -B 1 "biome-ignore lint/style/noNonNullAssertion" packages/nextjs/tests/middleware.test.ts | head -40Repository: muneebs/csrf-armor
Length of output: 1193
🏁 Script executed:
# Get a wider view to see the exact relationship between biome-ignore and the assertions
sed -n '351,356p' packages/nextjs/tests/middleware.test.ts
sed -n '373,378p' packages/nextjs/tests/middleware.test.ts
sed -n '379,384p' packages/nextjs/tests/middleware.test.tsRepository: muneebs/csrf-armor
Length of output: 585
🏁 Script executed:
# Count how many biome-ignore directives exist vs how many non-null assertions
echo "=== biome-ignore directives ===" && \
rg "biome-ignore lint/style/noNonNullAssertion" packages/nextjs/tests/middleware.test.ts | wc -l && \
echo "=== non-null assertions with ! ===" && \
rg '\w+!' packages/nextjs/tests/middleware.test.ts | wc -lRepository: muneebs/csrf-armor
Length of output: 134
🏁 Script executed:
# Check if any of the specific assertion lines have inline biome-ignore comments
for line in 353 375 381 435 442 452 455 486 495 497; do
content=$(sed -n "${line}p" packages/nextjs/tests/middleware.test.ts)
if echo "$content" | grep -q "biome-ignore"; then
echo "Line $line: HAS inline biome-ignore"
else
echo "Line $line: NO biome-ignore"
fi
doneRepository: muneebs/csrf-armor
Length of output: 321
🏁 Script executed:
# Show which exact lines have biome-ignore directives (line numbers)
rg -n "biome-ignore lint/style/noNonNullAssertion" packages/nextjs/tests/middleware.test.tsRepository: muneebs/csrf-armor
Length of output: 860
🏁 Script executed:
# Verify that Biome would actually flag these as errors by checking if there's a lint script
cat packages/nextjs/package.json | grep -A 5 "scripts"Repository: muneebs/csrf-armor
Length of output: 276
🏁 Script executed:
# Double-check the biome rule severity
grep -A 2 "noNonNullAssertion" biome.jsoncRepository: muneebs/csrf-armor
Length of output: 178
Remove non-null assertions that fail Biome's noNonNullAssertion rule.
The non-null assertions (!) at lines 353, 375, 381, 435, 442, 452, 455, 486, 495, and 497 are not suppressed with biome-ignore directives and will fail linting since noNonNullAssertion is configured as "error" in biome.jsonc.
🛠️ Suggested pattern to replace `!` safely
+const must = (value: string | undefined, label: string): string => {
+ expect(value).toBeDefined();
+ if (!value) throw new Error(`${label} should be defined`);
+ return value;
+};
- expect(headerToken!.split('.').length).toBe(3);
+ const headerTokenValue = must(headerToken, 'headerToken');
+ expect(headerTokenValue.split('.').length).toBe(3);
- 'x-csrf-token': signedToken!,
+ 'x-csrf-token': must(signedToken, 'signedToken'),
- { name: 'csrf-token', value: signedToken! },
+ { name: 'csrf-token', value: must(signedToken, 'signedToken') },🧰 Tools
🪛 Biome (2.4.10)
[error] 353-353: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nextjs/tests/middleware.test.ts` at line 353, Several tests use
non-null assertions (e.g., headerToken!) which break Biome's noNonNullAssertion
rule; remove the `!` usages and instead assert existence before dereferencing
(for example, add explicit checks like expect(headerToken).toBeDefined() /
not.toBeNull() or guard with if (!headerToken) throw new Error(...) ) and then
call methods like split on the now-validated value; locate occurrences by
searching for the identifier headerToken and the other failing non-null
assertions in middleware.test.ts and replace each `variable!` with an existence
assertion/guard followed by normal usage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests
Chores