Repository navigation
Korivi security v1 - #2
Merged
Merged
Conversation
The guard is keyed by names taken straight from the request body, and it
probed two plain objects with them. `allow["constructor"]` and
`map["toString"]` are truthy on any plain object, so a field named after
an Object.prototype member skipped the allow check, then read as
declared, then passed every rule after it — no unknown_field, no type
check, no enum check, no read-only check. The key went on to the
backend.
Confirmed against a running app: `bogus` was rejected, while
`constructor`, `toString`, `hasOwnProperty`, `valueOf` and `__proto__`
were all accepted with no violation at all.
This is the one guarantee the rules layer exists to make ("no silent
200s"), voided by nothing more than the field's name, in the path shared
by every adapter — and, since operation arguments now run through the
same rules, by every declared operation too.
Make the lookups own-property tests and give the field map a null
prototype. Object.prototype pollution itself was not reachable: JSON.parse
makes __proto__ an ordinary own property, and neither spread nor
Object.assign writes through it.
`server.listen(PORT, cb)` binds every interface. Three shipped servers
did that while printing a startup line that said 127.0.0.1, including
the blueprint template that every scaffolded UI app runs.
That matters more than a wrong log line, because a same-origin request
is trusted as the owner without any credential, and the origins those
apps allow are their own guessable localhost URLs. The two together made
a scaffolded Agent App readable and writable by anyone who could reach
the port, with no token:
curl -X POST http://<lan-ip>:8090/api/collections/tasks/records \
-H 'Origin: http://127.0.0.1:8090' -d '{"title":"..."}' -> 200
Verified against a real scaffolded app before the change (bound
0.0.0.0, wrote a record over the LAN address) and after (binds
127.0.0.1, remote connection refused, loopback unaffected). The reverse
proxy mattered most: it carries an owner-scoped grant with scopes ["*"].
The README already promises v0.1 is loopback-trust only. This makes the
code keep that promise; A2APP_HOST is there so exposing an app stays a
deliberate act.
assertInside compares strings, which is not the question a write needs answered. A path with no `..` and no drive letter is "inside" by that test even when a component of it is a link pointing somewhere else, and an app directory is untrusted input: `import` exists to take one from a stranger, and tar, zip and git all carry links — a Windows junction needs no privilege at all. So a shipped app carrying `hooks -> ~/.ssh`, plus a toolkit declaring `hooks/authorized_keys` among its system paths, had that file vendored straight through the link. Reproduced here: assertInside answered "contained" and the write replaced a file outside the project. vendorPaths now resolves containment physically — every component below the project is checked, while the project directory itself may still be a link, since the user chose where it lives. copyTree also replaces a link it finds at a destination rather than writing through it, so a link left in the tree cannot redirect a later sync. The lexical check keeps its own name and behaviour; it is still the right test for a path that is never opened.
Binding to loopback does not keep a browser out. A page on evil.com whose DNS re-resolves to 127.0.0.1 is, to the browser, same-origin with this server: it sends no Origin on a GET, so the origin rule has nothing to judge, and on a single-user app an anonymous caller already reads everything. The page walks describe, the records API, events and tasks, and posts them anywhere. The Host header is what still distinguishes the two: a rebound request carries `evil.com`, because that is the name the page was fetched from. Check it once at the router entry, ahead of the identity document, which is otherwise a free fingerprint for exactly that page. Loopback by address, by name and by IPv6 literal are served; the hosts of any declared allowedOrigins are served, so an app that configures its own UI origin is unaffected; allowedHosts covers a deliberate exception. A lookalike like `localhost.evil.com` is not served. Tested over a raw socket rather than in the conformance suite, because fetch forbids setting Host and a rebinding attacker is a browser that sets it implicitly.
npm strips files named `.gitignore` out of a published tarball. All four blueprint templates ship one, `files` includes `toolkits`, and none of them survive: `npm pack --dry-run` on framework/cli lists 189 files and zero gitignore entries. So `npm i -g agent-app` followed by `scaffold` produced an app directory with no ignore rules at all, and the next `git add -A && git push` published the agent token, the principal password and the live database. The rules the framework itself depends on are now written by the CLI on both scaffold paths, and the database rule follows the toolkit's declared lifecycle.dataDir rather than assuming `data/` — a pocketbase app gets `pb/pb_data/`. Existing lines are kept and only missing ones appended, so a blueprint's own rules survive and re-running changes nothing. Verified by scaffolding with the template's .gitignore deleted, which is what an npm install looks like: the app still comes out protected. blueprint-react-node's template was also the odd one out — it omitted .superuser and every .a2app path, and narrowed the database to data/db.json while backups are written to .a2app/backups/. Brought in line with the other three.
`await request.body()` concatenates the whole stream with no limit, and
uvicorn imposes none either. It ran before the origin check, the
credential check and the rate gate — before routing at all, so the path
did not even have to exist: POST /api/junk with a multi-gigabyte body was
buffered in full and only then answered 404.
adapter-core caps at 5 MiB and says why: "a protocol write is tiny;
anything larger is a mistake or an attack, and buffering it whole would
let a hostile client OOM the app". The react-node blueprint inherits
that; this one had no equivalent.
Count bytes as they arrive rather than trusting Content-Length, and
answer 413 in the protocol's own envelope. A malformed body now becomes
{"__unparsed__": ...} for the guard to reject on its own terms, matching
adapter-core, instead of raising out of json.loads into a bare 500.
main moved a long way while this sat open, and three files conflicted. Resolved so both sides keep what they added: - adapters/adapter-core/package.json - main added test/static.test.mjs, this branch added test/host.test.mjs. Both now run. main's formatting is kept; the wholesale reindent on this side was an accident of the tool that wrote it, not a change anyone asked for. - framework/cli/package.json - same shape: manifest-merge and containment both run, alongside copy-tree. - toolkits/blueprint-react-node/template/server.mjs - main moved static serving into the View (view.handler). Took main's structure and re-applied the loopback binding on top of it, which is the fix that mattered here: listen(PORT) alone binds every interface. All six security fixes verified present after the merge, and the scratch files that were reverted on main do not come back - the revert wins over this branch's older copy of them. build and typecheck clean; conformance 60/60; every suite on both sides passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.