chore(deps): resync the npm lockfile, and bound the name a preference is stored under - #921
Merged
Merged
Conversation
package.json asks for playwright and @playwright/test ^1.63.0 — bun.lock carries 1.63.0, package-lock.json still pinned 1.62.1 and still declared ^1.62.1 in its root entry. The two lockfiles had drifted, and `npm ci` refused the tree outright: npm error `npm ci` can only install packages when your package.json and npm error package-lock.json or npm-shrinkwrap.json are in sync. npm error Invalid: lock file's @playwright/test@1.62.1 does not satisfy npm error @playwright/test@1.63.0 CI and install.sh both build with `bun install --frozen-lockfile`, which is why nothing caught it. It still matters: package-lock.json is the manifest GitHub's dependency graph audits — fifteen of the seventeen open Dependabot alerts are filed against it — so a copy that no longer matches package.json makes those verdicts unreliable, and a contributor who reaches for npm cannot install at all. Regenerated with `npm install`; the diff is the playwright family and the three declared ranges that had already moved in package.json. After it, `npm ci --dry-run` and `bun install --frozen-lockfile --dry-run` both succeed, and `npm audit` reports 0 vulnerabilities with and without dev.
The only rule on a preference NAME was the route's prefix test, so
everything after `ui_` was free — any length, any character. Both doors put
that caller-supplied name straight onto an object:
result[key] = allConfig[`${PREFERENCE_KEY_PREFIX}${key}`]; // GET
entries[`${PREFERENCE_KEY_PREFIX}${key}`] = value; // POST
and the second one lands in config.json, so a caller with a session could
park unbounded arbitrary names in the owner's store — up to the 64 KB a
value may be, per name, with nothing bounding how many. Code scanning has
been flagging both writes (js/remote-property-injection, alerts #300 and
#301 on main); they are the two of the four open ones that beta had not
already closed.
`safePreferenceKey` is the rule, in the module that owns the preference
rules: a length bound and an alphabet, with the name REBUILT character by
character rather than tested and passed through — the same shape as
`safeProjectId` in code-projects.ts, and for the same reason, that a
`.test()` guard leaves the caller's own string in play at the sink. It also
refuses the three names every object literal already has, so a caller may
accumulate into a plain `{}` without that being load-bearing.
Applied at every door, not only the two the analyser named:
- the route's `isAllowed` becomes `allowedKey`, returning the rebuilt name,
so both writes above are made of the alphabet. A name that does not
survive is SKIPPED, exactly as a name with the wrong prefix already was —
the alternative, a 400, would start refusing whole desktop writes over one
stray entry.
- `validatePreference` checks the name before the value, which carries the
rule to the read path (`sanitizePreferences`, both GET branches) so a name
stored before this existed stops being served.
- `sanitizePreferenceValue` answers on the name before walking the value,
which carries it to the machine door (`sanitizePreferenceWrites` — app
install/uninstall, the webapp registry): those callers read entries back
and write them out again, so a junk name they carried is dropped there
rather than rewritten.
The MCP `preferences_set` tool needs nothing: it posts through this route
with a key bounded by its own enum.
128 characters is the bound, which leaves room for the one name this product
assembles at runtime — `app_<appId>_settings`, with a 64-character app id.
The suite pins that one, and the ten fixed names, against the rule.
KrasimirKralev
had a problem deploying
to
e2e-pull-request
September 18, 2026 11:37 — with 
GitHub Actions
Error
GitHub Actions
Error
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. The included review limit has been reached and this organization has disabled usage-based review continuation. Wait for reviews to reset or ask a billing admin to change After included review limits. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
Comment |
The drift the previous commit repaired has nothing standing in front of it. Dependabot does not maintain package-lock.json — the npm ecosystem is `open-pull-requests-limit: 0` — so a grouped bun bump moves package.json and bun.lock and leaves this file where it was, and nothing in the repo installs from it: CI and install.sh both build with `bun install --frozen-lockfile`, and there is no `npm ci` anywhere under .github/. The next weekly bump would have put it back out of sync with nobody the wiser until the dependency graph started reporting a tree we do not ship. The check is npm's own first test — `packages[""]` carries a copy of the three dependency blocks and `npm ci` compares them — plus a range check on the resolved top-level versions, which is the other half of what `npm ci` refuses. Both run in the unit suite that already runs on every PR, so the invariant costs no job. RED against origin/beta's package-lock.json: 3 failed | 3 passed (6), the three being the two dependency blocks and the resolved-range check.
Review of the previous commit, four things.
A name that failed the new rule was skipped and the response was still
`{ ok: true }` — the false-success shape, and it contradicted this route's
own header comment and preference-schema.ts's "the doors do not all answer a
bad value the same way" table, both of which say the user door rejects the
request whole. It matters because the desktop's writers branch on nothing but
the response: InstalledAppSettings renders "Saved" over `preferencesRes.ok`,
and usePreferenceWriter does not read the body at all.
The two cases are now told apart. A name this door does not own — one with no
preference prefix — is still skipped, because the caller was not asking this
route to store it. A name WITH the prefix but spelled impossibly is this
door's and is refused with a 400, like an impossible value, and nothing in
the request lands. The name is not quoted back: the body is both returned and
logged.
The 128-character bound was justified in a comment by a computation nothing
asserted — `app_` plus a 64-character app id plus `_settings` — while the only
boundary test derived its fixture from MAX_PREFERENCE_KEY_LENGTH itself, so it
would have passed at any value of it. Lowering the bound to 64 would have kept
every test green while an app with a long id silently lost its settings. The
widest assembled name is now a literal in the suite, with the constant
asserted against it.
Two comments corrected rather than left to mislead: the reason
`validatePreference` returns for a bad name is not surfaced by either caller
today, and the key in the rejected-write log line is no longer "only
prefix-checked" — `logSafe` there is now the second rule, not the only one.
And `sanitizePreferenceWrites` accumulates into a null-prototype object like
every other accumulator in that module. Its pass-through branch — the keys
that are not preferences at all — is the one assignment `safePreferenceKey`
does not stand in front of, so a config key named `__proto__` would have set
the object's prototype, `Object.keys` would have read it as empty and the
whole write would have been dropped in silence. No caller can send that name
today; all three build their updates from string literals.
Contributor
Author
|
CI green on One note for the record: the CodeQL's verdict on the branch:
|
Contributor
Author
|
@coderabbitai review |
|
…und the preference store
preferences: one write may name at most 32 keys — the read's own cap, the two
doors take the same names — and the store may hold at most 500 pref:* names,
which is what the KV route already bounds data/kv.json by. Both are refused 400
before anything is stored, and the second counts only NEW names so a box that
reached the cap can still change its wallpaper. The name was bounded in shape
by the previous commit and in nothing else: config.get() re-reads and re-parses
the whole file on every call, so a body of 5000 legal names took the desktop,
Settings and the wizard down with it. The accumulator is null-prototype like
every other one in this family, which is what the comment on safePreferenceKey
already claimed.
apps/settings: the writer table is a Map, so a lookup answers for own entries
only. As an object literal it answered for Object.prototype too — appId
"toString" resolved to a function that returns "[object Undefined]" without
throwing, and the route reported configWritten: true over a file it never
wrote, which the app renders as "Connected"; its siblings threw and leaked the
raw message with a 500. Every name Object.prototype carries is refused at the
door, and the ad-hoc charset rule is replaced by the shared APP_ID_RE, which
carries the 1-64 length the producers mint within — without it a caller could
park a megabyte-long skills.entries.<id> key in the harness's own config, and
an id the preferences route can no longer store app_<id>_settings under.
ai-models/configure: the same lookup one route over. PROVIDERS["toString"] is a
truthy function, so the "Unknown provider" guard under it never fired and the
handler carried on with a config that spreads to {}.
uninstall: the comment over the settings delete said setPreferences drops an
undefined. It does not — it would pass it through — so it says what is actually
true of that delete instead.
…sing entry The resolved-tree half of the new guard could report green on a lockfile npm ci refuses. Three holes, each with its own case now: - a caret on a 0.x range stops at the next MINOR, not the next major. The manifest carries two (@xterm/addon-fit ^0.11.0, @xterm/addon-web-links ^0.12.0) and 0.12.0 was judged to satisfy ^0.11.0, which npm calls Invalid. - a range shape the caret regex did not match was waved through. That was ^2 and ^1 today (@noble/ed25519, @noble/hashes) and every future ~x.y.z, >=, * and npm: alias. Partial carets are read properly; anything else is an offender rather than a pass, so the day the manifest grows a shape this judge cannot read, the list says so by name. - a dependency with no node_modules/<name> entry was skipped as "an optional dependency the tree does not carry" — but the skip ran for dependencies and devDependencies too, so a lockfile whose packages[""] was hand-edited to agree with package.json passed green where npm ci says "Missing: X from lock file". Only a declared optionalDependency is skipped now. The scan is a function taking (pkg, lock), which is what lets those last two cases be proved on synthetic input rather than on the repo's own files.
Contributor
Author
Delta review applied — two commits on top of
|
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
1 participant
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.
Why
A triage of the repository's Security tab — 17 open Dependabot alerts, 4 open code-scanning alerts, 0 secret-scanning alerts — turned up two things worth a branch. The rest of the list is already remediated on
betaand is open only because alerts are computed against the default branch,main.What this PR carries
1.
chore(deps): the npm lockfile had drifted out of sync withpackage.json.package.jsonasks forplaywright/@playwright/test^1.63.0,bun.lockcarries 1.63.0,package-lock.jsonstill pinned 1.62.1 and still declared^1.62.1in its root entry — sonpm cirefused the tree outright:CI and
install.shboth build withbun install --frozen-lockfile, and Dependabot's npm ecosystem isopen-pull-requests-limit: 0, which is why nothing caught it and why it would have happened again on the next grouped bump. It matters becausepackage-lock.jsonis the manifest GitHub's dependency graph audits — thirteen of the fifteen still-open Dependabot alerts are filed against it — so a copy that no longer matchespackage.jsonmakes those verdicts describe a tree nobody ships, and a contributor reaching for npm cannot install at all. Regenerated withnpm install, then pinned by a unit test (src/tests/unit/lockfiles-in-sync.test.ts) that makes npm's own first check —packages[""]againstpackage.json— an invariant of the suite that already runs on every PR.2.
fix(preferences): bound the name a preference may be stored under.The only rule on a preference NAME was the route's prefix test, so everything after
ui_was free — any length, any character. Both doors put that caller-supplied name straight onto an object, and the write one lands inconfig.json:So a caller with a session could park unbounded arbitrary names in the owner's store — up to the 64 KB a value may be, per name, with nothing bounding how many. Code scanning has been flagging both writes (
js/remote-property-injection, alerts #300 and #301); they are the two of the four open code-scanning alerts thatbetahad not already closed.safePreferenceKeyis the rule, in the module that owns the preference rules: a length bound (128) and an alphabet, with the name rebuilt character by character rather than tested and passed through — the same shape assafeProjectIdincode-projects.ts, and for the same reason, that a.test()guard leaves the caller's own string in play at the sink. Applied at every door, not only the two the analyser named: the route,validatePreference(which carries it to the read path so a name stored before this existed stops being served), andsanitizePreferenceValue(which carries it to the machine door,sanitizePreferenceWrites— app install/uninstall and the webapp registry). The MCPpreferences_settool needs nothing: it posts through this route with a key bounded by its own enum.The two ways a name can fail are told apart, because they are not the same thing. A name this door does not own — one with no preference prefix — is skipped, as it always has been; the caller was not asking this route to store it. A name with the prefix but spelled impossibly is this door's and is refused with a 400, like an impossible value, and nothing in the request lands. That is the contract the route's own header comment already stated, and the desktop's writers depend on it:
InstalledAppSettingsrenders "Saved" overpreferencesRes.okandusePreferenceWriterdoes not read the body at all, so a silently skipped write would have been drawn as a saved one. The name is never quoted back — the body is both returned and logged.The
all=1read sink (result[key.slice(…)] = value) is deliberately left unrebuilt: its key comes from the store, not from the request, andsanitizePreferencesnow drops any stored name that fails the rule before the response is built.Triage
Every Dependabot alert below is already at or above its patched version on
beta(next16.3.4,sharp0.35.4,fast-uri3.1.7,hono4.13.7,qs6.16.0,js-yaml4.3.2,vitest/@vitest/mocker4.1.11 — in bothpackage-lock.jsonandbun.lock), so no dependency bump is owed. They close whenmainreceives the release.not_usedimages.unoptimizedinnext.config.ts, andnext/imageis on the unauthenticated/loginpage. Measured on a box:/_next/image?url=…answers 200, and with an AVIF/WebPAcceptit negotiatesimage/webp— the optimizer answers requestsserverExternalPackagesnames it and/setup-api/pets/thumbis traced with the libvips.so. Measured on a box: 200,image/pngajv, a runtime dependency of@modelcontextprotocol/sdk, and the MCP server runs on the boxhonoarrives only through@modelcontextprotocol/sdk's HTTP transport (@hono/node-server), andmcp/clawbox-mcp.tsinstantiatesStdioServerTransportand nothing else. Left open rather than dismissed — it is already fixed, and a future HTTP transport would make it live againqsarrives throughexpress/body-parser, again only the MCP SDK's HTTP transport, never instantiated. Left open for the same reason@eslint/eslintrc); nothing on a box runs eslintCode scanning, 4 open on
refs/heads/main:src/lib/code-projects.ts:591deployWebappjoinswebappPath(appId), which rebuilds the id throughsafeProjectId. Closes on the release to mainsrc/lib/code-projects.ts:592src/app/setup-api/preferences/route.ts:74src/app/setup-api/preferences/route.ts:98Secret scanning: 0 alerts, open or closed.
Dismissals made
next, "Unauthenticated Remote Code Execution on windows-hosted servers", critical) →dismissed_reason: not_used, with the reason recorded on the alert: ClawBox ships only on Linux — Jetson arm64 and the x64 Ubuntu desktop installer — so the windows-hosted server path never runs.beta; dismissing an alert a release is about to close would only hide the next regression.Proof
RED, on unmodified
beta:The five key-shape cases fail; the sixth — the runtime-assembled
app_<appId>_settingsname must still be stored — passes, which is the guard against over-tightening.GREEN, on this branch:
For contrast,
npm auditagainstmain's lockfile:9 vulnerabilities (5 moderate, 3 high, 1 critical).On an OpenClaw box (web unit only —
clawbox-setuprestarted, the gateway never touched; branch head built withbun run build,BUILD_IDmoved, commit stamped inbuild-info.json):and the fix itself, against the live store:
Both refusals were whole: the legal
wp_opacitybeside them did not land either, and the only key the run added was the one the third POST legitimately stored. The box was then put back onbeta, rebuilt and restarted;config.jsonis byte-identical to before the test (same sha256, same 9pref:*keys, still600 clawbox:clawbox), the worktree is clean and both services are active.Not run: a full chat turn. The chat transport is a gateway WebSocket and driving one would have written into the owner's own session; the chat page, its capabilities endpoint and the gateway proxy all answer 200 on the branch build, which is as far as this went.
One finding this PR does NOT fix
has_playwright_chromium(install.sh:1697, same shape ininstall-x64.sh) probes for the presence of anychrome-linux*/chromeunder~/.cache/ms-playwright, not for the revision the installedplaywright-corepins — soensure_playwright_chromiumprints "already installed" and a playwright bump never pulls the Chromium it ships against (1.62.1 → revision 1234, 1.63.0 → revision 1243).playwrightis a runtime dependency here, not just an e2e tool:setup-api/browserand the coding-run screenshot verification launch it.Measured on a box on beta today:
playwright-core 1.63.0, cache holdschromium-1243andchromium_headless_shell-1243,browsers.jsonwants 1243 — matched, so the predicted skew is not present on the hardware available here. The probe is still a presence check rather than a revision check, and it belongs in its own PR with a device run of the update step; the bump that introduced the version gap was #832, not this branch.What closes only after a release to main
All fifteen remaining Dependabot alerts and all four code-scanning alerts are computed against
main. Merging this tobetacloses none of them. They close whenmainreceives the release that carries beta's dependency set and these fixes.