Skip to content

security(path): invalid 4096-character DoS premise; superseded by #598 - #588

Closed
seonghobae wants to merge 6 commits into
masterfrom
sentinel-path-length-limit-8684421708098555526
Closed

security(path): invalid 4096-character DoS premise; superseded by #598#588
seonghobae wants to merge 6 commits into
masterfrom
sentinel-path-length-limit-8684421708098555526

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

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 38f5acdd692f5c0975e845051239086157d0012f preserves 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.

@google-labs-jules

Copy link
Copy Markdown

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 41 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 60a7a946-dded-4b38-9839-8db8a9d3b7d1

📥 Commits

Reviewing files that changed from the base of the PR and between 55b39b4 and f7d148c.

📒 Files selected for processing (4)
  • .github/workflows/ci.yml
  • .jules/sentinel.md
  • src/main/kotlin/html4tree/main.kt
  • src/test/kotlin/html4tree/MainTest.kt

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@cwl-noema-review cwl-noema-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 check topDir.length <= 4096 is 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 test testGoRejectsExcessivelyLongPaths constructs a 4097-character path and asserts that go throws 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 test testGoRejectsExcessivelyLongPaths does exactly this and expects IllegalArgumentException with message 'Directory path is too long'. The source code contains require(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 like testGoEmptyDir and testGoRejectsSymlinkTopDir use 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]

@seonghobae seonghobae added bug Something isn't working priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks labels Sep 2, 2026 — with ChatGPT Codex Connector
@seonghobae seonghobae added the type: bug Defect or incorrect behavior label Sep 2, 2026 — with ChatGPT Codex Connector

@cwl-noema-review cwl-noema-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 using require, which throws an IllegalArgumentException if 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 test testGoRejectsExcessivelyLongPaths specifically 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 string topDir length. Subsequent calls to absoluteFile.toPath().normalize() and Files.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]

@seonghobae seonghobae changed the title 🛡️ Sentinel: [보안 향상] 디렉토리 경로 길이 제한 추가 (DoS 방지) security(path): invalid 4096-character DoS premise; superseded by #598 Sep 3, 2026
@seonghobae seonghobae closed this Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant