fix(media): media-delivery denylist is inert on Windows - #78162
fix(media): media-delivery denylist is inert on Windows#78162bbasketballer75 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-specific security gap in gateway media attachment delivery: the default (non-strict) media-delivery denylist previously consisted of POSIX roots that resolve to nonexistent paths on Windows, making the denylist effectively empty there. This change adds Windows-appropriate denied roots and validates the behavior with Windows-only regression tests.
Changes:
- Add Windows-specific denied roots sourced from
%SystemRoot%and%ProgramData%, plus additional user-profile credential store subpaths. - Extend
_media_delivery_denied_paths()to include the Windows deny entries when running onos.name == "nt". - Add Windows-only tests asserting system roots and credential stores are denied while common user directories remain deliverable, plus an end-to-end refusal check for a system file.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
gateway/platforms/base.py |
Adds Windows denylist entries and wires them into _media_delivery_denied_paths() for non-strict media delivery safety. |
tests/gateway/test_platform_base.py |
Adds Windows-only regression tests covering denylist coverage and end-to-end refusal behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
suggesting changes The new credential-store entries are still constructed under Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
_MEDIA_DELIVERY_DENIED_PREFIXES is POSIX-only, and the entries do not degrade
gracefully. Path("/etc").resolve() becomes C:\etc on the current drive, which
does not exist -- so on Windows every entry in that tuple is inert and the
default-mode denylist is effectively empty.
Measured on Windows against unmodified main:
/etc -> C:\etc exists=False
/sys -> C:\sys exists=False
/root -> C:\root exists=False
... all nine resolve to nonexistent paths
C:\Windows covered=False
C:\ProgramData covered=False
Consequence, verified end-to-end through the guard itself:
validate_media_delivery_path(r"C:\Windows\win.ini")
before -> <path> (deliverable as a gateway attachment)
after -> None (refused)
Strict mode was unaffected -- the ~/.ssh denial works, because those entries
are built from the live $HOME rather than hard-coded POSIX roots. This is the
default (non-strict) path, which is what most operators run.
Adds two Windows-specific lists:
- system roots resolved from %SystemRoot% and %ProgramData%, read from the
environment rather than hard-coded, because Windows need not live on C:
and a roaming profile need not sit under C:\Users.
- the credential stores under the user profile that have no POSIX
counterpart: Microsoft\Credentials (both roaming and local),
Microsoft\Protect (DPAPI master keys), Microsoft\Crypto, and
Local\Microsoft\Vault. The POSIX dotfile equivalents (.aws, .ssh, .azure,
.gcloud) use the same names on Windows and are already covered by
_MEDIA_DELIVERY_DENIED_HOME_SUBPATHS.
AppData is deliberately NOT denied wholesale. %LOCALAPPDATA%\Temp is a normal
home for generated artifacts, so a blanket rule would break legitimate media
delivery. A test asserts Temp, Pictures and Downloads stay deliverable.
Verification on Windows:
- 9 new tests; 6 of them fail when the production change is reverted, so the
coverage pins real behaviour rather than passing vacuously. The 3 that pass
either way are the must-stay-deliverable assertions.
- tests/gateway/test_platform_base.py: 2 failed before, 2 failed after --
set-diff of failure names is empty, nothing newly broken. Those 2 are
unrelated pre-existing failures.
- ruff clean.
Complementary to (not overlapping) the case-folding work in NousResearch#52045: that PR
rewrites the comparison helpers, this one fixes what is in the list to compare
against. Different hunks in the same file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lders Addresses @egilewski's review: the credential-store entries were built solely from ``expanduser("~")``, so they only matched the default profile layout. Windows Known Folder redirection (roaming profiles, Folder Redirection GPO) can point %APPDATA% / %LOCALAPPDATA% outside %USERPROFILE% entirely — e.g. %USERPROFILE%=C:\Users\svc while %APPDATA%=Z:\RoamingProfile\svc. In that layout the home-relative path was denied but the *live* credential store at Z:\... stayed deliverable, which is the path that actually holds the secrets. Split the subpaths by which AppData root they live under and resolve each against BOTH forms: - the env-derived root (%APPDATA% / %LOCALAPPDATA%), which is correct under redirection, and - the home-relative root, which is correct on the common layout and acts as a fail-closed fallback when the env vars are unset. Denying both costs a few extra entries and no I/O, and avoids trading one uncovered layout for another. Verified with the reviewer's exact scenario (%APPDATA%=Z:\RoamingProfile\svc, %LOCALAPPDATA%=Z:\LocalProfile\svc): all 5 redirected credential roots are now denied while the 33 home-relative entries are retained. tests/gateway/test_platform_base.py: 86 passed, 8 failed, 1 skipped — byte-identical to the pre-change baseline on this Windows box (the 8 are pre-existing POSIX-path/Docker-translation failures, unrelated to this change; same test names before and after).
f8987f2 to
71c6a8c
Compare
|
Fixed in The credential subpaths are now split by which AppData root they belong to and resolved against both the env-derived root ( Verified with your exact scenario ( Also rebased onto current
🤖 Addressed by Claude Code |
The bug
_MEDIA_DELIVERY_DENIED_PREFIXESis POSIX-only, and the entries don't degrade gracefully.Path("/etc").resolve()becomesC:\etcon the current drive — which doesn't exist. So on Windows every entry in that tuple is inert and the default-mode denylist is effectively empty.Measured against unmodified
mainon Windows:Verified end-to-end through the guard itself, not just the constant:
Strict mode was unaffected — the
~/.sshdenial works, because those entries are built from the live$HOMErather than hard-coded POSIX roots. This is the default (non-strict) path, which is what most operators run.The fix
Two Windows-specific lists:
System roots, resolved from
%SystemRoot%and%ProgramData%— read from the environment rather than hard-coded, because Windows needn't live onC:and a roaming profile needn't sit underC:\Users.Credential stores under the user profile that have no POSIX counterpart:
Microsoft\Credentials(roaming and local),Microsoft\Protect(DPAPI master keys),Microsoft\Crypto,Local\Microsoft\Vault. The POSIX dotfile equivalents (.aws,.ssh,.azure,.gcloud) use the same names on Windows and are already covered by_MEDIA_DELIVERY_DENIED_HOME_SUBPATHS.AppData is deliberately not denied wholesale.
%LOCALAPPDATA%\Tempis a normal home for generated artifacts, so a blanket rule would break legitimate media delivery. A test assertsTemp,PicturesandDownloadsstay deliverable.Verification (Windows)
tests/gateway/test_platform_base.py: 2 failed before, 2 failed after. Set-diff of failure names is empty — nothing newly broken. Those 2 are unrelated pre-existing failures.ruff checkclean.Relationship to #52045
Complementary, not overlapping. #52045 rewrites the comparison helpers to be case-insensitive (
_path_compare_key,_path_under_or_equal); this fixes what is in the list to compare against. Different hunks in the same file — #52045 touches the helpers around :52 and_path_under_denied_prefixat :1134, this touches the constants at :1196 and_media_delivery_denied_pathsat :1333.Both are needed: case-folding a denylist that contains only nonexistent paths still denies nothing.
🤖 Generated with Claude Code