Repository navigation
Subrouter cloud API under cmux.com/api/v1/sr - #9119
lawrencecchen wants to merge 2 commits into
Conversation
Subrouter's accounts live on one Mac mini. When that host exhausted its kernel mbuf pool today, every client lost routing at once and recovery needed a KVM console. Moving custody off a single machine is the fix; this is the server half. Routes are oRPC procedures on the existing router, so they surface at /api/v1/sr/* through the OpenAPIHandler already mounted there and appear in the generated OpenAPI document with no separate spec to maintain. Both checked-in specs are regenerated accordingly. Credentials are team-scoped: Stack team membership is the authorization boundary, so uploading an account to a team is the same act as sharing it with teammates, and there is no second ACL to drift. Payloads are sealed with AES-256-GCM whose key lives in SR_VAULT_KEY rather than the database, so a dump alone yields no usable OAuth refresh chain, and a missing or wrong-length key fails loudly instead of silently storing plaintext. Login is a device-code flow rather than a localhost redirect, because the CLI routinely runs over SSH and in containers where nothing can reach 127.0.0.1. Only a digest of the CLI-held device code is stored, approval is consumed single-use, and an unknown code is reported identically to an expired one so polling cannot probe for valid codes. User codes omit 0/O and 1/I/L since a human reads them aloud. Push takes a batch so the mini's seven accounts migrate in one request, and re-uploading an existing account refreshes it in place rather than duplicating. The list endpoint returns identities only and never credential material.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
📝 WalkthroughWalkthroughAdds the Subrouter cloud API with encrypted team credential storage, device-code authentication, account upload/list procedures, database schema, OpenAPI definitions, route registration, and tests. ChangesSubrouter vault and authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant SR API
participant Device Code Store
participant Browser
CLI->>SR API: Start device login
SR API->>Device Code Store: Store hashed device code
SR API-->>CLI: Return user code and device code
Browser->>SR API: Approve user code
SR API->>Device Code Store: Save approval metadata
CLI->>SR API: Poll device code
SR API->>Device Code Store: Read approval state
SR API-->>CLI: Return login status
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (24 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@web/orpc/server/sr/accounts.ts`:
- Around line 58-96: The account persistence logic in the sr_accounts.push flow
should replace the SELECT-then-INSERT/UPDATE sequence with a single atomic
insert using onConflictDoUpdate() targeting the composite teamId, provider, and
accountLabel uniqueness constraint. Preserve the existing sealed credential
fields and updatedAt on conflicts, retain createdByUserId for new rows, and
update the uploaded/updated counters according to whether the operation inserts
or conflicts.
- Around line 18-26: Update the FORBIDDEN error message in requireTeam to remove
the “Stack” vendor name and use product-neutral wording while preserving the
existing guidance to create or select a team.
In `@web/orpc/server/sr/device.ts`:
- Around line 27-62: Add a per-IP or equivalent client-based rate/cap guard at
the start of srDeviceStartProcedure before the cleanup or insert database
writes, limiting unauthenticated device-code creation over the device-code TTL
window. Preserve successful issuance for clients under the cap, and only fail
open when the client identity cannot be determined or the guard is unavailable.
- Around line 80-110: Make the approved-code claim atomic in the handler by
replacing the separate approved-row read and delete with a delete-and-return
operation constrained to the matching device code hash, unexpired row, and
non-null approvedAt/teamId; import and use and and isNotNull as needed. Return
the approved response only when the atomic delete returns a row, while
preserving expired and pending responses for rows that cannot be claimed.
In `@web/orpc/server/sr/schemas.ts`:
- Around line 54-56: Update srDevicePollInputSchema to add an explicit maximum
length to deviceCode, using a generous bound above the approximately
43-character values generated by generateDeviceCode while preventing oversized
unauthenticated poll inputs from reaching hashing.
In `@web/services/subrouter/vaultCrypto.ts`:
- Around line 66-77: Update the minimum ciphertext length guard in open so
raw.length < 16 is rejected, allowing the valid 16-byte AES-GCM representation
of an empty plaintext while preserving tag and body extraction. Add a round-trip
test in the vault crypto tests covering seal/open with an empty-string payload.
🪄 Autofix (Beta)
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: d72441ad-561d-4c40-a4a9-510671b49b04
📒 Files selected for processing (10)
Packages/Shared/CmuxAPIClient/Sources/CmuxAPIClient/openapi.jsonweb/db/migrations/20260728160000_sr_vault/migration.sqlweb/db/schema.tsweb/openapi/openapi.jsonweb/orpc/server/router.tsweb/orpc/server/sr/accounts.tsweb/orpc/server/sr/device.tsweb/orpc/server/sr/schemas.tsweb/services/subrouter/vaultCrypto.tsweb/tests/sr-vault.test.ts
| async function requireTeam(user: AuthedUser): Promise<string> { | ||
| const team = await resolveBillingTeam(user); | ||
| if (!team) { | ||
| throw new ORPCError("FORBIDDEN", { | ||
| message: "No Stack team resolved for this user; create or select a team first", | ||
| }); | ||
| } | ||
| return team.id; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^|/)accounts\.ts$|web/services/billing/teamResolution|web/i18n/routing|messages' | sed -n '1,120p'
echo
echo "== server snippet =="
if [ -f web/orpc/server/sr/accounts.ts ]; then
cat -n web/orpc/server/sr/accounts.ts | sed -n '1,80p'
fi
echo
echo "== billing/teamResolution =="
if [ -f web/services/billing/teamResolution.ts ]; then
cat -n web/services/billing/teamResolution.ts | sed -n '1,220p'
fi
echo
echo "== occurrences of Stack/team wording in tracked files =="
rg -n "\bStack\b|listTeams|selectedTeam|No stack|No stack |select a team|create or select a team" . --glob '!node_modules' --glob '!dist' --glob '!build' | sed -n '1,220p'Repository: manaflow-ai/cmux
Length of output: 42063
🌐 Web query:
Stack Auth API listTeams selectedTeam documentation
💡 Result:
In Stack Auth, you can manage team membership and selection using the SDK's team-related functions and properties [1]. Retrieving Teams To list the teams a user belongs to, you can use the listTeams method [1]. Client-Side (React): Use the useTeams hook to list all teams a user is a member of [1]. const allTeams = user.useTeams; Server-Side: Use the listTeams function on the user object retrieved from the server app [1]. const allTeams = await user.listTeams; Selected Team The selectedTeam property on the user object represents the team currently active or selected by the user [2][3]. You can access it directly to determine the user's current context [2][3]. // Accessing the selected team const currentTeam = user.selectedTeam; To simplify team management for users, Stack Auth provides a pre-built component, SelectedTeamSwitcher, which allows users to view and switch between their available teams in the UI [4][2]. For further details on implementing team selection and switching, refer to the official Stack Auth documentation on team selection concepts [5] and components [4].
Citations:
- 1: https://docs.stack-auth.com/docs/apps/orgs-and-teams.mdx
- 2: https://zenstack.dev/blog/stackauth-multitenancy
- 3: https://dev.to/zenstack/building-multi-tenant-apps-using-stackauths-teams-and-nextjs-2dk
- 4: https://docs.stack-auth.com/docs/components/selected-team-switcher
- 5: https://docs.stack-auth.com/docs/concepts/team-selection
Remove the Stack vendor name from the user-facing error.
resolveBillingTeam is backed by Stack Auth’s selectedTeam / listTeams API, but the displayed FORBIDDEN message should describe the product concept instead of leaking Stack Auth terminology.
✏️ Proposed rewording
throw new ORPCError("FORBIDDEN", {
- message: "No Stack team resolved for this user; create or select a team first",
+ message: "No team resolved for this user; create or select a team first",
});📝 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.
| async function requireTeam(user: AuthedUser): Promise<string> { | |
| const team = await resolveBillingTeam(user); | |
| if (!team) { | |
| throw new ORPCError("FORBIDDEN", { | |
| message: "No Stack team resolved for this user; create or select a team first", | |
| }); | |
| } | |
| return team.id; | |
| } | |
| async function requireTeam(user: AuthedUser): Promise<string> { | |
| const team = await resolveBillingTeam(user); | |
| if (!team) { | |
| throw new ORPCError("FORBIDDEN", { | |
| message: "No team resolved for this user; create or select a team first", | |
| }); | |
| } | |
| return team.id; | |
| } |
🤖 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 `@web/orpc/server/sr/accounts.ts` around lines 18 - 26, Update the FORBIDDEN
error message in requireTeam to remove the “Stack” vendor name and use
product-neutral wording while preserving the existing guidance to create or
select a team.
Source: Coding guidelines
| for (const account of input.accounts) { | ||
| const sealed = seal(account.credential); | ||
| const existing = await db | ||
| .select({ id: srVaultEntries.id }) | ||
| .from(srVaultEntries) | ||
| .where( | ||
| and( | ||
| eq(srVaultEntries.teamId, teamId), | ||
| eq(srVaultEntries.provider, account.provider), | ||
| eq(srVaultEntries.accountLabel, account.accountLabel), | ||
| ), | ||
| ) | ||
| .limit(1); | ||
|
|
||
| if (existing.length > 0) { | ||
| await db | ||
| .update(srVaultEntries) | ||
| .set({ | ||
| ciphertext: sealed.ciphertext, | ||
| nonce: sealed.nonce, | ||
| keyVersion: sealed.keyVersion, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(srVaultEntries.id, existing[0]!.id)); | ||
| updated += 1; | ||
| continue; | ||
| } | ||
|
|
||
| await db.insert(srVaultEntries).values({ | ||
| teamId, | ||
| provider: account.provider, | ||
| accountLabel: account.accountLabel, | ||
| ciphertext: sealed.ciphertext, | ||
| nonce: sealed.nonce, | ||
| keyVersion: sealed.keyVersion, | ||
| createdByUserId: context.user.id ?? "", | ||
| }); | ||
| uploaded += 1; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)accounts\.ts$|web/db/schema\.ts|package\.json|package-lock\.json|pnpm-lock\.yaml|yarn\.lock'
echo "== accounts.ts context =="
if [ -f web/orpc/server/sr/accounts.ts ]; then
wc -l web/orpc/server/sr/accounts.ts
cat -n web/orpc/server/sr/accounts.ts
fi
echo "== sr_vault_entries references =="
rg -n "sr_vault_entries|srVaultEntries|team_provider_account_unique|unique" web/db web/orpc -S || true
echo "== schema context around vault entries =="
if [ -f web/db/schema.ts ]; then
rg -n "srVaultEntries|vault|credential|accountLabel" web/db/schema.ts -C 3
fi
echo "== drizzle version pins =="
for f in package.json web/package.json package-lock.json pnpm-lock.yaml yarn.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n '"drizzle-orm"|drizzle-orm@|/drizzle-orm/|drizzle-kit' "$f" | head -50 || true
fi
doneRepository: manaflow-ai/cmux
Length of output: 35744
🌐 Web query:
drizzle-orm 1.0 beta onConflictDoUpdate target array exclusions example
💡 Result:
In Drizzle ORM, the onConflictDoUpdate method supports using an array in the target property to handle composite primary keys or composite unique constraints [1][2][3]. When a conflict occurs on a composite constraint, you provide the columns as an array in the target field. You can then use the excluded keyword (often via sql.raw or sql template tags) in the set object to refer to the values proposed for insertion [1][4][3]. Example of an upsert with a composite target: import { sql } from 'drizzle-orm'; import { inventory } from './schema'; // Upserting with a composite target (warehouseId, productId) await db.insert(inventory).values({ warehouseId: 1, productId: 1, quantity: 100 }).onConflictDoUpdate({ target: [inventory.warehouseId, inventory.productId], set: { // Using 'excluded' to reference the proposed insertion value quantity: sqlexcluded.quantity + 100 }, }); If you need to dynamically reference column names in the excluded set to avoid hardcoding strings, use sql.raw combined with the column's .name property [1][5]: await db.insert(inventory).values(data).onConflictDoUpdate({ target: [inventory.warehouseId, inventory.productId], set: { quantity: sql.raw(excluded.${inventory.quantity.name}) }, }); Note that the target array must match a defined primary key or unique constraint in your table schema for the conflict detection to work correctly [6][7][8]. If your schema does not have a composite unique constraint matching the columns in your target array, the database will return an error stating that no unique or exclusion constraint matches the specification [7].
Citations:
- 1: https://orm.drizzle.team/docs/guides/upsert
- 2: https://orm.drizzle.team/docs/singlestore/guides/upsert
- 3: https://drizzle-team-drizzle-orm.mintlify.app/queries/insert
- 4: https://github.com/drizzle-team/drizzle-orm-docs/blob/main/src/content/docs/guides/upsert.mdx
- 5: https://www.answeroverflow.com/m/1240423207765413908
- 6: [BUG]: SQLite onConflictDoUpdate does not work as expected on composite primary key or unique constraint drizzle-team/drizzle-orm#2998
- 7: https://stackoverflow.com/questions/78233343/drizzleorm-use-a-unique-index-in-conflict-updates
- 8: https://www.answeroverflow.com/m/1339650133637468271
Use an atomic upsert for sr_accounts.push.
The SELECT-then-INSERT/UPDATE path races with concurrent pushes on the same (teamId, provider, accountLabel) and can violate sr_vault_entries_team_provider_account_unique; it also costs 2 round-trips per account. Use .onConflictDoUpdate() on the composite unique target so inserts and updates happen in one pass.
🤖 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 `@web/orpc/server/sr/accounts.ts` around lines 58 - 96, The account persistence
logic in the sr_accounts.push flow should replace the SELECT-then-INSERT/UPDATE
sequence with a single atomic insert using onConflictDoUpdate() targeting the
composite teamId, provider, and accountLabel uniqueness constraint. Preserve the
existing sealed credential fields and updatedAt on conflicts, retain
createdByUserId for new rows, and update the uploaded/updated counters according
to whether the operation inserts or conflicts.
| export const srDeviceStartProcedure = os | ||
| .route({ | ||
| method: "POST", | ||
| path: "/sr/device/start", | ||
| operationId: "sr.device.start", | ||
| summary: "Begin a device-code login", | ||
| description: | ||
| "Issues a device code for the CLI and a short user code for the human to approve in a browser. Grants no access until approved.", | ||
| tags: ["Subrouter"], | ||
| successStatus: 200, | ||
| }) | ||
| .output(srDeviceStartOutputSchema) | ||
| .handler(async () => { | ||
| const db = cloudDb(); | ||
| // Sweep expired rows opportunistically so this table cannot grow unbounded | ||
| // without a separate cron. | ||
| await db.delete(srDeviceCodes).where(lt(srDeviceCodes.expiresAt, new Date())); | ||
|
|
||
| const deviceCode = generateDeviceCode(); | ||
| const userCode = generateUserCode(); | ||
| const expiresAt = new Date(Date.now() + DEVICE_CODE_TTL_SECONDS * 1000); | ||
|
|
||
| await db.insert(srDeviceCodes).values({ | ||
| userCode, | ||
| deviceCodeHash: hashDeviceCode(deviceCode), | ||
| expiresAt, | ||
| }); | ||
|
|
||
| return { | ||
| userCode, | ||
| deviceCode, | ||
| verificationUri: "https://cmux.com/sr/device", | ||
| expiresInSeconds: DEVICE_CODE_TTL_SECONDS, | ||
| intervalSeconds: POLL_INTERVAL_SECONDS, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for any rate-limiting middleware applied to /sr/device/start or the SR router generally.
rg -n -i 'rate.?limit' web --type=ts -g '!**/node_modules/**'
rg -n 'sr/device/start' web --type=ts -g '!**/node_modules/**'Repository: manaflow-ai/cmux
Length of output: 33454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'device\.ts|route\.ts|.*middleware.*|.*auth.*|env\.ts' web/orpc web/orpc/server web/services web/app 2>/dev/null | sed 's#^\./##' | head -200
echo
echo "== sr device outline =="
ast-grep outline web/orpc/server/sr/device.ts --view compact 2>/dev/null || true
echo
echo "== sr device relevant lines =="
cat -n web/orpc/server/sr/device.ts
echo
echo "== orpc server files =="
fd -a '\.(ts|tsx)$' web/orpc | sed 's#^\./##' | head -200
echo
echo "== route imports/usages for sr device =="
rg -n 'sr/device|srDevice|device.start|/sr' web --type=ts -g '!**/node_modules/**' | head -300Repository: manaflow-ai/cmux
Length of output: 14018
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== base route options =="
cat -n web/orpc/server/base.ts
echo
echo "== router definition =="
cat -n web/orpc/server/router.ts
echo
echo "== RPC route handling =="
ast-grep outline web/app/api/rpc/[[...rest]]/route.ts 2>/dev/null || true
cat -n web/app/api/rpc/[[...rest]]/route.ts
echo
echo "== openapi route handling =="
cat -n web/app/api/openapi.json/route.ts
echo
echo "== subrouter route handling =="
cat -n web/app/api/subrouter/accounts/route.ts
cat -n web/app/api/subrouter/accounts/[accountId]/route.tsRepository: manaflow-ai/cmux
Length of output: 18266
Bound unauthenticated device-code creation.
/api/rpc creates a fresh ORPCContext for each request, but /sr/device/start intentionally keeps context.user null so it can be called before authentication. With a 900s TTL and only opportunistic deletion on the next start call, sustained public traffic can keep inserting rows into sr_device_codes. Add a per-IP/client global cap or similar guard before the insert; keep the cap before any database write/failure-open only if unavoidable.
🤖 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 `@web/orpc/server/sr/device.ts` around lines 27 - 62, Add a per-IP or
equivalent client-based rate/cap guard at the start of srDeviceStartProcedure
before the cleanup or insert database writes, limiting unauthenticated
device-code creation over the device-code TTL window. Preserve successful
issuance for clients under the cap, and only fail open when the client identity
cannot be determined or the guard is unavailable.
| .handler(async ({ input }) => { | ||
| const db = cloudDb(); | ||
| const rows = await db | ||
| .select() | ||
| .from(srDeviceCodes) | ||
| .where(eq(srDeviceCodes.deviceCodeHash, hashDeviceCode(input.deviceCode))) | ||
| .limit(1); | ||
|
|
||
| const row = rows[0]; | ||
| if (!row) { | ||
| // An unknown code is indistinguishable from an expired one on purpose: | ||
| // neither confirms whether a code ever existed. | ||
| return { status: "expired" as const, teamId: null, userId: null }; | ||
| } | ||
| if (row.expiresAt.getTime() <= Date.now()) { | ||
| await db.delete(srDeviceCodes).where(eq(srDeviceCodes.id, row.id)); | ||
| return { status: "expired" as const, teamId: null, userId: null }; | ||
| } | ||
| if (!row.approvedAt || !row.teamId) { | ||
| return { status: "pending" as const, teamId: null, userId: null }; | ||
| } | ||
|
|
||
| // Single-use: consuming the row means a captured device code cannot be | ||
| // redeemed twice. | ||
| await db.delete(srDeviceCodes).where(eq(srDeviceCodes.id, row.id)); | ||
| return { | ||
| status: "approved" as const, | ||
| teamId: row.teamId, | ||
| userId: row.userId, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Race between reading approvedAt and deleting the row can defeat single-use.
The select at lines 82-86 and the delete at line 104 are separate round-trips. Two concurrent polls (e.g. a captured device code raced against the legitimate CLI, or a client retry overlapping an in-flight request) can both observe approvedAt/teamId set and both take the "approved" branch before either delete lands — both callers get back the same teamId/userId, contradicting the comment's stated single-use guarantee.
🔒 Proposed fix: make the claim atomic via delete-and-return
- if (!row.approvedAt || !row.teamId) {
- return { status: "pending" as const, teamId: null, userId: null };
- }
-
- // Single-use: consuming the row means a captured device code cannot be
- // redeemed twice.
- await db.delete(srDeviceCodes).where(eq(srDeviceCodes.id, row.id));
- return {
- status: "approved" as const,
- teamId: row.teamId,
- userId: row.userId,
- };
+ if (!row.approvedAt || !row.teamId) {
+ return { status: "pending" as const, teamId: null, userId: null };
+ }
+
+ // Atomic delete-if-approved: at most one concurrent poll can claim the
+ // row, so a captured device code cannot be redeemed twice even under a race.
+ const [claimed] = await db
+ .delete(srDeviceCodes)
+ .where(and(eq(srDeviceCodes.id, row.id), isNotNull(srDeviceCodes.approvedAt)))
+ .returning();
+ if (!claimed || !claimed.teamId) {
+ return { status: "pending" as const, teamId: null, userId: null };
+ }
+ return {
+ status: "approved" as const,
+ teamId: claimed.teamId,
+ userId: claimed.userId,
+ };Requires importing and, isNotNull from drizzle-orm.
📝 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.
| .handler(async ({ input }) => { | |
| const db = cloudDb(); | |
| const rows = await db | |
| .select() | |
| .from(srDeviceCodes) | |
| .where(eq(srDeviceCodes.deviceCodeHash, hashDeviceCode(input.deviceCode))) | |
| .limit(1); | |
| const row = rows[0]; | |
| if (!row) { | |
| // An unknown code is indistinguishable from an expired one on purpose: | |
| // neither confirms whether a code ever existed. | |
| return { status: "expired" as const, teamId: null, userId: null }; | |
| } | |
| if (row.expiresAt.getTime() <= Date.now()) { | |
| await db.delete(srDeviceCodes).where(eq(srDeviceCodes.id, row.id)); | |
| return { status: "expired" as const, teamId: null, userId: null }; | |
| } | |
| if (!row.approvedAt || !row.teamId) { | |
| return { status: "pending" as const, teamId: null, userId: null }; | |
| } | |
| // Single-use: consuming the row means a captured device code cannot be | |
| // redeemed twice. | |
| await db.delete(srDeviceCodes).where(eq(srDeviceCodes.id, row.id)); | |
| return { | |
| status: "approved" as const, | |
| teamId: row.teamId, | |
| userId: row.userId, | |
| }; | |
| }); | |
| .handler(async ({ input }) => { | |
| const db = cloudDb(); | |
| const rows = await db | |
| .select() | |
| .from(srDeviceCodes) | |
| .where(eq(srDeviceCodes.deviceCodeHash, hashDeviceCode(input.deviceCode))) | |
| .limit(1); | |
| const row = rows[0]; | |
| if (!row) { | |
| // An unknown code is indistinguishable from an expired one on purpose: | |
| // neither confirms whether a code ever existed. | |
| return { status: "expired" as const, teamId: null, userId: null }; | |
| } | |
| if (row.expiresAt.getTime() <= Date.now()) { | |
| await db.delete(srDeviceCodes).where(eq(srDeviceCodes.id, row.id)); | |
| return { status: "expired" as const, teamId: null, userId: null }; | |
| } | |
| if (!row.approvedAt || !row.teamId) { | |
| return { status: "pending" as const, teamId: null, userId: null }; | |
| } | |
| // Atomic delete-if-approved: at most one concurrent poll can claim the | |
| // row, so a captured device code cannot be redeemed twice even under a race. | |
| const [claimed] = await db | |
| .delete(srDeviceCodes) | |
| .where(and(eq(srDeviceCodes.id, row.id), isNotNull(srDeviceCodes.approvedAt))) | |
| .returning(); | |
| if (!claimed || !claimed.teamId) { | |
| return { status: "pending" as const, teamId: null, userId: null }; | |
| } | |
| return { | |
| status: "approved" as const, | |
| teamId: claimed.teamId, | |
| userId: claimed.userId, | |
| }; | |
| }); |
🤖 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 `@web/orpc/server/sr/device.ts` around lines 80 - 110, Make the approved-code
claim atomic in the handler by replacing the separate approved-row read and
delete with a delete-and-return operation constrained to the matching device
code hash, unexpired row, and non-null approvedAt/teamId; import and use and and
isNotNull as needed. Return the approved response only when the atomic delete
returns a row, while preserving expired and pending responses for rows that
cannot be claimed.
| export const srDevicePollInputSchema = z.object({ | ||
| deviceCode: z.string().min(1), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound deviceCode input length.
Every other client-supplied string in this file has an explicit max() (accountLabel ≤ 320, credential ≤ 64,000), but deviceCode on this unauthenticated poll endpoint has none. Generated device codes are ~43 chars (generateDeviceCode in vaultCrypto.ts), so a generous cap still leaves headroom while preventing oversized request bodies from reaching the hashing step on every poll.
🛡️ Proposed fix
export const srDevicePollInputSchema = z.object({
- deviceCode: z.string().min(1),
+ deviceCode: z.string().min(1).max(256),
});📝 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.
| export const srDevicePollInputSchema = z.object({ | |
| deviceCode: z.string().min(1), | |
| }); | |
| export const srDevicePollInputSchema = z.object({ | |
| deviceCode: z.string().min(1).max(256), | |
| }); |
🤖 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 `@web/orpc/server/sr/schemas.ts` around lines 54 - 56, Update
srDevicePollInputSchema to add an explicit maximum length to deviceCode, using a
generous bound above the approximately 43-character values generated by
generateDeviceCode while preventing oversized unauthenticated poll inputs from
reaching hashing.
| export function open(sealed: SealedSecret): string { | ||
| const key = resolveKey(); | ||
| const raw = Buffer.from(sealed.ciphertext, "base64"); | ||
| if (raw.length < 17) { | ||
| throw new Error("ciphertext too short to contain an auth tag"); | ||
| } | ||
| const tag = raw.subarray(raw.length - 16); | ||
| const body = raw.subarray(0, raw.length - 16); | ||
| const decipher = createDecipheriv("aes-256-gcm", key, Buffer.from(sealed.nonce, "base64")); | ||
| decipher.setAuthTag(tag); | ||
| return Buffer.concat([decipher.update(body), decipher.final()]).toString("utf8"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Off-by-one in the auth-tag length guard.
AES-GCM output length equals plaintext length, so a sealed empty-string payload produces exactly 16 raw bytes (0-byte body + 16-byte tag). The check raw.length < 17 incorrectly rejects that valid minimum-length case; it should be < 16. Not reachable today since the only caller enforces credential.min(1), but this is a shared crypto primitive and the boundary is wrong on its own terms.
🐛 Proposed fix
- if (raw.length < 17) {
+ if (raw.length < 16) {
throw new Error("ciphertext too short to contain an auth tag");
}Consider adding a test in web/tests/sr-vault.test.ts that round-trips an empty-string payload through seal/open to lock this boundary in.
📝 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.
| export function open(sealed: SealedSecret): string { | |
| const key = resolveKey(); | |
| const raw = Buffer.from(sealed.ciphertext, "base64"); | |
| if (raw.length < 17) { | |
| throw new Error("ciphertext too short to contain an auth tag"); | |
| } | |
| const tag = raw.subarray(raw.length - 16); | |
| const body = raw.subarray(0, raw.length - 16); | |
| const decipher = createDecipheriv("aes-256-gcm", key, Buffer.from(sealed.nonce, "base64")); | |
| decipher.setAuthTag(tag); | |
| return Buffer.concat([decipher.update(body), decipher.final()]).toString("utf8"); | |
| } | |
| export function open(sealed: SealedSecret): string { | |
| const key = resolveKey(); | |
| const raw = Buffer.from(sealed.ciphertext, "base64"); | |
| if (raw.length < 16) { | |
| throw new Error("ciphertext too short to contain an auth tag"); | |
| } | |
| const tag = raw.subarray(raw.length - 16); | |
| const body = raw.subarray(0, raw.length - 16); | |
| const decipher = createDecipheriv("aes-256-gcm", key, Buffer.from(sealed.nonce, "base64")); | |
| decipher.setAuthTag(tag); | |
| return Buffer.concat([decipher.update(body), decipher.final()]).toString("utf8"); | |
| } |
🤖 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 `@web/services/subrouter/vaultCrypto.ts` around lines 66 - 77, Update the
minimum ciphertext length guard in open so raw.length < 16 is rejected, allowing
the valid 16-byte AES-GCM representation of an empty plaintext while preserving
tag and body extraction. Add a round-trip test in the vault crypto tests
covering seal/open with an empty-string payload.
The hosted Subrouter account and Stack authentication flows landed in #9261. CodeRouter now stores credentials with KMS encryption (#9686) and enforces private/team account permissions and VM pools (#12771). Keep those shipped implementations instead of adding an unused SR_VAULT_KEY credential store and incomplete device-code flow. The resulting tree is identical to main at a149b7e. Current-path focused tests: 63 passed; web complexity gate passed.
|
All contributors have signed the CLA ✍️ ✅ |
|
cmux-reconcile: close-candidate Proposed action: Close this PR without merging; preserve the branch. Evidence checked September 18, 2026: GitHub currently reports 0 changed files, 0 additions, and 0 deletions, and the fetched PR diff is empty. The branch has no remaining patch to land. This does not claim the original product idea is unnecessary. This is a reconciliation recommendation for a maintainer with write access, not an automatic closure or runtime test result. Recheck for new commits or reports before acting. Search |
|
Fleet instruction update for head |
|
Closing as already on main: merging this branch into main at 8421357 produces main's own tree, so there's nothing left to land. The branch is kept; reopen if something here is still missing. Part of the backlog cleanup in manaflow-ai/cmuxterm-hq#563. |
2 participants
Why
Subrouter's accounts live on one Mac mini. Today that host exhausted its kernel mbuf pool (
5462/546216KB clusters,14134192requests for memory denied), stopped accepting TCP entirely, and took every client's routing down with it; recovery needed a KVM console. Getting credential custody off a single machine is the durable fix. This is the server half.Shape
Four oRPC procedures on the existing router, so they surface at
/api/v1/sr/*via theOpenAPIHandleralready mounted there:POST /sr/device/startPOST /sr/device/pollPOST /sr/accounts/pushGET /sr/accountsNo hand-maintained spec: Zod schemas generate the OpenAPI document, and both checked-in specs are regenerated so
account-me-orpc.test.ts's byte-identical assertion passes.Security decisions
Team-scoped custody. Stack team membership is the authorization boundary, so uploading to a team is the same act as sharing with teammates. No second ACL to drift out of sync.
Sealed at rest. AES-256-GCM with the key in
SR_VAULT_KEY, never the database, so key and ciphertext don't share a compromise boundary. A missing or wrong-length key throws rather than silently storing plaintext. Tampering fails on open via the GCM auth tag.Device-code over localhost redirect, because the CLI routinely runs over SSH and in containers where nothing can reach
127.0.0.1. Only a digest of the device code is stored, approval is single-use, and an unknown code returns the same response as an expired one so polling can't probe for valid codes. User codes omit0/Oand1/I/Lsince a human reads them aloud.List never returns credential material — asserted by a test that greps the generated response schema.
Verified
Not just typechecked. Against a live dev server on a real Postgres:
/api/openapi.jsonserves all four routesGET /api/v1/sr/accountsunauthenticated →401 UNAUTHORIZEDPOST /sr/device/start→{"userCode":"6KAF-2V7J","deviceCode":"7LbV_...","expiresInSeconds":900}{"status":"pending"}; poll with a bogus code →{"status":"expired"}bun run typecheckclean.bun test17 pass / 0 fail across the new and existing oRPC suites.Not in this PR
The Go client in
manaflow-ai/subrouter(hand-written onnet/http, no OpenAPI codegen, so no binary bloat), thecmux.com/sr/devicebrowser approval page that callsapproveDeviceCode, and thesr login/sr pushverbs. The approval helper is exported and ready for that page.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Retires the superseded Subrouter vault proposal. The hosted Subrouter and Stack authentication flows have shipped, CodeRouter now stores credentials with KMS encryption, and private/team account permissions and VM pools are enforced, so this PR keeps those implementations instead of adding an unused
SR_VAULT_KEYcredential store and incomplete device-code flow. The resulting tree is identical to main, so no runtime behavior changes ship.Written for commit 9268482. Summary will update on new commits.
Summary by CodeRabbit
New Features
Security
Tests