Repository navigation
fix(security): confine an LLC work product's storage_path before opening it (#17302, #17300 AC3) - #17408
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 9 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR centralises the LLC storage root and validates attachment and artifact paths with ChangesLLC storage validation
Priority: ⬆️ High Severity of issue fixed: High Merge Risk: 🟡 Moderate · up to Uploads now stay inside their work-item folder. However, a crafted filename can still make separate attachments write to the same file, so a later upload can overwrite an earlier attachment's contents. Fix this before merging. Security Architecture ReviewSecurity architecture risk: 🟠 High · up to The change blocks reads outside the configured storage area and ties uploads to the authenticated tenant. However, a crafted upload filename can still select a different file within its work-item directory. The effect of filesystem symlinks also depends on deployment permissions that are not established here. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
Review addressed at I wrote that switching Verified against the real code rather than the review's stdlib reproduction:
Both behaviours are better than what they replace — decode-before-resolve is The part that was an actual defect: that The 400 test is deliberately separate from the unit tests proving which inputs raise. Those assert "this filename is refused"; the router test asserts "a refusal becomes 400". Merged into one, either half could pass for the other's reason — a route that 400s on everything, or a Mutation: removing The file-size guard refused my first fix, verbatimCorrect refusal, and the ceiling is not raised. The two refusal arms are folded into one instead, landing the file at exactly 1416 — no growth. The fold also removes a LOW — docstring narrowed
LOW / non-blocking — filed, not droppedContainment is root-scoped, not tenant-scoped: an agent knowing another company's three UUIDs could point On the two tests that do not bind to a reverted checkThe reviewer is right that |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 1758f566-0af5-41d2-a1a5-62d36164e9aa
📒 Files selected for processing (7)
autobot-backend/llc/api/work_items.pyautobot-backend/llc/kb/artifact_ingestor.pyautobot-backend/llc/kb/artifact_ingestor_path_containment_17302_test.pyautobot-backend/llc/services/attachment_path_17300_test.pyautobot-backend/llc/services/attachment_service.pyautobot-backend/llc/storage_root.pyautobot-backend/llc/tests/test_attachment_upload_tenant_17300.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 63ca57a5-5f32-4dd4-887f-0887b0907482
📒 Files selected for processing (2)
autobot-backend/llc/kb/artifact_ingestor.pyautobot-backend/llc/services/attachment_service.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
…ing it (#17302, #17300) storage_path is a free-form optional string on the agent request model (llc/api/agent_api.py:256), stored unvalidated and opened later by the ingestor. Nothing in this codebase ever produces one -- grep shows the field is only ever set from a caller's request body -- so an authenticated agent could name any path on the host and have its contents read into the knowledge index, where they become retrievable. The only gate was _is_text_path, an extension allowlist. That is the near-miss worth naming: validation existed, and it was not the validation that was needed. "Is this a shape we can parse" was standing in for "are we allowed to read this", and the two questions do not overlap at all. Correcting #17302's own threat description while confirming it: .json and .csv are NOT in _TEXT_EXTENSIONS, so the issue's example of ".json credential files" does not apply. The real list still admits .ini, .cfg, .yaml, .toml, .sh, .py and .sql, so the finding stands unchanged in severity -- only its examples were wrong, and a test that used .json would have been refused as binary and proved nothing. _contained_storage_path resolves through the house validator with an explicit root and fails closed. It returns the VALIDATED string, and the call site opens that rather than the raw input -- THREAT_MODEL section 1's "the validated string is the string used", which is the bypass this shape invites. Also #17300 AC3, which was the one criterion that PR did not deliver as written: _storage_path enforced containment with a local is_relative_to rather than the house helper. Same property, but the safety was load-bearing on the UUID parse two lines up and nothing said so. It now routes through validate_path, so the call site inherits decode-before-resolve and the known-bypass coverage. All 16 existing #17300 tests pass unchanged, which is the evidence that this is an adoption change and not a behaviour change. llc/storage_root.py exists so the env var and its default have one definition. The ingestor needs the same root the attachment writer uses, and a second copy is how the two drift apart. attachment_service keeps its _LOCAL_STORAGE_PATH module attribute, which existing tests patch directly. Mutation-checked, and the first pass FAILED honestly: two of three mutations survived. Opening the raw storage_path again, and dropping the None guard, both left all six tests green -- my tests covered the helper and not the wiring, one hour after recording that exact lesson elsewhere. The gap was real: every test used paths whose raw and validated forms are identical, so "which string is opened" was unobservable. A percent-encoded in-root path makes them differ (_canonicalize decodes %73ub to sub, so the validated path exists and the raw one does not), and the refusal-logging test pins the explicit guard. All three mutations now caught, each by its own test: M1 open the RAW storage_path -> test_the_path_opened_is_the_decoded_one... M2 helper returns everything -> 5 tests M3 drop the None guard -> test_a_refusal_is_a_clean_skip... 38 passed across the ingestor, attachment-path and attachment suites. Closes #17302
…ming the switch was behaviour-preserving (#17302, #17300) Pre-merge review found that routing _storage_path's containment through validate_path is NOT behaviour-preserving, and that my evidence for saying it was -- "all 16 existing #17300 tests pass" -- could not have shown otherwise. None of those 16 uses a filename containing % or a non-ASCII character, so none reaches the input class that changed. A suite passing over inputs that never touch the change says nothing about the change. Verified against the real code rather than the review's reproduction: notes.txt -> <uuid>.txt notes.%74xt -> <uuid>.txt (decoded; old code wrote %74xt) notes.<15 fullwidth confusables> -> raises ValueError (old code accepted) validate_path runs _canonicalize -- multi-round percent-decode plus NFKC -- before resolving; the old local is_relative_to check did not. Decoding before resolving is the house rule and a literal %74 on disk was never wanted, so the new behaviour is better. It is still a change, and it now lives in two tests instead of in a claim. The confusable case matters more: NFKC folds U+FF0F to / and U+FE52 to ., so the extension reads as /../../.. after canonicalisation and walks above the storage root. Containment refuses it, which is correct -- but the ValueError was caught by nothing. It reached app_factory's global Exception handler and became a generic 500, telling the caller to retry an upload that can never succeed. The route now maps ValueError to 400 without echoing the internal path detail. The 400 test is deliberately separate from the unit tests that prove which inputs raise. Those assert "this filename is refused"; the router test asserts "a refusal becomes 400". Merged into one, either half could pass for the other's reason -- a route that 400s on everything, or a _storage_path that raises on nothing, both look correct through a single combined test. Mutation-checked: removing the ValueError from the except arm fails exactly test_an_unstorable_filename_is_a_400_not_a_500, 1 failed 3 passed. The file-size guard refused the first version of this fix, verbatim: autobot-backend/llc/api/work_items.py: 1424 lines, over its recorded ceiling of 1416. A grandfathered file may not grow (#14236) -- the exemption freezes the size it was granted for, it does not license more. That is a correct refusal and the ceiling is not raised. The two refusal arms are folded into one instead, which lands the file at exactly 1416 -- no growth. Folding also removes a `detail="Internal server error"` on a 413, which #12740 already established LLC domain errors must not return; that rule made the fold an improvement rather than a compromise forced by a line budget. Also narrows llc/storage_root.py's docstring, which claimed call-time freshness for "a test or a deployment" generally. That holds for the ingestor, which calls the function per use. attachment_service still binds _LOCAL_STORAGE_PATH once at import, unchanged from before this module existed, and its tests patch that attribute rather than the env var. One caller, not both. 44 passed across the ingestor, attachment-path, tenant-upload and attachment suites. Refs #17302, #17300
…17302) CodeQL reported two HIGH alerts against this PR -- "clear-text logging of sensitive information" at artifact_ingestor.py:284 and :299. Read rather than waved off, because a path-containment PR carrying two high alerts is the last place to assume a false positive. Both lines pre-date this branch (origin/main:241 and :251); CodeQL attributes them here because the surrounding code moved. That makes them re-attributed, not introduced -- and they are still wrong, in a file this PR is already editing: - CLAUDE.md forbids internal filesystem paths in logs, and both interpolated one verbatim. - In THIS file the path is agent-supplied. That is the subject of the PR. So it also wrote attacker-controlled content into the log stream. The binary-skip line now logs the extension, which is what the decision was actually made on, bounded to 16 chars. The read-failure line logs the exception TYPE rather than the exception: an OSError stringifies with its filename, so logger.exception would have put the path back through the traceback after it was removed from the format string. The cost is the traceback; the product id is in both messages and recovers the path from the row for anyone who needs it. Caught while doing it, and it is the reason for the extra test: rewording the read-failure message silently broke an existing negative assertion. `assert "could not read storage_path" not in caplog.text` went on checking for a literal that no longer appears anywhere in the tree -- passing unconditionally, for a string that could never be present. A negative assertion against a literal is worth exactly as much as the literal still existing, and nothing in the suite noticed. test_the_read_failure_log_literal_still_exists pins it: rename the message and that test fails loudly instead of quietly making the other one vacuous. Mutation-checked, 4 of 4 caught, each by its own test: open the RAW storage_path -> test_the_path_opened_is_the_decoded_one... helper returns everything -> 5 tests drop the None guard -> test_a_refusal_is_a_clean_skip... rename the log literal -> test_the_read_failure_log_literal_still_exists The other half of the CodeQL failure is a config warning -- two analyses present on main (/language:actions, /language:javascript-typescript) did not run for this PR. Not a finding about this code and not addressed here. Refs #17302
CodeQL still red against 047b266 -- one annotation, artifact_ingestor.py:291, "This expression logs sensitive data (secret) as clear text". The run started 15:15:56, after that commit, so it is a live read and not a stale one. It was flagging my own repair: os.path.splitext(product.storage_path)[1][:16] and it was right to. Two things were wrong with that line. A bound is not a sanitiser. Sixteen characters of attacker-chosen text is still attacker-chosen text -- including newlines and control characters that a log reader renders. I reached for a length limit where the property I needed was provenance. And the taint is real, not an analyser artefact: it propagates through splitext and survives a slice, so the sink genuinely received a value derived from the untrusted string. "CodeQL cannot see my sanitisation" would have been the comfortable reading and it was the wrong one -- there was no sanitisation, only a shortening. An allowlist does not rescue it in this branch, which is worth recording because it is the obvious next suggestion: this code runs precisely when the extension is NOT in _TEXT_EXTENSIONS. "Log it if allowlisted, else a constant" logs the constant every time, so the diagnostic value is zero and the complexity is not. So the derived value is gone. The message already states the reason, and the product id is the join key to the row that holds the path. Nothing logged in this file now derives from storage_path. Mutation-checked after the change, 3 of 3 still caught, so removing the log value did not weaken the containment tests: containment helper returns everything -> 5 failed open the RAW storage_path -> 1 failed drop the None guard -> 1 failed 31 passed across the ingestor, attachment-path and tenant-upload suites. The check-run title reads "2 configurations not found" again while the actual content is the failure annotation underneath -- second time today that title would have sent a reader to a config problem. The annotation route is `gh api repos/<repo>/check-runs/<id>/annotations`. Refs #17302
… shared root (#17302, #17300) Pre-merge review found a path traversal my own test had already "covered", and the test passed for the wrong reason. A filename of `notes.` plus three fullwidth-confusable groups gives an extension that NFKC-normalises to `/../../..`. The destination is built at `<root>/<company>/<item>/<name>`, so three `..` land exactly ON the storage root: 1 group -> <root>/<company>/<item>/x same directory 2 -> <root>/<company>/x escapes the work item 3 -> <root>/x escapes the TENANT, on the root 4, 5 -> above <root> escapes the root entirely `validate_path(..., allowed_roots=(root,))` accepted depth 3: the resolved path really is inside the root. The file landed outside its tenant's directory while being, technically, contained. My test used FIVE groups. Five climbs above the root and is refused by a root-level check, so `pytest.raises(ValueError)` passed -- against the depths that escape the root, never against the depth that stops on it. The defect sat one repetition away from the assertion that was supposed to cover it. Verified against real code, not inferred: the reviewer's exact filename returned `<root>/x` with `is_relative_to(root)` True. Two changes. Containment is now to the TENANT directory, and the result must be a DIRECT CHILD of it -- depth 1 resolves to the tenant directory itself, which `allowed_roots` accepts, and a directory is not a file path. The test is parametrised over depths 1-5 instead of one depth, because the depth is what decides where the file lands. Recorded honestly: a mutation shows `allowed_roots=(tenant_dir,)` is NOT independently observable -- widening it back to the storage root leaves every test passing, because a direct child of tenant_dir is necessarily inside it. The direct-child check is the load-bearing one. Both are kept so removing either leaves a containment check standing, and the comment says which is which rather than implying both are proven. Mutation-checked: dropping the direct-child check fails the traversal test by name. 50 passed across the attachment-path, attachments, tenant-upload and ingestor suites. The second review thread on this PR is the cross-tenant IDOR, already filed as #17416 before the review raised it -- out of #17302's mandatory ACs and not folded in here. Refs #17302, #17300
6925815 to
540e572
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: ec35c030-0778-4c1b-9af6-a8391cdacf0c
📒 Files selected for processing (3)
autobot-backend/llc/kb/artifact_ingestor.pyautobot-backend/llc/services/attachment_path_17300_test.pyautobot-backend/llc/services/attachment_service.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
2 participants
Closes the read-side sibling of #17300, and delivers the one #17300 criterion that #17301 did not.
Thinking Path
storage_pathis a free-form optional string on the agent request model (llc/api/agent_api.py:256), stored unvalidated and opened later by the ingestor. Nothing in this codebase ever produces one — grep shows the field is only ever set from a caller's request body — so an authenticated agent could name any path on the backend host and have its contents read into the knowledge index, where they become retrievable.The only gate between the two was
_is_text_path, an extension allowlist. That is the near-miss worth naming, and it is the same shape as #17300: validation existed, and it was not the validation that was needed. "Is this a shape we can parse" was standing in for "are we allowed to read this", and the two questions do not overlap at all.Correcting the issue's threat description while confirming it
#17302 names
.jsoncredential files and.csvas admitted extensions. Neither is in_TEXT_EXTENSIONS. I found this because my first test used.jsonand was skipped as binary — it would have passed while reproducing nothing.The finding stands unchanged in severity: the real list admits
.ini,.cfg,.yaml,.toml,.sh,.py,.sql,.mdand more. Only the examples were wrong, and a test written from them would have been decorative.What Changed
llc/storage_root.py(new) — one definition of the root and its env var. The ingestor needs the same root the attachment writer uses, and a second copy ofLLC_STORAGE_PATHand its default is how the two drift apart. Read at call time, not import.attachment_servicekeeps its_LOCAL_STORAGE_PATHmodule attribute, which existing tests patch directly.llc/kb/artifact_ingestor.py—_contained_storage_pathresolves throughvalidate_pathwith an explicit root and fails closed. It returns the validated string, and the call site opens that rather than the raw input —THREAT_MODEL.md§1's "the validated string is the string used"._is_text_path's docstring now says what it is not.llc/services/attachment_service.py— security(llc): an authenticated user can write a file anywhere on disk via the attachment company_id query param #17300 AC3, the one criterion fix(security): close an arbitrary file write and two path-injection holes (#17300) #17301 left unmet as written._storage_pathenforced containment with a localis_relative_torather than the house helper. Same property, but the safety was load-bearing on the UUID parse two lines up and nothing said so; it now routes throughvalidate_path.All 16 existing security(llc): an authenticated user can write a file anywhere on disk via the attachment company_id query param #17300 tests pass unchanged, which is the evidence that this is an adoption change and not a behaviour change.Retracted — review found this wrong. The 16 pass and do not cover the input class that changed:validate_pathdecodes (%XX) and NFKC-folds before resolving, which the old check did not, so a%-or-confusable extension now persists differently or is refused. Documented in two new tests, and the previously-uncaughtValueErroris now a 400 rather than a 500. See the review response comment. Detail and the alternative (record it inARCHITECTURE_EXCEPTIONS.mdinstead) are in #17300's AC verification.Verification
The first mutation pass failed, and that is the part worth reading
Three mutations, two survived:
My tests covered the helper and not the wiring — the same defect I had recorded as a lesson an hour earlier, on a different PR, reproduced immediately. The cause was concrete: every test used paths whose raw and validated forms are identical, so which string is opened was never observable. "It passes" and "it is attached" looked the same.
Two tests added to make the difference observable:
_canonicalizedecodes%73ub→subbefore resolving, so the validated path exists and the raw string does not. Open the wrong one and the content is lost. Binds M1.Noneguard, a refused path falls through toopen(None), raisesTypeError, and is swallowed by the broadexcept Exception. Identical return value; the difference is entirely what the operator is told, and "could not read this file" sends the reader looking for a missing file rather than a rejected one. Binds M3.All three now caught, each by its own test:
storage_pathtest_the_path_opened_is_the_decoded_one_not_the_raw_string(only)Noneguardtest_a_refusal_is_a_clean_skip_not_a_swallowed_exception(only)The refusal tests assert on what was opened, via a spy on
builtins.open, not on the return value: a refusal that reads the file and then returnsNoneis indistinguishable from one that never read it, if you only check the result. The property is that the bytes are never touched.A guard I hit and did not paper over
Block sync I/O calls inside async def (#7444)refused my test file —write_textinsideasync def test_.... Its escape hatch is# noqa: async_blocking_io, documented as "if the call genuinely runs in a thread (rare)". Mine does not, so thenoqawould have asserted something untrue. The setup moved into a plain sync_plant()helper instead: those writes are arrangement, not behaviour, and had no reason to be inside the coroutine.Not verified here
storage_pathremains caller-supplied — #17302's AC2 prefers the service derive it, as #17300 made the attachment route derive fromctx.org_id. There is no producer to derive from: no code path writes a work-product file and records its path, so there is nothing to mirror yet. Containment is enforced regardless of origin, which is what makes that a later cleanup rather than a gap in this fix. Stated rather than silently skipped.No live host exercise; this is covered by unit tests against a real temp-dir root, not by an observed ingest on
/opt/autobot.Model Used
Claude Opus 5 (1M context)
Closes #17302
Refs #17300
Summary by CodeRabbit