fix: route every direct preference write through one checked door - #379
Merged
Conversation
The uninstall route is a third writer that reaches the config store without going through POST /setup-api/preferences. It reads installed_apps and installed_meta, drops one entry and writes the rest back, so it carries whatever it read straight out again — the case the other two doors were just fixed for. Add setPreferences() and point all three at it, so the rules are applied in one place rather than restated per call site. Deliberately not folded into config-store.setMany: most of what that writes is not a preference (provider tokens, setup flags, updater state) and a length cap and a ban on control characters would be wrong for an opaque secret. sanitizePreferenceWrites now leaves keys outside the pref: namespace alone, so its contract matches its name.
Checking it only in initProject left the same user-facing label capped at 60 through `code init` and unbounded through `webapp create` and `code build`. deployWebapp is already documented as the chokepoint those two share, so the rule goes there as well. The webapps route mapped every thrown error to a 500; a refused field is the caller-s to fix, so ValidationError now answers 400 as it already does on the code route. Renamed to assertProjectName, matching assertMutableTarget in the same file — validateProjectId next to it returns a boolean, and the two contracts should not share a prefix.
PREFERENCE_KEY_PREFIX was introduced and then not used where the prefix is actually taken apart, leaving a bare slice(5) tied to its length by nothing. Also corrects the key-cap comment: the largest caller is the preferences_get MCP tool, which sends its whole readable-prefs allowlist in one request, not a single key. The cap is now counted before the allowlist filter so it bounds what a request names rather than what survives it.
The proxy copy of logSafe had drifted from the module it restates on the two points that module documents: it wrote U+FFFD as a literal glyph, and it accepted a non-string. Match the source, and drop the unused default and constant — it has one call site with an explicit cap. Tests: assert the whole outcome rather than short-circuiting through .ok, so a failure shows what was dropped instead of a boolean. Split the looped licence cases into named ones, clean up the temp directories they create, and drop two environment names that never existed.
📝 WalkthroughWalkthroughThe changes centralize preference sanitization and persistence, enforce shared preference-key handling, validate project names before writes, map validation errors to HTTP 400, and update logging, typing, and license-test utilities. ChangesPreference storage boundaries
Project and webapp validation
Targeted maintenance updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
🦀 ClawReviewClaws waving — here's what this change is about. Follow-up to #378 that closes a gap in the preference-write safety net: the app-uninstall route was a third direct config-store writer that #378 missed, and this PR routes all three through a new At a glance
Good to know
— ClawReview 🦀. I set the scene; CodeRabbit reviews the code; you decide. Conventions: docs. |
github-actions
Bot
added
area: install
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const result: Record<string, unknown> = Object.create(null); | ||
| for (const key of keys) { | ||
| result[key] = allConfig[`pref:${key}`]; | ||
| result[key] = allConfig[`${PREFERENCE_KEY_PREFIX}${key}`]; |
| return NextResponse.json({ error: check.reason ?? "Invalid preference value" }, { status: 400 }); | ||
| } | ||
| entries[`pref:${key}`] = value; | ||
| entries[`${PREFERENCE_KEY_PREFIX}${key}`] = value; |
| await fs.writeFile( | ||
| path.join(WEBAPPS_DIR, appId, "meta.json"), | ||
| JSON.stringify({ name: meta.name, color: meta.color || "#f97316", icon: meta.icon || "" }), | ||
| JSON.stringify({ name, color: meta.color || "#f97316", icon: meta.icon || "" }), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/hermes-dashboard-proxy.js`:
- Around line 115-119: Update the caller of logSafe in the configure route’s
model-name logging path to pass an explicit maximum length, such as 200 or the
existing shared maximum-length constant. Ensure the model name remains included
and the truncation suffix does not receive an undefined maxLength.
In `@src/tests/routes/webapps.test.ts`:
- Around line 22-28: Add a test in the webapp route tests that configures
deployWebapp to reject with the visible ValidationError mock, then asserts the
route returns HTTP 400 and the expected JSON error message. Keep the existing
empty-name test unchanged, since it does not exercise the err instanceof
ValidationError branch.
🪄 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: 7e123355-cc95-4217-8906-b728d1f48898
📒 Files selected for processing (16)
scripts/hermes-dashboard-proxy.jssrc/app/setup-api/apps/install/route.tssrc/app/setup-api/apps/uninstall/route.tssrc/app/setup-api/code/route.tssrc/app/setup-api/preferences/route.tssrc/app/setup-api/webapps/route.tssrc/lib/code-projects.tssrc/lib/hermes-dashboard-auth.tssrc/lib/preference-schema.tssrc/lib/preference-store.tssrc/lib/webapp-registry.tssrc/tests/routes/preferences-language.test.tssrc/tests/routes/preferences.test.tssrc/tests/routes/webapps.test.tssrc/tests/unit/edition-license.test.tssrc/tests/unit/preference-schema.test.ts
💤 Files with no reviewable changes (2)
- src/tests/routes/preferences.test.ts
- src/tests/routes/preferences-language.test.ts
Comment on lines
+115
to
+119
| function logSafe(s, maxLength) { | ||
| if (s.length <= maxLength) return s.replace(LOG_CONTROL_CHARACTERS, LOG_REPLACEMENT); | ||
| // Cut first, then sanitise the head only: every character the pattern matches | ||
| // is one UTF-16 code unit replaced by one, so no match can straddle the cut. | ||
| const head = s.slice(0, maxLength).replace(LOG_CONTROL_CHARACTERS, "�"); | ||
| const head = s.slice(0, maxLength).replace(LOG_CONTROL_CHARACTERS, LOG_REPLACEMENT); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the required maximum length at the caller.
logSafe no longer provides a default for maxLength, but src/app/setup-api/ai-models/configure/route.ts:387 still passes only one argument. JavaScript then produces ...[+NaN chars] and omits the model name. Pass the intended limit, such as 200, or a shared maximum-length constant.
🤖 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 `@scripts/hermes-dashboard-proxy.js` around lines 115 - 119, Update the caller
of logSafe in the configure route’s model-name logging path to pass an explicit
maximum length, such as 200 or the existing shared maximum-length constant.
Ensure the model name remains included and the truncation suffix does not
receive an undefined maxLength.
Comment on lines
+22
to
+28
| // The route maps this to a 400; deployWebapp throws it for a name it refuses. | ||
| ValidationError: class ValidationError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = "ValidationError"; | ||
| } | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for the ValidationError response path.
The mock class is correct, but no test throws it. The existing empty-name test returns before deployWebapp, so it does not cover err instanceof ValidationError. Configure deployWebapp to reject with new ValidationError(...) and assert the HTTP 400 response and JSON error message.
🤖 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/routes/webapps.test.ts` around lines 22 - 28, Add a test in the
webapp route tests that configures deployWebapp to reject with the visible
ValidationError mock, then asserts the route returns HTTP 400 and the expected
JSON error message. Keep the existing empty-name test unchanged, since it does
not exercise the err instanceof ValidationError branch.
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.
Follow-up to #378, from reviewing that diff. One real gap, one inconsistency, and three tidy-ups.
The gap: a third direct writer
#378 applied the preference rules at two writers that reach the config store without going through
POST /setup-api/preferences. There is a third —src/app/setup-api/apps/uninstall/route.ts. It readsinstalled_appsandinstalled_meta, drops one entry and writes the rest back, so it carries whatever it read straight out again. That is exactly the case the other two were fixed for, and the docstring #378 added claimed every door was covered. It wasn't.Rather than restate the same three lines at a third call site, this adds
setPreferences()(src/lib/preference-store.ts) and points all three at it.Not folded into
config-store.setMany, deliberately: of its 17 call sites, 13 write things that are not preferences at all — provider tokens,wifi_configured,ai_model_configured, updater state. A 4096-character cap and a ban on control characters would be wrong for an opaque secret, sosetManystays a general config writer and the preference rules sit in front of it.Related:
sanitizePreferenceWritesnow leaves keys outside thepref:namespace alone. It previously fell through and validated them as preferences, which is a contract its name does not promise — and is what made the deeper placement unsafe.The inconsistency: one label, two limits
#378 checked the project name in
initProject. The same user-facing label also arrives throughwebapp createandcode build, which reachdeployWebapp— documented in that file as the chokepoint those two share. Net effect was one label capped at 60 through one door and unbounded through the others, while the MCP schema already declareszText(60)for both.The rule now applies at
deployWebapptoo. The webapps route mapped every thrown error to a 500; a refused field is the caller's to fix, soValidationErroranswers 400 there as it already did on the code route.Renamed
validateProjectName→assertProjectName, matchingassertMutableTargetin the same file —validateProjectIdsits ten lines away and returns a boolean, so the two contracts should not share a prefix.Tidy-ups
PREFERENCE_KEY_PREFIXwas introduced in fix: validate preference input, bound logged fields, set an explicit redirect policy #378 and then not used where the prefix is actually taken apart, leaving a bareslice(5)tied to its length by nothing.preferences_getMCP tool, which sends its whole readable-prefs allowlist in one request (9 keys today). Corrected, with the headroom stated. The cap is now counted before the allowlist filter, so it bounds what a request names rather than what survives it.sanitizePreferenceValueskipped a second walk that could only reach the answer the first one gave — a prune that dropped nothing cannot change the verdict.CLOSED_DOMAINSis aMap, so a key named after something onObject.prototypereads as "no domain" by construction and thehasOwnPropertyhelper goes away.logSafehad drifted from the module it restates on the two points that module documents: it wrote U+FFFD as a literal glyph and accepted a non-string. Matched to the source; unused default and constant dropped..ok, so a failure shows what was dropped rather than a boolean. The looped licence cases are split into named ones, their temp directories are cleaned up, and two environment names that never existed are gone.Verification
Full unit suite: the same 29 pre-existing failures as
beta(Windows-only — path separators,0600modes, symlinks, exec bits), 2392 passing.eslintunchanged at 92 problems / 22 errors. Typecheck shows the same two pre-existing errors in files this branch does not touch.Summary by CodeRabbit