security(path): invalid 4096-character DoS premise; superseded by #598 - #588
security(path): invalid 4096-character DoS premise; superseded by #598#588seonghobae wants to merge 6 commits into
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. |
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
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 |
There was a problem hiding this comment.
Noema LLM review
The PR adds a defensive length check on the topDir input to prevent DoS from excessively long path strings, along with a corresponding unit test. The change is minimal, correctly placed at the boundary, and does not introduce regressions. The test validates the new behavior. Minor observation: the check applies to the raw input string rather than the normalized absolute path, but this is not a blocking issue.
Reviewed changed lines
src/main/kotlin/html4tree/main.kt:139 (RIGHT): New require checktopDir.length <= 4096is placed after the blank check and before the path traversal check. It correctly rejects overly long input strings with a clear error message. The limit is reasonable for typical filesystem path limits and prevents unbounded string processing.src/test/kotlin/html4tree/MainTest.kt:130 (RIGHT): New testtestGoRejectsExcessivelyLongPathsconstructs a 4097-character path and asserts thatgothrows IllegalArgumentException with the expected message. This directly covers the new validation logic and is well-formed.
Adversarial validation
src/main/kotlin/html4tree/main.kt:139 (RIGHT)falsified: The length check might not reject paths longer than 4096 characters, allowing the DoS it aims to prevent. — The new testtestGoRejectsExcessivelyLongPathsdoes exactly this and expectsIllegalArgumentExceptionwith message 'Directory path is too long'. The source code containsrequire(topDir.length <= 4096), which will throw for any length > 4096. The hypothesis is falsified.src/main/kotlin/html4tree/main.kt:139 (RIGHT)falsified: The length check might incorrectly reject valid paths (e.g., exactly 4096 characters) or break existing functionality. — The condition is<= 4096, so a path of length 4096 passes. Existing tests liketestGoEmptyDirandtestGoRejectsSymlinkTopDiruse normal paths and continue to pass (no changes to those tests). The check is placed before any filesystem operations, so it does not interfere with valid inputs. The hypothesis is falsified.- Residual risk: The length check is applied to the raw input string, not the normalized absolute path. A relative path with a very long current working directory could still result in an absolute path exceeding 4096 characters, but this is not a security regression since the input string itself is bounded and the OS will reject overly long paths. The limit is also not byte-aware for multi-byte characters, but this does not introduce a DoS vector beyond what the OS already handles.
Findings
-
[low] src/main/kotlin/html4tree/main.kt:139 (RIGHT): The length check is applied to the raw input string, not the normalized absolute path. A relative path with a long current working directory could still produce an absolute path exceeding 4096 characters. Consider validating the normalized absolute path length as well, though this is not a blocking issue.
-
Result: APPROVE
-
Head SHA:
a96f6e78e19686db362c7d544a33bf86fd8e6c16 -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
There was a problem hiding this comment.
Noema LLM review
The PR implements a path length limit of 4096 characters in the go function to mitigate potential Denial of Service (DoS) or Out-Of-Memory (OOM) vulnerabilities arising from excessively long input strings. The implementation uses a require block for early validation, is verified by a new test case in MainTest.kt, and is documented in .jules/sentinel.md for future reference.
Reviewed changed lines
.jules/sentinel.md:102 (RIGHT): Correctly documents the vulnerability, learning, and prevention strategy for path length validation.src/main/kotlin/html4tree/main.kt:139 (RIGHT): Implements the length check usingrequire, which throws anIllegalArgumentExceptionif the condition is not met, effectively blocking excessively long paths before any file system operations occur.src/test/kotlin/html4tree/MainTest.kt:131 (RIGHT): Adds a regression test that verifies the 4096 character limit is enforced and the correct error message is returned.
Adversarial validation
src/main/kotlin/html4tree/main.kt:139 (RIGHT)falsified: An input of exactly 4096 characters should be accepted, while 4097 should be rejected. — The code uses<= 4096. The testtestGoRejectsExcessivelyLongPathsspecifically uses 4097 to confirm rejection.src/main/kotlin/html4tree/main.kt:139 (RIGHT)falsified: The length check can be bypassed by using path traversal or encoding to cause OOM during processing. — The check is on the input stringtopDirlength. Subsequent calls toabsoluteFile.toPath().normalize()andFiles.isDirectory(..., NOFOLLOW_LINKS)handle the resolution and symlink security. The length check specifically targets the input string to prevent OOM during initial string processing.- Residual risk: Low. While some modern filesystems support paths longer than 4096 characters, this limit serves as a reasonable security boundary to prevent resource exhaustion attacks on the application layer.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
14eced775d18310b0eb7ba5e8de22c951edbd006 - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
Closed after live validation
This PR has no valid security delta to preserve.
Its entire intended behavior is an unconditional
require(topDir.length <= 4096)plus a 4,097-character rejection test and a generic Sentinel rule claiming that longer pathname strings cause OOM/DoS. That is not a portable filesystem contract and is not realistic resource-exhaustion evidence by itself.Canonical repair #598 now owns this finding. Exact head
38f5acdd692f5c0975e845051239086157d0012fpreserves the test-first opposite contract: pathname admission is not rejected solely by Kotlin string length, while actual filesystem validity remains authoritative. It also keeps the buyer-visible crawl-resource gap separate for measurement by directory count/fan-out, generated bytes, filesystem calls, elapsed time and peak RSS.Fresh #598 repair also removed an intervening descendant that had reintroduced this same 4,096 gate, deleted its portability regression and gap baseline, and changed CI policy; the restored current tree is byte-identical to the previously verified semantic tree (
00dd17e...38f5acdd: ahead-only,files=[]).Because #588 contributes only the rejected premise and no independent valid source/test/fixture/contract evidence, closure is
no valid delta, not duplicate-count cleanup. No merge, force-push, self-approval, gate weakening, or predecessor-evidence transfer is implied.