fix(integrations): close defects found by an independent cold audit - #6767
fix(integrations): close defects found by an independent cold audit#6767waleedlatif1 wants to merge 11 commits into
Conversation
An independent audit — eight cold readers, one per integration, given no prior findings — checked the eight integrations merged to staging today. Two defects broke an operation outright; both are fixed here. datadog: Update SLO rewrote every non-metric SLO to `metric`. The SLO Type dropdown carried a `metric` default and its condition covered both create and update, so an untouched control reached mergeSloUpdatePayload as an edit. A metric SLO requires `query`, and the merged body carries monitor_ids or sli_specification instead, so Datadog rejected it — Update SLO was unusable on monitor-based and time-slice SLOs, with no way to express "keep the current type". Update now has its own control defaulting to "Keep current". microsoft_ad: list_users, list_groups and list_service_principals emitted $count=true only alongside $search, so any $filter using an advanced operator (ne, not, endsWith, startsWith on non-indexed properties) returned 400. Graph requires $count=true with ConsistencyLevel: eventual for those. list_devices already did this correctly; the other three now match it. Also from the same audit: Datadog path IDs are trimmed before encoding in all 20 URL builders rather than 2, matching the existing get_monitor test.
From the independent cold audit. Cloudflare: DNS analytics no longer emits fabricated min/max telemetry (Cloudflare documents both as always empty); purge_everything defaults to specific-purge and errors when combined with target lists; three unsourced description claims corrected; Array.isArray guards on four older list transforms. CrowdStrike: IOC sort placeholder corrected to the dot form (created_on.desc, not the nonexistent created_timestamp); the 500-indicator cap relabelled as a Sim bound rather than a CrowdStrike one; credential failures return 401 rather than 500; prevent_no_ui noted as unenumerated. MSSQL: introspect no longer lets the model choose the database; row and byte caps on reads; introspection collapsed from 4N+2 to 6 fixed queries; WHERE and identifier guards run before the connection opens so rejections are 400 not 500; SAVE TRANSACTION, OPEN/CLOSE key, DEALLOCATE and ADD SIGNATURE added to the statement screen as two-token phrases; encrypt wording corrected to say TDS 7.4 encryption is negotiated, not guaranteed.
…k sends
The docs generator parses tool source text and resolves a shared `outputs`
const only from the family's `types.ts`, so Splunk's helpers in `utils.ts` were
invisible to it: seven operations published the block's union of every output
instead of their own. Run Search and Get Search Results each shipped a ~50-row
table naming savedSearches, alerts, indexes, and apps they never return, Cancel
Search Job lost `messages`, and the four list tools lost `total`/`offset`.
Inline the four helpers into each consuming tool and delete them, since
relocating a shared const only moves the trap.
Also:
- Add a `splunk-errors` extractor for the documented `{messages: [{type, text}]}`
envelope and set it on all twelve tools. A rejected SPL string, the most common
failure, previously fell through to the status text and reported "Bad Request".
- Read `searchEarliestTime`/`searchLatestTime` with `asNumber`. The job entry
documents them as bare epoch numbers, so `asString` returned null for every
`output_mode=json` response.
- Project the `<messages>` block of the XML job-control response. It is the only
payload that endpoint returns, so `cancel_search_job.messages` was always empty.
- Mark the nullable job outputs optional, matching the transform.
- Default `run_search` to `max_count=1000`. A oneshot search has no paging escape
hatch and Splunk's own default is 10000 rows in one buffered response.
- Add suggested skills to `SplunkBlockMeta`, grounded in `tools.access`.
The regenerated tool metadata also picks up the Cloudflare and MSSQL output
changes from the previous commit, which were never synced.
Cherry-picked from fix/okta-servicenow-audit-followups (6db0f54), whose base predated the earlier audit round; the duplicate isOktaFlagEnabled that produced is resolved in favour of the existing richer helper, which already accepts 'yes'/1/'on' as well as true/'true'. okta: get_logs no longer advertises hasMore forever. A System Log query with no 'until' is a polling query, and Okta always returns a next link for one, even on an empty page — so any loop driven by hasMore never terminated, including the one our own shipped skill instructs the agent to run. errorCauses is now surfaced, so a failed write reports the real reason instead of the useless 'Api validation failed: profile'. sendEmail routes through one coercion helper across all four lifecycle tools. update_group's declarative fallback throws rather than silently truncating an extensible group profile. servicenow: attachmentLimit and limit no longer overwrite each other. Neither assignment was scoped to an operation, so all 12 paginated operations could silently return a row count the user never asked for — defeating the block's own design, which gave attachmentLimit a unique id precisely to avoid this. All seven approval states are published by ServiceNow and are now reachable from the filter, with the space-vs-underscore punctuation documented. The five legacy generic tools route through the shared response helpers, and the folder's only 'any' is gone. Block skills now name the semantic operations.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Microsoft SQL Server adds 10k-row / 10MB response ceilings with Cloudflare stops fabricating DNS analytics Splunk aligns docs and tool output schemas (drops phantom union fields on run/get results), documents bounded oneshot defaults, types search command epoch bounds as numbers, and adds list paging CrowdStrike maps OAuth failures to Datadog adds a separate Update SLO type control defaulting to “keep current” so updates do not silently force ServiceNow scopes Okta docs clarify Reviewed by Cursor Bugbot for commit 5395058. Configure here. |
Greptile SummaryThe PR corrects integration request construction, output metadata, pagination, error reporting, and MSSQL safety limits across eight providers.
Confidence Score: 5/5The PR appears safe to merge because the previously reported MSSQL response-cap bypass is fixed and no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/tools/mssql/utils.ts | The response-cap loop now checks each row before admission, so an oversized first row is dropped and explicitly reported as truncated. |
| apps/sim/app/api/tools/mssql/utils.test.ts | Regression coverage directly verifies the previously reported oversized-first-row failure and the general row and byte ceilings. |
| apps/sim/tools/mssql/introspect.ts | Reworks MSSQL introspection into a fixed-query strategy while preserving bounded output behavior. |
| apps/sim/blocks/blocks/servicenow.ts | Scopes operation-specific limit parameters so attachment and ordinary pagination limits no longer overwrite one another. |
| apps/sim/tools/okta/get_logs.ts | Corrects System Log continuation semantics to avoid treating polling next links as perpetual pagination. |
| apps/sim/tools/splunk/run_search.ts | Adds bounded oneshot search behavior and aligns published output metadata with actual results. |
Reviews (2): Last reviewed commit: "fix(integrations): disclose MSSQL trunca..." | Re-trigger Greptile
Three defects the review round found in the audit fixes themselves. MSSQL capped a recordset and then reported it as complete: `executeQuery` computed `truncated`/`truncationReason` but all five statement routes returned only `message`, `rows`, and `rowCount`, so a caller could not tell paging was required. A shared `toRowsResponseBody` now folds the reason into `message` for an agent reading the status line and exposes the two fields for a caller that branches on them. The byte ceiling also admitted a single oversized row as a lone exception, so one `nvarchar(max)` value serialized an unbounded body — the ceiling bounded everything except the case it exists for. A row is now admitted only when it still fits, and the drop is disclosed rather than read as an empty table. Okta's `get_logs` nulled `nextCursor` alongside `hasMore` on an empty polling page. Terminating the loop is right, but the cursor is the resume handle Okta tells callers to persist, so a scheduled workflow that hit one quiet interval restarted from `since` and re-delivered events it had already processed. The two answer different questions and now diverge. Cloudflare's purge block no longer lets the invalid combination be built: the four target fields are hidden once Purge Everything is selected, so the tool's guard is a backstop rather than a reachable hard error.
|
Pushed 5395058 addressing this round. MSSQL — capped results reported as complete (both bots). `executeQuery` computed `truncated`/`truncationReason`; all five statement routes returned only `message`/`rows`/`rowCount`. A shared `toRowsResponseBody` now builds every success body, folding the reason into `message` and exposing both fields (optional on `sqlRowsResponseSchema` and on the five tools' `outputs`). MSSQL — byte ceiling bypassed by a single oversized row (Greptile). The lone-row exception meant one `nvarchar(max)` value serialized an unbounded body, so the ceiling bounded everything except the case it exists for. A row is admitted only when it still fits; the drop is disclosed rather than read as an empty table. Two further defects found by a self-review of the same fix pass: Okta — the poll cursor was nulled along with `hasMore`. Terminating the loop on an empty polling page is right, but `nextCursor` is the resume handle Okta tells callers to persist. A scheduled workflow that hit one quiet interval restarted from `since` and re-delivered events it had already processed. The two answer different questions and now diverge. Cloudflare — the rejected purge combination was still buildable. The four target fields are now hidden once Purge Everything is selected, so the tool guard is a backstop rather than a reachable hard error. Regression tests added for the MSSQL byte ceiling, the truncation disclosure, and the Okta cursor; the two behavioral ones were each verified to fail when their fix is reverted. Artifacts regenerated. `tool-metadata:check`, `integration-catalog:check`, `docs:check`, `check:api-validation` all pass; 74 test files / 1379 tests green. |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5395058. Configure here.
| truncationReason: | ||
| rows.length === 0 | ||
| ? `No rows returned: the first row alone exceeds the ${MSSQL_MAX_RESULT_BYTES / (1024 * 1024)} MB response ceiling. Select fewer columns, or slice large values with SUBSTRING.` | ||
| : `Result truncated to ${rows.length} row(s): a single statement returns at most ${MSSQL_MAX_RESULT_ROWS} rows or ${MSSQL_MAX_RESULT_BYTES / (1024 * 1024)} MB. Page with OFFSET ... FETCH NEXT to read the rest.`, |
There was a problem hiding this comment.
Empty truncated result misreports rowCount
Medium Severity
When capRecordset drops every row because the first exceeds the byte ceiling, rowCount still falls back to rowsAffected. For a SELECT, that leaves rows empty while rowCount stays non-zero, so the success message claims rows were returned and the new truncation text says none were.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5395058. Configure here.
Okta documents `since` and `after` on the System Log as mutually exclusive, so `get_logs` lets the cursor win rather than sending both — the shape a scheduled poll that persists the cursor would otherwise send. Seven boolean query params reached Okta interpolated raw, so an agent tool call supplying `yes` was rejected. Each now routes through `isOktaFlagEnabled`, keeping its existing send-or-omit behavior. A cleared ServiceNow limit/offset/quantity stayed `''` through the block mapper and was appended as a valueless `sysparm_limit=`. The mapper now resolves a blank to undefined, and the tools skip a blank as well.
…t findings MSSQL read-only screen - Screen RENAME, documented T-SQL DDL for Azure Synapse dedicated SQL pools and Analytics Platform System, which are reachable over TDS with exactly the connection fields this block exposes. `SELECT 1 RENAME OBJECT dbo.t TO t2` was a schema change passing an operation advertised as read-only. - Screen the Service Broker family: RECEIVE as a word, and END/MOVE/GET CONVERSATION and SEND ON CONVERSATION as two-token phrases, since END closes every CASE. RECEIVE is a destructive read and END CONVERSATION WITH CLEANUP drops a conversation's messages. MSSQL routes and block - Build the insert statement before connecting, matching update and delete, so a bad identifier answers 400 instead of burning a TLS+login and returning 500. - Declare `truncated`/`truncationReason` on the block, which the tools declare and the routes emit but the block left unreferenceable. Microsoft Entra ID - Pair `$count=true` with `ConsistencyLevel: eventual` conditionally. Graph documents `hasMembersWithLicenseErrors`, `isLicenseReconciliationNeeded`, and `identities/any(i:i/issuer)` as filterable only *without* advanced query parameters, and documents advanced queries as unsupported in Azure AD B2C tenants, so the unconditional pair broke filters that previously worked. When continuing from a nextLink the pairing is read off the link itself. - Request `LicenseAssignment.Read.All` instead of `Directory.Read.All`. The latter was needed by `GET /subscribedSkus` alone, whose permission table names the former as least privileged and does not list the ReadWrite scope we hold. - Enumerate the block's real output keys instead of a single `response` object no tool emits.
… query params
Splunk run_search: revert the `max_count=1000` default added last pass. It was
wrong on both halves. Splunk documents the parameter as "the number of events
that can be accessible in any given status bucket. Also, in transforming mode,
the maximum number of results to store" — so for a non-transforming oneshot it
bounds status buckets, not the response, and for a transforming search (`| stats`,
`| timechart`, which is what the block's own skills generate) it capped results
at 1000 where Splunk would have stored 10000, silently. The block's `maxCount`
placeholder already read `10000`, contradicting the code. Send `max_count` only
when the caller sets it and restate the description in Splunk's own terms,
matching create_search_job. The real guidance — a oneshot buffers the whole
result set, so use Create Search Job + Get Search Results for anything large —
moves into the tool description and the search-splunk-logs skill.
Datadog mute/unmute: send `scope`, `end`, and `all_scopes` as query parameters.
`MuteMonitor` and `UnmuteMonitor` declare no `requestBody` in the authoritative
spec (docs.datadoghq.com/resources/json/full_spec_v1.json — the generated
datadog-api-client-go v1 schema omits both operations and is a subset, not the
authority); all three parameters are `in: query`. Sent as a JSON body they are
dropped, so a scoped, time-boxed mute becomes an indefinite mute across every
scope and unmute's "all scopes" never applies — answered with a 200 and the full
monitor object, so nothing surfaces.
Datadog list_monitors: imply `page=0` when a page size is set without a page.
Datadog "returns all monitors without a `page_size` limit" when `page` is absent,
so Page Size was inert from a control that reads as a bound. `page` is not
defaulted when neither is set — that would silently truncate a caller relying on
the documented return-everything behavior.
Also:
- Note in get_fired_alerts that `name=-` returns every saved search's fired
alerts and the endpoint documents "Request parameters: None", so there is no
count/offset to bound it.
- Fix the Splunk block's `messages` output blurb: `[{type, text}]` holds for the
search and job-control operations, but get_search_job returns an object.
- Generalize the Datadog block's numeric coercion (`datadogPageNumber` →
`datadogNumber`) over all 32 bare `Number()` mappings, so a typo or unresolved
reference is omitted rather than sent as `NaN`/`null`, and an explicit `0`
survives the old truthiness guard.
- Disclose create_event's documented 18-hour `date_happened` ceiling, and that
send_logs' `ddsource: "custom"` is a Sim default rather than a Datadog one.
Cloudflare: clear purge_cache advanced targets across operations; send action_parameters/ref/logging on rate-limit rule updates; migrate off the deprecated batch zone-settings endpoint; correct MX/URI priority wording; stop coercing blank numerics to 0. CrowdStrike: seed includeHidden to match Falcon's documented default. Microsoft Entra ID: resolve a UPN to an object ID for app role assignment; wrap 21 array outputs in items.properties so nested paths resolve. Okta: route assign_user_role's notification flag through isOktaFlagEnabled. ServiceNow: drop the triage skill's claim of a default limit that does not exist. Splunk: always assign coerced numerics so raw values cannot leak through the executor's raw-input merge.
Eight cold readers — one per integration, given no prior findings and no history — audited the eight integrations merged to
staging. Every one returned a "production-ready" verdict, and between them they still found six real functional defects, two of which broke an operation outright. This PR closes all of them.Every fix has a regression test, and each test was individually verified to fail when its fix is reverted.
Broke an operation outright
metricdefault and its condition covered both create and update, so an untouched control reached the merge as an edit and rewrote the stored type. A metric SLO requiresquery; the merged body carriesmonitor_ids/sli_specificationinstead, so Datadog rejected it. There was no way to express "keep the current type" — update now has its own control defaulting to "Keep current".$filterreturned 400.list_users,list_groupsandlist_service_principalsemitted$count=trueonly alongside$search, but Graph requires it withConsistencyLevel: eventualforne,not,endsWith, andstartsWithon non-indexed properties.list_devicesalready did this correctly; the other three now match it.Silently wrong results
attachmentLimitandlimitoverwrote each other on all 12 paginated operations. Neither assignment was scoped to an operation, so List Incidents with a limit of 100 could send 5. This defeated the block's own design, which gaveattachmentLimita unique id specifically to avoid it.get_logsadvertisedhasMore: trueforever. A System Log query with nountilis a polling query, and Okta always returns a next link for one, even on an empty page. The default config is exactly that shape, so any loop driven byhasMorenever terminated — including the one our own shipped skill instructs the agent to run.types.ts, so Splunk's helpers inutils.tswere invisible: Run Search and Get Search Results each published ~50 phantom rows (savedSearches,indexes,apps…) they never return. Fixed by inlining the literals and deleting the helpers, which removes the mechanism rather than relocating it.docs:checkpassed throughout — the generator deterministically reproduces the wrong table.min/maxwere declared as seven numeric fields each mapped?? 0, though Cloudflare documents both as "currently always an empty object". An agent reading that got plausible-looking numbers instead of obviously-absent data.Also
Splunk gained an error extractor (a bad SPL string previously reported only "Bad Request"), a bounded oneshot
run_search,<msg>parsing so a cancel reports its confirmation, andasNumberon the epoch time bounds. Okta surfaceserrorCauses, so a failed write reports the real reason rather than "Api validation failed: profile". MSSQL addedWRITETEXT/UPDATETEXT-class statements to the read-only screen, row and byte caps, introspection collapsed from 4N+2 to 6 fixed queries, and guards that run before the connection opens so rejections are 400 rather than 500. CrowdStrike corrected an IOC sort placeholder naming a field that does not exist, and relabelled an unsourced cap as Sim's own. Datadog trims path IDs in all 20 URL builders rather than 2.Notes for review
N'...'literals, bracketed quotes, comments, backticks,FETCHabuse, Service Broker — and broke none, then proved why: a masker desync requires a backslash immediately before a quote, which is exactly the rejected pattern.Directory.Read.All→LicenseAssignment.Read.All. Two earlier analyses proposed removing it outright; that breakslist_subscribed_skus, whose least-privileged permission is the narrower scope. A tripwire test assertsGroupMember.ReadWrite.Allstays, sincegroup-post-membersdoes not acceptGroup.ReadWrite.All.utils.tswas measured and would change nine other integrations (ashby, context_dev, daytona, mintlify, okta, onepassword, persona, rabbitmq, trigger_dev), every inspected change a correction. Those are likely publishing wrong rows today. Tracked on docs generator: tool descriptions past a 600-char window publish as empty strings, silently #6760.Verification
type-checkclean,biomeclean, 42 test files / 965 tests passing, andtool-metadata:check,integration-catalog:check,docs:check,check:api-validationall pass. Artifacts regenerated with no unrelated drift.