🛡️ Sentinel: [MEDIUM] Fix strix logging for path traversal - #1194
🛡️ Sentinel: [MEDIUM] Fix strix logging for path traversal#1194seonghobae wants to merge 9 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. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7971fb989e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path = repr(source_path) | ||
| logger.warning("Security: path traversal detected in localSource.sourcePath: %s", path) |
There was a problem hiding this comment.
When an untrusted sourcePath, cacheRoot, or tempRoot contains a parent segment, this warning now records the entire path. repr() prevents newline forging but does not redact or truncate usernames, directory names, or an arbitrarily long payload, contrary to docs/security/app-security.md, which explicitly prohibits logging full original file paths. Log only the rejected field/reason or a bounded, redacted identifier instead. The repository makes its app-security rules mandatory for logging changes.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| path = repr(project_id) | ||
| logger.warning("Security: path traversal detected in projectId: %s", path) |
There was a problem hiding this comment.
In the normal desktop analysis flow these warnings never create an audit trail: cli.py configures Python logging to stderr, while apps/desktop/src-tauri/src/main.rs:415-420 drains that stderr into a byte buffer and line 486 discards the joined result. Consequently the new warnings are visible only during direct CLI use or tests, not when a traversal attempt arrives through the shipped desktop orchestration path. Forward a redacted warning to the app's logging sink or otherwise consume the captured buffer. The repository makes its app-security rules mandatory for logging changes.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Fresh single-writer review: keep this PR Draft. Its unique security delta (escaped traversal-attempt logging) touches |
Understood. Acknowledging the instruction to keep this PR as a draft and that the unique security delta should be ordinarily adopted into #866. I have also noted the privacy concern regarding logging full local paths and the overlap with #1176 for the formatter delta. I am stopping work on this PR as instructed. |
|
Single-writer/privacy finding after fresh overlap review: keep this Draft and do not merge |
|
Fresh single-writer review: keep this PR Draft and do not merge it independently. Preserve the genuinely distinct evidence rather than closing it: the only material behavior not already present in #866 is adding the bounded field-name warning for Handoff rule: adopt/adapt the unique sourcePath diagnostic regression into #866 by ordinary descendant work if it survives review, keep |
| } | ||
| try: | ||
| validate_analysis_job_request(malicious_payload) | ||
| except ValueError: |
|
Canonical #866 has now ordinarily adopted/adapted the unique runtime/test semantics from this preservation lane without merging this branch. RED Keep this PR Draft/open until the exact successor has qualifying current-head verification. Do not merge it independently. The failed branch-to-branch consolidation probe #1195 carried no unique delta and was closed. |
Acknowledged. I understand the privacy concern and the need to log only bounded field-names. |
Understood. Acknowledging the instruction to keep this PR as a draft and that the unique security delta should be ordinarily adopted into #866. I am stopping work on this branch as instructed. |
Understood. Acknowledging that the relevant changes have been adopted into canonical PR #866. I will leave this PR as a draft as instructed and stop work on this task. |
1 participant
Draft preservation / consolidation lane
This PR is not a second Resource Admission writer. Canonical
validate_analysis_job_requestownership remains Draft #866, current exact7a10f3cc8359ef44bfd52fbf31d7df287912d103, on protecteddevelop@314ddeae7b775a4957594b599358c8255617eb2e.Successor evidence
#866 has ordinarily adopted the only distinct runtime/test behavior from this branch without merging or rebasing #1194:
0e313169299eb6112343ca7d8fbac612d6df5942injects a parent-traversal segment plus a forged newline suffix and requires rejection while the malicious path remains absent from logger arguments.5546a17ccc848e2d043359586a2abc0bd198a66cadds the bounded fixed warningSecurity: path traversal detected in localSource.sourcePathbefore the existing rejection.7c0e858da327daa07b56c75b2a90a2761b4c4880restores unrelated text-only drift from the file replacement. Comparing pre-RED4cb93a74...with that descendant leaves one production-line addition plus the dedicated regression.af3efa803141cd139efd3d61fb1a9503fbbf316frecords CWE-117 rationale, rejected raw-path/repr()alternatives, and the claim boundary; later fix(audio): establish canonical local-audio resource policy #866 descendants retain it.The earlier branch-to-branch consolidation probe #1195 was non-mergeable and introduced no unique delta, so it was closed unmerged rather than using force-push or destructive rebase.
Authority / close rule
The original generated description was wrong to suggest logging attacker-controlled path content. The product diagnostic intentionally records only the fixed field identifier.
.jules/sentinel.mdis learning metadata, not a second product authority; its useful rationale is represented in canonical doctoring.Current #866 has advanced into persisted feature-cache resource-admission work. That later work neither changes nor weakens the succeeded source-path diagnostic semantics above. Keep this PR Draft/open until the exact #866 successor has qualifying current-head verification. Do not merge independently. Close as fully succeeded only after exact successor evidence demonstrates the runtime/test/doctoring semantics above are preserved; queued, blocked, predecessor, or self-authored review evidence does not satisfy that rule.