π‘οΈ Sentinel: [HIGH] projectId κ²½λ‘ νμ(Path Traversal) μ·¨μ½μ μμ - #703
π‘οΈ Sentinel: [HIGH] projectId κ²½λ‘ νμ(Path Traversal) μ·¨μ½μ μμ #703seonghobae wants to merge 3 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 reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review detailsβοΈ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: β Files ignored due to path filters (2)
π Files selected for processing (4)
β¨ Finishing Touchesπ§ͺ Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens the analysis-engine API boundary by rejecting path traversal attempts via projectId in validate_analysis_job_request, and expands unit tests to cover allowed/blocked projectId shapes. It also records the learning in the Jules Sentinel log.
Changes:
- Add explicit
projectIdpath-traversal validation (block.,..,/,\) with a security warning log. - Add unit tests to ensure interior-dot identifiers (e.g.
project..1) are accepted and obvious traversal payloads are rejected. - Add a Sentinel log entry documenting the vulnerability and prevention guidance.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| services/analysis-engine/src/bandscope_analysis/api.py | Adds projectId traversal guard + warning log inside request validation. |
| services/analysis-engine/tests/test_api.py | Adds acceptance test for interior dots and new rejection cases for traversal-shaped projectId. |
| .jules/sentinel.md | Documents the vulnerability/prevention guidance in the Sentinel log. |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| def test_validate_analysis_job_request_accepts_interior_dots() -> None: | ||
| """Ensure valid local-audio requests with interior dots in projectId are normalized.""" |
| ( | ||
| { | ||
| "sourceKind": "local_audio", | ||
| "projectId": "..", | ||
| "sourceLabel": "Late Night Set", | ||
| "roleFocus": [], | ||
| "localSource": { | ||
| "sourcePath": "/Users/test/Music/late-night-set.wav", | ||
| "fileName": "late-night-set.wav", | ||
| "extension": "wav", | ||
| "fileSizeBytes": 1024000, | ||
| }, | ||
| }, | ||
| "path traversal", | ||
| ), |
| ## 2026-07-03 - Project ID API Validation | ||
| **Vulnerability:** Similar to the previous entry, the `projectId` provided via the API payload was not explicitly validated against path traversal before being used in path concatenations. | ||
| **Learning:** Any identifier (such as `projectId`) provided via API payloads that eventually reaches a filesystem path join must be treated as untrusted. | ||
| **Prevention:** Apply explicit path traversal protections by rejecting exact string matches for `.` and `..`, as well as any path separators `/` and `\`, but allow normal identifiers with interior dots (e.g., `my..id`) to pass validation safely. Always log security warnings when invalid access attempts are detected. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
services/analysis-engine/tests/test_api.py:87
- Docstring says the request is "normalized", but this test asserts the payload is returned unchanged. Update the wording to avoid implying mutation/normalization that doesn't occur.
"""Ensure valid local-audio requests with interior dots in projectId are normalized."""
services/analysis-engine/tests/test_api.py:167
- The validator now rejects projectId == "." as well as "..", but the reject-cases only cover "..". Adding a "." case helps keep branch coverage aligned with the engineβs 100% coverage gate.
(
{
"sourceKind": "local_audio",
"projectId": "..",
"sourceLabel": "Late Night Set",
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
services/analysis-engine/tests/test_api.py:88
- The test docstring says the request is "normalized", but this validator call is only asserting acceptance (the output equals the input). This is misleading when reading the test intent.
def test_validate_analysis_job_request_accepts_interior_dots() -> None:
"""Ensure valid local-audio requests with interior dots in projectId are normalized."""
assert validate_analysis_job_request(
services/analysis-engine/pyproject.toml:16
setuptoolsis being added as a runtime dependency, but there are no imports/usages ofsetuptoolsin the engine code/tests. Declaring it as a direct dependency expands the public dependency contract without an obvious need; if this is only needed for packaging/build tooling, it should live under[build-system]instead.
"numpy>=1.26",
"setuptools>=82.0.0",
"soundfile>=0.13.1",
| "numpy>=1.26", | ||
| "setuptools>=82.0.0", | ||
| "soundfile>=0.13.1", | ||
| "urllib3>=2.7.0", | ||
| "yt-dlp>=2026.6.9", | ||
| "urllib3>=2.7.0", | ||
| "yt-dlp>=2026.7.0", |
|
Closing as closed-duplicate/superseded (path-traversal cluster) by autonomous product-completion loop. Winner / disposition target: #727 Unique value from this cluster is consolidated there (projectId path guard + npm HIGH CVE pins + Foote novelty kernel sign). Remaining micro-duplicates do not land additional commercial readiness. |
|
closed-closed-duplicate/superseded (path-traversal cluster) β #727 |
Understood. Acknowledging that this work is now obsolete and superseded by PR #727, and stopping work on this task. |
Acknowledged. |
π¨ μ¬κ°λ: HIGH
π‘ μ·¨μ½μ :
projectIdAPI νμ΄λ‘λλ₯Ό κ²μ¦νμ§ μμ λλ ν 리 κ²½λ‘ κ²μ (Path Traversal) 곡격μ μ·¨μ½νμ΅λλ€.π― μν₯: 곡격μκ°
projectIdλ₯Ό μ‘°μνμ¬ (..,/,\ν¬ν¨) μλνμ§ μμ λ‘컬 νμΌ μμ€ν κ²½λ‘μ μ κ·Όν μ μμμ΅λλ€.π§ ν΄κ²°μ± :
validate_analysis_job_requestμ λͺ μμ μΈ κ²μ¦ λ‘μ§μ μΆκ°νμ¬.,..μΌμΉ λ°/,\ν¬ν¨μ μ°¨λ¨νκ³ λ³΄μ κ²½κ³ λ‘κ·Έλ₯Ό λ¨κΈ°λλ‘ μμ νμ΅λλ€.project..1κ³Ό κ°μ΄ λ΄λΆ μ (dot)μ ν¬ν¨ν μ μ μλ³μλ νμ©λ©λλ€.β κ²μ¦:
test_api.pyμ μ μμ μΈ νμ΄λ‘λλ₯Ό κ±°λΆνκ³ μ μ νμ΄λ‘λλ₯Ό νμ©νλ λ¨μ ν μ€νΈλ₯Ό μΆκ°νκ³ ν΅κ³Όλ₯Ό νμΈνμ΅λλ€.PR created automatically by Jules for task 13767768366505933146 started by @seonghobae