test(security): add comprehensive security-utils unit test coverage#190
test(security): add comprehensive security-utils unit test coverage#190nikolasdehor wants to merge 2 commits intoSynkraAI:mainfrom
Conversation
Add 47 test cases for the security-utils module which had zero test coverage. Tests cover all 7 exported functions: validatePath, sanitizeInput (5 modes), validateJSON, getObjectDepth, RateLimiter (full class), safePath, and isSafeString. Includes path traversal, null byte injection, shell metachar escaping, HTML entity encoding, JSON depth/size limits, and rate limiter windowing tests. Refs SynkraAI#52
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a new comprehensive test suite at Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Hey @Pedrovaleriolopez @oalanicolas 👋 Security-critical test coverage PR — All pure functions, no mocking needed. Refs #52. Would appreciate a review + CI approval! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/core/utils/security-utils.test.js (2)
274-288: Useasync/awaitfor the timer-based test instead of a rawnew Promisecallback.The current pattern is valid (Jest awaits a returned Promise), but it embeds the assertion inside a
setTimeoutcallback — a style the project guidelines explicitly discourage.♻️ Proposed refactor
- it('should clean up expired entries', () => { + it('should clean up expired entries', async () => { const limiter = new RateLimiter({ maxRequests: 100, windowMs: 1 }); limiter.check('user1'); - // Wait for window to expire - return new Promise((resolve) => { - setTimeout(() => { - limiter.cleanup(); - // After cleanup, the map should be empty - expect(limiter.requests.size).toBe(0); - resolve(); - }, 10); - }); + // Wait for the 1 ms window to expire + await new Promise((resolve) => setTimeout(resolve, 10)); + limiter.cleanup(); + expect(limiter.requests.size).toBe(0); });As per coding guidelines, "Check for async/await best practices."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/utils/security-utils.test.js` around lines 274 - 288, Replace the raw Promise+setTimeout callback with async/await: make the test function async, use await new Promise(resolve => setTimeout(resolve, 10)) to pause, then call limiter.cleanup() and assert that limiter.requests.size is 0; locate the test that constructs RateLimiter, calls limiter.check('user1'), and uses limiter.cleanup()/limiter.requests in the assertion and update it accordingly.
284-284:limiter.requestsis an internal implementation detail — tightly couples the test to the private Map name.If
RateLimiterrenames or restructures its internal storage, this assertion breaks with no clear signal. Prefer a behaviour-based assertion if the class exposes one (e.g., check that subsequentcheck()calls aftercleanup()are allowed again), or at minimum document the dependency explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/utils/security-utils.test.js` at line 284, The test is tightly coupled to the private Map limiter.requests; update the assertion to be behavior-based: after calling limiter.cleanup(), assert via the public API (e.g., call limiter.check(...) or whatever public method RateLimiter exposes) that requests are allowed again (or that check returns the expected boolean/no error), rather than inspecting limiter.requests.size; if no suitable public behavior exists, add a small public accessor or method on RateLimiter (or a comment in the test) to document the dependency on internal storage before keeping the current assertion. Ensure references to RateLimiter, limiter, check(), and cleanup() are used to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/utils/security-utils.test.js`:
- Around line 59-62: The test "should enforce basePath restriction" currently
uses '../../outside' which is rejected by the traversal check, masking basePath
logic; change the test to use a path that does not contain '..' but is clearly
outside the basePath (e.g. '/other/dir' or '/outside') so the failure comes from
basePath enforcement; update the call to validatePath (in
tests/core/utils/security-utils.test.js, test named "should enforce basePath
restriction") to use that non-traversal outside path and keep the expectation
expect(result.valid).toBe(false).
---
Nitpick comments:
In `@tests/core/utils/security-utils.test.js`:
- Around line 274-288: Replace the raw Promise+setTimeout callback with
async/await: make the test function async, use await new Promise(resolve =>
setTimeout(resolve, 10)) to pause, then call limiter.cleanup() and assert that
limiter.requests.size is 0; locate the test that constructs RateLimiter, calls
limiter.check('user1'), and uses limiter.cleanup()/limiter.requests in the
assertion and update it accordingly.
- Line 284: The test is tightly coupled to the private Map limiter.requests;
update the assertion to be behavior-based: after calling limiter.cleanup(),
assert via the public API (e.g., call limiter.check(...) or whatever public
method RateLimiter exposes) that requests are allowed again (or that check
returns the expected boolean/no error), rather than inspecting
limiter.requests.size; if no suitable public behavior exists, add a small public
accessor or method on RateLimiter (or a comment in the test) to document the
dependency on internal storage before keeping the current assertion. Ensure
references to RateLimiter, limiter, check(), and cleanup() are used to locate
the change.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit test coverage for the previously untested security-utils.js module, introducing 47 test cases that validate security-critical functionality including path validation, input sanitization, JSON validation, rate limiting, and injection detection.
Changes:
- Added complete test suite for 7 exported security utility functions
- Covers edge cases for path traversal, null byte injection, XSS, shell injection, and rate limiting
- Includes tests for all sanitization modes (general, filename, identifier, shell, html)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should return remaining count', () => { | ||
| const limiter = new RateLimiter({ maxRequests: 5, windowMs: 60000 }); | ||
|
|
||
| // remaining is calculated before the request is recorded |
There was a problem hiding this comment.
This comment appears to contradict the actual behavior shown in the test. The test shows first.remaining equals 5 (the maxRequests value), suggesting 'remaining' represents requests still available before the current one is recorded. However, after the third check, remaining is 3, which would be consistent if the count includes the current request. Consider clarifying whether 'remaining' is calculated before or after recording the current request, as the comment and test behavior seem inconsistent.
| }); | ||
|
|
||
| it('should clean up expired entries', () => { | ||
| const limiter = new RateLimiter({ maxRequests: 100, windowMs: 1 }); |
There was a problem hiding this comment.
The windowMs value of 1 millisecond makes this test fragile and timing-dependent. Consider using a slightly larger window (e.g., 50ms) and explicit timing control to make the test more reliable across different system loads and CI environments.
| expect(getObjectDepth(42)).toBe(0); | ||
| expect(getObjectDepth(null)).toBe(0); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The expectation that a flat object returns depth 0 seems counterintuitive. Consider adding a comment explaining that depth measures nesting levels within an object, not the object itself, to clarify why { a: 1 } has depth 0 while { a: { b: 1 } } has depth 1.
| // Note: depth measures nesting *within* the root object; a flat object has depth 0, | |
| // while { a: { b: 1 } } has depth 1 because of one nested level. |
|
Addressed CodeRabbit review feedback — pushed fixes. Thanks for the thorough review! 🙏 |
- Add isolated basePath enforcement test using allowAbsolute to avoid path traversal detection masking the basePath check - Use behavior-based assertion for RateLimiter cleanup instead of checking internal Map state
nikolasdehor
force-pushed
the
test/security-utils-coverage
branch
2 times, most recently
from
fcb429a to
5534da4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/core/utils/security-utils.test.js (1)
319-346: Consider adding an empty-string edge case.The suite covers all notable injection patterns and type checks, but
isSafeString('')is untested. An empty string is a natural boundary: it either returnstrue(no patterns to detect) orfalse(explicitly rejected as non-content). Pinning that behavior prevents an unintentional contract change.✨ Proposed addition
it('should return true for normal strings', () => { expect(isSafeString('hello world')).toBe(true); expect(isSafeString('agent-dev_v2')).toBe(true); + expect(isSafeString('')).toBe(true); // empty string: no injection patterns });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/utils/security-utils.test.js` around lines 319 - 346, Add an explicit test for the empty-string edge case in the existing isSafeString suite: inside the describe('isSafeString') block add a new it block that calls isSafeString('') and asserts the expected behavior (e.g., expect(isSafeString('')).toBe(true)); reference the isSafeString function in tests/core/utils/security-utils.test.js so the contract for empty input is pinned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/utils/security-utils.test.js`:
- Around line 189-195: The test comment incorrectly states "depth > 3" while the
constructed object {a:{b:{c:{d:'deep'}}}} has a getObjectDepth of 3 and is being
compared against maxDepth 2; update the comment in
tests/core/utils/security-utils.test.js to accurately state that the nested
object has depth 3 (so 3 > maxDepth 2) and reference the validateJSON call (and
getObjectDepth convention) to clarify the boundary being tested.
- Around line 287-299: The test uses real setTimeout which is flaky; switch to
Jest fake timers to deterministically expire the window: call
jest.useFakeTimers() before creating the RateLimiter instance (RateLimiter),
perform the initial limiter.check('user1'), then advance timers with
jest.advanceTimersByTime(windowMs + 1) to simulate expiry, call
limiter.cleanup(), run any pending microtasks if needed, assert
limiter.check('user1').allowed is true, and finally restore timers with
jest.useRealTimers(); reference the RateLimiter class and its check and cleanup
methods when making these changes.
---
Nitpick comments:
In `@tests/core/utils/security-utils.test.js`:
- Around line 319-346: Add an explicit test for the empty-string edge case in
the existing isSafeString suite: inside the describe('isSafeString') block add a
new it block that calls isSafeString('') and asserts the expected behavior
(e.g., expect(isSafeString('')).toBe(true)); reference the isSafeString function
in tests/core/utils/security-utils.test.js so the contract for empty input is
pinned.
| it('should reject deeply nested JSON', () => { | ||
| // Build a nested object with depth > 3 | ||
| const nested = { a: { b: { c: { d: 'deep' } } } }; | ||
| const result = validateJSON(JSON.stringify(nested), { maxDepth: 2 }); | ||
| expect(result.valid).toBe(false); | ||
| expect(result.error).toContain('nesting depth'); | ||
| }); |
There was a problem hiding this comment.
Inaccurate comment — depth is 3, not > 3.
The comment says "Build a nested object with depth > 3" but by the getObjectDepth convention used in this file the object {a:{b:{c:{d:'deep'}}}} has depth exactly 3 (three nested-object transitions). The condition being exercised is depth (3) > maxDepth (2). The current wording implies the object itself has a depth greater than 3, which could mislead future maintainers about where the boundary sits.
📝 Proposed comment fix
- // Build a nested object with depth > 3
+ // Build a nested object with depth = 3, which exceeds maxDepth = 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should reject deeply nested JSON', () => { | |
| // Build a nested object with depth > 3 | |
| const nested = { a: { b: { c: { d: 'deep' } } } }; | |
| const result = validateJSON(JSON.stringify(nested), { maxDepth: 2 }); | |
| expect(result.valid).toBe(false); | |
| expect(result.error).toContain('nesting depth'); | |
| }); | |
| it('should reject deeply nested JSON', () => { | |
| // Build a nested object with depth = 3, which exceeds maxDepth = 2 | |
| const nested = { a: { b: { c: { d: 'deep' } } } }; | |
| const result = validateJSON(JSON.stringify(nested), { maxDepth: 2 }); | |
| expect(result.valid).toBe(false); | |
| expect(result.error).toContain('nesting depth'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/utils/security-utils.test.js` around lines 189 - 195, The test
comment incorrectly states "depth > 3" while the constructed object
{a:{b:{c:{d:'deep'}}}} has a getObjectDepth of 3 and is being compared against
maxDepth 2; update the comment in tests/core/utils/security-utils.test.js to
accurately state that the nested object has depth 3 (so 3 > maxDepth 2) and
reference the validateJSON call (and getObjectDepth convention) to clarify the
boundary being tested.
| it('should clean up expired entries', async () => { | ||
| const limiter = new RateLimiter({ maxRequests: 1, windowMs: 1 }); | ||
|
|
||
| limiter.check('user1'); | ||
| expect(limiter.check('user1').allowed).toBe(false); | ||
|
|
||
| // Wait for window to expire, then cleanup | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| limiter.cleanup(); | ||
|
|
||
| // After cleanup, expired entry is removed so user1 can request again | ||
| expect(limiter.check('user1').allowed).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Real-time setTimeout with a 1ms window is a flakiness risk in CI.
The native timer functions depend on real time to elapse; Jest can swap out timers with functions that allow you to control the passage of time. A windowMs: 1 combined with a setTimeout(resolve, 10) relies on the host OS actually delivering the callback after ≥1ms, which is not guaranteed under CPU contention on shared CI runners. CI environments have variable CPU and memory resources, causing timing-dependent tests to behave inconsistently; replace real delays with fake timers and use async assertions that wait for conditions rather than fixed durations.
Refactor to use jest.useFakeTimers() so the window expiry is deterministic:
🛡️ Proposed fix using fake timers
- it('should clean up expired entries', async () => {
- const limiter = new RateLimiter({ maxRequests: 1, windowMs: 1 });
-
- limiter.check('user1');
- expect(limiter.check('user1').allowed).toBe(false);
-
- // Wait for window to expire, then cleanup
- await new Promise((resolve) => setTimeout(resolve, 10));
- limiter.cleanup();
-
- // After cleanup, expired entry is removed so user1 can request again
- expect(limiter.check('user1').allowed).toBe(true);
- });
+ it('should clean up expired entries', () => {
+ jest.useFakeTimers();
+ try {
+ const limiter = new RateLimiter({ maxRequests: 1, windowMs: 50 });
+
+ limiter.check('user1');
+ expect(limiter.check('user1').allowed).toBe(false);
+
+ // Advance clock past the window without real wall-clock dependency
+ jest.advanceTimersByTime(51);
+ limiter.cleanup();
+
+ // After cleanup, expired entry is removed so user1 can request again
+ expect(limiter.check('user1').allowed).toBe(true);
+ } finally {
+ jest.useRealTimers();
+ }
+ });As per coding guidelines, verify async/await best practices and ensure tests don't rely on real-time delays.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/utils/security-utils.test.js` around lines 287 - 299, The test
uses real setTimeout which is flaky; switch to Jest fake timers to
deterministically expire the window: call jest.useFakeTimers() before creating
the RateLimiter instance (RateLimiter), perform the initial
limiter.check('user1'), then advance timers with
jest.advanceTimersByTime(windowMs + 1) to simulate expiry, call
limiter.cleanup(), run any pending microtasks if needed, assert
limiter.check('user1').allowed is true, and finally restore timers with
jest.useRealTimers(); reference the RateLimiter class and its check and cleanup
methods when making these changes.
|
Consolidated into #424 |
Summary
security-utils.jsmodule which previously had zero test coveragevalidatePath— path traversal detection, null byte injection, basePath enforcement (isolated)sanitizeInput— all 5 sanitization modes (general, filename, identifier, shell, html)validateJSON— parsing, size limits, depth limitsgetObjectDepth— nesting calculation for primitives, objects, arraysRateLimiter— full class coverage (check, reset, clear, cleanup with behavior-based assertions)safePath— safe path construction with base directory enforcementisSafeString— injection pattern detectionTest plan
npx jest tests/core/utils/security-utils.test.js)Closes #195
Refs #52
Summary by CodeRabbit