fix(incidentio,trigger_dev): reject path-traversal ids and free the reserved timeout param - #7265
fix(incidentio,trigger_dev): reject path-traversal ids and free the reserved timeout param#7265waleedlatif1 wants to merge 4 commits into
Conversation
…eserved timeout param
Both integrations interpolated LLM-writable identifiers straight into
request paths. `encodeURIComponent` does not neutralize a dot segment —
`.` and `..` are unreserved, so they survive encoding and the WHATWG URL
parser removes them afterwards, popping a segment off a fixed host with
the caller's bearer token still attached:
new URL('https://api.incident.io/v2/schedules/' + encodeURIComponent('..'))
.pathname // => '/v2/'
Every path-interpolated id now goes through `safeUrlPathSegment`, which
rejects dot segments and separators instead of encoding them. 30
incident.io call sites and 26 Trigger.dev call sites are covered,
including the shared Trigger.dev env-var URL builder.
The Trigger.dev create-waitpoint-token tool also declared a param named
`timeout`, which `tools/request-transport.ts` reads as the outbound HTTP
request deadline in milliseconds — the user's waitpoint lifetime silently
shadowed the transport deadline. The tool param is renamed to
`waitpointTimeout` (following the `functionTimeout` precedent) and mapped
back onto the wire field in `tools.config.params`, which runs after
variable resolution. The `timeout` subBlock id is unchanged, so saved
workflow state stays valid.
New `path_safety.test.ts` suites enumerate both barrels, so a new tool
with an unguarded path param fails CI on arrival.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe PR validates incident.io and Trigger.dev path parameters before URL interpolation and separates the Trigger.dev waitpoint lifetime from the transport request deadline.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/blocks/blocks/trigger_dev.ts | Preserves the legacy canvas field while mapping both canvas and agent inputs to waitpointTimeout and preventing transport-timeout leakage. |
| apps/sim/tools/trigger_dev/create_waitpoint_token.ts | Renames the public tool parameter and maps it to the Trigger.dev API’s timeout request-body field. |
| apps/sim/tools/trigger_dev/utils.ts | Validates environment-variable URL path segments before constructing Trigger.dev endpoints. |
| apps/sim/tools/trigger_dev/path_safety.test.ts | Enumerates barrel-exported tools to verify traversal rejection, valid identifiers, and waitpoint parameter behavior. |
| apps/sim/tools/incidentio/path_safety.test.ts | Verifies all barrel-exported incident.io path parameters reject traversal inputs while preserving legitimate identifiers. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Input[Tool input] --> Path{Path parameter?}
Path -->|Yes| Validate[safeUrlPathSegment]
Validate -->|Invalid dot segment or separator| Reject[Reject request]
Validate -->|Valid identifier| Request[Build authenticated API request]
Path -->|No| Waitpoint{Create waitpoint?}
Waitpoint -->|Canvas timeout| Remap[Map timeout to waitpointTimeout]
Waitpoint -->|Agent waitpointTimeout| Remap
Remap --> Clear[Clear reserved transport timeout]
Clear --> Body[Send body.timeout as waitpoint lifetime]
Reviews (2): Last reviewed commit: "fix(trigger_dev): keep the renamed waitp..." | Re-trigger Greptile
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
… at once The suites inherited a coverage hole from the template they were modelled on: `buildParams` filled every string param with the same fuzz value, and the assertion swallowed the vector on any throw. So the first guarded param on a tool aborted the whole case and its siblings were never exercised — the "a new unguarded param fails CI" property did not hold for any tool that already had one guard. The unit under test is now a (tool, parameter) pair. Each param is fuzzed alone with every sibling pinned to a safe value, and the pairs are discovered by probing one marked param at a time rather than listed by hand, so a new path param joins the matrix on arrival. The Trigger.dev env-var tools are the case this was hiding: `buildTriggerDevEnvVarsUrl` interpolates projectRef, environment, AND name into one path, so guarding projectRef alone would have read as full coverage for all three. All three are guarded, and an explicit assertion now pins that so a dropped guard is legible in the failure output. 30 incident.io pairs and 43 Trigger.dev pairs, 1821 assertions.
Follow-up: tests now fuzz (tool, parameter) pairs, not toolsThe suites carried the coverage hole described above. Rewritten so the unit under test is a (tool, parameter) pair: each param is fuzzed alone with every sibling pinned to Matrix: 30 incident.io pairs + 43 Trigger.dev pairs. 1821 assertions pass (was 1523).
|
… unbuildable tools Two review points from sibling PRs. The harness typed tools as `ToolConfig<any, any>` and cast the synthetic param bag with `as any`. Replaced with a local `FuzzableTool` interface naming the structural slice the harness actually needs, so the call to `request.url` is type-checked rather than cast. No `any` remains. Discovery also swallowed any error from building a tool's URL, so a tool that could not be built from all-safe values would leave the matrix and read as "nothing to guard" — the same class of bug as the swallowed per-vector error this suite was just rewritten to fix. Failures are now collected and asserted empty, naming the tool and the underlying error.
Rejection assertion: already present, and verified non-vacuousThe suite asserts throw, per (tool, parameter) pair, not just path shape: it('rejects a bare dot-dot segment, naming the offending parameter', () => {
expect(() => buildUrl(tool, param, '..')).toThrow(new RegExp(param))
})
it('rejects a bare dot segment, naming the offending parameter', () => {
expect(() => buildUrl(tool, param, '.')).toThrow(new RegExp(param))
})Confirmed the way you suggested. Reverting Note what is absent: Two review items from the sibling PRs
interface FuzzableTool {
id: string
params?: Record<string, { type?: string }>
request?: { url?: string | ((params: Record<string, unknown>) => string) }
}The type guard narrows from Discovery no longer swallows. It previously did Verified non-vacuous by making one tool's Gates1823 assertions pass (was 1821; +2 discovery guards). |
The `timeout` -> `waitpointTimeout` rename stops the param shadowing the
shared transport's own ms deadline, but the block mapping only read the
subBlock spelling, and it assigned unconditionally:
result.waitpointTimeout = scoped(params.timeout, [...])
`tools.config.params` is not only the UI mapper. `providers/utils.ts`
installs the same function as the provider `paramsTransform` and spreads
its result over the model's tool-call arguments, exactly as the generic
block handler spreads it over the resolved subBlock inputs. Every key it
assigns therefore wins — including when it assigns `undefined`.
On the agent path the model supplies the tool's own param name,
`waitpointTimeout`, and no `timeout` subBlock value exists, so the
assignment evaluated to `undefined` and erased what the model sent.
`body.timeout` was never set and the waitpoint was created with no
lifetime. Before the rename the model emitted `timeout`, which the
mapping never touched, so this path regressed with the rename itself.
Read both spellings through the same `??` chain the neighbouring
`taskIdentifier`, `delay`, `tags` and `idempotencyKey` mappings already
use, so a configured subBlock value still wins over the model argument.
Scope: this was the only unguarded rename of its kind. daytona, apify,
twilio, elasticsearch and firecrawl each guard the assignment on the
source being present.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
|
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 |
1 participant
Defect 1 — path traversal (both services)
incidentioandtrigger_devtools interpolatedvisibility: 'user-or-llm'identifiers directly into request path segments. A value like../../v2/incidents/victimre-aims an authenticated request — carrying the user's bearer token — at a different resource, including on the DELETE routes (schedules, workflows, custom fields, incident roles, env vars).encodeURIComponentis not sufficient..and..are unreserved, so they survive encoding, and the WHATWG URL parser removes dot segments after decoding:Only rejecting the value works, so every path-interpolated id now goes through
safeUrlPathSegment(value, paramName)from@/tools/url-path.actions_*,alerts_*,alert_events_create,custom_fields_*,escalation_paths_*,escalations_*,follow_ups_*,incident_roles_*,incidents_*,incident_timestamps_show,on_call_now,schedules_*,teams_show,users_show,workflows_*.buildTriggerDevEnvVarsUrlhelper (projectRef,environment,name).Not a risk — left alone
The
list_*tools intrigger_devinterpolate aqueryStringinto the URL, but it comes fromURLSearchParams.toString()and sits after the?, so it cannot reshape the path. Everyincidentiolist tool builds its URL withnew URL()+searchParams. No changes there.Defect 2 — reserved
timeoutparam (Trigger.dev)create_waitpoint_tokendeclared a param namedtimeout.tools/request-transport.tsreadsparams.timeoutas the outbound HTTP request deadline in milliseconds:so the user's waitpoint lifetime silently shadowed the transport deadline.
Renamed the tool param
timeout→waitpointTimeout(following thefunctionTimeoutprecedent from AWS Lambda), because Copilot callsexecuteAppToolwithout ever runningtools.config.params— a block-level workaround would not cover it. The value is mapped back onto the wire fieldtimeoutinrequest.body.In
blocks/blocks/trigger_dev.tsthe mapping lives intools.config.params(which runs after variable resolution, so<Block.output>references survive), never intools.config.tool. The subBlock idtimeoutis unchanged, so saved workflow state stays valid;result.timeoutis explicitly cleared so the merge ingeneric-handler.tscannot leak it to the transport.No param's
visibilitychanged, no subBlock id changed, and no behaviour changed for legitimate input.Tests
New
tools/incidentio/path_safety.test.tsandtools/trigger_dev/path_safety.test.ts, modelled ontools/vercel/edge_config_path_safety.test.ts:new URL(...)— the same normalizationfetchperforms — never string-matched.and..vectors are kept, since their absence is what makes anencodeURIComponent-only fix look correctLEGITIMATE_IDSlist proves real values (run_abc123,batch_9xkq2m, ULID-shaped incident.io ids,my.task.identifier,..foo,foo..) pass through unchangedVerified red first: with the source fix reverted and only the new tests present, 413 assertions fail across the two files (traversal reshaping plus both
timeout-shadowing cases). Restored, all 1523 tests in the two directories pass.Gates
bun run tool-metadata:generate(committed),bun run lint,bun run check:audits(all-green, docs regenerated),bun run apps/sim/scripts/check-block-registry.ts origin/staging. Type-check is clean for these files; the only errors are the pre-existing unrelated@aws-sdk/client-lambdaresolution failures.Follow-up: the rename regressed the agent path (a533389)
Validation audit against the Trigger.dev management API turned up a defect the rename itself introduced. The block mapping only read the subBlock spelling, and it assigned unconditionally:
tools.config.paramsis not only the canvas mapper.providers/utils.tsinstalls the same function as the providerparamsTransformand spreads its result over the model's tool-call arguments (result = { ...result, ...transformed }), exactly asgeneric-handler.tsspreads it over the resolved subBlock inputs (finalInputs = { ...inputs, ...transformedParams }). Every key it assigns therefore wins — including when it assignsundefined.On the agent path the model supplies the tool's own param name,
waitpointTimeout, and notimeoutsubBlock value exists. So the assignment evaluated toundefinedand erased what the model sent:body.timeoutwas never set and the waitpoint was created with no lifetime. Before the rename the model emittedtimeout, which the mapping never touched — so this is a regression of the rename, not a pre-existing gap.Fixed by reading both spellings through the same
??chain thattaskIdentifier,delay,tagsandidempotencyKeyalready use two lines above, so a configured subBlock value still wins over the model argument.Scope — this was the only unguarded rename of its kind. The other four
timeoutrenames in the batch each guard the assignment on the source being present, so none of them can clobber a model argument:if (rest.timeout !== undefined && rest.timeout !== '')if (rest.timeout)if (operation === 'make_call' && timeout)if (typeof params.timeout === 'string' && ...)if (params.mapTimeout != null && ...)Four new tests in
path_safety.test.tscover the canvas path, the agent path, subBlock-wins-over-model precedence, and non-leakage into another operation. Verified red first: reverting only the one-line fix turns exactly one of them red; 1101 tests pass restored.