mirror of
https://github.com/ComposioHQ/composio.git
synced 2026-09-22 11:46:35 +08:00
Alberto Schiabel
9e958948c5
# Description
Found by the scheduled security audit, while checking whether a test
failure on my other PR was pre-existing. It was — and the reason it
fails is a real gap in a shipped security control.
`isBlockedSensitiveFileUploadPath` (the GHSA-hp3h-89pf-5q58 denylist)
matched deny segments **only against the symlink-resolved path**. That
catches a benign name pointing at a secret — `~/innocent-name ->
~/nested/.aws/creds`, which the existing test covers — but misses the
inverse:
| Layout | Written path | Resolved path | Blocked before? |
|---|---|---|---|
| `~/innocent -> ~/.aws/creds` | no `.aws` | **`.aws`** | yes |
| `~/.claude -> /state/claude` | **`.claude`** | no `.claude` | **no** |
`~/.claude/settings.json` resolves to `/state/claude/settings.json`,
which has no `.claude` segment, so it sailed through — and `.claude` is
on the denylist precisely because it *"may contain API keys and project
context read by assistants"*.
This layout is not exotic. Dotfile managers (chezmoi, stow, yadm) and
containerised home directories produce it routinely — Composio's own
agent sandbox image has `/home/zen/.claude -> /state/claude`. For every
user in that shape the control was silently inactive, which is the worst
failure mode for a denylist: no error, no warning, upload proceeds.
The same hiding trick applies to the basename check (`~/.env ->
/state/plain-config`), so that path is fixed too.
## Why CI never caught this
`ts/packages/core/test/utils/sensitiveFileUploadPaths.test.ts` **already
asserts** the blocked behaviour:
```ts
expect(isBlockedSensitiveFileUploadPath(path.join(os.homedir(), '.claude', 'settings.json'))).toBe(true);
```
That assertion has been failing on `next` on any machine where
`~/.claude` is a symlink. It passes in CI only because `~/.claude` does
not exist on the runners: `existsSync` is false, no `realpath` runs, and
the written path keeps its `.claude` segment. The test is
environment-dependent, so green CI was never evidence the control
worked.
## The fix
The TypeScript `normalizePath` helper now returns both the written and
resolved segments, and the segment scan and basename check each consider
both. The Python guard now applies the same rule. Either path can carry
the denied name, so both SDKs inspect both forms.
# How did I test this PR
**The TypeScript fix is gated by tests — 3 fail without it, 13/13 pass
with it.**
Without the `src` change (test file only):
```
× blocks common credential directory segments
× blocks a sensitive directory that is itself a symlink to a plain path
× blocks a denied basename whose symlink target is named innocuously
Tests 3 failed | 10 passed (13)
```
With the fix:
```
Test Files 1 passed (1)
Tests 13 passed (13)
```
Note the first of those three is the **pre-existing** assertion quoted
above — this PR turns it green rather than adding it.
Three tests added, each building a real symlink in a temp dir:
- sensitive directory that is itself a symlink to a plain path (the
`~/.claude -> /state/claude` case), asserting both
`isBlockedSensitiveFileUploadPath` and that `assertSafeFileUploadPath`
throws
- denied basename whose symlink target is named innocuously (`.env ->
plain-config`)
- **negative case**: an ordinary file reached through a symlinked
directory (`docs/document.pdf`) is still allowed, so the fix does not
over-block
The Python parity change adds the same three cases. Before the Python
source change, the sensitive written directory and basename both
returned `False`; with the fix, all 11 focused Python tests pass.
**Full verification:**
| Command | Result |
|---|---|
| `vitest run` in `ts/packages/core` | **48 files, 1114 tests passed** |
| `pnpm typecheck` (workspace) | **14/14 tasks successful**, exit 0 |
| `oxlint` on both changed files | exit 0, clean |
| `prettier --check` on both changed files | "All matched files use
Prettier code style!" |
| `nox -s chk` in `python/` | Ruff and mypy passed |
| `nox -s tst` in `python/` | **1339 passed, 33 skipped**, exit 0 |
# Security
- No dependency changes, no new network calls, no new imports. The diff
is limited to the equivalent TypeScript and Python guards, their tests,
and the required `@composio/core` patch changeset.
- This **strengthens** an existing control and cannot weaken it: the
previous match set is a strict subset of the new one, so nothing that
was blocked before is allowed now. The added negative test pins that the
widening does not over-block ordinary files.
- **Grype** — `grype dir:ts/packages/core --only-fixed --fail-on medium`
→ reported below.
- **Socket** — could not run; `doppler secrets get SOCKET_API_TOKEN
--plain --project hermes --config dev_zen` returns empty in this cron
sandbox, so `socket ci` exits `Auth Error`. Reporting rather than
skipping silently.
- Unrelated pre-existing note: the repo's `pnpm audit --prod` comment
flags `extract-zip <=2.0.1` with `Patched versions >=2.0.2`, a version
that does not exist on npm. Details in #4217.
Origin: cron-48e51eab745f /
[zen-cron-44e260352d1a](https://zen.corp.composio.io/dashboard/#/chat/zen-cron-44e260352d1a)
Triggered by: saransh@composio.dev | Source: unknown
Session:
https://zen.corp.composio.io/dashboard/#/chat/zen-cron-44e260352d1a
Changesets
Hello and welcome! This folder has been automatically generated by @changesets/cli, a build tool that works
with multi-package repos, or single-package repos to help you version and publish your code. You can
find the full documentation for it in our repository
We have a quick list of common questions to get you started engaging with this project in our documentation