fix(security): sandbox local media paths in cloud AI providers (#1209) - #1216
fix(security): sandbox local media paths in cloud AI providers (#1209)#1216groupthinking wants to merge 4 commits into
Conversation
All three cloud AI providers dispatch `analyze_image(image_url, ...)` on the
string's prefix: `s3://` and `http(s)://` are treated as remote sources, and
anything else fell through to an unguarded `open()`. A caller-supplied
absolute path, `../` traversal, or symlink could therefore read any file
readable by the service account.
The same unguarded sink existed in all three providers, not just the one named
in the issue:
- aws_rekognition.py `_prepare_image_input`
- azure_vision.py `_prepare_image_input`
- google_cloud.py inline `open()` in `analyze_image`
Introduce `cloud_ai/media_paths.py` as the single policy for local reads:
- Local reads are opt-in via `CLOUD_AI_MEDIA_ROOT`. Unset (the default)
disables them entirely, restricting providers to `s3://`/`https://`.
This is fail-closed, and answers the issue's open question.
- When a root is configured, both root and candidate are fully resolved
(`Path.resolve()` follows symlinks) and the candidate must be contained by
the root -- covering symlink escapes, not just lexical `..` segments.
- Non-regular files (FIFO, device, directory) are rejected, so a FIFO placed
inside the root cannot pin a `to_thread` worker forever.
- Rejection raises the new typed `UnsafeMediaPathError(CloudAIError)` instead
of silently returning empty bytes. Each provider re-raises `CloudAIError`
subclasses unchanged so the type survives to the caller.
- Providers read from the resolved path, not the caller string, narrowing the
check-to-open race.
- Error messages echo only the caller-supplied value; the resolved path is
logged server-side for forensics rather than returned.
Adds tests/unit/test_cloud_ai_media_paths.py (44 tests) covering absolute
paths, `../` traversal, symlink escape, non-regular files, the disabled
default, and per-provider propagation. Existing local-file tests now set
`CLOUD_AI_MEDIA_ROOT`. Full cloud AI suite: 505 passed.
Closes #1209
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔍 PR Validation |
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in local-media sandbox shared by AWS, Azure, and Google cloud AI providers.
Changes:
- Validates local paths against
CLOUD_AI_MEDIA_ROOT. - Adds typed unsafe-path errors and provider propagation.
- Adds security and provider regression tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.env.example |
Documents local-media configuration. |
cloud_ai/__init__.py |
Exports sandbox APIs. |
cloud_ai/exceptions.py |
Adds UnsafeMediaPathError. |
cloud_ai/media_paths.py |
Implements path containment policy. |
providers/aws_rekognition.py |
Guards AWS local reads. |
providers/azure_vision.py |
Guards Azure local reads. |
providers/google_cloud.py |
Guards Google local reads. |
test_aws_rekognition_provider.py |
Configures sandbox in existing tests. |
test_azure_vision_provider.py |
Configures sandbox in existing tests. |
test_cloud_ai_media_paths.py |
Adds sandbox security tests. |
🟢 Merge-ready — awaiting human sign-offAutomated remediation pass. What changed and where this stands: Fixed this pass: the PR description was rewritten into the repo's canonical template (
All substantive & required checks are green on head One red remains — and it is not this PR's fault: Terminal state:
Generated by Claude Code |
Automated review — PR remediation routineA review was requested, so here is a full pass: adversarial code review, red-team of the diff, and a CI-gate breakdown. Code review — verdict: LGTM ✅The fix is well-constructed and the design choices are the right ones:
Red-team pass on the diff
Only a nit, non-blocking: a configured-but-nonexistent CI gateAll real engineering checks are green on head
Publish gate — deferred to youBase is protected I'm watching this PR and will re-check as CI settles. Generated by Claude Code |
|
The three Copilot review threads on this PR are addressed in #1218 (branched from this head):
Verification on #1218's head: focused suite 48 passed (44 + 4 new), cloud AI regression 355 passed, Generated by Claude Code |
… gaps (#1216) Addresses the three unresolved Copilot review threads on #1216: 1. Fail-closed on a misconfigured root. get_media_root() left resolve() non-strict, so CLOUD_AI_MEDIA_ROOT=/etc/passwd (a regular file) was accepted as the root; that file then passed its own is_relative_to() containment check and was returned as a permitted read. Require the resolved root to be an existing directory, raising ConfigurationError otherwise. This also surfaces a nonexistent-directory typo loudly instead of silently rejecting every candidate. 2. Cover the Google permitted-file branch. AWS/Azure verified successful reads but the Google class only had rejection cases, while the PR's coverage table claimed the check for all three providers. Add an end-to-end analyze_image test asserting the resolved file's bytes are assigned to vision.Image().content. 3. Correct the module docstring. Remote-scheme handling is provider- specific: only AWS Rekognition recognises s3:// (Azure and Google treat it as a local path, rejected while local reads are disabled), and all three accept plain http:// as well as https://. Focused suite: 47 passed (44 + 3 new). Full cloud AI provider suites: 368 passed. ruff/mypy clean; black formatted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFFcgtEzNyhrxJnHgimcd2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Remediation update — 3 review threads resolved; new blocker is a duplicate PRAddressed this pass (commit
Local verification: focused suite 47 passed (44 + 3 new), cloud-AI provider suites 368 passed; ruff/mypy clean, black-formatted.
|
✅ Now fully green — ready for merge sign-offUpdate to my previous status (which reported the governance gates red and #1218 open): both conditions have cleared.
Sole remaining red: Terminal state: Generated by Claude Code |
|
PR-remediation run — terminal state: HALTED (awaiting agent-completion evidence + human merge) Triggered by the Review — all three Copilot-reviewer threads resolved on this head; CodeRabbit approved. ✅ Independent red-team of the guard ( Governance (was red, now cleared by me) — Remaining blocker —
The security change itself is sound and ready; the only thing between it and green is the trusted-publication step. Generated by Claude Code |
|
Returned to draft during delivery control. This is the focused #1209 security implementation, but ready state preceded a complete current execution receipt and control verification. The code and branch are preserved while exact-head checks, review threads, and preview evidence are reconciled. |
Security review: correct and complete — merge once the red check clears
Two extra hardening points that are easy to omit and are present here:
Unset Completeness verified — every local-read sink in the package on
The
Known residual (accepted): TOCTOU between Verdict: merge once the failing check is green. No code changes requested. |
…stants (#1220) * feat(config): share validated env parsing for tunable concurrency constants `TAG_WRITE_CONCURRENCY` in intelligent_cache.py was a hardcoded literal, so tuning Redis tag-write fan-out for a given deployment required an application release. Its sibling in firestore_state.py was already env-parsed, but the parser was private to that module -- and had already been copy-pasted once into cloud_ai/providers/aws_rekognition.py. Extract the two parsers verbatim into youtube_extension/core/env_config.py and have both call sites import them, then wire TAG_WRITE_CONCURRENCY through positive_int_env(). Semantics are preserved exactly, including the deliberate split that the merged firestore implementation settled on: - absent or blank falls back to the shipped default, because Compose and Helm routinely render an empty string for an unconfigured value; and - malformed or out-of-range fails fast at import rather than being clamped, so an operator typo surfaces at startup instead of silently running the process with a concurrency limit or deadline nobody chose. The only behavioural change is the error text, which now names the offending variable and echoes the input instead of surfacing int()'s built-in message. core/ is chosen over core/config/ and utils/ because its __init__.py is empty: importing the helper pulls in no logging or proxy stack, which matters for a module read at import time. Imports are relative so the helper resolves under either package root in use in this repo (youtube_extension.* and src.youtube_extension.*) rather than loading a second copy of the package. Verification: - tests/unit/test_env_config.py (new, 46 tests) covers unset, blank, whitespace, valid, zero, negative, non-numeric, inf and nan for both parsers, and asserts the messages are diagnosable. - Import-time wiring is proven in a subprocess rather than with importlib.reload, which would rebind module classes and leave the rest of the session holding stale references. With the env unset the constants resolve to exactly the shipped 8 / 16 / 30.0; with an override set they take the override; with an invalid value the import exits non-zero. - The 9 pre-existing parser tests in test_firestore_state.py were repointed at the re-exported names and still pass unchanged, which is what demonstrates the extraction is behaviour-preserving. - 247 passed across test_firestore_state.py and test_intelligent_cache.py; ruff clean; mypy --strict clean on the new module. Deliberately out of scope: the duplicate parser in aws_rekognition.py, which is already modified by open PR #1216 and would conflict; and TAG_WRITE_POOL_RESERVE, which is a headroom allowance rather than a concurrency limit. Closes #1180 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix: make shared tunables fail safe and bounded * style: restore lint baseline and re-add dropped invalid-input cases The fail-safe rework left three lint regressions relative to the branch point, and dropped six malformed-input cases from the parametrize lists. - ruff I001: the absolute import in firestore_state.py is 89 chars, one over the limit, so the import block needed rewrapping. - black: _fallback()'s signature and the expected-value ternary in test_env_config.py both exceeded 88 chars. - Re-add the invalid inputs dropped in the rewrite: -42, 8x, 0x10 for int; NaN, 0.0, 12s for float. Each exercises a distinct rejection path (parse error, range check, finiteness check). Pre-existing debt left untouched: firestore_state.py is already black-dirty on origin/main, and the unused 'result' at test_firestore_state.py:530 predates this branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Merge blocker: #1209 was closed, so PR Governance can't go greenFlagging why the failing check won't clear on its own.
Its canonical-issue rule requires the linked issue to be open. #1209 was closed as completed on Aug 2 — but the fix isn't in
Net effect: the fix for a To resolve, cleanest path: reopen #1209 → Alternatively: the two governance checks ( I did not reopen #1209 or merge, since both are your calls on a protected branch. (Separately, Generated by Claude Code |
Automated remediation pass — ready-for-reviewVerdict: not mergeable yet — one blocker: a merge conflict with Review / CI state (head
|
…cloud-ai-local-image-paths # Conflicts: # src/youtube_extension/integrations/cloud_ai/providers/azure_vision.py # src/youtube_extension/integrations/cloud_ai/providers/google_cloud.py
PR-remediation routine — remediation complete, staged for merge approvalTerminal state: What was remediated on this run (head
CI on head Only red — pre-existing, not from this PR: Staged next step (human): once #1317/#1320 land — or with an admin override, as on #1323 — squash-merge this PR. Not auto-merged: protected base, no Generated by Claude Code |
|
|
Closing as superseded by #1333. #1333 is a rebase of this PR's two commits onto current Concretely, this PR is blocked and #1333 is not:
Per #1333's own agent-handoff note ("#1216 should be closed in favor of this one"), the security fix will land via #1333. No work is lost. Generated by Claude Code |
…e of #1216) (#1333) * fix(security): sandbox local media paths in cloud AI providers (#1209) All three cloud AI providers dispatch `analyze_image(image_url, ...)` on the string's prefix: `s3://` and `http(s)://` are treated as remote sources, and anything else fell through to an unguarded `open()`. A caller-supplied absolute path, `../` traversal, or symlink could therefore read any file readable by the service account. The same unguarded sink existed in all three providers, not just the one named in the issue: - aws_rekognition.py `_prepare_image_input` - azure_vision.py `_prepare_image_input` - google_cloud.py inline `open()` in `analyze_image` Introduce `cloud_ai/media_paths.py` as the single policy for local reads: - Local reads are opt-in via `CLOUD_AI_MEDIA_ROOT`. Unset (the default) disables them entirely, restricting providers to `s3://`/`https://`. This is fail-closed, and answers the issue's open question. - When a root is configured, both root and candidate are fully resolved (`Path.resolve()` follows symlinks) and the candidate must be contained by the root -- covering symlink escapes, not just lexical `..` segments. - Non-regular files (FIFO, device, directory) are rejected, so a FIFO placed inside the root cannot pin a `to_thread` worker forever. - Rejection raises the new typed `UnsafeMediaPathError(CloudAIError)` instead of silently returning empty bytes. Each provider re-raises `CloudAIError` subclasses unchanged so the type survives to the caller. - Providers read from the resolved path, not the caller string, narrowing the check-to-open race. - Error messages echo only the caller-supplied value; the resolved path is logged server-side for forensics rather than returned. Adds tests/unit/test_cloud_ai_media_paths.py (44 tests) covering absolute paths, `../` traversal, symlink escape, non-regular files, the disabled default, and per-provider propagation. Existing local-file tests now set `CLOUD_AI_MEDIA_ROOT`. Full cloud AI suite: 505 passed. Closes #1209 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(security): reject non-directory CLOUD_AI_MEDIA_ROOT; close review gaps (#1216) Addresses the three unresolved Copilot review threads on #1216: 1. Fail-closed on a misconfigured root. get_media_root() left resolve() non-strict, so CLOUD_AI_MEDIA_ROOT=/etc/passwd (a regular file) was accepted as the root; that file then passed its own is_relative_to() containment check and was returned as a permitted read. Require the resolved root to be an existing directory, raising ConfigurationError otherwise. This also surfaces a nonexistent-directory typo loudly instead of silently rejecting every candidate. 2. Cover the Google permitted-file branch. AWS/Azure verified successful reads but the Google class only had rejection cases, while the PR's coverage table claimed the check for all three providers. Add an end-to-end analyze_image test asserting the resolved file's bytes are assigned to vision.Image().content. 3. Correct the module docstring. Remote-scheme handling is provider- specific: only AWS Rekognition recognises s3:// (Azure and Google treat it as a local path, rejected while local reads are disabled), and all three accept plain http:// as well as https://. Focused suite: 47 passed (44 + 3 new). Full cloud AI provider suites: 368 passed. ruff/mypy clean; black formatted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFFcgtEzNyhrxJnHgimcd2 * test: set CLOUD_AI_MEDIA_ROOT in read-offload tests added on main The event-loop offload tests from #1304/#1323 pass raw local paths to _prepare_image_input/analyze_image; with local reads now fail-closed behind CLOUD_AI_MEDIA_ROOT, they must opt in via tmp_path, matching the other pre-existing local-file tests. Generated with [Linear](https://linear.app/myxstack/issue/GRV-296/land-pr-1216-fixsecurity-sandbox-local-media-paths#agent-session-3138b916) Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com> * fix(security): correct s3:// provider guidance in media-path guard The disabled-reads UnsafeMediaPathError message and .env.example both suggested s3:// as a recovery scheme for all three cloud AI providers, but only AWS Rekognition recognizes s3://. Azure Vision and Google Vision route s3:// through the disabled local-path branch, so following that guidance just raises UnsafeMediaPathError again. Reword both to scope s3:// to AWS Rekognition and point Azure/Google callers at https:// (valid for every provider). No logic change; the guard behavior is unchanged. Addresses the two Copilot review threads on this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8yJv2udCCnwN4u586e9PL --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com>
Canonical issue
Closes #1209.
Outcome
All three cloud AI providers implement
BaseCloudAI.analyze_image(image_url, ...)and dispatch on the string's prefix —s3://andhttp(s)://are handled as remote sources, and anything else fell through to an unguardedopen(). A caller-supplied absolute path,../traversal, or symlink could therefore read any file the service account could read.After this PR, local reads are fail-closed: disabled entirely unless
CLOUD_AI_MEDIA_ROOTis set. When enabled, every candidate path is fully resolved (Path.resolve()follows symlinks) and must be contained within the equally resolved root; symlink escapes and non-regular files (FIFO/device/dir) are rejected with a typedUnsafeMediaPathErrorrather than a silent empty read. A misconfigured root (a regular file such as/etc/passwd, or a non-existent path) is now rejected withConfigurationErrorat resolution time rather than silently allowing a read.Scope
cloud_ai/media_paths.py) applied by all three providers (AWS Rekognition, Azure Vision, Google Vision); typedUnsafeMediaPathError(CloudAIError);.env.exampledocumentation; security tests.analyze_videolocal-path handling; movinggoogle_cloud.py's synchronousread_bytes()off the event loop (a perf concern, not a security one).integrator.py:297-298, but those lines are provider initialization, not image analysis, and no HTTP route callsanalyze_imagetoday. This is defense-in-depth hardening of the integration package's public API surface — ahead of the first route that wires it up — not a live, actively-reachable exploit.Risk
CLOUD_AI_MEDIA_ROOTdefaults to unset, so any caller currently passing a local filesystem path toanalyze_imagenow receivesUnsafeMediaPathErroruntil they set the variable. No route wiresanalyze_imagetoday, so real-world breakage risk is ~zero, and fail-closed is the correct default for a security boundary. Documented in.env.example.Verification
tests/unit/test_cloud_ai_media_paths.py(47 passed) + per-provider guard tests54161715416171and marked resolvedFollow-up commit
5416171resolves the three Copilot review threads: (1) fail-closed hole —get_media_root()now requires the root to be an existing directory (a file root like/etc/passwdpreviously passed its own containment check); (2) added Google permitted-file coverage; (3) corrected the module docstring's provider-specific remote-scheme description.Providers were already non-
black-compliant onmain; they were not reformatted, to keep the diff to the security change. Two pre-existing local-file tests intest_aws_rekognition_provider.py/test_azure_vision_provider.pynow setCLOUD_AI_MEDIA_ROOTto theirtmp_path.Production evidence
Not applicable — backend security hardening with no user-facing surface. This changes the cloud AI integration package only; no HTTP route currently reaches
analyze_image, so there is no runtime request path to demonstrate in a deployment. The Vercel preview deployed successfully (frontend unaffected). The operative evidence is the automated test + security-scan suite on head5416171: 47 focused security tests plus the full cloud AI provider suites, with CodeQL and Secret Scan green.Acceptance criteria → coverage
TestResolveLocalMediaPathRejections::test_absolute_path_outside_root_is_rejected../traversaltest_dotdot_traversal_is_rejected, plus per-providertest_traversal_rejected×3test_symlink_escaping_root_is_rejected+ per-provider ×3TestGetMediaRoot(unset / empty / relative / whitespace / unresolvable / non-directory / file-as-root).env.exampleblock; default = disabledTestResolveLocalMediaPathAccepts, per-providertest_permitted_file_still_reads×3test_https_source_unaffected_by_guard×3test_analyze_image_propagates_typed_error×3Also covers non-regular files (FIFO/dir), empty/whitespace input, misconfigured roots, and the disabled-by-default posture.
The fix (detail)
New
cloud_ai/media_paths.pyis the single policy for local reads, shared by all three providers.CLOUD_AI_MEDIA_ROOT. Unset (the default) disables them entirely. A configured root must resolve to an existing directory; a file or non-existent path raisesConfigurationError(fail-closed). Recognised remote schemes are provider-specific: only AWS Rekognition honourss3://; Azure and Google treat it as a local path, and all three accept plainhttp://.Path.resolve()is applied to both root and candidate, so symlink escapes are rejected, not just lexical..segments.asyncio.to_threadworker forever.UnsafeMediaPathError(error_code="UNSAFE_MEDIA_PATH") instead of silently returning empty bytes. Each provider'sanalyze_imagegained anexcept CloudAIError: raiseahead of its broadexcept Exception, so the type survives to the caller.openat/O_NOFOLLOWproofing was judged over-engineering here and is documented in the module docstring.WARNINGlog for forensics.🤖 Generated with Copilot CLI
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description maintained by the automated PR-remediation routine in the repository's canonical governance sections (Canonical issue / Outcome / Scope / Risk / Verification / Production evidence). All original author content is preserved; the follow-up commit
5416171and refreshed CI head are reflected above.