Repository navigation
fix(security): close an arbitrary file write and two path-injection holes (#17300) - #17301
Merged
Merged
Conversation
…oles (#17300) Found by triaging the 111 open CodeQL alerts that block #16801. 78 were verified false positives and dismissed with written reasons; these are three of the seven that were real. 1. ARBITRARY FILE WRITE, llc attachments. _storage_path built `root / company_id / ...` from an unvalidated Query parameter. This was never a traversal bug: pathlib DISCARDS everything left of an absolute segment, so company_id="/tmp/evil" replaced the root rather than climbing out of it. Verified by running it: root / "/tmp/evil" / wi / a.txt -> /tmp/evil/wi/a.txt The uuid.UUID(company_id) cast that would have caught it ran three lines AFTER mkdir + write_bytes + read_text. The ValueError aborted only the DB insert; the bytes were already on disk, in a directory the caller chose. Every id is now parsed to a UUID inside _storage_path, before anything touches the filesystem, with a redundant containment assertion so the property is checkable without first reasoning about what uuid.UUID rejects. The attacker-supplied extension is bounded rather than trusted. It was also a tenancy break independent of the write: the route's IDOR guard proves the work item belongs to ctx.org_id, then passed the QUERY-STRING company_id to build the path, so the storage tree could disagree with the check that had just passed. The route now passes ctx.org_id. 2. manifest_loader joined role_name as its own path segment with no validation, from a `role` query param and a `role_name` body field: role_name="../../../../etc" walked out of the infra base before exists() and open(). Allowlisted to the shape an ansible role name actually has, plus the same redundant containment check. Returns None rather than raising, which is the caller's existing "no manifest for this role" path and the honest answer for a name that cannot be a role. 3. code_source returned a typo suggestion computed by listing an arbitrary directory -- handing any authenticated user one real filename from anywhere on the host. These routes are gated by get_current_user only, NOT by an admin role, which is what made it worth fixing rather than accepting. The suggestion is still computed and logged for whoever is debugging; it no longer travels in the response. Tests assert on the resulting PATH, not on an exception type: what matters is that nothing outside the root is addressable, not that bad input is refused loudly. One test pins the pathlib mechanism itself, so a later "fix" that checks for ".." instead of parsing the UUID fails on the case that actually mattered. 29 tests, each parametrised set carrying a positive case so a guard that rejected everything could not pass. Both touched API files sit exactly on their size ceilings, so the explanatory comments are one line each rather than the paragraph this deserves; the reasoning lives in the tests and in #17300. Refs #17300 Refs #16801
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 12 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 ignored due to path filters (2)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe changes validate attachment and manifest filesystem paths, use the authenticated organisation ID for attachment storage, and remove local repository path suggestions from HTTP 400 details. ChangesAttachment storage paths
Manifest role-name paths
Local repository path responses
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to The path hardening works as intended, but one existing attachment test will now fail in the standard test run until its fixture uses a UUID. Malformed uploader IDs can also still leave orphaned files on disk. Fix both before merging. The remaining remote path-suggestion leak and the role-name regex gap are small follow-ups. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
…17300) The last two of the seven real CodeQL findings, appended here rather than opened as a second PR - same scope, same review, one CI run. companies.py:741 returned str(exc) to the client from a BARE except Exception, over a pm_config that had just been decrypted. An HTTP-client error routinely embeds the URL it was handed, and that URL carries the integration's credentials. Unlike the knowledge_mcp sites - which catch narrow types and fall back to a generic message, and which I dismissed as false positives on exactly that basis - this one caught everything and forwarded it. Logged in full server-side, generic to the caller. system_secrets_vault.py:166 logged the secret NAME on a read failure, forty lines below a sibling function that deliberately refuses to log the same value and documents why ("CodeQL traces it from a secret source into this sink"). Not a leak of a value, but the file disagreed with itself, and the caller already knows which key it asked for - the exception type is what this line adds. Now matches its sibling. companies.py sits on its size ceiling, so the line the log statement needs was reclaimed by collapsing an adjacent four-line HTTPException that fits on one within the line limit. Ceiling lowered 1732 -> 1731 to match. I briefly raised it to 1734 instead, which is backwards - a ratchet only turns down - and reverted before committing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Suppress remote path suggestions in assign_code_source. · code_source.py:270
autobot-slm-backend/api/code_source.py:270
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick winInformation Disclosure
Reachability: External
Exploitability: Moderate
CWE: CWE-200 — Exposure of Sensitive Information to an Unauthorized ActorSuppress remote path suggestions in
assign_code_source.The router requires
Permission.SERVICE_MANAGEMENT, so this route is not available to every authenticated user. Service-management users can still receivesimilar_path, which discloses a remote repository path in the HTTP 400 detail. Remove the remote suggestion.Remove the remote path suggestion
- similar_path = await _find_similar_paths(node, repo_path) - error_detail = f"Repository path does not exist on source node: {repo_path}" - if similar_path: - error_detail += f". Did you mean: {similar_path}?"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: b792c3ab-6df7-42d3-b70b-ad75e1d25ce2
📒 Files selected for processing (6)
autobot-backend/llc/api/work_items.pyautobot-backend/llc/services/attachment_path_17300_test.pyautobot-backend/llc/services/attachment_service.pyautobot-slm-backend/api/code_source.pyautobot-slm-backend/services/manifest_loader.pyautobot-slm-backend/tests/services/manifest_role_name_17300_test.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
5 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
5 tasks
…roved nothing (#17300) A pre-merge review pass found four things. Three are fixed here; the fourth is filed as #17302. 1. MY OWN TEST WAS A FALSE NEGATIVE. test_a_refused_name_never_reaches_the _filesystem planted `name: pwned` as the manifest to be traversed to -- which fails RoleManifest validation, which _load_from_disk swallows in its generic `except Exception` and turns into None. So the test passed whether or not the guard existed: it was asserting that pydantic rejects a malformed file, not that traversal was blocked. Confirmed by reverting the guard and watching it stay green. The fixture is now a schema-valid manifest, so None can only mean the guard fired. Mutation-checked the other way too: with the guard removed the traversal now LOADS the planted manifest and the test fails. A control test pins that the fixture really does validate, so it cannot drift back into being invalid and silently restore the false negative. This is the exact defect class I have been flagging all session, in my own work, in a security test. A test that passes with the fix reverted is worse than no test: it certifies the thing it never checked. 2. code_source was HALF fixed. I removed the "Did you mean: <path>?" disclosure from the local-node branch and left the identical line in the remote/SSH branch twenty lines below -- same leak, same rule, a real filename from any directory on the remote node. Both branches now log rather than tell. Ceiling lowered 720 -> 719. 3. The route fix had NO test. Reverting work_items.py's `company_id=str(ctx.org_id)` back to the query parameter passed the entire existing suite, because test_work_items_idor.py mocks upload wholesale and never inspects its kwargs. Three tests added in their own file (that one is at its size ceiling): the service receives the authenticated org and not a different well-formed UUID from the query string, the absolute exploit string never reaches the service, and cross-tenant upload is still refused -- the contrast, without which a route that ignored company_id entirely would pass. Mutation-checked: 2 of 3 fail with the fix reverted. Filed #17302 for the finding outside this diff: WorkProduct.storage_path is an agent-supplied free-form string opened directly by ArtifactIngestor, gated only by an extension allowlist with no containment check -- arbitrary file read, and the sink is an indexer, so the disclosure persists in the KB. Refs #17300 Refs #17302
…nput (#17300) Four more review findings, and two of them say the same thing from different directions: my manifest fix was correct and not PROVABLY correct. CodeQL still flagged manifest_loader.py:93 and :104 after the allowlist went in (alerts 1152, 1153) -- it does not recognise a regex as a sanitiser, and it was right not to, because the property depended on reasoning about what the regex excludes rather than on anything structural. _manifest_path now builds the path from a DIRECTORY LISTING: the argument only selects an entry that already exists under the infra base. There is no string concatenation left for an attacker to steer, so traversal is impossible rather than filtered, and impossible in a way a reader (and a scanner) can see. The allowlist and the containment check stay as two cheaper filters in front. The allowlist itself was wrong: `match()` with a trailing `$` also matches before a final newline, so "backend\n" was accepted. `fullmatch()` now, and the case is in the rejected set. The containment check had never been exercised. Every rejected name in the test file fails the ALLOWLIST first, so `is_relative_to` could have been deleted without a single failure -- the same false-negative shape as the fixture defect in the previous commit. A symlinked role directory passes the name check and the listing and is caught only by the containment check; that case is now pinned. Also in this commit, both surfaced by touching the files: - test_attachments.py::test_storage_path_structure passed "att-001" as the attachment id. Production has always passed str(uuid.uuid4()); only the fixture was a stand-in, and _storage_path now parses all three ids. - A pre-existing #7444 violation (Path.write_text inside an async test) in the same file. Fixed rather than exempted. Contract change worth noting for review: a role must now EXIST to be resolvable. A well-formed name for a role with no directory returns None instead of a path that merely happens to be empty. Pinned by its own test.
hooks-path-override 6525 -> 6527 (population 6927), prompt-injection 2955 -> 2956 (population 3256). Both measured on the tree that merges with main. The prompt-injection number is confirmed twice -- CI's figure and a direct git ls-files count -- because a local run of that floor only became trustworthy after #17298 made the sweep import declarations living outside repo_tests/. Before that it checked a smaller set and reported it identically. Twelfth re-pin of hooks-path-override, third today. This branch contributes two counting files, both security regression tests; main had already spent the rest of the allowance. Recorded again on #17142: at this cadence the fixed 400-file growth allowance -- 5.8% of a ~6900-file tree -- is what needs changing, not the floor.
Review finding, and it is my own defect repeated inside the same function. I moved the validation for the three ids that FORM the path and left the two that do not -- uploaded_by_agent_id and uploaded_by_user_id were still cast after _write_local, so a malformed one raised with the file already on disk. Orphaned file per attempt, repeatable, which is a disk filler. The original bug was "validation ran after the write". I fixed it for the parameters I was looking at and reproduced it for the parameters I was not. All four ids are parsed before anything touches the filesystem now. Tests assert on the DIRECTORY being empty, not on the exception: ValueError was already raised before this change, so asserting the raise would have passed throughout. What changed is that nothing is written. Plus the contrast -- a well-formed upload still writes its bytes -- so a change that refused every upload could not pass. Mutation-checked: reverting the early parse fails them. The contrast test tripped #7444 (Path.read_bytes inside an async def); wrapped in asyncio.to_thread rather than exempted, same as the earlier occurrence.
mrveiss
added a commit
that referenced
this pull request
Sep 23, 2026
…453) The last two of the nine open CodeQL alerts. Folded in here per the owner rather than opened as a third security PR. Seven of the nine were already fixed in main by #17301 and cleared on the rescan that has since run (9 -> 2 open). These two did not, and I should have seen that when I wrote the first fix: I removed the "Did you mean: <path>?" DISCLOSURE and left the sinks. repo_path still reached Path(...).is_dir() and .iterdir() unconfined, so any authenticated user could still test whether an arbitrary path exists on the host. The oracle was quieter, not closed. repo_path now goes through validate_path with roots from AUTOBOT_CODE_SOURCE_ALLOWED_ROOTS. Unset defers to path_validator's own default rather than restating "/opt/autobot" here -- a second copy is a value that can drift from the one enforced, which is this PR's subject one layer down. The hardcoded-values guard caught the literal in my first attempt. Why confinement rather than an admin role: these routes are gated by get_current_user only, and NO admin-role dependency exists anywhere in autobot-slm-backend/api/. Adding one is a new authorisation tier for that service -- an architecture decision, not a security patch. Three existing tests used /tmp paths and started failing, correctly. They now declare their root via the env var rather than the guard being weakened for them. test_validate_local_path_not_exists moved inside the allowed root on purpose: "/nonexistent/path" trips confinement first and returns a different 400, so the test would have silently stopped exercising the does-not-exist branch it is named for. Helpers live in api/_code_source_paths.py because code_source.py is grandfathered at 719 lines and may not grow; it is back at exactly 719. Refs #17300
mrveiss
added a commit
that referenced
this pull request
Sep 23, 2026
…ed (#15333) #17301 merged and CodeQL's rescan of main closed the seven alerts it fixed, so the count went 9 -> 2 and this PR's own gate went red: [codeql-ceiling] 2 open alert(s), ceiling 9 [codeql-ceiling] FAIL: 2 open alert(s) is BELOW the ceiling of 9. Which is the behaviour it was written for, arriving faster than expected. A ceiling left above the real count re-licenses exactly the alerts that were just adjudicated -- the next finding lands in the slack and nothing goes red. The gate refused its own stale number before anyone had to notice. The two that remain are the code_source.py path-injection pair, fixed in #17304; that PR takes this to 0.
mrveiss
added a commit
that referenced
this pull request
Sep 23, 2026
…T and confine repo_path (#17304, #17300) (#17309) * fix(deps): move the six Dependabot packages into the constraints SSOT (#17304) 58 open Dependabot alerts were six packages. The count was duplication: one advisory seen through several resolution paths, and 48 of the 58 attributed to pyproject.toml, which declares no dependencies at all -- it is [tool.black], [tool.isort] and [tool.mypy]. What was actually exposed was two manifests, and they were exposed by DRIFT: aiohttp had been raised to the patched 3.14.3 in five requirements files and left at 3.13.4 in two others. Nothing compares a pin against the floor its siblings already adopted, so a security bump applied to "the requirements file" missed two of the seven that declare it. The fix is adoption, not invention. constraints/shared.txt has been the single source of truth since #10524 and the constraint-drift workflow already fails any PR that re-declares a constrained version -- it simply constrained two packages, numpy and openvino. These six were never brought in, which is the whole reason they could drift. Now guarded: aiohttp, chromadb, cryptography, nltk, numpy, openvino, pypdf, transformers. Versions removed from 15 component files; three files that declared a constrained package without referencing the constraints at all (requirements.txt, docs/guides/requirements-local.txt, autobot-backend/requirements-media.txt) now reference it, as do the requirements-ci fragments, which their own header documents as individually installable. Verified rather than assumed: * every -c target resolves from the file that declares it; * no floor regressed -- each package's constraint equals the highest pin that existed anywhere before, checked against origin/main; * check_constraint_drift.py passes. chromadb stays at >=1.5.9,<2.0.0 with 11 advisories and NO patched release upstream. Constrained anyway so the version moves in one place the day a fix ships. Three nltk advisories are likewise unpatched. Those 14 cannot be resolved by bumping and need a dismissal decision. Refs #17304 * fix(security): confine the code-source repo_path (#17300, CodeQL 452/453) The last two of the nine open CodeQL alerts. Folded in here per the owner rather than opened as a third security PR. Seven of the nine were already fixed in main by #17301 and cleared on the rescan that has since run (9 -> 2 open). These two did not, and I should have seen that when I wrote the first fix: I removed the "Did you mean: <path>?" DISCLOSURE and left the sinks. repo_path still reached Path(...).is_dir() and .iterdir() unconfined, so any authenticated user could still test whether an arbitrary path exists on the host. The oracle was quieter, not closed. repo_path now goes through validate_path with roots from AUTOBOT_CODE_SOURCE_ALLOWED_ROOTS. Unset defers to path_validator's own default rather than restating "/opt/autobot" here -- a second copy is a value that can drift from the one enforced, which is this PR's subject one layer down. The hardcoded-values guard caught the literal in my first attempt. Why confinement rather than an admin role: these routes are gated by get_current_user only, and NO admin-role dependency exists anywhere in autobot-slm-backend/api/. Adding one is a new authorisation tier for that service -- an architecture decision, not a security patch. Three existing tests used /tmp paths and started failing, correctly. They now declare their root via the env var rather than the guard being weakened for them. test_validate_local_path_not_exists moved inside the allowed root on purpose: "/nonexistent/path" trips confinement first and returns a different 400, so the test would have silently stopped exercising the does-not-exist branch it is named for. Helpers live in api/_code_source_paths.py because code_source.py is grandfathered at 719 lines and may not grow; it is back at exactly 719. Refs #17300 * fix(deps): back out the constraints consolidation; the SSOT cannot reach ansible (#17304) Three guards failed and two of them were right about something I had not checked. Reverting the consolidation and keeping the security fix. WHAT THE GUARDS FOUND windows_npu_worker_transformers_bound_test: I turned `transformers==5.16.1` into a bare name, which is exactly the regression #15298 fixed. That file is pinned to an EXACT version on purpose -- an unbounded floor there is "one dependency bump away from being affected, silently", in its own words. My change would have let a fresh resolve land on 5.17+ on the one platform that was deliberately held back. ansible_requirements_parity_test: five ansible sites restate these versions, and the guard's advice is to pass `-c <checkout>/constraints/shared.txt` instead, "as roles/npu-worker and roles/tts-worker do". That advice does not transfer. Neither dependency_patching nor redis/tasks/chromadb.yml references code_source_dir or /opt/autobot at ALL: the first patches whatever venv host_venv_path names on whatever host, the second builds a standalone venv on the redis host. Adding a `-c` pointing into a checkout that may not exist there would turn a security-patching role into a failing one -- strictly worse than the duplication it removes. THE ACTUAL LIMIT, which is worth recording constraints/shared.txt works for components installed FROM the checkout. numpy and openvino are in it because their consumers are exactly that. It structurally cannot serve a role that builds a standalone venv on an arbitrary host, and all five packages I moved have such a site. "Defined in a single place" is right as a goal and is not reachable by this mechanism for these packages. WHAT SURVIVES The actual vulnerability: aiohttp raised from >=3.13.4 to >=3.14.3 in docs/guides/requirements-local.txt and the windows npu worker -- the two of seven manifests that a previous security bump missed. All seven now agree. Plus the repo_path confinement (CodeQL 452/453) unchanged. hooks-path-override floor 6527 -> 6529, thirteenth re-pin, fourth in two days.
mrveiss
added a commit
that referenced
this pull request
Sep 23, 2026
) (#17303) * feat(ci): the open CodeQL alert backlog may not grow (#15333, #17300) Detection was never the gap. CodeQL had already flagged all three lines of the arbitrary file write found on 2026-09-23 (#17300); the alerts sat in a backlog of 111 because nothing required anyone to look. So this is not another scanner. It is a ceiling on how many findings may sit unadjudicated, and it only comes down. Adjudicated means one of two things, both of which reduce the count: the code changed, or the alert was dismissed WITH A WRITTEN REASON. A dismissal with no reasoning is not adjudication -- the reason is the artifact, the state change is its receipt. Enforced from code-quality.yml rather than codeql.yml, deliberately: code-quality is in branch protection's required contexts and CodeQL is NOT, so today a red CodeQL cannot block a merge. This can. Fails closed on every way of not getting an answer -- no token, API error, unparseable ceiling, missing file. "Could not look" must never render as "found nothing", which is the confusion this whole area is about. It also fails when the count drops BELOW the ceiling, because a ceiling left above the real count re-licenses exactly the alerts that were just adjudicated. Mutation-checked rather than reasoned about. The script: ceiling too high, too low, file missing, API unreachable -- four paths, four distinct failures. The repo_test that keeps it wired: removing the invocation, dropping security-events: read, and adding continue-on-error are each caught. Pinned at 9, the current count -- all nine are the sites fixed in #17301 and clear when it merges. The number comes down in that same change. Context for whoever reads this next: two OTHER security gates in this repository have never executed once. ai-security-review needs CLAUDE_CODE_OAUTH_TOKEN and lint-conventions' third-party-name check needs CONVENTIONS_DENYLIST; `gh secret list` returns nothing, so both emit a notice and skip. Those need credentials, not code, and are recorded on #15333. Refs #15333 Refs #17300 * fix(ci): the alert-ceiling gate could be fooled the way it exists to prevent (#15333) Three review findings, and the middle one is the gate failing open in exactly the manner it was written to stop. `jq -s 'add | length'` returns a number for ANY JSON, not just a list. An API error body with nine keys counted as nine alerts -- and at a ceiling of nine, PASSED. Verified: $ echo '{"message":"Not Found", ...9 keys...}' | jq -s 'add | length' 9 So a script whose whole argument is that "could not look" must never render as "found nothing" was doing precisely that to itself. Every page must now BE an array before anything is counted, and a non-array body fails closed naming the reason. The ceiling parser took `head -1`, so a file with a second number silently below the first was accepted, and `tr -d '[:space:]'` turned "9 0" into "90". Both make a malformed ceiling MORE permissive, which is the direction that matters. Exactly one value now, and only OUTER whitespace is trimmed -- "9 0" is rejected as not a number rather than quietly becoming ninety. The wiring guard read only the live tree, which proves what the repository looks like today and nothing about whether the guard would notice a change. Each check is now a predicate with a positive and a negative fixture. I had mutation-tested these by hand and reported it; a hand-run once is not the same as a case that runs every time, and the difference is the entire subject of this PR. Mutation-checked: two values, zero values, "9 0", an error body from a stubbed gh -- four distinct failures, none of them a pass. * fix(ci): lower the alert ceiling 9 -> 2, which the gate itself demanded (#15333) #17301 merged and CodeQL's rescan of main closed the seven alerts it fixed, so the count went 9 -> 2 and this PR's own gate went red: [codeql-ceiling] 2 open alert(s), ceiling 9 [codeql-ceiling] FAIL: 2 open alert(s) is BELOW the ceiling of 9. Which is the behaviour it was written for, arriving faster than expected. A ceiling left above the real count re-licenses exactly the alerts that were just adjudicated -- the next finding lands in the slack and nothing goes red. The gate refused its own stale number before anyone had to notice. The two that remain are the code_source.py path-injection pair, fixed in #17304; that PR takes this to 0. * fix(ci): cover the ceiling script in the python filter, and stop a fixture posing as a read (#15333) Two guard failures, one of them mine twice over. python_filter_covers_its_guards: codeql_alert_ceiling_is_wired_15333_test.py reads pipeline-scripts/check_codeql_alert_ceiling.sh by literal, but the python-suite path filter did not cover that file -- so a change confined to the script alone would take the required-context shim's green while the guard that watches it never ran. Filter widened. The same guard also flagged "./pipeline-scripts/check_codeql_alert_ceiling.sh", which is not a read at all: it is the quoted path inside my own test FIXTURE, picked up by the detector's quoted-literal scan. Over-collecting is the detector's documented and deliberate posture, so the fix belongs on my side -- the fixture now spells the invocation without the "./" prefix, which is also the form the real filter entry matches. The predicates search for the basename, so nothing about what the test proves changes. hooks-path-override floor 6527 -> 6529, measured 6929. Notable only because #17304 needed the identical re-pin an hour ago on its own branch: two open branches, same floor, same number, neither responsible. That is #17142's argument with a second data point. * fix(ci): validate the PR title that becomes the squash subject (#15473) A squash merge takes its commit subject from the PR title, and nothing validated the title. `fix(deps+security): ...` passed every local hook, merged, and put a subject on main that lint-conventions.sh then rejected for every PR opened afterwards -- including the release promotion, which carries 682 commits. Local commit subjects were guarded at commit-msg time; the one subject a human never types directly was not. The rule now has ONE source. scripts/lib/commit-subject.ere holds the ERE; lint-conventions.sh reads it for both its commit-msg and range checks, and validate_pr_body.py compiles the same file for the new title gate. #15473 asked for exactly this -- a second copy would have rebuilt the defect one layer up, two enforcers of one rule free to drift. Both readers fail closed: a rule file that is missing, empty, or carrying more than one pattern is fatal, never a subject that passes. The gate runs in pr-template-check, whose triggers include `edited` -- the only event that sees a title changed after the last push, and the title at merge time is what becomes the subject. `--title-only` treats an empty title as fatal there, because CI always has one, so empty means the lookup failed. In --file mode empty still means "no PR yet", which is the honest answer before `gh pr create`. Commit 59f4be8 cannot be amended, so it is exempted by SHA rather than by widening the pattern: `+` is not a scope separator this repo uses -- one occurrence in 600 commits and it is that one. The exemption is written to seven characters because CI's %h abbreviates against a three-object shallow store; the ten-character form read off a full clone would have matched where it was authored and missed where it has to fire. Two tests pin that: no exemption longer than CI's abbreviation, and every exemption resolving to exactly one object. CodeQL ceiling 2 -> 0. Measured, not assumed: state=open returned 0 while state=dismissed returned 393 in the same query, so the zero is a count and not a query that failed. Found while fixing this and filed rather than folded in: #17310 -- check 3 runs at fetch-depth 2, so it walks one commit and reports that as a clean scan of the whole range. Closes #15473 * fix(ci): register the conventions linter and its rule in the path filters (#15473) Found while adding the shared rule file: editing `scripts/lint-conventions.sh` triggered nothing. It is not in `code-quality.yml`'s `paths:`, so the workflow that RUNS it never started, and `code-quality-required-context.yml` then published the required `code-quality` context green -- on a linter no job had executed. It is not in `python-paths.yml` either, so its own 34 tests did not run on a change confined to it. `scripts/lib/commit-subject.ere` had the second half of the same gap. The workflow trigger and the per-job filter both carry `scripts/lib/**`, so the LINTER runs when the rule changes; nothing matched it in `python-paths.yml`, so the TESTS asserting the two readers agree did not. Those are different questions and they need separate entries. This is the #16249 shape one level out: `**/*.py` does not match a `.sh`, the suite computes `python != 'true'`, and the shim reports green. check_workflow_path_filters.py agrees the inline list and the canonical set are still in step -- 26 entries, all carried. Refs #15473 * fix(ci): do not judge a promotion PR's title as a squash subject (#15473) Caught by running the new gate against the open release-promotion PR before merging it, rather than after: it went red on `chore(release): promote main to release`, and it was the gate that was wrong. A main -> release promotion is merged, not squashed. The last one landed as a merge commit (b94f51a, two parents, `Merge pull request #15326 from ...`) -- a subject GitHub composes and lint-conventions.sh exempts by prefix. The title never becomes a subject there, so a promotion title correctly carries no issue reference: a promotion is not an issue fix. Both enforcers skip it, and a test asserts they agree. The workflow job adds `base.ref == 'main'`; pr_fields now reads baseRefName and validate() takes `base`. If only one skipped, a promotion PR would be red in exactly one place -- and the wrong one is whichever nobody is looking at. The default is "main", not "skip". --file mode has no PR to read a base from, and a default that skipped would leave every ordinary PR unguarded while still looking enforced. The residual case costs nothing: were a promotion squash-merged after all, that subject lands on `release`, and the range check reads release..main, so a subject ON release is never in a scanned range. Verification -- each mutation killed its test: drop the promotion skip -> FAILED test_a_promotion_pr_skips... default base -> 'release' -> FAILED x2 (the skip is not the default) workflow loses base.ref -> FAILED test_the_workflow_job_skips... $ python3 scripts/validate_pr_body.py --pr 16801 PR title: not checked -- base is 'release', not 'main'. ... exit=0 Refs #15473 * fix(ci): full-sha exemptions, a CI-safe existence check, and no fork skip (#15473) Review findings on the squash-subject gate, all confirmed before acting. THE EXEMPTION IS NOW A FULL SHA. The abbreviated form was wrong twice over. Ten characters, read off a full clone, missed in CI's shallow checkout, which prints seven -- that one I caught. Seven fixed it and introduced the other half: `continue` skips BOTH the format check and the issue-reference check, so a prefix that ever collided would exempt an unrelated commit from the whole of check 3. `git log` now carries %H beside %h; %h is kept only for the message. THE EXISTENCE TEST WOULD HAVE REDDENED CI. It ran `rev-parse --disambiguate=` against the repository, and ci.yml's python-shard (215) and coverage.yml (81) both check out with a bare actions/checkout@v7 -- fetch-depth 1. The object is two hops back, so it is absent, and `--disambiguate=` exits 0 with empty output for a missing object: indistinguishable from a dead exemption. The test would have asserted 0 == 1 and failed red for a reason unrelated to the change under test, which is precisely the defect this branch exists to fix. It now uses `cat-file -e`, which can tell those apart, and SKIPS with a stated reason on a shallow store rather than asserting through it. Both mechanisms are pinned rather than assumed: one test builds a shallow repo by writing .git/shallow and asserts the detection fires, another asserts `cat-file -e` really does fail on an absent object. NO FORK SKIP. check-title had copied check-template's, and the two guard different things. Skipping forks is right for a template convention a drive-by contributor was never shown (#14153); it is wrong for a squash subject that becomes permanent and unamendable, where the population least familiar with the convention is exactly the one going unchecked. The justification for skipping -- an owner can edit the title at merge -- is the same unenforced manual step whose absence caused the incident. THE PARSER CANNOT PASS VACUOUSLY. Both exemption tests iterate over what the regex finds, so a drifted pattern turned them green. Now counted against an independent loose scan. BASH FAIL-CLOSED IS TESTED, not just inspected. "Both readers fail closed" was asserted on the python side and true only by reading on the other. Four cases: missing, empty, comment-only, two patterns. ONE TABLE, BOTH ENGINES. Each side hard-coded its own expectations, so a future edit to commit-subject.ere introducing an engine-divergent construct would be caught only if someone updated both tables identically -- which is the drift a shared rule file exists to remove. Verification -- every mutation killed its test: exemption back to a 7-char glob -> FAILED parser_finds_what_is_there bash guards replaced by a default -> FAILED all 4 unusable_rule_file `(?:` into the shared rule -> FAILED 4 grep_and_python_agree (non-capturing group in python, a literal in ERE) 96 passed (lint_conventions_test + validate_pr_body_test) Verified and NOT changed: main's required contexts are the 11 listed by `gh api .../branches/main/protection`, and pr-template-check is not among them, so this gate shows red without blocking. Adding it is a branch-protection change, which is the owner's, not a PR's. Refs #15473 * docs(ci): drop the superseded exemption rationale (#15473) Second review pass. The full-sha change spliced in a new rationale paragraph starting at "Seven characters, not the ten..." and left the paragraph above it, so the file carried both "Recorded by SHA" and "Recorded by FULL sha" -- the first describing the mechanism the same commit had just replaced. Editing debris, no functional effect, and exactly the kind a reader trips over while trying to work out which rule is live. The rest of that pass was clean and independently reproduced rather than taken on trust: the 5-field parse verified by running the script over two real ranges, the exemption confirmed to be doing real work (the same subject fed through the shared ERE directly gives NO_MATCH), the shallow skip confirmed not to mask any checkable environment -- `shallow` is a shared/common git file, so a linked worktree reads the same state as the main checkout -- and every new test reproduced as non-vacuous. One caveat recorded rather than resolved: fork-PR behaviour of check-title rests on documented token semantics (public repo, read-only token can GET pull request metadata) plus the safety net that the job is not a required context. No fork PR existed to observe it on, so the first real one should be eyeballed. Refs #15473 * fix(test): derive the commit-msg fixture's dependencies, re-pin the reach floor (#15473) Shards 11/12 and 12/12, two unrelated causes. SHARD 11 -- repo_tests/commit_msg_hook_17029_test.py listed the files its throwaway repo needs as a hardcoded tuple of three. Adding lib/commit-subject.ere to what lint-conventions.sh reads made that list stale, so the fixture built something that was not a checkout. The script then refused to run, correctly and loudly ("FATAL cannot read the commit-subject rule"), and six tests read that refusal as the hook being broken. The list is now derived from the script, which is what scripts/lint_conventions_test.py already does for its own fixture and for this exact reason (#15245). Two fixtures with the same shape, one of them fixed and one not; the second one is the one that broke. Mutation-checked: narrowing the derivation back to *.sh reproduces the CI failure (7 failed), widening it restores 8 passed. SHARD 12 -- reach floor 6529 -> 6531, measured 6931. FOURTEENTH re-pin, fifth in two days. This branch adds exactly two counting files out of 401 of allowance; main had consumed the other 399 before the branch existed. Recorded on #17142 rather than absorbed silently again: five re-pins in 48 hours, every one this same shape, is a growth allowance that does not describe how fast this tree adds files, not a floor that keeps being set wrong. 64 passed (reach_declarations + hooks_path_override + commit_msg_hook) Refs #15473, #17142 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Triaging the 111 open CodeQL alerts blocking #16801 turned up seven genuine
py/path-injectionfindings among 78 verified false positives. Three are fixed here; they share a shape worth naming.Each had validation — running too late, or on the wrong value. The attachment path parsed
company_idas a UUID three lines after writing the file. The work-item route ran a correct IDOR guard againstctx.org_id, then built the storage path from a query parameter nothing had tied to it. This is not "validation was missing", it is "validation was present and not load-bearing", which is why four review passes over that code never flagged it.The attachment case also is not the bug it looks like. It is not traversal:
pathlibdiscards everything left of an absolute segment, so an absolutecompany_idreplaces the storage root rather than climbing out of it. No amount of".." not in valuewould have caught it. That is why the test suite pins the pathlib mechanism itself and not just the outcome.What Changed
llc/services/attachment_service.py— arbitrary file write (high)_storage_pathnow parses every id to a UUID before returning, so a value that is not a UUID can never become a path segment, and asserts containment under the resolved root — deliberately redundant, so the property is checkable without first reasoning about whatuuid.UUIDrejects. The attacker-supplied extension is bounded rather than trusted.llc/api/work_items.pypassesctx.org_idinstead of the query-stringcompany_id. The parameter stays in the signature for API compatibility; it is simply no longer authoritative. Before this, one tenant could write into another's attachment tree by passing their id — a tenancy break independent of the traversal.autobot-slm-backend/services/manifest_loader.py— traversalrole_name(arolequery param, arole_namebody field) was joined as its own path segment unvalidated;"../../../../etc"walked out of the infra base before bothexists()andopen(). Allowlisted to the shape an ansible role name actually has, plus the same containment check. ReturnsNonerather than raising, which is the caller's existing "no manifest for this role" path and the honest answer for a name that cannot be a role.autobot-slm-backend/api/code_source.py— information disclosureThe "Did you mean: …?" typo suggestion was computed by listing an arbitrary directory and returned to the caller. These routes are gated by
get_current_useronly, not by an admin role, so it handed any authenticated user one real filename from anywhere on the host. Still computed and logged for whoever is debugging a failed assignment; no longer in the response.Verification
test_a_well_formed_upload_lands_under_the_rootandtest_a_real_role_name_resolves_under_the_infra_baseare those cases.test_the_absolute_case_would_have_escaped_without_the_fixpins the pathlib behaviour directly, so a later "fix" that swaps the UUID parse for a".."check fails on the case that actually mattered.autobot-slm-backend/tests/services/test_token_denylist.pyneeds a live Redis (Redis database 'main' not ready) and is unrelated to this change.Not verified: whether any poisoned
storage_pathrows already exist in a live database.download/deletereadrow.storage_pathback, so a row written before this fix would still be honoured. Finding and remediating those is data work, which no agent does unattended — flagged on #17300 for a human decision rather than actioned here.What this does NOT do
Four of the seven real findings remain, all in
autobot-slm-backend. They are listed on #17300. #16801 stays red until they land, which is the gate working correctly.Model Used
Claude Opus 5 (1M context)
Refs #17300
Refs #16801
Summary by CodeRabbit