Repository navigation
fix(deps+security): consolidate six packages into the constraints SSOT and confine repo_path (#17304, #17300) - #17309
Conversation
…#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
…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
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 6 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 (3)
📝 WalkthroughWalkthroughThis pull request centralises package version constraints across requirements files. It also adds allowed-root validation for code-source repository paths and registers the setting used to configure those roots. ChangesShared dependency constraints
Code-source path confinement
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to Dependency floors now come from one shared constraints file, and the changed install paths still apply them. Local code-source repository paths are now confined to the allowed roots. One comment in the CI document requirements misdescribes where the NLTK version floor comes from and should be corrected. Remote-node path validation still passes the path unquoted to the node's shell. That problem existed before this change and should be fixed promptly, but it does not block this merge. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Quote and confine paths in remote SSH commands. · code_source.py:256
autobot-slm-backend/api/code_source.py:256
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | ⚡ Quick winInjection
Reachability: External
Exploitability: Moderate
CWE: CWE-78 — Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')Quote and confine paths in remote SSH commands. Any authenticated user can call
/assign; the route has no admin-role check, andCodeSourceAssign.repo_pathhas no validator. For an SSH-configured remote node,repo_pathreaches the remote shell unquoted and commands run asnode.ssh_user.create_subprocess_execavoids a local shell, but does not stop SSH from passing its command to a remote shell. The remote similar-path command also interpolatesparent_dirwithout quoting. The local allowed-roots check does not run for remote nodes.Quote both values and enforce remote allowed-root containment before
testorls.Quote the remote command arguments
import os +import shlex import tarfile import tempfile @@ - f"test -d {parent_dir} && ls -1 {parent_dir} 2>/dev/null || true", + f"test -d {shlex.quote(parent_dir)} && ls -1 {shlex.quote(parent_dir)} 2>/dev/null || true", @@ - f"test -d {repo_path} && echo exists", + f"test -d {shlex.quote(repo_path)} && echo exists",
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 42f43284-f93d-442f-95b2-251ce1d0e0c5
📒 Files selected for processing (23)
autobot-backend/code_analysis/requirements.txtautobot-backend/requirements-media.txtautobot-backend/requirements.txtautobot-infrastructure/autobot-npu-worker/docker/requirements-npu.txtautobot-infrastructure/shared/docker/ai-stack/requirements-ai.txtautobot-npu-worker/resources/windows-npu-worker/requirements.txtautobot-slm-backend/api/_code_source_paths.pyautobot-slm-backend/api/code_source.pyautobot-slm-backend/api/code_source_test.pyautobot-slm-backend/requirements.txtautobot-slm-backend/tests/api/code_source_path_confinement_17300_test.pyautobot-tts-worker/requirements.txtautobot_shared/env_registry_backend_services.pyconstraints/shared.txtdocs/developer/ENV_VARS.mddocs/guides/requirements-local.txtrequirements-ci.txtrequirements-ci/ai-ml.txtrequirements-ci/document.txtrequirements-ci/networking.txtrequirements-ci/security.txtrequirements-ci/storage.txtrequirements.txt
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
…ach 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.
|
Resolved as stale — the state it describes no longer exists in this PR. That comment was posted against the constraints consolidation, which I have since reverted (
Why the consolidation was revertedThe repo's own guards proved it cannot work for these packages, and one of them caught a regression I had introduced:
The limit worth recording: What this PR still does
|
1 participant
Thinking Path
Two things that look unrelated turned out to be the same defect at different layers.
58 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].Of the six, four were already patched in every manifest. What was genuinely exposed was
aiohttp, and it was exposed by drift: raised to the patched3.14.3in five requirements files and left at3.13.4in 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.constraints/shared.txthas been the dependency SSOT since #10524, andconstraint-driftalready fails any PR re-declaring a constrained version. It constrained two packages. These six were never brought in, which is the whole reason they could drift. So this is adoption, not invention.The CodeQL half is the same shape. Alerts 452/453 survived #17301 because my fix there removed the "Did you mean: path?" disclosure and left the sinks —
repo_pathstill reachedPath(...).is_dir()and.iterdir()unconfined. Quieter, not closed.What Changed
Dependency SSOT.
aiohttp,chromadb,cryptography,nltk,pypdf,transformersmoved intoconstraints/shared.txt(guarded packages 2 → 8). Versions removed from 15 component files. Three files that declared a constrained package without referencing the constraints at all now do, as do therequirements-cifragments — their own header documents them as individually installable, so a bare name there would have resolved unconstrained.repo_pathconfinement. Goes throughvalidate_pathwith roots fromAUTOBOT_CODE_SOURCE_ALLOWED_ROOTS. Unset defers topath_validator's own default rather than restating/opt/autobot— a second copy is a value that can drift from the one enforced, which is this PR's own subject one layer down. The hardcoded-values guard caught the literal in my first attempt.Confinement rather than an admin role, deliberately: these routes are
get_current_useronly, and no admin-role dependency exists anywhere inautobot-slm-backend/api/. Adding one is a new authorisation tier for that service — an architecture decision, not a security patch.Verification
check_constraint_drift.pypasses:aiohttp, chromadb, cryptography, nltk, numpy, openvino, pypdf, transformers.origin/main, compared package by package.-ctarget resolves from the file that declares it — checked programmatically, not by eye.code_source_test.pyand the new confinement suite; 12 of those are new, each parametrised set carrying a positive case so a guard that refused everything could not pass.Three existing tests broke, correctly, because they use
/tmppaths. They now declare their root via the env var rather than the guard being weakened for them. One needed more care:test_validate_local_path_not_existsused/nonexistent/path, which now trips confinement first and returns a different 400 — it would have silently stopped exercising the does-not-exist branch it is named for. Moved inside the allowed root.What this does NOT do
14 alerts cannot be fixed by bumping. 11
chromadband 3nltkadvisories have no patched release upstream.chromadbis constrained anyway so the version moves in one place the day a fix ships, but those 14 need a dismissal decision, not a version.I also did not force the other 14 drifting packages into lockstep. Most of that divergence is legitimate — an NPU worker pins a different
torchon purpose — and flattening it would break builds to satisfy a tidiness argument.Model Used
Claude Opus 5 (1M context)
Refs #17304
Refs #17300
Summary by CodeRabbit
Security
/opt/autobot.Dependency Management