fix: validate preference input, bound logged fields, set an explicit redirect policy - #378
Merged
Merged
Conversation
…ntry Preference values reach the config store through three doors: POST /setup-api/preferences, the app-install route, and the webapp registry. Only the first applied the shape rules in preference-schema.ts, so the other two could store a value the rules do not allow. Two changes, either of which is enough on its own: - sanitizePreferenceWrites applies the rules at the two direct writers, covering both the entry being added and the entries they carry over from the read they just did. boundPreferenceText reduces the display labels those doors accept — from the remote store listing and from the webapp caller — to one line within the length a stored string may have. - On read, the rules now apply per entry. A preference that holds a collection is rebuilt from the members that pass on their own, so one member the rules reject no longer takes the whole key with it. The rebuilt collection is checked again as a whole, so the per-key caps still hold. Scalars and closed domains have no members and are unchanged.
On /setup-api/ai-models/configure the model name comes from the request body, and for a local provider it is the whole of the apiKey field, which nothing further constrains. It reaches these lines directly and inside a subprocess error that quotes the command it ran. The cloud-provider model IDs pass a character check but no length one. Route them all through logSafe (src/lib/log-safe.ts), which keeps one value on one line and caps the field. scripts/hermes-dashboard-proxy.js logs an upstream response body with a length cap but no control-character handling. It is CommonJS in its own process and cannot import the TypeScript module, so it restates the same two rules locally.
Both fetch calls omitted the redirect option, so they used Node fetch's default of "follow". A followed redirect returns the response from wherever Location pointed rather than from the path that was asked for, and carries the request there. Set redirect: "manual" on both, for the same reason mcp/lib/api.ts already does. Callers already gate on res.ok, so a 3xx lands in the path they use for "not signed in" — which is what a redirect from this dashboard means.
initProject wrote the project directory and project.json before the name reached the starter templates, so a name the templates could not render left a project that existed but was empty — and the next attempt was refused as a duplicate. The name also had no length limit on the HTTP door, though the MCP door declares one (zText(60)). Check shape and length first, in initProject, so both doors get the rule and a refused name leaves nothing behind.
The keys parameter had no limit, and each key cost one config.get, which re-reads and re-parses the whole store file synchronously. Read the store once for the whole request and cap how many keys one read may name; every caller in the app asks for a single key.
verifyDualLicense reads the licence itself from the environment and from disk, so the key it verifies against is the one input that has to be fixed. Nothing covered that directly. These sign a licence with their own keypair and offer the matching public key through every environment name the module has read, plus the blank shapes that used to reduce an overridable key to an empty one.
📝 WalkthroughWalkthroughThis pull request adds preference sanitization, bounded preference reads, project-name validation, safe logging, and manual redirect handling. It also updates route and unit tests for the new validation, storage, logging, authentication, and license-verification behavior. ChangesPreference safety
Project name validation
Request and logging boundaries
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🦀 ClawReviewYour friendly reef crab, here with the lay of the land. This PR closes three gaps in how ClawBox handles untrusted input: preference values now go through the shape rules at every write door (not just the POST route), request-derived strings are bounded before reaching log lines in the AI config route and the dashboard proxy, and both fetch calls in the Hermes dashboard auth module now declare an explicit redirect policy instead of inheriting Node's follow-by-default. On the read side, preference collections now degrade one member at a time rather than dropping the whole key when one entry is malformed. At a glance
Good to know
— ClawReview 🦀, your resident reef crab. Just orientation — CodeRabbit does the line-by-line, humans do the merge. Conventions: docs. |
github-actions
Bot
added
area: install
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/setup-api/preferences/route.ts`:
- Around line 54-64: Update the key-count validation in the preferences route
before filtering with isAllowed: count the raw values from keysParam.split(",")
and reject requests exceeding MAX_KEYS_PER_READ, while retaining filtering for
allowed keys afterward. Add coverage proving a request containing more than 32
unallowed keys returns the limit error without invoking config.getAll().
In `@src/lib/code-projects.ts`:
- Line 235: Update the commentName sanitization near projectName so it replaces
all JavaScript line terminators, including U+2028 and U+2029, before embedding
the untrusted name in the generated comment. Add regression coverage for both
characters and preserve existing CR/LF sanitization.
In `@src/lib/hermes-dashboard-auth.ts`:
- Around line 33-41: Update the dashboard origin configuration around
DASH_ORIGIN and HERMES_DASH_HOST to permit only loopback hosts over HTTP, or
require HTTPS for non-loopback hosts. Reject or safely handle insecure remote
values before requests send dashboard credentials or session cookies, while
preserving the existing local default.
In `@src/tests/unit/edition-license.test.ts`:
- Around line 34-39: Extend the afterEach cleanup in the edition-license tests
to remove every temporary directory created by the tests, in addition to
restoring process.env. Track the created temporary-directory paths and delete
them recursively after each test, including the setup and test cases referenced
by the comment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: baea78fe-6729-429c-901a-8889a1e26e72
📒 Files selected for processing (14)
scripts/hermes-dashboard-proxy.jssrc/app/setup-api/ai-models/configure/route.tssrc/app/setup-api/apps/install/route.tssrc/app/setup-api/code/route.tssrc/app/setup-api/preferences/route.tssrc/lib/code-projects.tssrc/lib/hermes-dashboard-auth.tssrc/lib/preference-schema.tssrc/lib/webapp-registry.tssrc/tests/routes/preferences-language.test.tssrc/tests/routes/preferences.test.tssrc/tests/unit/code-projects.test.tssrc/tests/unit/edition-license.test.tssrc/tests/unit/preference-schema.test.ts
Comment on lines
54
to
+64
| const keys = keysParam.split(",").filter(isAllowed); | ||
| if (keys.length > MAX_KEYS_PER_READ) { | ||
| return NextResponse.json( | ||
| { error: `at most ${MAX_KEYS_PER_READ} keys per request` }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
| // One read of the store rather than one per key: config.get() re-reads and | ||
| // re-parses the whole file synchronously on every call, so the work of a | ||
| // request would otherwise follow the length of its `keys` parameter. | ||
| const allConfig = await config.getAll(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Count supplied keys before whitelist filtering.
Line 54 removes unallowed keys before the limit check. A request with more than 32 unallowed keys bypasses the cap and still calls config.getAll(). Count the raw split values before filter(isAllowed). Add a test that uses more than 32 unallowed keys and asserts no store read.
Proposed fix
- const keys = keysParam.split(",").filter(isAllowed);
- if (keys.length > MAX_KEYS_PER_READ) {
+ const requestedKeys = keysParam.split(",");
+ if (requestedKeys.length > MAX_KEYS_PER_READ) {
return NextResponse.json(
{ error: `at most ${MAX_KEYS_PER_READ} keys per request` },
{ status: 400 },
);
}
+ const keys = requestedKeys.filter(isAllowed);As per path instructions, src/app/**/*.ts* requires review for resource constraints on embedded hardware.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const keys = keysParam.split(",").filter(isAllowed); | |
| if (keys.length > MAX_KEYS_PER_READ) { | |
| return NextResponse.json( | |
| { error: `at most ${MAX_KEYS_PER_READ} keys per request` }, | |
| { status: 400 }, | |
| ); | |
| } | |
| // One read of the store rather than one per key: config.get() re-reads and | |
| // re-parses the whole file synchronously on every call, so the work of a | |
| // request would otherwise follow the length of its `keys` parameter. | |
| const allConfig = await config.getAll(); | |
| const requestedKeys = keysParam.split(","); | |
| if (requestedKeys.length > MAX_KEYS_PER_READ) { | |
| return NextResponse.json( | |
| { error: `at most ${MAX_KEYS_PER_READ} keys per request` }, | |
| { status: 400 }, | |
| ); | |
| } | |
| const keys = requestedKeys.filter(isAllowed); | |
| // One read of the store rather than one per key: config.get() re-reads and | |
| // re-parses the whole file synchronously on every call, so the work of a | |
| // request would otherwise follow the length of its `keys` parameter. | |
| const allConfig = await config.getAll(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/setup-api/preferences/route.ts` around lines 54 - 64, Update the
key-count validation in the preferences route before filtering with isAllowed:
count the raw values from keysParam.split(",") and reject requests exceeding
MAX_KEYS_PER_READ, while retaining filtering for allowed keys afterward. Add
coverage proving a request containing more than 32 unallowed keys returns the
limit error without invoking config.getAll().
Source: Path instructions
| // as code when the built app loads on the ClawBox origin (stored XSS). | ||
| const commentName = name.replace(/[\r\n]+/g, " "); | ||
| const innerName = jsTemplateEscape(escapeHtml(name)); | ||
| const commentName = projectName.replace(/[\r\n]+/g, " "); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
XSS (CWE-79): Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')
Reachability: External
Reachability path
● Entry
src/app/setup-api/code/route.ts:58
initProject
│
▼
● Sink
src/lib/code-projects.ts
Sanitize all JavaScript line terminators before generating commentName.
The request-derived name reaches the generated // comment. The current replacement omits U+2028 and U+2029, which can terminate the comment and enable stored XSS when the webapp loads. Replace them or remove the untrusted name from the comment. Add regression tests for both characters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/code-projects.ts` at line 235, Update the commentName sanitization
near projectName so it replaces all JavaScript line terminators, including
U+2028 and U+2029, before embedding the untrusted name in the generated comment.
Add regression coverage for both characters and preserve existing CR/LF
sanitization.
Comment on lines
+33
to
+41
| // Every request below sets this. Node's fetch defaults to "follow", which makes | ||
| // a redirect invisible to the caller — the response that comes back is the one | ||
| // from wherever Location pointed, not from the path we asked for, and a | ||
| // redirected request can carry its body and headers there. Resolving redirects | ||
| // manually keeps each call's answer the answer to the call it made: a 3xx from | ||
| // the dashboard means the request did not reach the API, which is what the | ||
| // callers below already treat as "not signed in". Same rule and same reason as | ||
| // mcp/lib/api.ts. | ||
| const REDIRECT_POLICY = "manual" as const; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/lib/hermes-dashboard-auth.ts --items all
rg -n -C 4 '\bDASH_ORIGIN\b|dashboardFetch\s*\(' src/lib/hermes-dashboard-auth.ts .Repository: ID-Robots/clawbox
Length of output: 8090
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- hermes-dashboard-auth.ts ---'
sed -n '1,110p' src/lib/hermes-dashboard-auth.ts
printf '%s\n' '--- HERMES_DASH_HOST / HERMES_PORT references ---'
rg -n -C 3 'HERMES_DASH_HOST|HERMES_PORT|HERMES_DASH_USERNAME' . \
-g '!node_modules' -g '!dist' -g '!build'Repository: ID-Robots/clawbox
Length of output: 17318
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal
Reachability path
● Entry
src/app/setup-api/apps/install/route.ts:112
attempt
│
▼
● Sink
src/lib/hermes-dashboard-auth.ts
Restrict HERMES_DASH_HOST to a local-only endpoint or use HTTPS.
DASH_ORIGIN always uses http://, while HERMES_DASH_HOST can override the loopback default. A non-loopback value sends the dashboard password and session cookie without transport encryption.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/hermes-dashboard-auth.ts` around lines 33 - 41, Update the dashboard
origin configuration around DASH_ORIGIN and HERMES_DASH_HOST to permit only
loopback hosts over HTTP, or require HTTPS for non-loopback hosts. Reject or
safely handle insecure remote values before requests send dashboard credentials
or session cookies, while preserving the existing local default.
Comment on lines
+34
to
+39
| afterEach(() => { | ||
| for (const [name, value] of saved) { | ||
| if (value === undefined) delete process.env[name]; | ||
| else process.env[name] = value; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove temporary test directories after each test.
These tests create temporary directories but never remove them. Repeated test runs leave files in the system temporary directory.
Proposed fix
const saved = new Map<string, string | undefined>();
+const tempRoots = new Set<string>();
+
+function makeTempRoot() {
+ const root = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-licence-"));
+ tempRoots.add(root);
+ return root;
+}
afterEach(() => {
+ for (const root of tempRoots) fs.rmSync(root, { recursive: true, force: true });
+ tempRoots.clear();
for (const [name, value] of saved) {
if (value === undefined) delete process.env[name];
else process.env[name] = value;
}
});
-const root = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-licence-"));
+const root = makeTempRoot();Also applies to: 78-81, 92-92, 98-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tests/unit/edition-license.test.ts` around lines 34 - 39, Extend the
afterEach cleanup in the edition-license tests to remove every temporary
directory created by the tests, in addition to restoring process.env. Track the
created temporary-directory paths and delete them recursively after each test,
including the setup and test cases referenced by the comment.
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.
Three groups of changes: validating values on the way into the preference store, bounding request-derived fields before they reach a log line, and stating a redirect policy where one was left to the default.
Preference input and reads
Preference values reach the config store through three doors —
POST /setup-api/preferences, the app-install route, and the webapp registry — and only the first applied the shape rules insrc/lib/preference-schema.ts.sanitizePreferenceWritesapplies those rules at the two direct writers. Both of them read the current value, merge one entry into it and write the whole thing back, so the check covers the carried-over entries as well as the new one.boundPreferenceTextreduces the display labels those doors accept — from the remote store listing, and from the webapp caller — to one line within the length a stored string may have.Either half is enough on its own; both are here because the second is what holds if something new gets past the first.
Bounded log fields
src/app/setup-api/ai-models/configure/route.ts— the model name comes from the request body, and for a local provider it is the whole of theapiKeyfield, which nothing further constrains. It reaches these lines directly and inside a subprocess error that quotes the command it ran; the cloud-provider model IDs pass a character check but no length one. All of them now go throughlogSafe(src/lib/log-safe.ts), which keeps one value on one line and caps the field. Left alone: theproviderfield, which is already constrained to a key of thePROVIDERStable.scripts/hermes-dashboard-proxy.js— logs an upstream response body with a length cap but no control-character handling. It is CommonJS in its own process and cannot import the TypeScript module, so it restates the same two rules locally.Redirect policy
src/lib/hermes-dashboard-auth.ts— bothfetchcalls omitted theredirectoption and so used Node fetch's default offollow. A followed redirect returns the response from whereverLocationpointed rather than from the path that was asked for, and carries the request there. Both now setredirect: "manual", for the same reasonmcp/lib/api.tsalready does. Every caller already gates onres.ok, so a 3xx lands in the path they use for "not signed in" — which is what a redirect from this dashboard means.Bounds
initProject(src/lib/code-projects.ts) wrote the project directory andproject.jsonbefore the name reached the starter templates, so a name the templates could not render left a project that existed but was empty, and the next attempt was refused as a duplicate. The name also had no length limit on the HTTP door, though the MCP door declares one (zText(60)). Shape and length are now checked first, ininitProject, so both doors get the rule and a refused name leaves nothing behind.keys, and each key cost oneconfig.get, which re-reads and re-parses the whole store file synchronously. It now reads the store once per request and caps how many keys one read may name; every caller in the app asks for a single key.Tests
src/tests/unit/preference-schema.test.ts— per-entry degradation (one malformed entry, the others survive and are still readable),sanitizePreferenceWrites, andboundPreferenceText.src/tests/unit/code-projects.test.ts— the namesinitProjectaccepts, and that a refused one creates nothing.src/tests/routes/preferences.test.ts— the single-read behaviour and the key cap.src/tests/unit/edition-license.test.ts(new) —verifyDualLicensereads the licence itself from the environment and from disk, so the key it verifies against is the one input that has to be fixed. Nothing covered that directly. These sign a licence with their own keypair and offer the matching public key through every environment name the module has read, plus the blank shapes that would reduce an overridable key to an empty one.Verification
Full unit suite run before and after: identical 29 pre-existing failures (Windows-only — path separators,
0600file modes, symlinks, executable bits), 2356 → 2382 passing.eslintunchanged at 92 problems / 22 errors. Typecheck shows the same two pre-existing errors in files this branch does not touch.Deferred
safePathcontainment is lexical in all three implementations (src/app/setup-api/files/route.ts,src/app/setup-api/files/[...path]/route.ts,src/lib/code-projects.ts) — the reported "two" undercounts. Making containment follow links is not a small change: all three are synchronous and pure,realpathwould make them async across ~15 call sites, and it returns ENOENT for the create/write/mkdir/upload destinations that do not exist yet, so each site needs a nearest-existing-ancestor strategy. That is its own piece of work, not a line in this one.Summary by CodeRabbit
Security & Reliability
Bug Fixes
Developer Experience