fix(integrations): close defects found by an independent cold audit - #6767
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Integration documentation is updated across Cloudflare, CrowdStrike, Datadog, Microsoft Entra ID, MSSQL, Okta, ServiceNow, and Splunk so parameters and outputs match vendor behavior (e.g. Cloudflare zone types, DNS analytics CrowdStrike maps failed OAuth token exchange to the real HTTP status (e.g. 401) via Microsoft SQL Server tools cap query result rows and UTF-8 response size with Workflow editor adds shared password masking ( Reviewed by Cursor Bugbot for commit 9a0383e. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR resolves functional defects across eight integrations and adds regression coverage for the corrected behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported MSSQL response-cap defects are fixed in the current code.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/tools/mssql/utils.ts | The revised result cap rejects an oversized first row, measures UTF-8 bytes accurately, accounts for array delimiters, and reserves sufficient space for the bounded response envelope. |
| apps/sim/app/api/tools/mssql/utils.test.ts | Regression coverage includes oversized first rows, CJK and astral-plane text, and actual full-envelope serialization at the byte boundary. |
| apps/sim/app/api/tools/mssql/query/route.ts | Query responses consistently pass through the corrected capped-result and response-envelope helpers. |
Reviews (5): Last reviewed commit: "refactor(editor): drop the dead isSearch..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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 |
|
Pushed Secrets rendered in plaintext outside
Narrowing A per-option abort failed a workspace-wide query. Gates: 271 tests across the touched trees, Two things I want to flag rather than quietly ship:
|
|
@cursor review |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36207972 | Triggered | Generic High Entropy Secret | aab4bd9 | apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/code/code.test.tsx | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Pushed Greptile P1 — response byte cap measured in the wrong unit. Confirmed, and worse than the report suggested. Rows are now measured with The envelope half of the finding was real but minor: a packed-ASCII case overshot by 5,518 bytes, 0.05%. One thing the report did not catch — a row No lone-oversized-row exception was reintroduced, and every truncation stays disclosed through Bugbot — masked secrets revealed by workflow search. Partly right, and I went the other way on the fix. Correcting the report: for Rather than wire the reveal up consistently, I removed it. The reasoning, since it is a behavior change:
Consistency-by-unmasking would have made the weakest renderer the standard. All four now read one shared Enforcement: the audit suite gained a Revert-verification: 3 red for the MSSQL accounting (each a genuine numeric gap, not a vacuous pass); 7 red for restoring a search-reveal conjunct, 8 with 266 tests green across the touched trees; One follow-up I deliberately did not do here: |
|
@cursor review |
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.
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.
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.
…op a per-option abort from failing a shared query config.password only reached the short-input renderer, so eight credential fields rendered in plaintext: private keys on ssh/sftp/pi/kalshi, the Secrets Manager payload, the STS web-identity and SAML assertions, and the Browser Use variables table. long-input, code, and table now honor the flag. Code fields mask through the highlighter because react-simple-code-editor paints its textarea transparent; the table masks every column but the first so key/value rows stay distinguishable. A registry-walking audit test fails both on a password flag sitting on a type that cannot honor it and on any of the eight fields losing its flag. credential-group threaded a per-option AbortSignal into the fetch registered under the workspace-wide credential group list key, so closing one option panel rejected every co-observer with an AbortError that is not a React Query cancellation. The shared fetch now runs on its own lifecycle signal.
…from unmasking secrets capRecordset sized rows with JSON.stringify(row).length, which counts UTF-16 code units while the emitted body carries raw UTF-8. CJK is the worst case at 3 bytes per unit, so a recordset admitted as 10 MB serialized to 28 MB. Rows are now measured with Buffer.byteLength, serialized once each, with array punctuation charged exactly and a reserve held back for the response envelope. Workflow search revealed masked credentials without the user touching the field: the search panel keeps focus in its own input and only scrolls the match into view, so typing a guess painted a private key on screen. The index is built client-side from values already in page memory, so this was never a privilege boundary, but masking exists to prevent incidental display and a screenshare-visible reveal defeats it. Focus is now the only reveal, applied through one shared policy across all four renderers.
The masking fixtures carried a literal OPENSSH private key header, which GitGuardian flags as a committed secret even though the body was only the base64 of "openssh-key-v1". The fixtures now use an obvious marker string, and the assertions derive their match text and dot counts from the fixture instead of restating its bytes.
No renderer consumed it. The editor computed it at two call sites and sub-block passed a hardcoded false into renderLabel's slot for it, so even the one function that declared a parameter never saw the real value. Its only live effect was in the memo comparator, where an unconsumed value changing forced a re-render for nothing. The name stays in the masking audit's forbidden-inputs list, which guards against a search signal being wired back into a masking decision.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fbef74b. Configure here.
fbef74b to
9a0383e
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9a0383e. Configure here.
1 participant
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.