fix: data-guard coverage, interrupted-flow cleanup, log-field bounds - #372
Merged
Conversation
…d list The protected set for DATA_DIR was a hand-maintained list of filenames, and it had fallen behind the code: the OAuth flow files, the login and credentials-change state, the tunnel state, and the cloudflared directory were all absent from it. A list also cannot describe this directory in principle — an atomic write stages `<name>.tmp.<hex>` beside its target, so some entries are named at runtime. Invert it. Everything under DATA_DIR is covered except an explicit set of user-facing subtrees (webapps, icons, catalog-cache, code-projects, llamacpp), each verified against the module that owns it. DATA_DIR itself stays listable, since the Files API filters a listing entry by entry and that is what surfaces the public subtrees. Also include the backup tool's own directory (~/.clawkeep, holding its portal token and the device's backup passphrase) in the named set, and add `clawkeep` to the MCP name pattern. The route in front of it was already classed as sensitive in middleware; the store behind it was not. Implementation notes: - a prefix test rather than path.relative, which costs about six times the rest of the guard put together on a per-entry path (a file search scans up to 20k) - the first segment is found with indexOf rather than a split on a class of both separators, which on POSIX would read a backslash — a legal filename character — as a path separator Tests cover the full inventory, a runtime-named sidecar, a name invented for the test, over-block guards for each public subtree and for DATA_DIR itself, and the backslash-in-a-name case (POSIX only).
Three gaps in the OAuth token handoff file's lifecycle: - device-poll removed the flow state on its two provider-failure branches but not the handoff tokens, so a failed exchange left them on disk - configure treated a file with no `createdAt` as fresh: the age comparison was guarded by the field's presence, so a file without one never expired. A file whose age cannot be established is now refused and removed, like an expired one - nothing removed a handoff file by age. device-poll's POST now sweeps one past the TTL, alongside the flow-state check it already ran The file had five call sites across five routes with no owner: two writers, one reader, one clear-on-start, and now a sweep — with the TTL spelled twice and a comment in one of them asserting the two windows were the same. Move the path, the TTL and the clear/sweep helpers into src/lib/oauth-handoff.ts so they cannot drift apart, and give the authorization-code flow the same clear-on-start the device-code flow already had, so a handoff file always belongs to the sign-in in progress rather than to whichever one was abandoned last. Tests: an exchange failure leaves no token file, a file without `createdAt` is rejected and removed, an over-age file is swept, and one inside the TTL is kept.
The two console.warn sinks in the wifi update route interpolated the SSID and an nmcli error message straight into a log line. Neither was bounded, and neither had its control characters removed, so one value could span several journal records. The second sink's already-scrubbed text carries the SSID too, since nmcli echoes it back — so it needs the same treatment, and the first now logs a string rather than handing console.warn a raw error object. Add src/lib/log-safe.ts: replace control characters (so one value stays one line and renders as text in a terminal) and cap the length with a count of what was dropped (so the size of a record does not follow the size of its input). It cuts before sanitising, which gives the same string without walking a caller-sized value to produce a bounded line — an execFile message is bounded only by its 1 MB maxBuffer. Also cap the SSID at 32 octets. 802.11 defines the SSID element as at most 32 octets, so a longer value cannot name a network nmcli could act on; counting octets rather than characters is what makes that check mean what it says. It is also what actually bounds how much reaches the log, which the sanitiser alone does not do.
Both accumulators in the preferences GET handler, and the one in sanitizePreferences that they are handed to, are built from key names that come from outside the function. Assigning into a plain object literal can reach an inherited name rather than defining an own property; Object.create(null) means an assignment always defines one. sanitizePreferences matters most of the three — it builds the object that actually reaches the response, so fixing only the callers would move the question one hop rather than settle it. Also run the rejected-write log line through logSafe. The reason string is built from the rejected key, which is caller-supplied and only prefix-checked, so it is a request-derived log field like any other.
📝 WalkthroughWalkthroughThe changes centralize OAuth handoff storage and cleanup, expand credential and ClawBox data-path protection, add bounded log sanitization, enforce Wi-Fi SSID byte limits, and add regression coverage. ChangesSecurity and OAuth hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthStart
participant DevicePoll
participant OAuthHandoff
participant FileSystem
OAuthStart->>OAuthHandoff: Clear previous handoff tokens
DevicePoll->>OAuthHandoff: Sweep stale handoff tokens
OAuthHandoff->>FileSystem: Stat and remove stale file
DevicePoll->>FileSystem: Atomically persist exchanged tokens
DevicePoll->>OAuthHandoff: Clear tokens after failed flow
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 |
🦀 ClawReviewPoked my eyestalks out for this one. Quick tour: Tightens the file-access guard, patches three lifecycle gaps in the OAuth sign-in flow, adds a log-sanitization helper, and fixes a handful of small correctness issues (null-prototype accumulators, SSID octet-length validation). No new runtime behaviour for anything that was already working correctly — this is a coverage and correctness sweep. 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: gateway
![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/ai-models/configure/route.ts`:
- Around line 496-502: Separate the HANDOFF_TOKENS_PATH file read from
JSON.parse in the handoff-loading flow, preserving the existing 400 response for
missing or unreadable files. When parsing fails, remove the malformed handoff
file before returning the 400 response, and add a regression test covering
malformed JSON cleanup and the returned error.
- Around line 505-511: Strengthen the credential validation around
handoff.createdAt so it must be a finite number no later than the current time
before applying HANDOFF_TTL_MS; reject and unlink the handoff for malformed,
NaN, or future timestamps. Add tests covering non-numeric and future createdAt
values while preserving existing stale-token cleanup behavior.
In `@src/app/setup-api/ai-models/oauth/device-poll/route.ts`:
- Around line 32-36: Update discardFlow() in
src/app/setup-api/ai-models/oauth/device-poll/route.ts:32-36 to remove only
STATE_PATH and stop calling clearHandoffTokens(). Update the failure test in
src/tests/routes/ai-models/device-poll.test.ts:322-355 to assert STATE_PATH is
removed while TOKENS_PATH is retained.
In `@src/app/setup-api/ai-models/oauth/start/route.ts`:
- Around line 19-22: Move clearHandoffTokens in
src/app/setup-api/ai-models/oauth/start/route.ts#L19-L22 to run only after the
new OAuth state has been validated and persisted. Apply the same ordering in
src/app/setup-api/ai-models/oauth/device-start/route.ts#L15-L18: validate the
provider response and persist the new device state before clearing the
superseded handoff; both sites require direct changes.
🪄 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: 77816d97-c695-45c5-b605-886dbbe7161e
📒 Files selected for processing (19)
mcp/lib/guard.tssrc/app/setup-api/ai-models/configure/route.tssrc/app/setup-api/ai-models/oauth/device-poll/route.tssrc/app/setup-api/ai-models/oauth/device-start/route.tssrc/app/setup-api/ai-models/oauth/exchange/route.tssrc/app/setup-api/ai-models/oauth/start/route.tssrc/app/setup-api/preferences/route.tssrc/app/setup-api/wifi/update/route.tssrc/lib/file-guard.tssrc/lib/log-safe.tssrc/lib/oauth-handoff.tssrc/lib/preference-schema.tssrc/tests/routes/ai-models/configure.test.tssrc/tests/routes/ai-models/device-poll.test.tssrc/tests/routes/files/path.test.tssrc/tests/routes/wifi-saved-update.test.tssrc/tests/unit/file-guard.test.tssrc/tests/unit/log-safe.test.tssrc/tests/unit/mcp-path-guard.test.ts
Comment on lines
505
to
+511
| if ( | ||
| !handoff.access_token || | ||
| (handoff.createdAt && Date.now() - handoff.createdAt > 15 * 60 * 1000) | ||
| !handoff.createdAt || | ||
| Date.now() - handoff.createdAt > HANDOFF_TTL_MS | ||
| ) { | ||
| // Stale/invalid credential material — consume it so it can't linger. | ||
| await fs.unlink(tokensPath).catch(() => {}); | ||
| await fs.unlink(HANDOFF_TOKENS_PATH).catch(() => {}); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the timestamp value at runtime.
A truthy non-number value produces NaN during subtraction. A future timestamp produces a negative age. Both values bypass this expiry check and let the handoff proceed.
Require a finite numeric createdAt that is not later than the current time before applying HANDOFF_TTL_MS. Add tests for malformed and future timestamps.
🤖 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/ai-models/configure/route.ts` around lines 505 - 511,
Strengthen the credential validation around handoff.createdAt so it must be a
finite number no later than the current time before applying HANDOFF_TTL_MS;
reject and unlink the handoff for malformed, NaN, or future timestamps. Add
tests covering non-numeric and future createdAt values while preserving existing
stale-token cleanup behavior.
Contributor
Author
There was a problem hiding this comment.
Taken — this was a real hole in the line the branch had just changed.
Both cases you name slip through for the same reason: Date.now() - createdAt is NaN for a non-numeric value and negative for a future one, and neither is > TTL, so the file was accepted no matter how old it was. Replacing !handoff.createdAt with a presence check alone would not have covered either.
The age is now computed only from a createdAt that is a finite number, and a null age or a negative one is refused and removed exactly like an over-age file. Tests cover a string, NaN and a future timestamp.
Fixed in 6162632.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git show --stat --oneline 6162632
git show --format=fuller --find-renames --find-copies -- \
src/app/setup-api/ai-models/configure/route.ts \
src/tests/routes/ai-models/configure.test.ts \
6162632
printf '\n--- Current validation hunk ---\n'
rg -n -A22 -B8 'createdAt|HANDOFF_TTL_MS|handoffAge' \
src/app/setup-api/ai-models/configure/route.ts
printf '\n--- Relevant tests ---\n'
rg -n -A15 -B8 'createdAt|future|NaN|non-numeric|handoff' \
src/tests/routes/ai-models/configure.test.tsLength of output: 2147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Current OAuth handoff validation ---'
rg -n -A30 -B10 'createdAt|HANDOFF_TTL_MS|handoffAge' \
src/app/setup-api/ai-models/configure/route.ts || true
printf '%s\n' '--- Configure route test structure ---'
ast-grep outline src/tests/routes/ai-models/configure.test.ts --items all --type function
printf '%s\n' '--- Timestamp-related regression tests ---'
rg -n -A20 -B10 'createdAt|future|NaN|non-numeric|handoff|expired' \
src/tests/routes/ai-models/configure.test.ts || trueLength of output: 12933
@KrasimirKralev, verification is inconclusive for commit 6162632 because that commit is not available in the checked-out repository.
The current configure/route.ts still uses:
!handoff.createdAt ||
Date.now() - handoff.createdAt > HANDOFF_TTL_MSThis code still accepts truthy non-numeric timestamps and future timestamps. The current tests cover missing and expired timestamps only. They do not include the stated string, NaN, or future-timestamp cases.
Please retry after the PR branch contains 6162632. The finding remains unresolved.
🧬 Code Graph Analysis Results
src/lib/oauth-handoff.ts:18,24
export const HANDOFF_TOKENS_PATH = path.join(DATA_DIR, "oauth-device-tokens.json");
export const HANDOFF_TTL_MS = 15 * 60 * 1000;Defines the OAuth handoff file location and its 15-minute expiration interval.
src/app/setup-api/ai-models/oauth/device-start/route.ts:85-106
await fs.writeFile(
tmpPath,
JSON.stringify({
provider: providerName,
device_id: deviceId,
user_code: userCode,
interval,
createdAt: Date.now(),
}),
{ mode: 0o600 }
);
await fs.rename(tmpPath, STATE_PATH);Creates device-auth state with createdAt as a numeric millisecond timestamp and writes it atomically.
src/app/setup-api/ai-models/oauth/device-poll/route.ts:195-210
// 15-minute expiry
if (Date.now() - stored.createdAt > HANDOFF_TTL_MS) {
await fs.unlink(STATE_PATH).catch(() => {});
return NextResponse.json(
{ error: "Device auth session expired. Please start again." },
{ status: 400 }
);
}Provides the analogous device-auth expiry check and removes expired state files.
You are interacting with an AI system.
This was referenced Aug 11, 2026
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.
Coverage, correctness and cleanup across the file guard, the OAuth sign-in flow, and two log/accumulator details. No behaviour change for anything that was already working.
1. Guard the data directory by containment rather than an enumerated list
src/lib/file-guard.tskept a hand-maintained list of filenames underDATA_DIR. It had fallen behind the code — the OAuth flow files, the login and credentials-change state, the tunnel state and thecloudflared/directory were all missing from it. A list also cannot describe this directory in principle: an atomic write stages<name>.tmp.<hex>beside its target, so some entries are named at runtime.Inverted to containment. Everything under
DATA_DIRis covered except an explicit set of user-facing subtrees —webapps,icons,catalog-cache,code-projects,llamacpp— each checked against the module that owns it.DATA_DIRitself stays listable, because the Files API filters a listing entry by entry and that is what surfaces the public subtrees.Two implementation details worth a look:
path.relative. This runs once per entry in a directory listing (a file search scans up to 20k), andpath.relativemeasured at roughly six times the cost of the rest of the guard put together.indexOf(path.sep)rather than a split on a character class of both separators. On POSIX a backslash is a legal filename character, so the class form would read a single entry namedwebapps\xas a path into thewebappssubtree.2. Include the backup tool's directory in the protected set
~/.clawkeep(CLAWKEEP_DATA_DIR) holds the portal token and the device's backup passphrase, and sits inside$HOME. It had no entry inPROTECTED_DIR_RESand noclawkeepalternative in the MCPSECRET_NAME_RE.middleware.tsalready classes/setup-api/clawkeepas sensitive — this is the same rule applied to the store behind the route.3. Remove interrupted sign-in state instead of leaving it
Three gaps in the OAuth handoff file's lifecycle:
device-pollremoved the flow state on its two provider-failure branches but not the tokens.configureguarded its age comparison oncreatedAtbeing present, so a file without one never expired. A file whose age cannot be established is now refused and removed, like an expired one.device-poll's POST now sweeps one past the TTL.The file had five call sites across five routes with no owner — two writers, one reader, one clear-on-start, and now a sweep — with the TTL spelled twice and a comment in one of them asserting the two windows were the same. Path, TTL and the clear/sweep helpers now live in
src/lib/oauth-handoff.ts, and the authorization-code flow gets the same clear-on-start the device-code flow already had, so the handoff file always belongs to the sign-in in progress.4. Bound and sanitise a log field
New
src/lib/log-safe.ts: replace control characters so one value stays one line, and cap the length with a count of what was dropped so a record's size does not follow its input's. Applied at bothwifi/updatesinks (the second one's scrubbed nmcli text re-embeds the SSID) and at thepreferencesrejected-write line, whose reason string is built from the caller-supplied key.normalizedSsidis also capped at 32 octets — 802.11 defines the SSID element as at most 32 octets, so counting octets rather than characters makes the check mean what it says, and it is what actually bounds how much reaches the log.5. Null-prototype accumulators
preferences/route.tsandsanitizePreferences. The last of the three is the one that builds the object reaching the response, so fixing only the callers would have moved the question one hop rather than settled it.Testing
bun run test: 2261 passing. 29 failures are pre-existing on the dev machine (Windows:chmod/symlink/spawn-env semantics) — the failing set is byte-for-byte identical before and after this branch, verified by running the suite against a stash of the changes.tsc --noEmitclean apart from two pre-existing errors in untouched test files;eslintreports no errors on every changed file.path.relativeimplementation over a 66-path corpus: zero divergence, plus the backslash-name case it fixes.e2eande2e-installcover the routes on this PR.Scope
Deliberately left alone, noted for follow-up:
system/hotspot/route.tsenforces the same 32-SSID rule in characters rather than octets, with its own error string. Unifying them changes hotspot validation, so it is a separate change.persistTokensAndAckis still duplicated betweendevice-pollandexchange; the new module owns the path and lifetime but not the writer.searchTreeinfiles/route.tsruns the guard for entries whose result it then discards; reordering two lines would skip roughly 19.7k of 20k calls per search.preferencesGET does one full config read and parse per key where theall=1branch above it already shows the one-read form.Summary by CodeRabbit
Security
Bug Fixes