🛡️ Sentinel: [SECURITY] Prevent DoS and improve version parsing #791
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
🦋 Changeset detectedLatest commit: 016b5da The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #791 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 41 64 +23
Branches 14 28 +14
=========================================
+ Hits 41 64 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add `MAX_VERSION_LENGTH` constant (256) to limit input length. - Optimize `v` prefix stripping to handle multiple `v`s and avoid regex. - Update security tests to verify DoS prevention and robust parsing.
srod
force-pushed
the
sentinel-security-fix-11867803423465228644
branch
from
880700c to
131c4a3
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryImplemented DoS prevention by limiting version string length to 256 characters and hardened input validation with fail-closed behavior. Added robust 'v' prefix stripping using charCodeAt loop to handle multiple prefixes. Introduced
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant API as NodeVersion API
participant compareTo as compareTo()
participant Validation as Input Validation
User->>API: isAtLeast("vv99.0.0")
API->>compareTo: compareTo("vv99.0.0")
compareTo->>Validation: Check whitespace & length
Validation-->>compareTo: Valid (no trim needed, ≤256 chars)
compareTo->>Validation: Strip 'v' prefixes
Note over Validation: Loop charCodeAt check<br/>118='v', 86='V'
Validation-->>compareTo: Stripped: "99.0.0"
compareTo->>Validation: Validate segments
Note over Validation: Check each segment<br/>non-empty & digits only
Validation-->>compareTo: Valid segments
compareTo->>compareTo: Compare version parts
Note over compareTo: 20.0.0 < 99.0.0
compareTo-->>API: Returns -1
API->>API: Check !isNaN(-1) && -1 >= 0
API-->>User: false
Note over User,Validation: Fail-closed behavior:<br/>Invalid input → NaN → false
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const segment of s2) { | ||
| if (segment === "" || !/^\d+$/.test(segment)) { | ||
| return NaN; | ||
| } | ||
| } |
There was a problem hiding this comment.
style: Validation checks each segment but only validates digits. Leading zeros in version segments like "010.0.0" parse as 10 (see test at src/security.test.ts:68), but this may not match semver expectations where leading zeros could be considered invalid in some contexts.
| for (const segment of s2) { | |
| if (segment === "" || !/^\d+$/.test(segment)) { | |
| return NaN; | |
| } | |
| } | |
| for (const segment of s2) { | |
| if (segment === "" || !/^\d+$/.test(segment)) { | |
| return NaN; | |
| } | |
| // Reject leading zeros (except "0" itself) per strict semver | |
| if (segment.length > 1 && segment[0] === "0") { | |
| return NaN; | |
| } | |
| } |
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: src/index.ts
Line: 88:92
Comment:
**style:** Validation checks each segment but only validates digits. Leading zeros in version segments like `"010.0.0"` parse as `10` (see test at src/security.test.ts:68), but this may not match semver expectations where leading zeros could be considered invalid in some contexts.
```suggestion
for (const segment of s2) {
if (segment === "" || !/^\d+$/.test(segment)) {
return NaN;
}
// Reject leading zeros (except "0" itself) per strict semver
if (segment.length > 1 && segment[0] === "0") {
return NaN;
}
}
```
<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 { mockVersion } = vi.hoisted(() => ({ | ||
| mockVersion: { node: "20.0.0" }, | ||
| })); | ||
|
|
||
| vi.mock("node:process", () => ({ | ||
| versions: mockVersion, | ||
| release: {}, | ||
| })); |
There was a problem hiding this comment.
syntax: vi.hoisted is not available in the version of vitest used by this project (based on test failure). Use the same mocking pattern as src/index.test.ts:11-14.
| const { mockVersion } = vi.hoisted(() => ({ | |
| mockVersion: { node: "20.0.0" }, | |
| })); | |
| vi.mock("node:process", () => ({ | |
| versions: mockVersion, | |
| release: {}, | |
| })); | |
| const { mockVersion } = vi.hoisted(() => ({ | |
| mockVersion: { node: "20.0.0" }, | |
| })); | |
| vi.mock("node:process", () => ({ | |
| versions: mockVersion, | |
| release: {}, | |
| })); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/security.test.ts
Line: 5:12
Comment:
**syntax:** `vi.hoisted` is not available in the version of vitest used by this project (based on test failure). Use the same mocking pattern as `src/index.test.ts:11-14`.
```suggestion
const { mockVersion } = vi.hoisted(() => ({
mockVersion: { node: "20.0.0" },
}));
vi.mock("node:process", () => ({
versions: mockVersion,
release: {},
}));
```
How can I resolve this? If you propose a fix, please make it concise.| { | ||
| "lockfileVersion": 1, | ||
| "configVersion": 1, | ||
| "configVersion": 0, |
There was a problem hiding this comment.
style: configVersion downgraded from 1 to 0. Verify this was intentional and doesn't affect dependency resolution.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bun.lock
Line: 3:3
Comment:
**style:** `configVersion` downgraded from 1 to 0. Verify this was intentional and doesn't affect dependency resolution.
How can I resolve this? If you propose a fix, please make it concise.
Implemented DoS prevention by limiting the length of version strings in
compareTofunction. Also optimized the removal of 'v' prefixes to be more robust and performant. Added tests to verify these security enhancements.PR created automatically by Jules for task 11867803423465228644 started by @srod
Summary by cubic
Prevents DoS in version comparison by rejecting inputs longer than 256 characters and makes 'v' prefix stripping robust for multiple prefixes. Invalid or overly long strings now fail-closed across all comparators; also exposes eolDate for the current major.
Written for commit 016b5da. Summary will update on new commits.