fix(security): require CLOUD_AI_MEDIA_ROOT to be an existing directory (#1216 review) - #1218
fix(security): require CLOUD_AI_MEDIA_ROOT to be an existing directory (#1216 review)#1218groupthinking wants to merge 2 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>
…review) Address the three unresolved Copilot review threads on PR #1216, which hardens local media paths in the cloud AI providers: - get_media_root() now rejects a CLOUD_AI_MEDIA_ROOT that resolves to a regular file or a non-existent path. Previously a misconfiguration such as CLOUD_AI_MEDIA_ROOT=/etc/passwd passed is_relative_to() against itself and was handed straight to the caller, defeating the fail-closed policy. Misconfiguration now raises ConfigurationError loudly. - Correct the module docstring: remote-scheme recognition is provider-specific (only aws_rekognition honours s3://; azure_vision and google_cloud treat s3:// as a local path), not the uniform s3://+https:// claimed before. - Add Google-provider permitted-file coverage asserting the resolved bytes reach vision.Image.content, closing the gap the PR coverage table claimed. Adds 4 tests (file-as-root, nonexistent-root, end-to-end file-root rejection, Google permitted-file read); suite now 48 passed, full cloud AI regression 355 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ZVtZfzdPJ9XbncLxu9qZ1
|
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: BLOCKEDReasons: Machine-readable verdict{
"details": {
"invalid_fields": [
"policy.agent_login",
"policy.run_id"
]
},
"reasons": [
"invalid_payload"
],
"verdict": "blocked"
} |
CI status — two failing checks, both the repo-wide agent-lock policy (not fixable in this PR)Substantive code checks are green on head Two checks fail, and both are the same admin-gated policy — not something a PR change can clear:
Terminal state:
|
|
Closing as superseded by #1216's commit While this PR was open, the same three Copilot review threads on #1216 were fixed directly on that branch — with equivalent changes: Since the fix belongs on the PR under review rather than a fork of its old head, keeping both open would be two competing PRs for one issue. #1216 is the canonical one, so this duplicate is closed. No code is lost — #1216 carries the equivalent fix (its focused suite: 47 passed; mine: 48 — the one-test delta was a redundant end-to-end variant of a case #1216 already covers at the unit level). #1216 remains Generated by Claude Code |
2 participants
Canonical issue
Closes #1209 (extends PR #1216; resolves its three open Copilot review threads)
Outcome
Closes a fail-closed hole in the cloud AI local-media guard shipped by #1216: a
misconfigured
CLOUD_AI_MEDIA_ROOTthat points at a regular file (e.g./etc/passwd) or a non-existent path no longer silently allows a read — itnow raises
ConfigurationErrorloudly at resolution time.Scope
get_media_root()now requires the resolved root to be an existingdirectory. Previously
CLOUD_AI_MEDIA_ROOT=/etc/passwdresolved to thatfile, which
is_relative_to()considers "inside" itself, so the file washanded straight to the caller — defeating the fail-closed policy.
provider-specific — only
aws_rekognitionhonourss3://;azure_visionand
google_cloudtreats3://as a local path — not the uniforms3://+https://the docstring claimed.reach
vision.Image.content, closing the gap the fix(security): sandbox local media paths in cloud AI providers (#1209) #1216 coverage tableclaimed for all three providers.
and provider wiring) is unchanged; this only tightens the root check and its
documentation/tests. This branch sits on the same head as fix(security): sandbox local media paths in cloud AI providers (#1209) #1216.
Risk
path and is covered by new tests. Behaviour for a correctly configured
directory root is unchanged.
CLOUD_AI_MEDIA_ROOTto afile or missing path now fails loudly at first use instead of reading that one
file. That is the intended fail-closed correction.
get_media_root()plus tests.Verification
Run against head
b269555.tests/unit/test_cloud_ai_media_paths.py: 48 passed(44 original + 4 new: file-as-root rejection, non-existent-root rejection,
end-to-end file-root rejection, Google permitted-file read).
(
test_cloud_ai_media_paths,test_aws_rekognition_provider,test_azure_vision_provider,test_google_cloud_provider): 355 passed.ruff checkclean on both changed files.black --checkclean on both changed files.mypy --strict media_paths.py: no issues in the file (therepo-wide errors are pre-existing in unrelated
integrations/*modules).addressed by a code or test change above.
Production evidence
Not applicable as a runtime artifact: this touches only the Python package
src/youtube_extension/integrations/cloud_ai/, which has no HTTP route onmain(no route calls
analyze_image; see #1216's scoping note). Behavioural evidenceis the test suite, which exercises the real filesystem via
tmp_path(realfiles, real directories, real symlinks).
Agent handoff
open review threads on fix(security): sandbox local media paths in cloud AI providers (#1209) #1216
(misconfigured root errors rather than degrading to a single-file
allowlist), matching fix(security): sandbox local media paths in cloud AI providers (#1209) #1216's stated default.
Generated by Claude Code