fix(security): harden untrusted-input sinks (find file-write, source links, deep links)#4158
fix(security): harden untrusted-input sinks (find file-write, source links, deep links)#4158obchain wants to merge 3 commits into
Conversation
find -fprintf/-fprint/-fprint0/-fls write output to a named file — an arbitrary-path write that side-steps the gated file-write tools and their workspace confinement. They were classified Read (no approval prompt), so classify them Write alongside -delete so Supervised gates them.
extractAgentSources renders the model's raw tool-call url arg as an anchor href. That arg is prompt-injection-influenceable, so gate it to http(s) — a javascript:/data:/file: value must never reach an <a href>.
deep_link/deepLink derive from untrusted inbound provider content and are fed straight to navigate(). Honor them only when they are a relative in-app path (single leading slash, no //, no backslash); unsafe values fall through to the provider/category default.
📝 WalkthroughWalkthroughThe PR tightens deep-link handling in notification routing, filters agent source URLs to http(s), and expands Rust ChangesNotification route hardening
Timeline source URL filtering
Find command write classification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cca877d531
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| | "-fprintf" | ||
| | "-fprint" | ||
| | "-fprint0" | ||
| | "-fls" |
There was a problem hiding this comment.
Normalize quoted find write predicates
Because classify_command builds args with split_whitespace() and never removes shell quotes, a command like find . '-fprint' /tmp/out leaves the arg as '-fprint' and misses these exact matches, so it is classified as Read; the shell then strips the quotes before invoking GNU find, whose help lists -fprintf FILE, -fprint0 FILE, -fprint FILE, and -fls FILE as file-writing actions. In read-only/supervised contexts this lets the newly guarded arbitrary-file-write path run without the intended block or prompt, so normalize quoted args (or use a shell lexer) before matching these predicates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/utils/__tests__/toolTimelineFormatting.test.ts (1)
275-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this suite next to
toolTimelineFormatting.ts.This adds new coverage in
app/src/utils/__tests__/toolTimelineFormatting.test.ts, but the repo rule forapp/src/**/*.test.{ts,tsx}is to co-locate tests alongside the source file. Please move this suite toapp/src/utils/toolTimelineFormatting.test.tsinstead of growing the__tests__location further.As per coding guidelines,
app/src/**/*.test.{ts,tsx}: Co-locate unit tests as*.test.ts(x)underapp/src/**alongside source files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/utils/__tests__/toolTimelineFormatting.test.ts` around lines 275 - 309, The new extractAgentSources coverage is placed in the shared __tests__ folder, but it should be co-located with the source utility per the app/src test layout rule. Move the describe('extractAgentSources') suite from the current __tests__ file into toolTimelineFormatting.test.ts alongside toolTimelineFormatting.ts, and keep the same assertions and helpers so the behavior stays covered in the colocated test file.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/utils/__tests__/toolTimelineFormatting.test.ts`:
- Around line 275-309: The new extractAgentSources coverage is placed in the
shared __tests__ folder, but it should be co-located with the source utility per
the app/src test layout rule. Move the describe('extractAgentSources') suite
from the current __tests__ file into toolTimelineFormatting.test.ts alongside
toolTimelineFormatting.ts, and keep the same assertions and helpers so the
behavior stays covered in the colocated test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16481101-a52b-4f45-a782-0e9b64208ccf
📒 Files selected for processing (6)
app/src/lib/notificationRouter.test.tsapp/src/lib/notificationRouter.tsapp/src/utils/__tests__/toolTimelineFormatting.test.tsapp/src/utils/toolTimelineFormatting.tssrc/openhuman/security/policy/policy_command.rssrc/openhuman/security/policy/policy_tests.rs
Summary
classify_commandnow treatsfind -fprintf/-fprint/-fprint0/-flsasWrite(approval-gated), alongside the existing-delete.http(s)URLs; notificationdeep_link/deepLinkis honored only when it is a relative in-app path.Problem
findfile-write actions bypassed the approval gate.findis allowlisted and read-only by default.-exec/-execdir/-ok/-okdirare blocked and-deleteis classifiedWrite, but the-fprintf/-fprint/-fprint0/-flsprimaries — which write find's output to a named file at an arbitrary path — were classifiedRead, so they ran with no approval prompt even at the defaultSupervisedtier. That is an arbitrary-path write that side-steps the gated file-write tools and their workspace confinement (e.g. appending to a shell rc file).extractAgentSourcesbuilds the "sources visited" list from the model's rawurltool-call argument (prompt-injection-influenceable) and renders it as an<a href>. The scheme was not checked, so ajavascript:/data:/file:value could reach the anchor (CSP blocks script execution, so the practical risk is forced navigation — this closes the gap regardless).resolveIntegrationRoute/resolveSystemRoutereturned the core-supplieddeep_link/deepLinkverbatim intonavigate(). That value derives from untrusted inbound provider content; HashRouter contains most abuse, but nothing kept the value a relative in-app route.Solution
classify_segment(src/openhuman/security/policy/policy_command.rs): add-fprintf/-fprint/-fprint0/-flsto thefind→Writematch, mirroring-delete. Now approval-gated atSupervised.extractAgentSources(app/src/utils/toolTimelineFormatting.ts): newisHttpUrlfilter — onlyhttp(s)URLs become source rows.notificationRouter.ts: newisSafeInAppPath(single leading/, no//, no backslash); unsafe values fall through to the provider/category default and are logged as ignored.Submission Checklist
classify_find_file_write_actions_are_write(Write actions + read-only search); VitestextractAgentSources(http(s) vsjavascript:/data:/file:) andnotificationRouterunsafe-deep_link cases.pnpm debug uniton the two suites: 64 passed; focusedcargo teston the policy module: 157 passed incl. the new case).## Related— N/A: no matrix feature rows affected.Closes #NNN— N/A: hardening surfaced by an internal security review; no tracking issue.Impact
findfile-write) and two untrusted-URL/route hardening gaps. No change for legitimate input — benignfindsearches stayRead,http(s)sources still link, relative in-app deep links still navigate.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check— Prettier clean on changed filespnpm typecheck— ESLint clean on changed files; no type errors in touched modulesnotificationRouter+toolTimelineFormattingVitest (64 passed);cargo testpolicy module (157 passed)cargo fmt --checkclean on the two policy filesValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
findfile-write/delete actions require approval at Supervised; non-http(s) agent sources and non-relative notification deep links are dropped.Parity Contract
findsearches stayRead;http(s)sources and relative in-app deep links behave exactly as before.Duplicate / Superseded PR Handling
Summary by CodeRabbit
findactions as write operations, improving safety checks.