Uh oh!
There was an error while loading. Please reload this page.
fix(v2-api): close three secret disclosures, make the surface consistent, and align docs with signatures - #6560
Merged
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview Flattens nested response envelopes for file shares, folders, knowledge bases/documents/folders, MCP servers, skills, custom tools, and secrets from Corrects behavioral docs that were wrong or incomplete: free-plan billing periods and lifetime credits, soft-delete/archive for files, connector-backed knowledge document exclusion, workspace API key rejection rules, billing-log Fills missing error responses ( Reviewed by Cursor Bugbot for commit 6abc1b4. Configure here. |
waleedlatif1force-pushed
the
improvement/v2-route-standardization
branch
from
August 11, 2026 21:43
95b5cbb to
c8498b8CompareTwo P0 disclosures, five correctness bugs, and the standardization and
guard work that came out of auditing them.
**Secret disclosure — workflow version state.** `GET /api/v2/workflows/{id}/
versions/{version}` served the deployed graph unsanitized, so a read-role
workspace API key received plaintext block-password values and OAuth
credential ids. The sibling export route has always sanitized. Every other v2
response is protected structurally because the builder re-parses it, but this
field is `z.custom<WorkflowState>()` — a predicate that validates nothing —
which is why it survived earlier audits. Sanitization now lives in the use
case, secure by default, with a named `includeCredentialValues` opt-in that
only the session-authed deploy-preview route sets.
**Secret disclosure — MCP headers.** The internal list and update routes
returned custom `Authorization` headers verbatim to any read-role member;
headers are stored unencrypted. Values are now gated on write permission and
projected through one shared helper. The settings UI genuinely prefills from
them, so blanking outright would wipe headers on unrelated edits — write-only
headers plus encryption at rest are the follow-up.
Correctness:
- v2 execute ignored `X-Sim-Via`, resetting the call chain on every hop and
defeating the recursion guard. Wired on both the keyed and anonymous paths.
- v2 knowledge search accepted `searchMode` and dropped it, silently serving
vector-only results for a hybrid request, and allowed 50MB bodies where
internal caps at 2MiB.
- v2 run cancel never released the plan concurrency slot and half-cancelled
group runs; a group conflict now returns 409 instead of reporting success.
- v2 table row writes stamped no secret provenance, so the next internal read
reported the whole page incomplete. `secretProvenance` is now required on
the primitives, making the next omission a compile error.
- Folder conflicts and malformed paths returned 500; they are 409/404/400 now.
`FolderPathError` splits from `FolderHierarchyError` so a corrupt stored
tree stays a 500 and stays in 5xx alerting.
Standardization and documentation:
- `PUT /files/{id}/share` -> PATCH. The resource is not round-trippable
(`hasPassword`, never the password), so merge-on-omission is the only
implementable semantics.
- ~40 spec truthfulness fixes: a 410 the API cannot emit, eight 423s with no
lock guard, ~30 reachable-but-undocumented 404/400/413s, and six inverted
field claims. Eleven operations that always reject a workspace key now say
so — four of them answer 404, so a workspace key was told the resource did
not exist.
- `NAME_PATTERN` lost its `/i` through `z.toJSONSchema`, publishing 15
patterns that reject names the runtime accepts. Every generated client
rejected any capitalized table or column name, and two of the spec's own
examples failed the spec's own schema.
Guards, so these classes cannot recur:
- `check:route-verbs` (new) cross-checks all 212 builder routes' exported verb
and path against their contract. The builders only compare at runtime, so a
half-done rename previously passed CI and 500'd in production.
- Example validation now runs against the published JSON Schema with formats
on, covering 225 nodes instead of 100 — this is what caught the regex bug.
- The list-pagination sweep is union-aware and fails loudly on a schema it
cannot introspect, rather than counting it compliant.BREAKING: 31 endpoints that returned `{ data: { <resource>: T } }` now return
`{ data: T }`.
This corrects drift, not a design decision. PR #5273 added skills, custom
tools, MCP servers, secrets, and knowledge nested while adding workflows,
files, and logs flat — and in the same commit wrote the `v2/shared.ts`
docblock declaring `single resource: { data: T }` is the standard. The nested
half appears to have been modelled on the v2 tables surface (#6067), which
landed twelve days earlier. Lists were already `{ data: T[], nextCursor }`, so
flat single-resource is what actually matches them; nesting made every client
destructure a layer that carries nothing.
Doing it now because the cost only grows: `v2-api` is still dark-launched, so
today this breaks no one. After GA it needs a deprecation window.
Payloads that carry real information were deliberately left alone — this was a
classification exercise, not a mechanical sweep. Unchanged: delete
acknowledgements (`{ id, deleted }`, `{ path, deleted, deletedItems }`), the
knowledge search envelope (which echoes query, knowledgeBaseIds, topK and
totalResults alongside hits), upload payloads carrying signed tokens and
transfer instructions, bulk-operation counts, `{ row, operation }` upserts,
named acknowledgement scalars (`{ dispatchId }`, `{ cancelled }`), and
`{ columns: [...] }` — a collection, where a bare `{ data: T[] }` would be
indistinguishable from the list envelope but without `nextCursor`.
Also flattened the two file-share responses, which were not in the original
survey: leaving them would have put one resource in two shapes on one path.
`GET /files/{id}/share` now returns `{ "data": null }` when a file has never
been shared.
No consumer is affected. Both SDKs touch exactly two v2 endpoints — execute
and run status — and both were already flat. No docs MDX, client hook, or
internal caller reads a changed response; Copilot table tools call the
application use cases directly rather than the HTTP surface.
The shared `v2FolderSchema` is untouched: every folder flatten was achievable
at the response site, which is itself evidence flat was the intended shape.…erent
**Secret disclosure — run snapshot.** `GET /api/v2/logs/{runId}` returned
`workflowState` straight from `workflowExecutionSnapshots.stateData`, which is
the workflow graph: `blocks[].subBlocks[].value` holds `password: true` field
values and `oauth-input` credential ids. Nothing on that path sanitized it, and
the field was typed `z.unknown()`, so the builder's response parse stripped
nothing. A read-role workspace API key could read plaintext credentials.
This is the third instance of one pattern, and the pattern is the finding: the
builder protects every response by re-parsing it, so the only fields that can
leak are the ones typed `z.unknown()` or `z.custom()`. Both prior disclosures
sat behind exactly such a field. The snapshot is now sanitized in the use case
and the field is typed object-or-null. An inventory of every remaining
`z.unknown()` in the v2 contracts is in the PR description; two carry data with
no projection behind them and are named there as follow-ups.
**Concealment was bypassable.** `createV2ResourceConcealmentPolicy` rewrites
resource-authorization failures to 404 so a caller cannot probe for existence.
Workflows and files applied it on every verb; tables and knowledge applied it
only on reads. A caller could therefore probe with PATCH, read the 403, and
learn the resource exists — the read-side concealment bought nothing. Nine
mutation sites now conceal, plus the three table-column verbs, which were
inconsistent with their own sibling sub-resources.
`lib/logs/api/route-policies.ts` was a second, divergent implementation that
sniffed `response.status === 403` and so also swallowed workspace-policy
denials the canonical helper deliberately preserves. It now uses the helper. A
third such sniff survives in the upload-control helper and is noted as a
follow-up.
Also:
- `DELETE /tables/{tableId}/rows/{rowId}` returned the bulk `{deletedCount,
deletedRowIds}` shape while nine sibling single-resource deletes return
`{id, deleted}`. It now matches them.
- Nine operations can 404 on an unknown folder path and did not document it;
`createWorkflow` could 413 on an oversized folder tree and did not; getting a
run can 409 when trace data was truncated and did not.
- `queryTableRows` documented a 413 it cannot emit and `resumeWorkflowRun` a
423 with no lock guard anywhere in its path — the same un-producible-status
class already cleared for 410 elsewhere.
- Execute's 409 description covered only the run-id case after the
recursion-guard fix added a second cause, and named a code the route does not
emit: the wire carries `error.code: CONFLICT` with the specific cause in
`error.details.code`. `x-sim-via` is now a declared request header.
- Deploy and rollback published examples that were impossible: `isDeployed:
true` beside `activeDeployment: null`, where the route computes the former
from the latter.
- `afterRowId`/`beforeRowId` were published on row insert and silently dropped
by the route, so a positional insert became a tail append.
- A generated document whose script fails permanently answered "still being
generated, try again" forever; the underlying cause is now preserved.Structural parity between contracts and specs is CI-enforced; semantic truth is
not. These are claims the spec made that the code does not honour.
Outright false:
- `DELETE /files/{fileId}` said it deletes "the stored bytes". It archives:
the row is retained with a deletion timestamp and the bytes are never
removed. Restore exists, but only on the internal API, so the description now
says so rather than implying v2 offers it.
- Execute documented `409 EXECUTION_ID_CONFLICT` in three places. The wire
carries `error.code: CONFLICT` with `error.details.code: RUN_ID_CONFLICT`;
only v1 ever emitted the documented string.
- The files spec claimed every endpoint uses the canonical envelopes while
`GET /files/{fileId}` returns octet-stream.
- The shared timestamp rule justified itself with a rendering claim that is
false — 29 bare-form sites publish `format: date-time` identically. The real
difference is runtime validation, so the rule now says that. It was softened
rather than enforced: responses are re-parsed, so adding `.datetime()` to a
field whose producer can emit a non-ISO string turns a working read into a
500, and that could not be proven for all 29 without a much larger audit.
Misleading:
- The billing ledger silently defaults to a 30-day window, so a client
paginating to `nextCursor: null` believes it has the whole ledger.
- Deleting a connector-backed knowledge document does not delete its chunks —
the row survives as excluded and the embeddings remain.
- `listTables` said "all tables"; it is keyset-paged with a default limit.
- `GET /files/{id}/share` omitted the `data: null` never-shared case its own
schema and example already declare.
- The share PATCH matrix omitted two hard 400s, so following it literally
against a never-shared file fails.
- Five knowledge operations render a canonical folder path back and can 413 on
an oversized tree without carrying the sentence that says so.
Also: the upload-control helper was a third implementation of concealment by
sniffing `response.status === 403`, which masks workspace-policy denials the
canonical helper deliberately preserves. It now uses the shared policy, so
those denials keep their 403. And the shared docblock's search-field
enumeration was presented as exhaustive while omitting two lists, and its
error-envelope claim omitted the two upload data-plane routes that emit a bare
`{error: string}` — both now carry the carve-out the CI allowlist already had.…mantics #6557 narrowed `createV2ResourceConcealmentPolicy` to conceal only the three cross-tenant authorization classes, deliberately letting a same-workspace policy denial keep its 403 so the caller learns why. My test predated that and asserted a workspace-key denial was concealed as 404. Split into two cases that pin the distinction rather than paper over it: a cross-tenant reach conceals, a workspace-key policy denial does not.
…ge-search 413 The v2 log presenters parsed status against a five-value enum, but the execution logger persists a sixth, redacting, while a finished run's output is scrubbed. Any such row failed the response parse; on the list route one row 500'd the whole page. The enum is now derived from PersistedWorkflowExecutionStatus with a compile-time exhaustiveness assertion, so a future status is a type error rather than a production 500. POST /api/v2/knowledge/search declared maxBodyBytes without payloadTooLargeResponse, so its 413 returned a bare string instead of the v2 error envelope. It now matches the sibling deploy/rollback routes.
waleedlatif1force-pushed
the
improvement/v2-route-standardization
branch
from
August 11, 2026 22:28
be0ac08 to
6abc1b4CompareArchive extraction into workspace files/ was rewritten onto the authorized application-operation boundary, and three behavioral regressions came with that move. Together they broke every archive containing a subdirectory, and 100% of copilot extract() calls (materialize-file always passes rootFolderSegments: [baseName], and its catch only handles ArchiveError). 1. Non-canonical folder path. The extractor joined the folder segments with "/" and passed the result as `path` to createWorkspaceFileFolderOperation. That path reaches requireNonRootFolderPath -> parseFolderPath, which requires a leading "/" and byte-for-byte canonical per-segment encoding, so "bundle/data" threw FolderPathError before anything was written — and a folder name containing a space or a reserved character would still have thrown after merely prefixing a slash. 2. exactName: true. createWorkspaceFileFromBuffer was told to demand the exact leaf name, which sets maxAttempts = 1 and raises FileConflictError when the name already exists. The extractor's rollback then deleted every file written so far, so one colliding name destroyed the whole extraction. Reachable today for flat archives through the unzip action of POST /api/tools/file/manage. Restored to auto-suffixing via allocateUniqueWorkspaceFileName. 3. Wrong folder primitive. createWorkspaceFileFolderAtPath creates exactly one leaf, conflicts on an existing path, and requires the parent to exist already. The extractor never creates intermediates and caches by full path, so the first nested entry asked for a folder whose parent was never created. The correct semantics are ensureWorkspaceFileFolderPath: walk every segment, reuse what exists, create only what is missing. Rather than bypass the operation boundary by calling the manager primitive directly, this adds ensureWorkspaceFileFolderPathOperation — an authorized application use case under files.folders.create that expresses "ensure this whole chain exists" — and routes the extractor through it with raw decoded segments, so no path string is built and no encoding can be malformed. archive.test.ts previously mocked the folder operation and asserted the broken shape (path: 'bundle'), which is why this shipped. The suite now fakes the workspace-file store in memory while enforcing the real rules: folder paths run through the production parseFolderPath family, the create-one-leaf operation conflicts and requires a parent, and exactName governs conflict vs auto-suffix. Nested, reuse, encoded-name, and collision cases are covered and each fails against the pre-fix code.
This was referenced Aug 11, 2026
Merged
Extraction now materializes folders before uploading files, but the failure path only deleted the extracted files — every folder the call created was left behind. That is not cosmetic: `materialize_file` guards re-extraction by looking up the root folder path and refusing when it has any child, so a half-extracted nested archive turned every retry into "already extracted — delete that folder first" until a human cleaned up the tree by hand. The rollback must delete only folders this call actually inserted, never one it reused: extracting into an existing path is normal (a sibling entry, an earlier successful extraction), and deleting a pre-existing folder would destroy unrelated user data. `ensureWorkspaceFileFolderPath` already distinguishes the two while walking the segment chain, so it (and its application operation) now reports `createdFolderIds` alongside the leaf id. The extractor accumulates those ids in creation order and, on failure, deletes them in reverse — parents are recorded before their children, so reverse order is deepest-first and a parent is never removed out from under a child. Folder cleanup is best-effort like the existing file cleanup, so a cleanup failure never masks the original error.
waleedlatif1
commented
Aug 11, 2026
CollaboratorAuthor
waleedlatif1
commented
Aug 11, 2026
CollaboratorAuthor
@cursor review |
`GET /api/v2/billing/status` resolved the workspace's payer and projected that payer's pooled allowances — credits used, credit limit, credits remaining, and the payer entity's storage usage and quota — to any caller holding only `read` on the workspace, including a personal API key. The payer pool is shared across every workspace that payer funds, and the platform already treats it as privileged: the workspace credit-availability surface computes `canViewPayerPool` from `canManageWorkspaceBilling` and substitutes member-scoped or null figures for everyone else. The new versioned endpoint had no equivalent gate. `credits` and `storage` are now projected only to a caller who may manage the resolved payer's billing: the billed account holder of a personally hosted workspace, an admin of the hosting organization, or a workspace API key, which only a workspace admin can provision. The endpoint stays at `read` so a plain member keeps the plan, period, and standing the workspace UI already shows them, and an exceeded pooled limit still reports as `limit_exceeded` without disclosing the numbers behind it. Both fields are nullable on the wire and in the regenerated OpenAPI spec. The decision lives in the application use case, resolved from canonical workspace state, not in the route: billing authority is payer identity and organization role, which the workspace permission ladder cannot express — a plain workspace `admin` is deliberately not enough.
…int labels `withPublicApiRouteHandler` and 27 `ApiEndpoint` union members landed together in #5273, but the v2 surface shipped on `defineV2JsonRoute` + `v2RateLimits` instead. The builder had no production caller — only its own test — and the v2 rate limiter never reads an `ApiEndpoint` label, so those members were never emitted to telemetry by symbol or by string literal. Remaining members are exactly the labels a v1 route passes to `checkRateLimit` or `authenticateRequest`. Drops the now-unreachable `hasZodUsage` branch from the API validation audit; no ratchet metric moves (route total stays 1093).
The first pass gated `credits` and `storage` on billing authority for personal API keys but let a `workspace_api_key` principal through unconditionally, which left the excluded role a way back in. Any workspace `admin` may mint a workspace API key, and a workspace `admin` is deliberately not a billing manager, so an admin who reads `null` as themselves could mint a key and read the full pool with it. On an organization-hosted workspace that pool is the organization's, spanning workspaces the admin has no standing in. Billing authority is payer identity or an organization admin role — a property of a person. A workspace API key is deliberately actor-less, so it can never satisfy it and now reads both fields as `null`. Attributing the key to its creator was rejected: it would launder the same workspace-admin role, it breaks when the creator's authority is revoked while the key lives on, and substituting a key's owner for the acting principal is what the application operation boundary forbids. The reasoning sits in TSDoc at the decision point. The key keeps the plan, period, and standing it needs to monitor a workspace, including `limit_exceeded` and `billing_blocked`. No in-repo caller reads `credits` or `storage` from this endpoint. The payer storage pool is now read only once disclosure is authorized, so a caller who may not see it no longer triggers the query at all.
`createWorkflow` and `updateWorkflow` each resolve a folder two ways inside one function. The folderPath branch goes through `resolveWorkflowFolderPath`, which loads the path index with `maxRows: MAX_FOLDERS_PER_WORKSPACE`; the folderId branch loaded it with no bound at all, issuing a `SELECT` over every active folder row in the workspace. In `updateWorkflow` the unbounded read and the bounded fallback sit thirty lines apart in the same function. Passes the cap at both sites, matching the read sites that already opt in. Exceeding it throws `FolderCollectionLimitExceededError` rather than truncating, because a partial path index resolves real folder paths to `undefined` and re-roots resources at the workspace root. `maxRows` deliberately stays opt-in rather than becoming the default. Folder creation does not refuse at the same ceiling on every path — `POST /api/folders` goes through the `createFolder` name/parentId variant, which passes no `maxFolderRows`, so the count guard in `executeCreateFolderAtPath` never runs and a workspace can already hold more than `MAX_FOLDERS_PER_WORKSPACE` folders. Defaulting the bound would make every path-index consumer throw for a state the product allows to exist. Reconciling reader and writer is a separate change with a user-facing limit, not a chore.
GET /api/v2/tables/{tableId}/rows coerced an undecodable pagination cursor to
offset 0 and re-served page one. A client paging forward reads that as a fresh
first page and can loop over it forever. Every sibling v2 cursor list — logs,
files, workflows, workflow runs, workflow versions, workspace members, tables,
knowledge documents — already rejects with a validation error instead.
Extracts the offset-cursor decode both offset-paginated v2 routes had inlined
into `decodeOffsetCursor`, next to the existing `decodeSortedCursor`, so the
reject-don't-restart rule has one home.…ssage leak
The v1 table routes were rewritten to consume `lib/table/orchestration`
results, and two response behaviors drifted from what the live API returned.
Information disclosure: an unclassified failure's `outcome.error` carries
whatever text the fault happened to have. Drizzle wraps a throw raised inside
a transaction in an error whose own message is the failed statement and its
bound parameters, so `DELETE /api/v1/tables/{tableId}` and
`DELETE /api/v1/tables/{tableId}/rows/{rowId}` returned that verbatim in the
500 body to any API-key holder. Previously these returned a fixed generic
string.
Lost `lock` field: the 423 body used to be `{ error, lock }`. The delete,
row-delete, and column-update routes (v1 and internal) dropped the lock kind
the orchestration result already computes, leaving clients unable to tell
which lock to clear.
Both are fixed at one altitude: `orchestrationOutcomeErrorResponse` in
`app/api/table/utils.ts` is now the only way a table route projects an
orchestration failure onto the wire. It renders the route's fallback for an
unclassified failure and the real message for a classified one (validation,
not-found, conflict, locked keep their specific text), and carries `lock` on a
423. A future route cannot reintroduce either bug by hand-spelling the body.
Duplicate table names on `POST /api/v1/tables` keep answering 409 rather than
reverting to the previous 400. 409 is the correct semantic, and every other v1
duplicate-name surface (knowledge, files, workflow import) already answers 409;
the tables 400 was the outlier. v1 tables appears in no published OpenAPI
document and no in-repo client branches on the status, so the compatibility
cost is limited to a caller matching 400 specifically for a name collision.The built-in-name guard ran on every update that carried a `name`, without comparing it to the skill's current persisted name. Skills created before the guard existed can legitimately carry a built-in's name (they simply shadowed the built-in at read time), and the skill modal always submits the full object including the unchanged name — so every save of such a skill returned 400 with "The skill name ... is reserved by a built-in skill", with no way to fix it short of renaming. Move the guard in `updateSkill` to after the canonical row is loaded and run it only when the submitted name differs from the current one. Creating a skill with a built-in name, and renaming an existing skill into one, are still rejected. The check stays in the shared orchestration primitive because that is the only layer both the internal `/api/skills` adapter (via `performUpdateSkill`) and `updateSkillUseCase` (v2 + Copilot) pass through, and it is where the current name is in hand.
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 6abc1b4. Configure here.
Contributor
Greptile SummaryThe PR hardens v2 API secret projection and authorization concealment, corrects cancellation and execution behavior, flattens resource response envelopes, and synchronizes generated API documentation.
Confidence Score: 4/5The PR is not yet safe to merge because a workflow-group cancellation conflict can still stop execution while retaining its plan concurrency slot. The group-sidecar claim runs after Redis, queue, local, and resume cancellation effects, while conflict branches throw before Files Needing Attention: apps/sim/lib/execution/cancel-workflow-execution.ts and apps/sim/lib/execution/cancel-workflow-execution.test.ts
|
| Filename | Overview |
|---|---|
| apps/sim/lib/execution/cancel-workflow-execution.ts | Adds workflow-group cancellation and reservation release, but the previously reported conflict path still throws after cancellation effects and before releasing the slot. |
| apps/sim/lib/execution/cancel-workflow-execution.test.ts | Adds cancellation coverage, including an assertion that preserves the outstanding slot leak on refused group claims. |
| scripts/check-route-verbs.ts | Adds a build-time route verb/path consistency check; the reported template-string security lead has neither an attacker-controlled source nor an HTML sink. |
Reviews (2): Last reviewed commit: "fix(v2-api): accept the redacting log st..." | Re-trigger Greptile
Comment on lines
+290
to
+298
| const groupCancellation = workflowGroupWorkspaceId | ||
| ? await cancelWorkflowGroupExecution({ | ||
| workspaceId: workflowGroupWorkspaceId, | ||
| workflowId, | ||
| executionId, | ||
| }) | ||
| : null | ||
| if (groupCancellation?.kind === 'conflict') { |
Contributor
There was a problem hiding this comment.
When a workflow-group cancellation returns conflict or not_workflow_group, Redis, local, queue, and resume cancellation effects have already run, but the subsequent exception bypasses releaseExecutionSlot, causing a rejected cancellation to stop work while retaining its plan concurrency slot until expiry.
Knowledge Base Used:Workflow Execution Flow
waleedlatif1
commented
Aug 11, 2026
CollaboratorAuthor
waleedlatif1
commented
Aug 11, 2026
CollaboratorAuthor
@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 6abc1b4. Configure here.
…er-path fix(uploads): restore archive extraction folder parity
fix(billing): withhold the payer credit and storage pools from callers who cannot manage billing
chore(api): remove a dead route builder, bound folder-index reads, reject invalid cursors
fix(api): stop leaking failed SQL in v1 table 500s and restore the 423 lock field
…lision fix(skills): only reject a built-in name collision on an actual rename
Uh oh!
There was an error while loading. Please reload this page.
mzxchandra added a commit
that referenced
this pull request
Aug 12, 2026
Staging fixed the deployment version route param independently (#6560, `z.coerce.number()`) and covers both the numeric and `active` cases in `deployments.test.ts`, so this branch's variant and its regression test are redundant. Revert the contract to staging's exactly, leaving this PR scoped to the deploy permission change. Also drop TODOS.md and rewrite the two comments that pointed at it so each one states its own condition for removal.
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 freeto 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.
Follow-up to #6542. Four commits: three secret disclosures, a set of correctness fixes, a breaking envelope flatten, and ~50 documentation corrections — plus the guards that stop each class recurring.
v2-apiis still dark-launched, which is why the breaking changes are here rather than deferred: today they cost nothing, after GA they need a deprecation window.Three secret disclosures, one pattern
All three were the same shape, and the pattern is the finding worth carrying forward.
The v2 builder re-parses every response against its contract, so over-exposure is structurally impossible — except for fields typed
z.unknown()orz.custom(), which validate nothing. Every disclosure sat behind exactly such a field:GET /workflows/{id}/versions/{version}GET /logs/{runId}GET /api/mcp/servers(internal)AuthorizationheadersThe first two are now sanitized in the application layer — secure by default, so a new surface inherits redaction rather than opting into it. The third gates header values on
write, because the settings UI genuinely prefills from them; write-only headers and encryption at rest are named follow-ups.The PR description carries a full inventory of every remaining
z.unknown()in the v2 contracts. Two carry data with no projection behind them and are flagged there rather than silently fixed.Concealment was bypassable
createV2ResourceConcealmentPolicyrewrites resource-authorization failures to 404 so a caller cannot probe for existence. Workflows and files applied it on every verb; tables and knowledge applied it only on reads — so probing withPATCHreturned a 403 and disclosed the resource, and the read-side concealment bought nothing. Nine mutation sites plus the three table-column verbs now conceal.Two further implementations concealed by sniffing
response.status === 403, which also swallowed workspace-policy denials the canonical helper deliberately preserves. Both now use the shared policy, so a "personal API keys are disabled here" denial reaches the caller instead of masquerading as 404.Correctness
X-Sim-Via, resetting the call chain every hop and defeating the recursion guard on both the keyed and anonymous pathssearchModeand dropped it — a hybrid request silently got vector-only results — and allowed 50MB bodies where internal caps at 2MiBsuccess: truesecretProvenanceis now required on the primitives, making the next omission a compile errorFolderPathErrorsplits fromFolderHierarchyErrorso a corrupt stored tree stays a 500 and stays in 5xx alertingNAME_PATTERNlost its/ithroughz.toJSONSchema, publishing 15 patterns that rejected names the runtime accepts — every generated client refused any capitalized table or column name, and two of the spec's own examples failed the spec's own schemaBreaking: the response envelope
31 endpoints returning
{ data: { <resource>: T } }now return{ data: T }.This corrects drift, not a decision. #5273 added skills, custom tools, MCP servers, secrets, and knowledge nested while adding workflows, files, and logs flat — and in the same commit wrote the docblock declaring
{ data: T }the standard. The nested half appears modelled on the v2 tables surface from #6067, twelve days earlier. Lists were already{ data: T[], nextCursor }, so flat is what actually matches them.Payloads carrying real information were deliberately left alone: delete acknowledgements, the knowledge search envelope, upload payloads with signed tokens, bulk counts,
{ row, operation }, named acknowledgement scalars, and{ columns: [...] }— a collection, where a bare{ data: T[] }would be indistinguishable from the list envelope but withoutnextCursor.Also changed:
PUT /files/{id}/share→PATCH(the resource exposeshasPassword, never the password, so it is not round-trippable and merge-on-omission is the only implementable semantics), andDELETE /tables/{tableId}/rows/{rowId}now returns{ id, deleted }like its nine siblings instead of the bulk shape.No consumer is affected, verified exhaustively rather than assumed: both SDKs touch exactly two v2 endpoints and already parse flat; no docs MDX in any of six locales, no client hook, and no internal caller reads a changed response; Copilot table tools call the use cases directly.
Documentation
~50 corrections. The dominant failure was error sets copy-pasted rather than derived — a 410 the API cannot emit, eight 423s with no lock guard, ~30 reachable-but-undocumented 404/400/413/409s. Plus eleven claims that were simply false:
DELETE /files/{id}said it deletes stored bytes (it archives), execute documented a conflict code only v1 emits, a spec claimed every endpoint uses the canonical envelope while one returns octet-stream, and the timestamp rule justified itself with a rendering claim that 29 counter-examples disprove.Eleven operations always reject a workspace API key and now say so — four answer 404, because concealment rewrites the denial, so a workspace key was previously told the resource did not exist.
Guards
Three new or hardened, each verified non-vacuous by injecting a defect and watching it fail:
check:route-verbs— cross-checks all 212 builder routes' exported verb and path against their contract. The builders only compared at runtime, so a half-done rename passed CI and 500'd in production. Catches wrong verb, wrong path, and three classes of unresolvable binding; refuses to pass vacuously.Verification
bun run type-check, 7,922 tests across the affected suites,check:openapi(7 specs, 128 operations, 130 contracts, 225+100 examples),check:route-verbs(212 handlers),check:api-validation, and Biome — all pass. Rebased on latest staging.Every new test was verified to fail against the unfixed code.
Known and deliberately not in scope
{ data: { arrayKey: [...] } }on seven endpoints (defensible, undocumented as a rule);[id]vs[fooId]naming across 22 directories; no idempotency on any of 130 endpoints; the per-operation rate-limit bucket, which makes the account-level ceiling ~130× the advertised rate; fourdynamic/revalidatevariants; and the four internal domains never migrated to the shared use cases, which is the structural cause behind most of what was found here.