fix(security): reject path-traversal IDs in Google Vault, AgentMail and Algolia tools - #7266
fix(security): reject path-traversal IDs in Google Vault, AgentMail and Algolia tools#7266waleedlatif1 wants to merge 5 commits into
Conversation
…nd Algolia tools Google Vault, AgentMail and Algolia tools interpolate LLM-writable identifiers (matter/hold/export/savedQuery IDs, inbox/thread/message/draft IDs, index names and object IDs) straight into request paths. A value of `..` re-aims an authenticated request at a different resource on the same host with the user's credential still attached, including on DELETE. `encodeURIComponent` does not help: `.` and `..` are unreserved, so they survive encoding, and the URL parser removes dot segments after decoding. Only rejecting the value works. Every site now routes through `safeUrlPathSegment`. Algolia object IDs are arbitrary caller-chosen strings and legitimately contain `/`, so `safeAlgoliaObjectId` guards each slash-delimited piece instead of refusing the separator, keeping the emitted text byte-identical to the previous `encodeURIComponent` for every non-traversal value. Adds a `path_safety.test.ts` per service that enumerates tools from the barrel, so a new unguarded path parameter fails CI.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR prevents model-writable identifiers from escaping URL path segments in Google Vault, AgentMail, and Algolia requests.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/tools/algolia/utils.ts | Safely preserves the prior encoding semantics for arbitrary separator-bearing object IDs while rejecting whole-value dot segments. |
| apps/sim/tools/algolia/path_safety.test.ts | Exercises each discovered Algolia path parameter independently and verifies both traversal rejection and legitimate object-ID compatibility. |
| apps/sim/tools/agentmail/path_safety.test.ts | Provides typed, independently parameterized coverage for AgentMail path identifiers, including encoded email-address inbox IDs. |
| apps/sim/tools/google_vault/path_safety.test.ts | Provides typed path-normalization and rejection coverage for Google Vault identifiers. |
Reviews (3): Last reviewed commit: "test(security): drop a stray format spec..." | Re-trigger Greptile
There was a problem hiding this comment.
2 issues found across 56 files
Confidence score: 3/5
apps/sim/tools/algolia/utils.tscan alter or reject slash-bearing Algolia IDs containing internal whitespace or empty components, potentially breaking valid IDs; preserve non-dot pieces without per-piece trimming or empty-value rejection.apps/sim/tools/google_vault/path_safety.test.tsskips the segment containing the interpolated ID, so traversal protection could regress while the test still passes; assert that the ID remains within a single encoded path segment.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/tools/algolia/utils.ts">
<violation number="1" location="apps/sim/tools/algolia/utils.ts:31">
P2: Slash-bearing Algolia IDs with internal whitespace or empty components are changed or rejected. Preserve non-dot pieces without `safeUrlPathSegment`'s per-piece trim and empty-value rejection, while still rejecting exact `.` and `..`.</violation>
</file>
<file name="apps/sim/tools/google_vault/path_safety.test.ts">
<violation number="1" location="apps/sim/tools/google_vault/path_safety.test.ts:137">
P2: The traversal assertion skips the segment that holds the interpolated ID (`if (segment.includes(SAFE_ID)) return`), so it only proves segment count and origin are unchanged, not that the ID stayed in a single encoded slot. That lets the test's own canonical vector slip through: for a reverted (raw-interpolation) tool, `buildUrl(tool, 'matter123/../../matters/victim')` resolves to `/v1/matters/victim` — the same 4-segment length as the baseline `/v1/matters/SAFEID` with identical non-ID segments — so the test passes while the request is re-aimed at `victim`. This is exactly the `../../matters/victim` escape the file header documents, so a future regression on `delete_matters` and the other single-ID tools would go undetected. Assert the ID slot is exactly the percent-encoded value instead of skipping it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
… once The suites inherited a coverage hole from the Vercel template they were modelled on: `buildParams` filled every string parameter with the same hostile value and the assertion swallowed the throw, so the moment one parameter was guarded the whole vector was skipped and the tool's remaining IDs stopped being tested. The "a new unguarded parameter fails CI" property did not hold for any tool that already had one guard. Each suite now enumerates (tool, parameter) pairs and fuzzes exactly one parameter at a time, holding every sibling at a known-safe placeholder, and asserts a thrown error names the parameter under test rather than discarding it. Adds explicit coverage floors and named assertions for the second ID of every two-ID route. Coverage goes from 49 tools to 77 (tool, parameter) pairs; 1201 -> 1695 tests. Measured on the reverted-guard check: with `delete_matters_holds`'s `holdId` unguarded, the old suite reported 1 failure (incidental, from the one assertion outside the try/catch) while the tightened suite reports 11.
…d probes Three follow-ups to the path-safety harnesses, all test-only — the guard implementations are byte-identical. Assert rejection, not just path shape. A bare `.` in the FINAL segment is invisible to a shape check: `https://host/a/.` normalizes to `https://host/a/`, preserving the segment count and every leading segment. `delete_matters`, `delete_inbox` and `delete_index` all end in a guarded id and are all destructive, so a shape-only assertion was weakest exactly where the damage is worst. Every (tool, param) pair now asserts `.`, `..` and `' .. '` throw and name the parameter. `safeAlgoliaObjectId` guards per slash-delimited piece, so the same blind spot exists one level down (`catalog/.` encodes to `catalog%2F.`, a single segment no normalization touches); that is now asserted explicitly. Remove `any`. `ToolConfig<any, any>` and `as any` were inherited from the Vercel harness these were copied from. Replaced with `ToolConfig<Record<string, unknown>, ToolResponse>`, params built as `Record<string, unknown>` so `url(...)` needs no cast, and the barrel seeded as `readonly unknown[]` so the type guard is the single narrowing point — a barrel's element union is not assignable to a widened `ToolConfig`, because the param type sits in the contravariant position of `request.url`. One `as Record<string, unknown>` remains per file, inside the guard, TSDoc'd. Surface skipped probes. Discovery recorded every probe that threw while being built from entirely safe placeholders and asserts that set is empty, so a tool silently dropped from the suite fails CI instead of vanishing. `apps/sim/tsconfig.json` excludes `**/*.test.ts`, so `bun run type-check` never covered these files. Verified with a temporary tsconfig overriding `exclude` (not committed); it found 6 real TS2345 errors from `as const` on `CREDENTIAL_PARAMS`, now fixed. 1695 -> 1721 tests, all passing.
…d slot
Two real review findings, both against the first commit.
Empty and whitespace-bearing object id components (greptile P1, cubic P2).
`safeAlgoliaObjectId` split on `/` and ran each piece through
`safeUrlPathSegment`, which trims each piece and rejects empty ones — so
`catalog//sku` and `catalog/sku/` started failing with "objectID is required"
and `catalog/ sku` was silently rewritten to `catalog/sku`. All three are
legitimate Algolia object ids that the prior `encodeURIComponent` preserved.
The split was not merely harmful, it was unnecessary. Percent-encoding collapses
the value into a single path segment and the URL parser never decodes `%2F`, so
no interior piece is ever a path segment that dot-segment removal could act on:
new URL('https://x/1/indexes/p/' + encodeURIComponent('catalog/../../1/keys'))
// => /1/indexes/p/catalog%2F..%2F..%2F1%2Fkeys (intact)
A value containing a separator therefore cannot be the dangerous shape, which is
a value whose entire text is `.` or `..`. It is now encoded whole, byte-identical
to the encoding it replaced, and the earlier per-piece rejection assertions —
which tested a risk that does not exist — are replaced by cases pinning that
interior dot pieces stay intact while whole-value dot segments are still refused.
The traversal assertion skipped the id's own segment (cubic P2). Skipping it
proved only the segment count and the surrounding segments, and a traversal can
satisfy both: against raw interpolation `matter123/../../matters/victim`
resolves `/v1/matters/<id>` to `/v1/matters/victim` — same length, identical
non-id segments. The suite's own canonical vector, named in the file header,
passed against unguarded code. The id slot is now asserted equal to the trimmed
value percent-encoded.
Scoped revert of `delete_matters` now fails all 10 traversal vectors instead of
7, and the four-site revert goes from 37 failures to 45. 1721 -> 1741 tests.
|
@greptile review |
|
@cubic-dev-ai review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
…ue test names The template appended `%j` to a `legit_verb` that already carried one, so every legitimate-value case rendered as "passes \"id\" through unchanged undefined".
|
@greptile review |
|
@cubic-dev-ai review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 2/5
- In
apps/sim/tools/algolia/utils.ts, slash-containingobjectIDvalues bypasssafeUrlPathSegmentfor each component, allowing dot-segment or backslash data into generated paths and creating a concrete path-safety risk—validate every/-separated piece before constructing the path.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/tools/algolia/utils.ts">
<violation number="1" location="apps/sim/tools/algolia/utils.ts:41">
P1: When an `objectID` contains `/`, this branch skips `safeUrlPathSegment` for every component, so dot-segment and backslash-containing IDs are accepted as path data. Validate each `/`-separated piece with `safeUrlPathSegment` before encoding the object ID, so `catalog/..`, `../sku`, and separator variants are rejected.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
Closing for now — not because of a defect. This batch grew to 17 PRs across ~700 changed call sites, and we would rather revisit it as smaller, independently testable pieces than merge this much at once. Nothing here is lost: the branch |
The defect
Google Vault, AgentMail and Algolia tools interpolate LLM-writable identifiers into URL path segments. All of these params are
visibility: 'user-or-llm', so prompt injection controls them:matterId,holdId,exportId,savedQueryIdinboxId,threadId,messageId,draftIdindexName,objectID,taskIDencodeURIComponent, which is not enough)A value like
../../matters/victimescapes its API prefix oncefetchnormalizes the URL, re-aiming the request — with the user's Google OAuth token / AgentMail API key / Algolia admin key still attached — at a different resource.delete_matters,delete_matters_holds,delete_saved_query,delete_inbox,delete_thread,delete_draft,delete_index,delete_recordandclear_recordsare all destructive.encodeURIComponentis not sufficient, which is why the Algolia sites were vulnerable despite already encoding..and..are unreserved characters, so they survive encoding verbatim, and the WHATWG URL parser removes dot segments after decoding:Encoded separators are not decoded (
..%2F..%2Fmatters%2Fvictimstays intact), so the only dangerous shape is a segment that is exactly.or..— and the only fix is rejection.The fix
Every site now routes through
safeUrlPathSegment(value, paramName)from@/tools/url-path.Algolia
objectIDis the one exception. It is an arbitrary caller-chosen string, and Algolia's own clients percent-encode it, so a record keyedcatalog/sku-123is legitimate and has always worked —safeUrlPathSegmentrejects separators outright and would have broken those records.safeAlgoliaObjectId(new,tools/algolia/utils.ts) encodes a separator-bearing id whole, byte-identical to theencodeURIComponent(objectID.trim())it replaces, and defers everything else tosafeUrlPathSegment.A separator-bearing id needs no dot-segment check of its own, because percent-encoding collapses the value into a single path segment and the URL parser never decodes
%2F:So a value containing a separator cannot be the dangerous shape — that is a value whose entire text is
.or...No param
visibility, subBlock id, block definition, or generated artifact changed —tool-metadata:generateis a no-op on this branch.An AgentMail inbox ID legitimately is an email address (
example@agentmail.to).safeUrlPathSegmentpercent-encodes it, so ~30 call sites now sendexample%40agentmail.toinstead ofexample@agentmail.to. This is a real change on the wire.I verified it is safe against AgentMail's own SDKs before relying on it:
agentmail-to/agentmail-node) builds every path as`/v0/inboxes/${core.url.encodePathParam(...)}`, andsrc/core/url/encodePathParam.tsis a thin wrapper aroundencodeURIComponent. Every request the official Node SDK makes already sends%40.agentmail-to/agentmail-python) buildsf"v0/inboxes/{jsonable_encoder(inbox_id)}"— no percent-encoding, the@goes out raw.Both SDKs ship and work, so the AgentMail server decodes percent-encoding normally. The change is inert.
The AgentMail test asserts this explicitly: legitimate IDs must round-trip through
decodeURIComponentback to the exact value supplied, and there is a dedicated case proving an email-address inbox ID is encoded rather than rejected.Not a risk — deliberately left alone
downloadGoogleVaultExportFile(lib/internal/google-vault/operations.ts, the internal handler behindgoogle_vault_download_export_file; there is noapp/api/tools/google_vault/download-export-file/route.tson staging). It interpolatesbucketNameandobjectNameinto a GCS path, but both arevisibility: 'user-only'— not LLM-writable — and a GCSobjectNamelegitimately contains/, sosafeUrlPathSegmentwould break every real export download.matterIdis not used in that URL at all.applicationId. It lands in the host, not the path, and isvisibility: 'user-only'. Not a dot-segment vector.pageToken,attributesToRetrieve,permanent, …). They go throughURLSearchParamsorencodeURIComponentin a query position, where dot segments carry no meaning.Tests
New
path_safety.test.tsunder each of the three tool folders. 1741 tests pass.url.pathname, so a newly added unguarded path parameter fails CI automatically. Coverage floors plus named assertions pin the second ID of every two-ID route (.../holds/{holdId},.../exports/{exportId},.../threads/{threadId},.../messages/{messageId},.../drafts/{draftId},.../{objectID},.../task/{taskID}).new URL(...)— the same normalizationfetchperforms — never string-matched.LEGITIMATE_IDSlist proves real values pass through unchanged: an email-address inbox ID, Vault numeric matter IDs andholdId123456, Algolia index names with_,-and., plus..foo/foo... Algolia additionally provescatalog/sku-123still resolves to one encoded segment.Hole #1 (fixed, 2nd commit): all-params-at-once fuzzing
The first revision copied
tools/vercel/edge_config_path_safety.test.ts, which fills every string parameter with the same hostile value and then doestry { ... } catch { return }. The moment one parameter is guarded the whole vector is skipped, so/matters/{matterId}/holds/{holdId}and/inboxes/{inboxId}/threads/{threadId}were reported as covered while the second ID was never exercised.Measured directly — with
delete_matters_holds'sholdIdreverted to${params.holdId.trim()}:try/catch)Hole #2 (fixed, 3rd commit): a trailing
.is invisible to a shape checkhttps://host/a/.normalizes tohttps://host/a/— same segment count, same leading segments — so a shape-only assertion passes even with the guard removed.delete_matters,delete_inboxanddelete_indexall end in a guarded ID and are all destructive, so a shape check was weakest exactly where the damage is worst. Every pair now asserts.,..and' .. 'throw.safeAlgoliaObjectIdguards per slash-delimited piece, so the same blind spot exists one level down: a trailing.piece (catalog/.) encodes tocatalog%2F., a single segment no dot-segment normalization would ever touch. That is now asserted explicitly (catalog/.,catalog/..,catalog/./sku,catalog/../sku,./sku,../sku).Did these changes move the numbers? Yes — 37 → 45 failures. Scoped revert of the four trailing-ID destructive sites:
google_vault/delete_mattersmatterIdagentmail/delete_inboxinboxIdalgolia/delete_indexindexNamealgolia/delete_recordobjectID(
delete_recorddrops because the 6 per-piece assertions tested a risk that does not exist and were replaced by 3 whole-value rejections.)At all four sites, zero shape assertions fail on the bare
.— it is caught only by rejection. Andalgolia_delete_indexremains the clearest proof in the batch that encoding is not a fix: withencodeURIComponentrestored it fails precisely — and only — on the bare.,..and' .. 'vectors, passing every multi-segment escape, encoded separator, backslash, query and fragment vector, because encoding genuinely does neutralize those. That observation is recorded in all three file headers.Guards restored, 1741/1741 green.
Hole #3 (fixed, 4th commit): the traversal assertion skipped the ID's own segment — found by cubic
if (segment.includes(TARGET)) returnproved only the segment count and the surrounding segments. A traversal can satisfy both:The file header's own canonical escape passed against raw interpolation. Confirmed against the guard: a scoped revert of
delete_mattersproduced 7 traversal failures andmatter123/../../matters/victimwas not among them. The ID slot is now asserted equal toencodeURIComponent(value.trim()), so the same revert fails all 10 vectors — the three newly caught being cubic's vector,matter123#fragment, and the bare..Hole #4 (fixed, 4th commit):
safeAlgoliaObjectIdcorrupted valid object ids — found by greptile (P1) and cubicSplitting on
/and guarding each piece withsafeUrlPathSegmenttrims each piece (rewritingcatalog/ sku→catalog/sku) and rejects empty ones (catalog//skuandcatalog/sku/failed withobjectID is required). All three are legitimate ids the prior encoding preserved.Investigating showed the split was not merely harmful but unnecessary — see the Algolia note above. It now encodes whole. The earlier per-piece rejection assertions tested a risk that does not exist and were replaced by parity cases:
catalog/sku-123,catalog/ sku,catalog//sku,catalog/.,catalog/..,catalog/../../1/keys,./sku,../skuand a backslash-bearing id each assert equality withencodeURIComponent(value.trim()), while whole-value.,..and' .. 'still throw.I had claimed in an earlier revision that the trailing-dot blind spot "exists one level down" for pieces. That was wrong —
catalog%2F.is a single segment no normalization touches. The correction is in the code, the tests and the module TSDoc.Typing (3rd commit)
The harnesses inherited
type AnyTool = ToolConfig<any, any>andas anyfrom the Vercel file they were copied from, which CLAUDE.md forbids. Now:Params are built as
Record<string, unknown>sourl(...)needs no cast. Thereadonly unknown[]seed matters: a barrel's element union is not assignable to a widenedToolConfig, because the param type sits in the contravariant position ofrequest.url— seeding asunknown[]makes the type guard the single narrowing point. Oneas Record<string, unknown>remains per file, inside the guard, with a one-line TSDoc.Note for the batch:
apps/sim/tsconfig.jsonexcludes**/*.test.ts, sobun run type-checkdoes not cover any of these harnesses and CI would not catch type errors in them. I verified with a temporary tsconfig overridingexclude(not committed) — it found 6 realTS2345errors fromas constonCREDENTIAL_PARAMS, now fixed. The landedtools/vercel/edge_config_path_safety.test.tsandtools/daytona/*still carry theToolConfig<any, any>/as anyshape; out of scope here, flagging as follow-up.Gates
bun run lint,bun run check:audits(39 audits green includingcheck:api-validation:strict,check:tool-request-boundary,tool-metadata:check),bun run apps/sim/scripts/check-block-registry.ts origin/staging, andtsc --noEmitclean across all touched files.