fix: refuse control characters in supervisor definitions - #17
Open
devin-ai-integration[bot] wants to merge 1 commit into
Open
fix: refuse control characters in supervisor definitions#17devin-ai-integration[bot] wants to merge 1 commit into
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
Validate native supervisor definition paths before rendering line-oriented systemd units and property lists, and validate the shutdown client envelope against the protocol schema instead of an unchecked cast. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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
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.
Summary
Two input-validation gaps found while auditing the codebase for hardcoded secrets, SQL injection, unvalidated input, CORS/debug exposure, missing auth, and vulnerable dependencies. Everything else came back clean (details below).
1. Supervisor definition injection (
src/supervision/{systemd,launchd}.js).quoteSystemdArgumentescaped\,", and%, andescapeSystemdSpecifiersescaped only%— neither escaped newlines, and systemd units are line-oriented. A--home/PORTREEVE_HOME/PORTREEVE_SOCKETpath containing a newline therefore terminatedExecStart=and appended attacker-chosen directives to the generated unit, e.g.Both renderers now
SupervisorDefinitionSchema.parse(...)first, rejecting\u0000-\u001fand\u007fin any of the five definition paths. Same-user scope only (a caller who controls the flags could run the command directly), so this is hardening of the trusted-path boundary rather than a privilege escape — but the generated unit persists and runs at login, which the flag itself does not.2. Shutdown envelope validated by cast (
src/server/server.js)./v1/server/stopparsedclientasz.record(z.string(), z.unknown())and then cast it to the compatibility shape, sonegotiateCompatibilityread an unvalidatedclient.protocoland a malformed body produced a 500internalinstead of a validated 400. It now parsesClientCompatibilitySchema, matching every other mutating endpoint.Audit findings (no change needed)
randomBytes(32), stored only as SHA-256 and compared withtimingSafeEqual; tokens never reach history payloads or the diagnostic log.registry.js/migrations.jsuses$namebindings. The one interpolated fragment (listStacks'sWHERE ${clauses.join(' AND ')}) is built from fixed clause literals with bound values;PRAGMA user_version = ${...}takes an internal migration constant.assertCompatibleindependently of the router.0600Unix socket in a0700owner-verified directory — no TCP listener, no CORS surface, no unauthenticated network path./v1/logsand/v1/historyare reachable only through that socket.contextIsolationon,nodeIntegrationoff, permission handlers deny, window-open/navigation/webview blocked, customapp://scheme is path-canonicalized with a strict CSP, IPC verifiessenderFrame.urland schema-parses both directions, and the bundled CLI is sha256-verified against its manifest before spawn (shell: falsethroughout). NoinnerHTML/evalanywhere.bun auditreports no vulnerabilities.Testing
bun run check(typecheck, lint, format, tests). The 23 failing tests are preexisting onmainin this environment (nolsof,docker, orrubyavailable); the two added tests pass.Link to Devin session: https://app.devin.ai/sessions/5c7ecf1575ac4564be008f8170c5f3cf
Requested by: @TrentBrown