Skip to content

fix(storage): block path traversal in file-download (GHSA-6px9-99p6-7j7g, 0.60.1) - #409

Merged
padak merged 1 commit into
mainfrom
claude/gifted-margulis-e810b7
Jun 12, 2026
Merged

fix(storage): block path traversal in file-download (GHSA-6px9-99p6-7j7g, 0.60.1)#409
padak merged 1 commit into
mainfrom
claude/gifted-margulis-e810b7

Conversation

@padak

@padak padak commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

Fixes a path-traversal arbitrary-file-write in kbagent storage file-download — finding H2 from the 2026-06-12 security audit (private advisory GHSA-6px9-99p6-7j7g).

When --output was omitted, the downloaded bytes were written to the Storage API's file_detail["name"] verbatim (storage_service.py:1746/1770client.download_file which does out_path.parent.mkdir(parents=True)). A malicious or compromised API response (e.g. a file in a shared/invited project, where name labels are loosely constrained) with a name like ../../../../.zshrc — or an absolute path like /Users/victim/Library/LaunchAgents/x.plist — could overwrite an arbitrary file on the user's machine with fully attacker-controlled content. A clean write-to-RCE primitive.

Fix

New _safe_download_target(base, server_name) helper in storage_service:

  • strips leading separators so an absolute name cannot override the target dir;
  • preserves legitimate nested subpaths (e.g. exports/2026/report.csv still creates the subdir, as before);
  • asserts the resolved path stays within the chosen base (CWD, or the --output directory) via resolve() + is_relative_to(), rejecting escapes with INVALID_ARGUMENT.

Explicit --output FILE is unchanged — it is the user's own trusted choice. For honest file names behavior is identical to before; only ../ / absolute escapes are blocked, so no regression for legitimate use.

Tests

New TestDownloadFilePathTraversal (4 cases): rejects ../ escape with and without an --output directory, neutralizes absolute names under CWD, and preserves benign nested names. Full suite green: 3959 passed, 132 skipped; lint/format/ty/changelog-check clean.

Audit context

The other HIGH finding (auto-update artifact integrity, GHSA-3322-j9h9-mg9p) was accepted as residual risk by the repo owner (GitHub as trust root; silent auto-update retained as the patch-delivery channel) and closed won't-fix. The remaining MEDIUM/LOW advisories stay open as draft for separate triage.


Open in Devin Review

`storage file-download` wrote the downloaded bytes to the Storage API's
`file_detail["name"]` verbatim when --output was omitted. A malicious or
compromised API response (e.g. a file in a shared/invited project) with a
name like `../../../../.zshrc` -- or an absolute path -- could overwrite an
arbitrary file on the user's machine with attacker-controlled content, a
clean write-to-RCE primitive.

Add `_safe_download_target()`: strip leading separators (an absolute name
can no longer override the target), preserve legitimate nested subpaths,
and assert the resolved path stays within the chosen directory (CWD or the
--output dir) via resolve() + is_relative_to(), rejecting escapes with
INVALID_ARGUMENT. Explicit `--output FILE` is unchanged (trusted user input).

Private advisory GHSA-6px9-99p6-7j7g.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 0 potential issues.

Open in Devin Review

@padak
padak merged commit a73d94a into main Jun 12, 2026
5 checks passed
@padak
padak deleted the claude/gifted-margulis-e810b7 branch June 12, 2026 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant