fix(integrations): validate runtime values the type system only claims to constrain - #7244
Merged
waleedlatif1 merged 2 commits intoAug 29, 2026
Merged
Conversation
…s to constrain Three defects surfaced by review of the v0.8.16 release PR (#7224), each one a declared type standing in for a check that never runs. datadog: `DatadogSite` is a compile-time union, erased at runtime, and `site` is interpolated straight into the request host while every Datadog request carries DD-API-KEY and DD-APPLICATION-KEY. An unvalidated value therefore chose where the workspace's Datadog credentials were sent: `evil.com` addresses api.evil.com, and `datadoghq.com@evil.com` addresses evil.com with the expected host as userinfo. The site list is now a runtime array with the type derived from it, and both host builders -- `datadogApiUrl` (31 call sites) and the logs intake in send_logs -- resolve through one validator. Not reachable from the editor today, since the block renders a dropdown and the param is user-only; the value still survives in stored workflow state, which imports and programmatic edits write directly. cbinsights: `params.x?.trim()` guards undefined, not the type, so a block-to-block reference resolving to a number threw a bare TypeError naming no parameter. The sibling history operation already used `parseOptionalStringParam`; the remaining 19 sites now do too. Behaviour is otherwise unchanged -- `compactBody` already dropped '' and undefined alike, and every non-compactBody use tests falsiness. cbinsights rag: the guard admitted a 10,000-character message while its own error and the tool's param description both say "under 10,000". managed-agent: `denyMessage` was trimmed above the try block, so a non-string threw past every `success: false` path the operation otherwise returns. It is now coerced the same way `decision` is a few lines above. Two further findings on that PR were checked and left alone: the Drive addParents/removeParents overlap matches Google's own documented move sample in all four languages, and Bitbucket's Range behaviour on a zero-byte file is undocumented, so neither is a verified defect.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Greptile SummaryThe PR adds runtime validation where TypeScript declarations previously allowed malformed stored or referenced values to reach integrations.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/tools/datadog/utils.ts | Adds a runtime Datadog-site allowlist and makes the shared API URL builder validate its host input. |
| apps/sim/tools/datadog/types.ts | Defines the supported Datadog sites as runtime data and derives the compile-time union from that canonical list. |
| apps/sim/tools/datadog/datadog.test.ts | Covers every exported Datadog request URL builder against an attacker-selected site and verifies valid regional hosts. |
| apps/sim/lib/internal/cbinsights/operations/rag.ts | Validates the message runtime type and rejects the documented 10,000-character boundary. |
| apps/sim/lib/internal/managed-agent/operations/respond-tool-confirmation.ts | Normalizes scalar confirmation fields so malformed runtime inputs remain within the operation’s structured result behavior. |
Reviews (2): Last reviewed commit: "fix(datadog): route every host builder t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 19 files
Confidence score: 4/5
- In
apps/sim/lib/internal/managed-agent/operations/respond-tool-confirmation.ts, a stored workflow object with a non-functiontoStringcan throw before thetryblock, preventing a structured failure response; guard the runtime value or use safe scalar coercion.
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/lib/internal/managed-agent/operations/respond-tool-confirmation.ts">
<violation number="1" location="apps/sim/lib/internal/managed-agent/operations/respond-tool-confirmation.ts:42">
P2: When stored workflow data contains an object with a non-function `toString` field, this call throws before the `try`, so the operation returns no structured failure. Guard the runtime value or use safe scalar coercion before trimming.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…scalars safely Review round 1 on #7244 found the first pass incomplete, and both findings reproduce. The site allowlist only covered the shared `datadogApiUrl` and the logs intake. Ten tools build the host inline -- `const site = params.site || 'datadoghq.com'` in cancel_downtime, create_downtime, create_event, create_monitor, get_monitor, list_downtimes, list_monitors, query_logs, query_timeseries and submit_metrics -- so they never reached the validator while still attaching DD-API-KEY and, where the endpoint needs it, DD-APPLICATION-KEY. All ten now resolve through it. The new test sweeps the tool registry rather than naming tools, so a future tool that reintroduces an inline builder fails instead of shipping an unguarded request. `(value ?? '').toString()` was itself unsafe: an object whose `toString` is not a function, and one with a null prototype, both throw TypeError, and `String(value)` throws on the same two. That read sits above the try block, so it escaped the structured `success: false` result this operation promises. `normalizeScalarText` converts only the scalar kinds `String()` cannot fail on and returns '' otherwise, matching how `normalizeStringList` already treats a value of the wrong type. The identical hazard on `decision` two lines above is fixed with it as well.
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four verified defects from the review of the v0.8.16 release PR (#7224). Each is the same shape: a declared TypeScript type standing in for a check that never runs.
Two other findings on that PR were checked and deliberately not changed — details at the bottom.
Datadog —
sitedecided where the credentials wentDatadogSiteis a compile-time union, erased at runtime.siteis interpolated straight into the request host, and every Datadog request carriesDD-API-KEYandDD-APPLICATION-KEY:An unvalidated value therefore chose the destination for the workspace's Datadog credentials.
evil.comaddressesapi.evil.com;datadoghq.com@evil.comaddressesevil.comwith the expected host parsed as userinfo.The site list is now a runtime array with
DatadogSitederived from it, so the two cannot drift, and both host builders resolve through one validator:datadogApiUrl(31 call sites) and the separate logs-intake host insend_logs. That ternary insend_logshad three branches that all produced the same string, so it collapses to the one expression.Not reachable from the editor today — the block renders
siteas a dropdown and the param isuser-only, so neither a user nor a model can set it. The value still survives in stored workflow state, which imports and programmatic edits write without passing through that dropdown. This is the defence-in-depth layer, not a live exploit.CB Insights — optional chaining is not a type check
params.x?.trim()guardsundefined, not the type. A block-to-block reference resolving to a number reached.trim()and threw a bareTypeErrornaming no parameter. The direct sibling,get-commercial-maturity-history, already usedparseOptionalStringParamfor the identicalstartDate/endDatepair; the remaining 19 sites now do too.Everything else is unchanged, and that is checked rather than assumed:
compactBodyalready dropped'',null, andundefinedidentically, and every use outside it only tests falsiness — so''andundefinedwere already interchangeable at all 19 sites. The only behaviour that changes is the non-string case.CB Insights RAG — the guard contradicted its own contract
message.length > 10_000admitted a 10,000-character message, while both the thrown error and the tool's param description say "under 10,000 characters". Now>= 10_000, so the code, the error, and the documented contract agree.Managed Agent — a throw that escaped the result contract
denyMessagewas trimmed above thetry, so a non-string threw past everysuccess: falsepath the operation otherwise returns. It is now coerced the same waydecisionis a few lines above it.Checked and left alone
Drive
addParents/removeParents— the finding says sending the destination in both parameters can make Drive reject the update. Google's own documented move sample does exactly what this code does — fetch all current parents, pass them all asremoveParents, pass the destination asaddParents, with no exclusion — in C#, Java, Python, and JavaScript. No documentation supports the claimed failure, so changing it would mean deviating from the vendor's canonical pattern on a guess.Bitbucket zero-byte
Range— the finding saysRange: bytes=0-Nagainst an empty file can return 416 instead of an empty result. RFC 9110 makes that plausible, but Bitbucket's actual behaviour on the raw endpoint is undocumented and there is no test coverage either way. Unverified, so unchanged.Verification
bun run check:audits— 39/39bun run lint— cleantype-check— no errors in any touched file (the worktree's other errors are the known stale workspace-package resolution and exist on the base)tools/datadog,tools/cbinsights,tools/managed_agenttool-metadata:check,docs:check,integration-catalog:check, block-registry — all pass