Commit c930830
authored
fix(v2,cli): second audit pass over the v2 API and CLI (#7126)
* fix(chat): resolve a caller-supplied conversation id through its owner
The v2 chat route used the caller-supplied conversationId verbatim, with no
existence, owner, or workspace check, against a store keyed by bare text with
no owner column. A caller who knew another user's conversation id reached that
conversation. Ids now resolve through the same owner-scoped loader the web chat
path uses, and anything unresolvable answers one uniform 404 before any
lifecycle work runs. Omitting the id mints a server-issued conversation.
The contract also accepted any 1-128 character string for a column typed uuid,
so a malformed id raised a driver error and rendered 500 while an unknown but
well-formed id rendered 404 - a shape oracle, and a 500 on ordinary input.
The ownership predicate had no coverage anywhere: the route test mocked the
module and the lifecycle test drove a chain mock that ignores its where clause,
so deleting the owner condition left both suites green. It is now asserted by
composition and by condition count, which is what catches a dropped condition.
Also renames the reply's model identifier away from a term the project's own
copy rules forbid on a user-facing surface.
* fix(v2): conceal workspace absence, and stop archived tables faulting their page
Two reads answered a caller more than they were entitled to know.
A workspace a caller cannot reach at all returned FORBIDDEN while one that does
not exist returned NOT_FOUND, so a workspace-key holder could enumerate which
workspace ids exist by diffing the two. Both now answer the same absence, using
the concealment policy the billing routes already use. A refusal from inside the
workspace - a member whose role is too low - still answers FORBIDDEN, because
that caller already knows the workspace exists.
Separately, archiving a folder cascades onto its tables but leaves each table
pointing at the archived folder row. The archived listing resolved those paths
strictly, so one such row faulted the whole page and no cursor could step past
it - which also made the ids undiscoverable and left restore unreachable for
exactly the tables that need it. The archived scope now resolves leniently to
the root, where a restore would place them, matching the shipped workflows
behavior. Active listings still fault loudly on a dangling folder.
* fix(knowledge): validate upload processing options without stranding live sessions
recipe and lang were accepted as free strings up to their length caps, silently
discarded, and echoed back nowhere, so a typo was unobservable: uploading with a
misspelled recipe returned 200 and quietly used the default. Both are now
validated at the boundary and a bad value answers 400 naming what is accepted.
The accepted recipe set deliberately includes the sentinel every first-party
caller sends today alongside the three real chunker recipes, and the three are
derived from the chunker's own union so removing one there is a compile error
here rather than a silent 400 in production.
The same schema also parses metadata read back off a persisted upload session,
so tightening it would have thrown out of resume and complete for any session
created before this - a 500 on work that could then never finish. The read-back
path now drops a value it no longer recognises instead of rejecting it; the
request boundary stays strict.
Neither field reaches chunking, so nothing here moves chunk boundaries,
embeddings, or search results.
* fix(v2): honour a requested stats window, and answer a claimed graph id with a conflict
Log statistics accepted a start and an end, filtered the totals by them, and
then built the series against wall-clock now. Bucket width was computed over a
span the caller never asked for, and every bucket past the requested end was
structurally empty - so a bounded historical query returned a wrong-width series
with fabricated trailing buckets, under a window label that disagreed with the
request. Each edge now honours the bound it was given and keeps its previous
derivation when omitted, so an unbounded request is unchanged.
Separately, block, edge and subflow ids are global primary keys while the
delete that precedes a state replace is scoped to one workflow. An id owned by
another workflow survived that delete, the insert violated the key, and because
callers pass their own transaction the driver error escaped unclassified as a
server fault. The write now refuses such an id up front with a conflict naming
it, and re-classifies the same violation if one races past the check, since the
lock covers only the workflow being written. The dry run checks the ids a commit
would insert and reports the warnings a commit would report, which is what its
own contract already promised.
* fix(secrets): let a workspace secret change its metadata without resending the value
Restoring redaction cost more than removing it. The only way to flip a secret
back to redacted was to re-send the plaintext, because the write required a
value and omitting it fell into an interactive prompt that cannot run in CI.
A workspace secret can now change its description or visibility on its own; the
stored value is never re-encrypted or rewritten, a write that names no existing
secret answers not-found rather than creating one, and a personal secret still
requires a value because it has no other writable field.
The path parameter was also one shared schema across the write and the delete,
so a single description had to cover both and the delete documented an argument
that could create and replace. Split, mirroring the credentials pair.
The metadata write is a new update against the credentials table, so its scope
is asserted by composition and by condition count: an unscoped update would let
one workspace flip another workspace's identically-named secret out of
redaction, and the cache invalidation would then carry that flag into the other
workspace's runtime catalog.
* fix(v2): say what an error means in terms the caller can act on
A size-limit refusal collapsed every value under a kilobyte to "0 Bytes", so a
28-byte file over a 27-byte ceiling read "is 0 Bytes, above the 0 Bytes limit" -
self-contradictory, and useless for choosing a value that would work.
Errors and field descriptions also told callers to invoke raw HTTP endpoints.
These strings serve the REST reference and the CLI's own help equally, so they
now name the operation and its object rather than a method and a path. A sweep
test walks every v2 schema description and holds the line, with the remaining
offenders in files this change does not own recorded explicitly rather than
left to be rediscovered.
Listing the editors of a built-in skill claimed the skill did not exist, while
reading the same id succeeded - a well-formed request for a real resource is
not malformed, so the list answers an empty roster and only the mutations
refuse.
Bulk folder deletion recorded only the leaf name in its audit trail while the
single delete recorded the full path, leaving two same-named folders under
different parents indistinguishable after the fact.
Bulk chunk enable, disable and delete each treated an unmatched id differently
behind one sentence of documentation. They now follow one rule.
A workspace-scoped list refused with the name of a resource the caller never
addressed, which reads as an empty workspace rather than an unreachable one.
* fix(cli): stop a config value forging a section it was never meant to write
The config file is written by joining names and values into INI lines, and
nothing checked what was in them. A profile name carrying a newline and a
section header wrote a section that merged into a different profile and took
over its endpoint - and the next command sent that profile's stored API key
there. A workspace value could do the same from the other side, since only the
endpoint flag validated its input.
The refusal now lives at the writer, the single place untrusted text enters the
document, with the flag-level checks kept for the better message. Either alone
blocks the forgery; the pair is deliberate.
Rejecting rather than escaping, because the format has no escape syntax and
these files are hand-edited and read by other tools that would not decode one
we invented. The forbidden set covers control characters and the two Unicode
line separators, which the previous guard missed - those parse as an unreadable
line, so the key silently vanished on read and the next write appended a
duplicate while the command reported success.
Login also wrote the key before the settings, so a malformed response from the
deployment could leave a key on disk with no endpoint beside it, and the next
command would send it to the default host. Settings are written first, and the
response is checked before anything touches disk.
Name validation applies only when creating a profile, so a hand-written one
that predates the rule keeps working.
* fix(docs): tell the reader which key a command needs, and stop the ids contradicting the CLI
Around sixty v2 operations refuse a workspace API key, and the CLI's help said
nothing about it - the caller found out from a 403 after the request went out.
The restriction is already stated in the API spec, so the generator now reads it
from there and the command description carries it. The sentinel sentences are
imported from the spec's own constants rather than copied, so a reword cannot
silently unmark every command, and the test pins the count as well as named
operations because a reword confined to one family would otherwise slip past.
The generated reference also rendered an empty default as a sentence pointing at
nothing - "Defaults to ." - for every repeatable filter. Omitted now, while
false and zero still render, which is the trap that shape of check usually
walks into.
The hand-written guides used a workflow-shaped id for workflows that the CLI's
own help says never names one, and five other families were equally wrong. All
of them now match the scheme the CLI declares, consistently per entity across
pages, with the shared ones taken from that help text so the two read as one
voice.
The page documenting every flag was linked from nowhere; both landing links
pointed at the overview instead. And the generator's test file was absent from
the hand-maintained list CI runs, so its guards never executed.
* fix(cli): stop a page-size default capping a destructive filter
Every request field named limit inherited the pager's default of 100, but only
a cursor-paginated command interprets that flag. The two filter-based row
mutations declare no cursor, so the default went onto the wire as a row cap: a
filter matching 250 rows deleted 100, exited 0, and said nothing - while the
confirmation the user had just answered promised every matching row. The flag's
own help offered 0 for everything, which those endpoints reject; the unbounded
form is the field being absent. The pager's default now applies only where the
pager runs, and the tests pin the omission on the request body rather than in
help text.
A cap typed alongside an explicit row list was silently ignored; it is now
refused on the client, where refusing costs nothing to already-installed
versions.
Lists also truncated at a hundred with no signal in any format, and the two
inventory endpoints that do report truncation had that field dropped on the way
out - so a caller reconciling against a clipped list could not tell. One note
now goes to stderr while stdout stays a bare array, and a flag raised on a later
page survives the fold.
Also: a folder whose name contains the separator no longer prints a path that
resolves to a different folder; validation errors name the flag the user typed
instead of the wire field; an unknown subcommand with --help exits non-zero
instead of printing the parent's help; a fractional or negative page size is
refused rather than floored; an empty query filter is refused rather than
silently returning everything; and the two spellings of the missing-workspace
message became one.
* fix(cli): gate destructive table imports and fix follow-mode rendering
A `tables import --mode replace` empties the table before its first batch,
so the only warning was in the describe. It now confirms, and the wording
tells the truth per mode: cancelling a replace leaves a prefix of the new
file with the originals already gone, while an append re-adds its rows if
the file is imported twice. `--yes` skips the gate, and the gate runs
before the file is opened.
Import and export cancellation carried no describe at all; the import one
now confirms, the export one records why it deliberately does not.
Follow-mode output truncated cells to whatever the first row happened to
measure, so a longer status or workflow name arrived clipped with no
signal. Cells now clamp at a shared ceiling and pad to the lock, and the
log columns carry width floors so a short first page cannot pin a column
narrower than its own values.
Interrupting a staged download left the staging directory behind; it is
now removed on SIGINT and SIGTERM before the signal is re-raised.
`--select-output` without `--follow` selected from a response that does
not carry outputs, and said nothing. It is refused client-side, with a
separate message for `--async`. Its describe now names what the path
addresses.
`secrets set` always read a value, even when only metadata flags were
passed. Off a TTY that was an immediate refusal, so a metadata-only edit
exited 1 in CI for a value it was never asked for; on a TTY it stopped to
prompt, and the prompt rejects an empty entry, so there was no way to say
"leave the stored value alone" short of re-typing the secret. The read is
now skipped and the field omitted, which is what lets a metadata-only edit
run unattended. On a TTY, setting only a description no longer prompts.
Passing both spellings of the reveal flag is refused rather than silently
resolved.
Four mandatory hand-authored flags now say so, `billing logs` names its
key-type scope, and the dispatch list declares its columns.
* fix: close the gaps an adversarial review of this branch found
A conflict handler added earlier in this branch was dead code. It read the
Postgres error code off the thrown object, but the driver error arrives
wrapped with the real one on `cause`, so the check returned false on its
first line and the 409 never fired. Its test passed only because it threw a
flat shape production never produces. It now reads through the cause chain
with the shared helpers, compares the constraint name exactly instead of
matching a substring of the SQL, and its test throws the real wrapped error.
Resuming a conversation checked its workflow and its workspace but not its
type, so a conversation created by the web surface could be continued as a
CLI turn. It now refuses through the same uniform 404 as every other
mismatch, which closes the same omission on the web posting path. Minting
one no longer leaves a blank untitled row at the top of the Chat list.
The pre-write check on a minted API key refused fewer characters than the
writer does, so a key the check accepted could still fail at the write —
after the endpoint beside it was already stored, pairing a new endpoint with
the previous key. The two had drifted because the set was spelled three
times; there is now one.
A description claimed a processed count reported only the chunks that
changed. The update returns every row it matched, so re-enabling chunks that
were already enabled counts them all. Two OpenAPI sentences promised no
conflict detection and no persistence warnings in a dry run, both of which
the same branch had just made false. A described window was wrong whenever a
start was supplied without an end.
Listing the editors of a built-in skill answered a read with a modification
refusal on the internal surface. Archived table listings could reach the
strict folder projector again through a third scope value the input type
still allowed. A metadata-only secret write skipped the guard its
personal-scope twin has. The internal document boundary still took the two
processing fields as unbounded strings. Truncation was reported only from
the response envelope, so a clipped file body, row search and workflow-stats
list said nothing.
A staged download stopped watching for signals before it finished removing
its directory, and cleared every listener for the signal rather than its own.
Three tests asserted a contract constant against itself; they now drive
rendered help, real argv, or real render output.
* chore: regenerate the API reference, CLI surface, and CLI docs
The published reference still marked a secret value required and described
the delete parameter as one that also creates, the CLI surface still lacked
the marker that says which operations refuse a workspace key, and the
reference rendered an empty sentence for every repeatable filter whose
default is an empty list.
* test(cli): use the package's own delay helper in the staging poll
The audit bans a hand-rolled setTimeout promise. `sim-cli` does not depend
on the shared utils package, and its own idiom is `node:timers/promises`.
* fix: act on a second review round, and correct two earlier claims
The conflict pre-check read block ids from the wrong side. The writer
inserts each block's own `id` field while the check read the record key,
and the two can diverge because preparation copies a value under its key
without reconciling them. Edges already read the value and subflows are
genuinely keyed by the record key, so only blocks were wrong — collecting
every family from the values, as first suggested, would have broken
subflows instead.
A minted API key carrying leading or trailing whitespace passed the
pre-write check but failed the writer, leaving the new endpoint on disk
beside the previous key. It is refused up front now rather than trimmed: a
key is opaque, so trimming would store a value the server never issued and
turn a loud failure into an unexplained 401 later. The endpoint normalizer
does trim, which is what made a padded `--endpoint` fail only after the
browser flow had already minted a key.
A metadata-only secret write raced with deletion returned 500, because the
follow-up read that only assembles the response body threw an unclassified
error; it now reports the same not-found the non-racing miss already gave.
An unusable output format in the environment silently printed a table
instead of refusing. Two validation messages printed control characters
verbatim. A dry run now reports the preparation warnings its own commit
path returns.
The chat route created a titled conversation and never wrote a message, so
it appeared in the Chat list promising content it did not have. Both sides
of a successful turn are now persisted; a failed turn still writes nothing,
so a question is never stored without its answer.
Two claims of mine were wrong. The earlier commit message said `secrets
set` sent an empty value that overwrote the stored secret — it did not; the
prompt refuses off a TTY and rejects empty on one, so the old behaviour was
a clean refusal. And the delay helper commit said this package's idiom is
`node:timers/promises`; the package carries its own `sleep`, which is the
audit's sanctioned home and has five callers. It uses that now.
Also: a test asserting a deadlock stays unclassified could not fail, since
every candidate rejects it; it now pins a unique violation carrying no
constraint name. Workflow ids spelled with the file prefix are corrected in
the remaining fixtures, leaving the genuine file ids alone.
* fix: close a credential-misdirection path this branch had opened
Making the endpoint normalizer trim handled whitespace around a value but
not a control character inside one, and the URL parser removes those from
anywhere in its input — so a value that reads as one host could resolve to
another, and the profile's key went with it. The flag and environment paths
never touch the config writer, so its guard did not cover this. The
normalizer now refuses the same character set the writer does, which also
keeps the invariant that nothing it blesses can be refused by the write
that stores it. Comparing the parsed URL back against its input was the
alternative and is wrong: the parser rewrites percent-encoding, case,
internationalized hosts and default ports, so legitimate endpoints would be
refused.
The blank-query guard tested for exactly empty, so a whitespace-only value
still reached the wire — as a real zero on a numeric filter, an explicit
false on a boolean one, and as an encoded space the server then rejected.
It now refuses any value that is blank once trimmed, while a body string
keeps its meaning, an explicit zero still sends, and a value with content
around its whitespace is passed through untouched rather than trimmed.
A graph-id conflict reported 409 on the v2 route and fell through the older
persistence wrapper as an unclassified 500. That wrapper now classifies
orchestration failures through the cause chain, which also fixes a
pre-existing case where a workflow archived between authorization and the
locked read reported 500 rather than 404.
Persisting a chat turn claimed its row by id alone, so a conversation
soft-deleted mid-turn still received the messages and was bumped back up
the list. It now requires a live row. A turn whose caller hung up after the
model had already answered persisted nothing, though the work was done and
billed; it now persists and still reports the connection as closed.
An empty workspace id from the login response was read as no workspace at
all. A published description still promised a language-tag standard the
schema does not enforce.
The test asserting that a turn is stored before the final event drained the
whole response first, so it held whichever order the code used. It now
reads the stream incrementally and fails if the write moves after the
event.1 parent 2d4dfcf commit c930830
148 files changed
Lines changed: 7712 additions & 696 deletions
File tree
- apps
- docs
- content/docs/en/cli
- sim
- app
- api
- logs/stats
- mothership/chats
- v2
- chat-deployments
- chat
- secrets/[name]
- workspaces/[workspaceId]
- members
- workspace/[workspaceId]
- knowledge/hooks
- skills/[skillId]
- lib
- api/contracts
- knowledge
- v2
- __tests__
- openapi
- copilot
- chat
- tools/server/table
- credentials
- knowledge
- application
- chunks
- logs
- application
- mcp/application
- mothership/inbox
- secrets/application
- skills/application
- table/application
- workflows
- application
- persistence
- workspace-files/application
- workspaces/api
- packages/sim-cli/src
- commands
- protocol
- config
- contract
- generated
- runtime
- scripts
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
| 19 | + | |
18 | 20 | | |
19 | 21 | | |
20 | 22 | | |
| |||
41 | 43 | | |
42 | 44 | | |
43 | 45 | | |
| 46 | + | |
| 47 | + | |
44 | 48 | | |
45 | 49 | | |
46 | 50 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
27 | | - | |
| 27 | + | |
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
| |||
47 | 47 | | |
48 | 48 | | |
49 | 49 | | |
50 | | - | |
| 50 | + | |
51 | 51 | | |
52 | 52 | | |
53 | 53 | | |
| |||
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
61 | | - | |
| 61 | + | |
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
| |||
108 | 108 | | |
109 | 109 | | |
110 | 110 | | |
111 | | - | |
| 111 | + | |
112 | 112 | | |
113 | | - | |
| 113 | + | |
114 | 114 | | |
115 | 115 | | |
116 | 116 | | |
| |||
130 | 130 | | |
131 | 131 | | |
132 | 132 | | |
133 | | - | |
| 133 | + | |
134 | 134 | | |
135 | 135 | | |
136 | 136 | | |
| |||
151 | 151 | | |
152 | 152 | | |
153 | 153 | | |
154 | | - | |
155 | | - | |
| 154 | + | |
| 155 | + | |
156 | 156 | | |
157 | 157 | | |
158 | 158 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
31 | 31 | | |
32 | 32 | | |
33 | 33 | | |
| 34 | + | |
| 35 | + | |
34 | 36 | | |
35 | 37 | | |
36 | 38 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
31 | | - | |
| 31 | + | |
32 | 32 | | |
33 | 33 | | |
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
38 | | - | |
| 38 | + | |
39 | 39 | | |
40 | 40 | | |
41 | 41 | | |
| |||
76 | 76 | | |
77 | 77 | | |
78 | 78 | | |
79 | | - | |
| 79 | + | |
80 | 80 | | |
81 | 81 | | |
82 | 82 | | |
83 | 83 | | |
84 | | - | |
| 84 | + | |
85 | 85 | | |
86 | 86 | | |
87 | 87 | | |
88 | | - | |
| 88 | + | |
89 | 89 | | |
90 | 90 | | |
91 | 91 | | |
| |||
132 | 132 | | |
133 | 133 | | |
134 | 134 | | |
135 | | - | |
136 | | - | |
137 | | - | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
138 | 138 | | |
139 | 139 | | |
140 | 140 | | |
141 | 141 | | |
142 | 142 | | |
143 | 143 | | |
144 | 144 | | |
145 | | - | |
| 145 | + | |
146 | 146 | | |
147 | 147 | | |
148 | 148 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
| 19 | + | |
18 | 20 | | |
19 | 21 | | |
20 | 22 | | |
| |||
78 | 80 | | |
79 | 81 | | |
80 | 82 | | |
| 83 | + | |
| 84 | + | |
81 | 85 | | |
82 | 86 | | |
83 | 87 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
24 | | - | |
| 24 | + | |
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
| |||
107 | 107 | | |
108 | 108 | | |
109 | 109 | | |
110 | | - | |
| 110 | + | |
111 | 111 | | |
112 | 112 | | |
113 | 113 | | |
| |||
194 | 194 | | |
195 | 195 | | |
196 | 196 | | |
197 | | - | |
| 197 | + | |
198 | 198 | | |
199 | 199 | | |
200 | 200 | | |
| |||
220 | 220 | | |
221 | 221 | | |
222 | 222 | | |
| 223 | + | |
| 224 | + | |
223 | 225 | | |
224 | 226 | | |
225 | 227 | | |
| |||
239 | 241 | | |
240 | 242 | | |
241 | 243 | | |
242 | | - | |
| 244 | + | |
243 | 245 | | |
244 | 246 | | |
245 | 247 | | |
| |||
258 | 260 | | |
259 | 261 | | |
260 | 262 | | |
261 | | - | |
| 263 | + | |
262 | 264 | | |
263 | 265 | | |
264 | 266 | | |
| |||
280 | 282 | | |
281 | 283 | | |
282 | 284 | | |
283 | | - | |
| 285 | + | |
284 | 286 | | |
285 | 287 | | |
286 | 288 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
77 | 77 | | |
78 | 78 | | |
79 | 79 | | |
80 | | - | |
81 | | - | |
82 | | - | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
83 | 83 | | |
84 | 84 | | |
85 | 85 | | |
86 | 86 | | |
87 | 87 | | |
88 | 88 | | |
89 | 89 | | |
90 | | - | |
| 90 | + | |
91 | 91 | | |
92 | 92 | | |
93 | 93 | | |
| |||
106 | 106 | | |
107 | 107 | | |
108 | 108 | | |
109 | | - | |
110 | | - | |
| 109 | + | |
| 110 | + | |
111 | 111 | | |
112 | 112 | | |
113 | 113 | | |
| |||
146 | 146 | | |
147 | 147 | | |
148 | 148 | | |
149 | | - | |
150 | | - | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
151 | 153 | | |
152 | 154 | | |
153 | 155 | | |
| |||
156 | 158 | | |
157 | 159 | | |
158 | 160 | | |
159 | | - | |
| 161 | + | |
| 162 | + | |
0 commit comments