π‘οΈ Sentinel: [CRITICAL/HIGH] projectId κ²½λ‘ μ‘°μ(Path Traversal) μ·¨μ½μ μμ - #696
π‘οΈ Sentinel: [CRITICAL/HIGH] projectId κ²½λ‘ μ‘°μ(Path Traversal) μ·¨μ½μ μμ #696seonghobae wants to merge 4 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. |
There was a problem hiding this comment.
Pull request overview
This PR strengthens analysis-engine request validation by adding explicit projectId path-traversal guards in the Python API boundary, and extends unit coverage to ensure safe failures for traversal-like IDs while still allowing benign identifiers containing ...
Changes:
- Added
projectIdvalidation to reject./..and any/or\separators invalidate_analysis_job_request. - Updated/expanded Python unit tests to confirm
my..projectremains valid and traversal-shapedprojectIdvalues are rejected. - Documented the validation approach in the Sentinel knowledge file.
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 checks and associated error/telemetry behavior. |
| services/analysis-engine/tests/test_api.py | Updates an acceptance test to allow .. inside IDs and adds rejection cases for traversal-like IDs. |
| .jules/sentinel.md | Captures the intended validation strategy for future reference. |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not isinstance(project_id, str) or not project_id.strip(): | ||
| raise ValueError("Invalid analysis job request: invalid field 'projectId'") | ||
| if project_id in {".", ".."} or "/" in project_id or "\\" in project_id: | ||
| logger.warning("Security: path traversal detected in projectId") | ||
| raise ValueError("Invalid analysis job request: path traversal detected in 'projectId'") |
| ( | ||
| { | ||
| "sourceKind": "local_audio", | ||
| "projectId": "..", | ||
| "sourceLabel": "Late Night Set", | ||
| "roleFocus": [], | ||
| "localSource": { | ||
| "sourcePath": "/tmp/a.wav", | ||
| "fileName": "a.wav", | ||
| "extension": "wav", | ||
| "fileSizeBytes": 1024000, | ||
| }, | ||
| }, | ||
| "path traversal detected in 'projectId'", | ||
| ), |
| ## 2025-10-24 - Project ID path traversal guard validation approach | ||
| **Vulnerability:** Any project identifier that can reach a filesystem path join must be treated as untrusted. | ||
| **Learning:** Checking for substrings like `..` can erroneously block legitimate inputs like `my..id`. | ||
| **Prevention:** When validating identifiers like `projectId`, reject exact matches for `.` and `..`, and block any path separators (`/` and `\`), rather than blocking any string containing `..`. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
services/analysis-engine/tests/test_api.py:315
- The new projectId traversal guard also rejects the exact value ".", but the test matrix only covers ".." and path separators. Add a failing case for projectId=="." so the behavior is explicitly regression-tested.
(
{
"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 2 comments.
Comments suppressed due to low confidence (1)
services/analysis-engine/src/bandscope_analysis/api.py:283
- The path-traversal guard checks the raw
project_idvalue, but the earlier validation only usesproject_id.strip()for emptiness. That means inputs like" .. "bypass the{'.','..'}check (and may later be normalized/trimmed elsewhere), weakening the intended protection. Strip once and validate against the stripped value.
if not isinstance(project_id, str) or not project_id.strip():
raise ValueError("Invalid analysis job request: invalid field 'projectId'")
if project_id in {".", ".."} or "/" in project_id or "\\" in project_id:
logger.warning("Security: path traversal detected in projectId")
raise ValueError("Invalid analysis job request: path traversal detected in 'projectId'")
| ( | ||
| { | ||
| "sourceKind": "local_audio", | ||
| "projectId": "..", | ||
| "sourceLabel": "Late Night Set", | ||
| "roleFocus": [], | ||
| "localSource": { | ||
| "sourcePath": "/tmp/a.wav", | ||
| "fileName": "a.wav", | ||
| "extension": "wav", | ||
| "fileSizeBytes": 1024000, | ||
| }, | ||
| }, | ||
| "path traversal detected in 'projectId'", | ||
| ), |
| "numpy>=1.26", | ||
| "setuptools>=83.0.0", | ||
| "soundfile>=0.13.1", | ||
| "urllib3>=2.7.0", | ||
| "urllib3>=2.7.0", | ||
| "yt-dlp>=2026.6.9", |
|
Superseded by #713 for the base-branch security remediation, and its Specifically, #713 (
Auto-merge disabled here to avoid landing the un-hardened guard ahead of #713. This PR can be closed once #713 merges; leaving it open for now so nothing is lost if #713's review path stalls. Generated by Claude Code |
Understood. Acknowledging that this work is superseded by #713 and stopping work on this task. |
|
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 closed as a duplicate and superseded by #727, and stopping work on this task. |
Acknowledged. |
π¨ Severity: CRITICAL/HIGH
π‘ Vulnerability: API μμ²μμ μ λ¬λ°λ
projectIdκ° κ²½λ‘ μ‘°μμ μ·¨μ½ν μνμμ΅λλ€..μ΄λ..,/,\λ±μ κ°μ ν΅ν΄ νμΌμμ€ν κ²½λ‘μ κ°μ ν μ μλ μνμ΄ μμμ΅λλ€.π― Impact: 곡격μκ° μμμ κ²½λ‘λ₯Ό μμ±νμ¬ μλ² νμΌ μμ€ν μ μ κ·Όνκ±°λ μ°κΈ°λ₯Ό μνν κ°λ₯μ±μ΄ μ‘΄μ¬νμ΅λλ€.
π§ Fix:
services/analysis-engine/src/bandscope_analysis/api.pyλ΄μvalidate_analysis_job_requestμ λͺ μμ μΈ κ²μ¦ λ‘μ§μ μΆκ°νμ΅λλ€.projectIdκ°.λλ..μΈ κ²½μ°λ λλ ν 리 ꡬλΆμ(/,\)λ₯Ό ν¬ν¨ν κ²½μ° μ°¨λ¨νλλ‘ λ³κ²½νμκ³ , μ ν¨ν μλ³μ(μ:my..id)λ ν΅κ³Όνλλ‘ μ²λ¦¬νμ΅λλ€.β Verification:
uv run pytestλ°quickcheck.shν μ€νΈλ₯Ό λͺ¨λ ν΅κ³Όνμ΅λλ€.PR created automatically by Jules for task 5899358760909572075 started by @seonghobae