fix(integrations): harden paths across 20 services, repair Elastic Cloud, align declared shapes - #7133
Open
waleedlatif1 wants to merge 58 commits into
Open
fix(integrations): harden paths across 20 services, repair Elastic Cloud, align declared shapes#7133waleedlatif1 wants to merge 58 commits into
waleedlatif1 wants to merge 58 commits into
Conversation
safeUrlPathSegment guards a single segment; storage paths are legitimately multi-segment, so add safeUrlPath which splits on '/', rejects any empty or dot segment, and encodes each part. encodeStoragePath split and encoded per segment, which reads as sanitisation but maps '../..' through unchanged. It was the only guard on bucket/path for nine storage tools. Corrects the url-path TSDoc: no encoding scheme neutralises a dot segment, because the WHATWG parser removes them after decoding, so %2e%2e is popped too. Only value rejection works. The table/functionName tools are deliberately left alone: they already call validateDatabaseIdentifier with a stricter allowlist, so a second guard would be unreachable. url-safety.test.ts pins that validator instead.
…n scope serper: - the places mapper read link/snippet/reviews, none of which the /places endpoint returns, while SearchResult.link was typed non-optional -- so every places result shipped link: undefined against a schema promising a string - ratingCount was declared as an output and never populated; the places branch read a nonexistent 'reviews' key instead. The find-local-businesses skill asks the agent for a review count, which was unreachable - the news mapper dropped the publisher 'source' - deleted five phantom output fields (knowledgeGraph, answerBox, peopleAlsoAsk, relatedSearches, topStories) that transformResponse never populates, plus 14 zero-reference output-property constants, and rewrote the tool description which advertised them to the model linkedin: - w_member_social was described to users as 'Access LinkedIn profile'. It is a write scope -- post, comment and like on the member's behalf -- and that string renders in the OAuth consent modal, so users were told the app wanted read access when it will post as them - postUrl was declared and never written; it is constructible from the URN the tool already reads. Described as viewable by an authorized member rather than as a public permalink, per LinkedIn's own wording - neither tool declared outputs at all; added ones matching exactly what each transformResponse returns
…mment
langsmith:
- a TSDoc claimed only /api/v1/* paths exist in the published spec. The live
spec has ~303 v1 and ~38 v2 paths, and the v2 set contains the replacement
for our deprecated get_run -- so the comment actively misdirected that fix
- create_feedback never sent session_id, and the block's session_id subBlock
was condition-gated to create_run so it could not be supplied at all. Added
as optional: it is absent from the spec's required array and LangChain's own
SDKs still post without it, so requiring it would break saved blocks today to
pre-empt a 2027 removal
- get_run stays on the deprecated v1 read, deliberately. The v2 replacement
needs a project_id this tool never collects, and silently re-types four
values existing workflows already read. A dated, loud break in Jan 2027 with
the migration recipe written beats a silent wrong-data break now
- feedback value could only ever be a string: the block renders a short-input,
so 1 went on the wire as "1". Parses scalars and objects at runtime, keeps
unparseable text as text
firecrawl:
- search declared outputs.data as an array while /v2/search returns it
source-keyed, so every call emitted {web: [...]} where an array was promised.
Flattens in a fixed web/news/images order
- eight params were read in body but never declared, so six were unreachable
from both the block and the model
…h params 129 path-zone interpolation sites across 97 files, every one visibility 'user-or-llm', now guarded with safeUrlPathSegment. An LLM-supplied '..' pops a path segment after the URL parser normalises, so a DELETE re-aims at a sibling endpoint carrying the user's credential. Encoding does not prevent it -- '.' and '..' are unreserved, and dot segments are removed after decoding, so %2e%2e is popped too. Only value rejection works. Three Cloudflare sites already had encodeURIComponent and were live holes for exactly that reason. cloudflare/get_zone_settings builds its path in a module-scope helper shared by request.url and a directExecution fetch, so guarding the helper covers a path the transport layer never sees. Query-zone and host-zone interpolations are deliberately untouched. Tests resolve through new URL() and assert segment count plus fixed-segment identity -- a prefix check alone stays green when a single segment is popped. Each suite also poisons one path param at a time, because filling every param masks an unguarded second one behind the first guard's throw.
… JSON traversal: 105 path-zone sites across 69 files, all visibility 'user-or-llm', now guarded with safeUrlPathSegment. Every one was a bare interpolation except the two emoji params, which used encodeURIComponent and were live holes anyway. Multi-param templates are guarded in full -- Discord remove_reaction has four path params, Attio get_attribute has three. attio json: 12 sites across 11 files swallowed a JSON parse failure and sent a substituted value while reporting success. Ten replaced the caller's input with an empty array or object; create_list and update_list sent the raw unparsed string into a field the API expects as an array. Both params are LLM-writable, so slightly-off JSON silently discarded what the caller asked for. All now throw, matching the existing update_record precedent. Attio documents no empty-array semantics, so this is stated as discarded input rather than confirmed remote data loss. Query-zone interpolation is out of scope here and left as-is; assert_record still passes matchingAttribute raw into a query param.
67 tool files. These proxy the user's Google or Atlassian OAuth token, so a popped path segment re-aims an authenticated DELETE at a sibling endpoint against their own Drive, Vault or Jira data. BigQuery already used encodeURIComponent, which does not help: '.' and '..' are unreserved and dot segments are removed after decoding. Vault, Drive and most of Jira had no encoding at all. Two site classes are invisible to request.url() and were found by reading: google_drive/move builds its URLs inside directExecution, and 12 Jira tools rebuild the same path a second time in transformResponse. Both are guarded. BigQuery identifiers are safe under the guard -- dataset ids are letters, numbers and underscores only, and a qualified reference uses a colon rather than a slash, so project:dataset and example.com:legacy-project pass through. Jira issueKey previously fell back to an empty string, producing a malformed /issue//comment; it now throws. Every affected param is required, so this turns a guaranteed 404 into a clear error. Left alone: Jira cloudId (hidden, server-derived) and Drive file ids taken from Google's own responses rather than from the caller.
…l defects
Cloud ID: buildBaseUrl substituted the user-chosen label for the Elasticsearch
UUID, so every Elastic Cloud deployment resolved to a hostname that does not
exist -- a DNS failure with no clue for the user, across all 13 tools. A
decoded Cloud ID is <host>$<es-uuid>$<kibana-uuid> and the label before the
colon is decorative; six official Elastic clients discard it. Payload is now
split at the first colon only, an absent label is accepted, and the port is
appended only when it is not 443.
Buffer.from(x,'base64') never throws -- it silently drops non-alphabet
characters -- so the empty catch was unreachable and its comment claimed a
fallback that did not exist. Deleted, and replaced with an explicit alphabet
and structure check so a malformed id fails loudly instead of decoding garbage.
The 13 duplicated copies are consolidated into one utils.ts: net -698 lines.
Also:
- every if (!response.ok) inside transformResponse was dead code, because the
executor throws first. get_document advertised found:false and
delete_document advertised result:'not_found', neither of which can occur.
Branches deleted and the output descriptions corrected
- get_index declared an 'index' output that exists at no level of Elastic's
index-keyed response; now flattened so the declaration is true
- size:0, the standard aggregations-only idiom, was dropped by a truthiness
gate, so ES returned its default 10 documents
- the ES duration param shadowed the transport's reserved millisecond timeout.
Copilot bypasses the block's param mapper, so a model emitting "30" got a
30ms deadline. Renamed the tool param; the subBlock id is unchanged so no
saved workflow state is orphaned. An endsWith('s') check also turned 1m into
1ms
…e path params 96 path-zone sites across 70 files, every one visibility 'user-or-llm'. qdrant search_vector already used encodeURIComponent and read as guarded, but collection='..' still gave /collections/../points/query -> /points/query. The cluster host is user-only, so this is not SSRF; the LLM-controllable path segment re-aims an authenticated request at a different endpoint on the user's own cluster. fetch_points and upsert_points were raw. algolia's taskID is declared type:'number', so it is converted with String() at the callsite rather than loosening the helper signature. Excluded deliberately: the algolia and qdrant hosts (user-only, host zone), query-zone params, and body-zone values. No Spotify URI reaches a path segment -- they all travel in a body or query. The suite proves the assertion design rather than assuming it: against the unfixed search_vector, a prefix-only check passes on a one-segment pop while the segment-count and shape check fails. X's manage_* tools build their second path param conditionally, so the independence block drives those branches and asserts the reached param list matches the declared one.
… params 124 path-zone sites across 103 tools, every one visibility 'user-or-llm'. 100 were bare interpolations; 24 already had encodeURIComponent and were exactly as exploitable -- reverting a guard back to encodeURIComponent still fails all five rejection vectors. Five sites assign into a local variable first and are invisible to a grep sweep; they were found by resolving each tool's request.url() with a sentinel per declared param. HubSpot's association tools carry up to four path params each, all guarded. No param here legitimately carries multiple segments, so safeUrlPathSegment is correct throughout and safeUrlPath was not needed. Behavior note: the previously-raw sites now percent-encode, so an AgentMail inbox id that is an email address travels as yourinbox%40agentmail.to. It is not rejected, decodeURIComponent round-trips byte-identically, and AgentMail's own generated clients encode path params the same way. An email local-part containing a literal slash would now be rejected.
131 path-zone sites across 114 files. Every one already had
encodeURIComponent(params.X.trim()), so every one looked guarded and none was:
'.' and '..' are unreserved and survive encoding, and the URL parser removes
dot segments after decoding.
Concretely, with an unguarded keyId of '..':
GET /api/v2/tailnet/{tailnet}/keys/.. resolves to /api/v2/tailnet/{tailnet}/
so the request re-aims at the tailnet resource itself with the user's bearer
token, while a startsWith() check on the prefix stays green. The suites assert
segment count and fixed-segment identity, which is what actually catches it.
Okta's domain comes from its own validator and is host-zone; Rippling and
Tailscale use fixed hosts. Query-zone params are out of scope and untouched --
the only encodeURIComponent calls left in these directories are the five
Rippling ?expand= sites, confirmed query-zone by the probe.
No param here legitimately carries multiple segments; the closest, Rippling's
externalId, is a single-segment customer key.
…d fields sixtyfour: both enrich tools call the sync endpoints, which the vendor documents at P95 ~5 minutes and tells clients to allow 15 minutes for, against a 300s platform default. Injects a 900s deadline via tools.config.params -- a tool-level 'default' would have been a no-op, since param.default is only read to build the LLM/UI schema and never applied at execution. Two limits worth knowing: the agent-tool path never calls tools.config.params, so an LLM invoking the tool directly still gets the platform default; and on the free plan the 300s workflow sync ceiling binds first, so the fix only bites on pro and above, and on async runs. Also: struct was required on the lead path though the spec requires only lead_info (company genuinely does require it), and tier -- the vendor's research-depth and cost control -- was not exposed at all. enrow: any non-202 poll response aborted the job. The docs say 429 and 5xx are safe to retry with backoff, but also that an expired search id returns 500, so 500 is ambiguous. Bounded retries via the shared backoffWithJitter helper, with every retry wait charged against the existing 120s budget so a flaky upstream costs polls rather than wall-clock. A persistent 500 still exits with the upstream status. vercel: update_edge_config_items returned a hardcoded status:'ok' without reading the body Vercel guarantees. daytona: toolboxProxyUrl is a required per-sandbox field that mapDaytonaSandbox silently dropped from its allow-list. Surfaced, and the URL helper now accepts a base. Threading it through the six toolbox tools is deferred -- request.url is synchronous, so a lookup would need an async builder or a per-tool cache.
… multi segment
These tools split two ways, and getting the split wrong breaks them.
Multi-segment (safeUrlPath): GitHub 'path' (src/newfile.ts), 'branch'
(feature/foo works today, so a single-segment guard would break delete_branch),
and get_commit's 'ref' (documented as heads/BRANCH_NAME); google_contacts
'resourceName' (people/c123). Slashes survive; only an empty or dot segment is
rejected.
Single-segment (safeUrlPathSegment): owner, repo, gist ids, label names, and
all 25 Salesforce record ids.
Numeric path params needed guarding too: tools/params.ts performs no runtime
coercion of type:'number', so a block input can deliver the string '..' to
${params.issue_number}. That is a live traversal on delete_comment,
delete_release, delete_milestone and the reaction deletes. Converted with
String() at the callsite rather than loosening a helper signature.
salesforce nextRecordsUrl is an entire path appended at the org root, where
dot-segment rejection alone buys little because there is no prefix left to pop.
Guarded with safeUrlPath plus an assertion that it stays under services/data/,
which every cursor Salesforce emits does.
Left unguarded deliberately: compare_commits base/head. Refs may contain
slashes, and GitHub documents a cross-fork USERNAME:BASE form that encoding
would break. Marked UNVERIFIED rather than guessed at.
Also fixed an existing job_logs test that asserted the old encode-only
behavior, splitting it into a rejection case and a query-escaping case.
… param
request-transport reads params.timeout as the outbound fetch deadline in
milliseconds. Eight tools declared a user-facing param with that exact name
meaning something else, so the value silently became a deadline:
daytona run_code / execute_command seconds -> 10 became a 10ms deadline
apify run_actor_sync/async/task seconds -> 300 became 300ms
twilio_voice make_call seconds -> the 60s ring default, 60ms
new_relic nrql_query seconds
trigger_dev create_waitpoint_token duration -> '30s' is inert, '3600' is 3.6s
Each user-facing param is renamed; the value sent to the provider is
byte-identical (daytona still sends body.timeout, apify still sets ?timeout=,
twilio still appends Timeout=, new relic still emits timeout: N in NRQL).
The rename has to be on the tool, not the block mapper: Copilot calls
executeAppTool directly and never runs tools.config.params. And the mapper must
set timeout: undefined explicitly, because generic-handler spreads
{...inputs, ...transformedParams}, so merely omitting the key still leaks the
raw subBlock value.
Every subBlock id stays 'timeout', so no saved workflow state is orphaned.
Left alone deliberately: firecrawl, function/execute, sixtyfour and
http/request all genuinely mean milliseconds -- http/request's purpose is to
set the transport deadline.
Reflects the param, output and description changes in this branch across apify, daytona, elasticsearch, firecrawl, langsmith, linkedin, new_relic, serper, sixtyfour, trigger_dev, twilio_voice and vercel.
sixtyfour: three block param assignments were unguarded, so on the agent path a spread of an explicitly-undefined key overwrote the model's value and the call went out missing a required field. leadInfo, targetCompany and struct are all required:true on their tools; only the lead struct had the guard. sixtyfour: the struct description claimed omitting it lets the vendor choose the fields. The docs say the opposite -- only fields listed in struct appear in structured_data, and there is no default set -- so omitting it returns an empty structured_data while still billing the tier. That text ships to the model in tool-metadata, so it has to be true. enrow: the hosted-credit rate cited a Starter plan at $24 for 2,000 credits. No such plan exists; the tiers are Start $17/1k, Pro $87/10k, Scale $397/50k. Repriced to the entry Start tier at $0.017, matching how datagma and leadmagic justify theirs. The old 0.012 matched no tier and under-billed by 29%. vercel: the block never surfaced the status the tool now returns, and the transform parsed the body unconditionally, so an empty or 204 response would throw where the previous hardcoded value returned.
The places mapper had been written against the /maps response, not /places. serper.dev's tab-switcher has ten panels and BOTH Maps and Places return a top-level 'places' array with different item shapes; Maps is the one carrying a top-level 'll'. An earlier extraction collapsed the two. Consequences: category read item.type and snippet read item.description, both of which exist only on /maps, so both were permanently undefined. The test fixture was built from the same Maps example, so it confirmed the wrong endpoint. Rebuilt from the real /places example, plus two guard tests that a Maps-only key must not produce output. /places genuinely has no link and no snippet analogue, so those stay unmapped; latitude and longitude are now surfaced, which the find-local-businesses skill needs to rank by proximity and previously could not. Also restored knowledgeGraph, peopleAlsoAsk and relatedSearches, which /search really returns and which a previous pass deleted as phantoms. They were never populated -- but deleting the declarations meant discarding real, already-billed data. Gated on the search vertical so other verticals cannot emit them. answerBox and topStories stay deleted; they appear in none of the ten panels. images and shopping mappers also read a snippet neither endpoint returns. linkedin: /v2/ugcPosts returns a ugcPost URN, not the legacy urn:li:share: family the descriptions and every test fixture claimed.
langsmith value: widening the param to 'json' made buildParameterSchema
advertise {"type":"object"} to every model, so an agent could no longer send
the ordinary scalar case the spec union exists for. Reverted to string.
The real bug was elsewhere: parseLangsmithFeedbackValue ran only in the block's
param mapper, so the LLM and direct-tool paths got no coercion at all. It now
runs in request.body. Also closed three parser gaps -- the literal text 'null'
was posting JSON null, and '1.0', '007' and long numeric ids lost their form.
The round-trip guard is scoped to numbers so objects still parse.
langsmith sessionId: the description invented a deprecation date. POST
/api/v1/feedback is not deprecated and returns no sunset header; the 31 Jan
2027 date belongs to the run-read endpoint. Now quotes only the migration doc
and the spec field description, and tells the caller never to guess the UUID,
naming the two places it actually comes from.
firecrawl search: flattening the source-keyed response into one array was the
wrong call. news items carry snippet not description, images' url is the
containing page while imageUrl is the image, position is per-source, and
metadata is absent for images -- so the flattened array silently mislabelled or
dropped every non-web field. The declared output is now the envelope Firecrawl
documents, with three separately typed optional arrays. limit is per-source, so
three sources at limit 100 returns up to 300 results; the description says so.
Also: ignoreInvalidURLs was declared for search but its subBlock condition
listed only batch_scrape.
…declared-shape defects
Cloud ID: every component of a decoded Cloud ID may carry its own port, and the
ES UUID's port overrides the parent DN's. We used parts[1] raw, so a Cloud ID
of that form produced https://uuid:9244.host:9243 -- an invalid URL that fetch
rejects with no actionable message. Elastic's own beats fixtures pin all three
directions, including that Kibana's port never affects the ES origin.
get_index: the target accepts wildcards and comma-separated lists, returning
one key per resolved index. Keeping Object.keys(data)[0] silently discarded
every other index. Now also emits the full keyed map and a matched count, so
data_stream and lifecycle survive too.
index names: rejecting a slash broke Elastic's documented date-math form
<logstash-{now/d}>. An index name cannot contain a literal slash, so the
rejection protected nothing -- and an encoded slash is inert, because the URL
parser does not treat %2F as a separator, unlike %2e which it decodes and
removes. A local helper drops only the separator check; documentId stays on the
strict shared guard.
esTimeout: the unit coercion lived only in the block, and Copilot calls
executeAppTool without ever running tools.config.params -- so a model sending
"30" hit ES with a unitless duration and got a 400. Normalization now lives in
the tool and the block calls the same helper.
aggregations was declared as an output on both tool and block but no aggs param
exists, so it could never be populated. Removed, and the size:0 rationale
corrected -- it asks for hits.total without materializing documents, which is
not the aggregations idiom.
algolia, box, x and spotify received path guards but had no test files at all, so a future edit dropping a guard would go unnoticed. All 20 guarded services now carry the same suite shape: tools enumerated from the barrel, params classified by where a sentinel lands in the resolved URL so query- and host-zone values drop out structurally, dot segments asserted to throw, encoded vectors asserted inert, and one param poisoned at a time so an unguarded second param cannot hide behind the first guard's throw. Two service specifics the probe had to handle: algolia's taskID is declared type:'number', so a string-only sentinel skips it entirely; and X builds its second path param conditionally on an action value, so the suite drives those branches and asserts the reached param list matches the declared one. Also fixes a soft spot in the discord suite: a tool whose URL builder threw unconditionally was silently skipped rather than reported, so a tool that became unbuildable looked covered. Those are now recorded and asserted empty.
…alues Three false rejections the guards introduced, all verified live before changing: GitHub label names legitimately contain slashes -- area/apiserver, kind/bug -- and both the literal and encoded forms return 200 with the same label id, with GitHub echoing the literal form as canonical. remove_label was using the single-segment guard, so it hard-threw on a label GitHub accepts. safeUrlPath trimmed each segment, so a Supabase key of 'folder/ report .csv' was silently rewritten to a different object -- 404 or the wrong file, with no error. Supabase's own server regex permits a literal space in both object keys and bucket names. Now only the whole value is trimmed. This cannot re-open traversal: the URL parser pops a segment only when it is exactly '..', and an encoded space keeps it inert. A trailing slash on a GitHub contents path used to work -- GitHub 302s to the slash-free form and fetch follows it -- and get_tree's own description invites a directory path. Stripped at the GitHub callsites via a local helper rather than weakening safeUrlPath for its other ~700 callers. Also: a number reaching a string param became '' and threw 'X is required' even though it was supplied. That regressed the 53 sites whose baseline was a bare interpolation. Now coerced with String(), with null and undefined rejected before coercion so they still throw rather than becoming 'null'.
… parts The base64-alphabet check validated the encoded payload, not the decoded components, so a Cloud ID could inject into the assembled origin. Elastic's own decoder rejects #@?/ in the host and each UUID, with fixtures named inject-es, inject-kb and inject-host. An '@' is the serious one: it turns the UUID into userinfo and hands the origin to the attacker, so the ApiKey header is sent to their host. The other three truncate the authority to a bare label. Guarded on the decoded component before assembly rather than on the finished string, since checking the assembled URL means re-parsing the parse being subverted. ':' is rejected too, beyond Elastic's set: the port split takes only the last colon, so a second one produces a two-colon authority and a bare Invalid URL -- the same unactionable failure the port fix removed. Both are no-ops on a real hostname and a hex UUID. The Kibana UUID is deliberately not checked: we only ever build the Elasticsearch origin, so rejecting on a component that never reaches our URL would refuse a Cloud ID that works fine for search.
…ixes Reflects the corrected Serper places mapping and restored search extras, the Firecrawl source-keyed search output, the LangSmith feedback value and sessionId changes, and the Elasticsearch index and get_index changes.
Contributor
|
Too many files changed for review (911 files, 500 file limit). |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 752 files
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
waleedlatif1
changed the title
…eout race
map declared links as an array of strings, but v2 returns MapDocument objects
{url, title?, description?} -- so every consumer reading <firecrawl.links> as
URLs got [object Object]. Same declared-vs-real class this branch fixed for
search, one file over.
scrape's transformResponse read data.data.markdown unguarded. Firecrawl's error
bodies carry no data key, so any non-happy path threw a TypeError instead of
surfacing the error; every sibling tool already guards.
The country description claimed an unconditional "US" default. The schema
applies it only when location is unset.
sources and categories were unconstrained strings against a strict-object
schema, so an LLM emitting an invalid value got a hard 400. Constrained with
const unions, verified to survive into the model-visible schema. Adds the
'developer' category, which is a live enum member with its own aliases.
timeout shadowed the transport's reserved param on the three external tools.
The units agree, so this looked safe -- but the 30s headroom the executor adds
applies only to internal routes, and the external branch passes the deadline
bare. Sim's clock starts earlier, so Sim always won the race and the user never
saw Firecrawl's structured 408. parse is internal-route and keeps the old name.
The subBlock's condition only hid the field, so a stale value still reached the
transport on operations that never declared it; the mapper now clears it once.
…ansport Renaming the reserved timeout param in this branch removed the only thing that was setting a fetch deadline for Apify, so every sync run began riding the bare 300000ms default -- and Apify's sync endpoint returns 408 at exactly 300s. A run near the boundary was a coin flip between Apify's structured 408 and Sim's generic timeout, which loses the provider's diagnosis. The two sync operations now get a deliberate 330s transport deadline, mirroring the 30s headroom the executor already applies to internal routes. run_actor_async polled with bare global fetch, so the dataset read bypassed the response-size cap entirely. It now re-enters executeTool -- the pattern luma and google_docs already use -- reusing apify_get_run and apify_get_dataset_items rather than new request code, so the read is byte-bounded. The loop also checked status before its first sleep, and its exhaustion message no longer hardcodes a duration that the configurable ceiling can contradict. An explicit 0 was dropped by a truthy guard while Apify documents 0 as 'no timeout', so an unbounded run was inexpressible. Note the value that was being dropped is a numeric 0 from a block-output reference; a subBlock string '0' was always truthy. run_actor_sync fabricated runId: 'sync-execution'. The endpoint returns only pagination headers and no run id, so any downstream get_run wired to it was a guaranteed 404. The field is omitted rather than invented. Two limits remain and are documented in the code: the agent path bypasses the block mapper, and executeNestedTool does not forward the abort signal.
…ex coercion buildBaseUrl accepted a scheme-less host and returned it relative, so the executor resolved it against Sim's own origin while buildAuthHeaders still attached the credential: 'es.internal' -> https://sim.ai/es.internal/... credential to Sim '//evil.com' -> https://evil.com/... credential to an attacker-chosen origin The protocol-relative form is the serious one -- it inherits Sim's scheme and exfiltrates the ES ApiKey to any host. The SSRF check passes in both cases because the resolved host is not the user's cluster. A scheme is now required, and the original string is returned rather than the parser's normalized href so hosts reach ES exactly as typed. No auto-prefixing: silently upgrading a plaintext cluster to https would turn a typo into a different host. safeIndexPathSegment, added for the date-math carve-out, had copied the 'typeof value === string ? trim : ""' form that toGuardedString had just been changed away from -- so a numeric index threw 'index is required' while a numeric documentId in the same URL worked. It now mirrors the shared helper, and a test pins the two producing identical output. The parseCloudId comment attributed its rules to the wrong clients. The first-colon split is Python and .NET; Beats uses last-colon. Per-service ports are Beats and .NET. Omitting :443 is .NET only. The forbidden-character set is Beats only, and only on recent branches. Rewritten to attribute each rule and to state where Sim deliberately diverges. The block required an index for bulk while the tool declares it optional -- and _bulk legitimately runs against /_bulk with per-action _index.
Collaborator
Author
|
@greptile review |
shell-layout.test.ts carries a 'biome-ignore lint:' comment that suppresses nothing, which biome reports as suppressions/unused. It is identical on staging, so lint:check fails there too; removing it unblocks CI. Verified the suppression was masking no real violation and the test still passes.
Pi search set params.timeout, which served as both the transport deadline and Firecrawl's own body timeout -- the collision this PR exists to remove. Renaming the tool param dropped the body field from the host path while the sandbox path still hardcoded it, so the two diverged. Setting firecrawlTimeout restores the documented intent that both paths send the same effective deadline.
The status output added in this PR was conditioned on 1 of the 7 operations that emit it, and filterOutputsByCondition deletes a failing output rather than leaving it empty -- so delete_alias, whose only response field is status, surfaced nothing real. - narrow state to the two ops that emit it; declare readyState for the two that emit that instead - narrow the deleted condition; delete_alias and delete_deployment never emit it - drop the framework option the API rejects; add the two deployment states the filter accepts - trigger_dev described env var values as plaintext, but the SDK redacts secrets; surface isSecret so a workflow can tell the difference, failing safe when it is absent
…k outputs The feedback description told the model the official SDK omits session_id. It does not -- it warns, and raises on SmithDB-only deployments. LangSmith documents the field as required while only 'key' sits in the OpenAPI required array; both halves are now stated. parseLangsmithFeedbackValue was applied in the block and again in request.body, and it is not idempotent: a quoted "1" became a number and a quoted "null" dropped the field. The tool body covers every path including Copilot, so the block call is removed. Jira declared commentBody and newStatus, which no tool emits, while the real keys were undeclared; status and assignee are objects declared as string. LinkedIn's postUrl doc named a URN family the regex never checks.
…rds labels
An earlier pass removed v1-only fields from the v2 tools but stopped short:
description (summary is the real field), external_issue_reference on
actions_create/update, and total_record_count on catalog_entries_list were
all still declared and permanently undefined. on_call_now keeps it -- it
scans /v2/schedules, which really does return a total.
runs_on_incidents offered two values the API rejects, in a dropdown and two
model-facing descriptions.
Vault offered an OWNER role the API refuses outright ('The role cannot be
owner'), and described matters.delete as permanent when it is a reversible
soft delete the same block exposes an undelete for.
…r pageSize 0 GitHub documents workflow_id as 'the ID of the workflow. You can also pass the workflow file name as a string', and list_workflows prints that exact path to the model -- which the next call then threw on. Verified live: .github%2Fworkflows%2Fci.yml returns 200. Dropped the String() wrappers on 35 guarded ids; they defeated the guard's null rejection, so a missing id requested a resource named 'undefined'. People API documents pageSize 0 as 'use the default' (100 for list, 10 for search); the clamp rewrote it to 1, returning a single contact. The test pinned that behavior and is corrected.
Salesforce counts relationship levels, not dotted segments -- its own example is 'Contact.Account.Owner.FirstName (three levels)', four segments. The check compared segment count, so a legal five-level path was rejected, and the test asserted that rejection as correct. 2000 is the synchronous batch size, not a LIMIT maximum. Capping there removed the paging workflow these tools ship query_more for: a larger LIMIT returns the first batch with done:false and a locator. Also allows the documented FIELDS()/toLabel()/FORMAT()/convertCurrency() wrappers, recursing into the same field-path check so only a validated API name reaches the statement; seven new vectors attack the wrapper syntax.
…r behavior - restore firecrawl's conditional country default; the server applies 'us' only when location is unset, and flattening it to the OpenAPI's bare default was the regression - declare firecrawl's six undeclared subBlocks in inputs - agentmail's justification claimed no trash exists; it does. The operative point stands: the endpoint takes no deletion-mode parameter - drop apify's synthesized status; the sync 201 declares no status field, matching the runId removal this branch already made - daytona documents no timeout default, and reserves 'combined output' for session commands, not these two - elasticsearch's scheme guard is belt-and-braces: the transport rejects a scheme-less external URL before it resolves. Both the TSDoc and the user-facing error said otherwise.
… probe claim - list_indices interpolated user-or-llm page/hitsPerPage raw into the query string; the block declared them string while the tool declared number, with no coercion between - .trim() on ids that arrive as JSON numbers threw a TypeError. Box ids are numeric strings and '0' is the root folder, which a truthiness guard was dropping entirely; a whitespace-only id went out as an empty parent - get_records fell back to the tool-level indexName whenever a per-request value was not a string, silently querying a different index - attio target is a two-value enum the tool layer left unconstrained, so a direct LLM call could address /v2/<anything> - the Algolia guard's rationale cited an unreproduced live probe alongside spec-backed claims; the probe is now quarantined and nothing depends on it
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Collaborator
Author
|
@greptile review |
There was a problem hiding this comment.
All reported issues were addressed across 911 files
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
Four are side effects of earlier fixes in this PR: - the '!= null' guard added so an explicit 0 would survive also admitted '', sending a bare 'timeout=' to Apify on a direct tool call - the pagination coercion turned a whitespace-only input into 0, slipping past the tool's own non-negative-integer check - the dot-segment guard trimmed before comparing, so a GCS object legally named ' ..' was rejected even though '%20..' cannot collapse a segment - the 7MiB media cap threw inside a try whose catch reported success with no file, making an over-limit recording look like an empty one; it now returns 413, and the other errors that catch handles are unchanged The fifth is pre-existing, exposed by the new validator: BUILT_IN_PATH is a plain object literal, so an inherited key like '__proto__' resolved up the prototype chain and threw a TypeError. Fixed in all three copies -- both routes and the poller, where 'in' walks the chain the same way.
Collaborator
Author
|
@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.
No issues found across 913 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
The visibility:'hidden' filter added earlier dropped 40 unsettable rows, but it over-removed: a param can be hidden on the tool because the block injects it, while the block still renders it as a required field the user fills in. Mailchimp's apiKey was the only place its page documented an API key -- the page went to zero mentions for an integration that cannot run without one. Zendesk's apiToken and SharePoint's siteId had the same shape. A hidden param is now dropped only when the block declares no subBlock for it, matched on id or canonicalParamId. Restores 128 rows across 11 pages, additions only. Jira cloudId, Salesforce idToken/instanceUrl and NetSuite instanceUrl stay removed -- all genuinely injected, none with a subBlock.
…g breaks blankStringsAndComments kept the first and last character of every match. That is correct for a quoted string, where both are delimiters, but for a '//' comment the last character is arbitrary source text -- so a commented-out '// options: [' left an unbalanced bracket inside the subBlocks span, findMatchingClose returned -1, and the extractor reported that the block exposes nothing. google_drive lost three user-settable mimeType rows that way. The block renders mimeType as an Export Format dropdown. A parse failure and 'this block exposes nothing' were indistinguishable, and the fallback was the destructive branch. Parsing now throws a SubBlockParseError when the bracket scan fails or when an array holding literal objects yields no ids; the call site reports it and exits non-zero in both generate and check mode rather than dropping the page.
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.
Summary
Fixes the defects found by an audit of the integration surface, then re-verified by an adversarial second pass against each provider's primary docs. Every fix is pinned by a test verified to fail before it.
Path traversal — ~693 sites across 20 services
An LLM-writable value interpolated into a URL path segment could re-aim an authenticated request at a sibling endpoint carrying the user's credential, including on DELETE. Encoding does not prevent it:
.and..are unreserved, and the URL parser removes dot segments after decoding, so%2e%2eis popped too. Only value rejection works.safeUrlPathSegmentfor single-segment ids,safeUrlPathfor params that legitimately carry multiple segments (GitHubpath/branch/ref,google_contactsresourceName, Supabase storage keys)encodeURIComponentand were exactly as exploitablesupabase'sencodeStoragePathsplit and encoded per segment, so it read as sanitisation while passing../..through unchangedpath_safety.test.tsthat enumerates tools from the barrel, so a new unguarded param fails CIReserved
timeoutcollision — 8 toolsrequest-transportreadsparams.timeoutas the outbound deadline in milliseconds. Eight tools declared a user-facing param with that name meaning something else, so Daytona's documented "10 seconds" set a 10ms deadline and Twilio's 60-second ring default set 60ms. Renamed on the tool — Copilot callsexecuteAppToolwithout ever runningtools.config.params, so a block-level workaround would not have covered it. Every subBlock id is unchanged, so no saved workflow state is orphaned.Elastic Cloud was unusable
buildBaseUrlsubstituted the user-chosen label for the Elasticsearch UUID, so every Cloud ID resolved to a hostname that does not exist — a DNS failure with no clue for the user, across all 13 tools. Six official Elastic clients discard the label. Also fixed: per-service ports, and URL-special characters in the decoded components (an@in the UUID makes it userinfo and sends the ApiKey header to an attacker's origin). 13 duplicated copies collapsed into oneutils.ts.Declared shapes that did not match reality
serperplaces mapper was written against/maps, not/places—categoryandsnippetwere permanentlyundefined. RestoredknowledgeGraph/peopleAlsoAsk/relatedSearches, which/searchreally returnsfirecrawlsearch declared an array while/v2/searchreturns a source-keyed envelope; news carriessnippetnotdescription, and images'urlis the containing pageelasticsearchget_indexdeclared anindexkey that exists at no level, and silently kept one index on a wildcardlangsmithfeedback value coercion ran only in the block, so the LLM and direct-tool paths got nonelinkedinw_member_socialwas described to users as "Access LinkedIn profile" in the OAuth consent modal — it is a write scopeData integrity
attioswallowed JSON parse failures at 12 sites and sent a substituted empty value while reporting successsixtyfournulled LLM-supplied required fields on the agent pathenrowaborted a polling job on any transient 5xx; now bounded retries within the existing budgetenrowhosted-credit rate cited a pricing tier that does not existBehavior changes worth a reviewer's eye
yourinbox%40agentmail.to. Verified safe — AgentMail's Node SDK encodes, its Python SDK does not, which proves the server decodes normallyfound: false. The old branch was unreachable — the executor throws beforetransformResponseType of Change
Testing
bun run check:audits33/33, including subblock-ID stability (no saved state orphaned)type-check,lint,check:api-validationcleanChecklist