Make Cap fully operable by agents#2020
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (token.length !== 36) return yield* authRequired(); | ||
|
|
||
| const [row] = yield* database.use((db) => | ||
| db | ||
| .select({ | ||
| id: Db.users.id, | ||
| email: Db.users.email, | ||
| activeOrganizationId: Db.users.activeOrganizationId, | ||
| source: Db.authApiKeys.source, | ||
| }) | ||
| .from(Db.authApiKeys) | ||
| .innerJoin(Db.users, eq(Db.authApiKeys.userId, Db.users.id)) | ||
| .where(eq(Db.authApiKeys.id, token)) | ||
| .limit(1), | ||
| ); | ||
| if (!row) return yield* authRequired(); | ||
| if (!isLegacyAgentKeySource(row.source)) return yield* authRequired(); | ||
| if (!isAgentReadAccessEnabled(row.email)) { |
There was a problem hiding this comment.
The legacy path (token.length === 36) queries authApiKeys using eq(Db.authApiKeys.id, token) — meaning the token is the primary key and is stored in plaintext in the database. New cap_cli_* tokens are properly hashed with SHA-256 before storage. If the authApiKeys table is ever exfiltrated, all legacy session tokens are immediately usable by the attacker. This is a backward-compatibility constraint, but it is worth considering a migration path to hash these values and retire the plaintext storage.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/web-backend/src/AgentAuth.ts
Line: 150-167
Comment:
**Legacy API keys are looked up by plaintext token value (as DB primary key)**
The legacy path (`token.length === 36`) queries `authApiKeys` using `eq(Db.authApiKeys.id, token)` — meaning the token _is_ the primary key and is stored in plaintext in the database. New `cap_cli_*` tokens are properly hashed with SHA-256 before storage. If the `authApiKeys` table is ever exfiltrated, all legacy session tokens are immediately usable by the attacker. This is a backward-compatibility constraint, but it is worth considering a migration path to hash these values and retire the plaintext storage.
How can I resolve this? If you propose a fix, please make it concise.
|
hey @greptileai, please re-review the PR |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| @@ -0,0 +1,227 @@ | |||
| import { createHash } from "node:crypto"; | |||
There was a problem hiding this comment.
Minor: avoids relying on a global crypto (runtime/TS lib differences) and keeps UUID generation consistent with the other node:crypto usage.
| import { createHash } from "node:crypto"; | |
| import { createHash, randomUUID } from "node:crypto"; |
| import { Effect, Layer, Schema } from "effect"; | ||
| import { Database } from "./Database.ts"; | ||
|
|
||
| const requestId = () => crypto.randomUUID(); |
There was a problem hiding this comment.
(Follow-up from the import change.)
| const requestId = () => crypto.randomUUID(); | |
| const requestId = () => randomUUID(); |
| }); | ||
| } | ||
|
|
||
| if (token.length !== 36) return yield* authRequired(); |
There was a problem hiding this comment.
This enables legacy UUID-style tokens for the agent API. If you want an easy way to phase these out (or quickly disable them if a legacy key leaks) without a DB migration, a dedicated env kill-switch helps.
| if (token.length !== 36) return yield* authRequired(); | |
| if (token.length !== 36) return yield* authRequired(); | |
| if (process.env.CAP_AGENT_API_DISABLE_LEGACY_KEYS === "true") { | |
| return yield* authRequired(); | |
| } |
|
hey @greptileai, please re-review the PR |
| database | ||
| .delete(agentApiKeys) | ||
| .where( | ||
| or( |
There was a problem hiding this comment.
Nice catch adding revokedAt cleanup. One perf thought: OR (expiresAt < cutoff OR revokedAt < cutoff) can prevent using the existing expires_at_idx, and there’s no index on revokedAt currently. Might be worth adding an index on revokedAt (or splitting this into two deletes) so the cron doesn’t degrade as agent_api_keys grows.
|
hey @greptileai, please re-review the PR |
| import { readFileSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| describe("self-hosted proxy routes", () => { | ||
| it("allows browser-based CLI authorization pages", () => { | ||
| const source = readFileSync(join(process.cwd(), "proxy.ts"), "utf8"); | ||
| expect(source).toContain('path.startsWith("/cli/")'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
process.cwd() can vary depending on how vitest is invoked, so this can be a bit flaky. Using import.meta.url keeps the path stable.
| import { readFileSync } from "node:fs"; | |
| import { join } from "node:path"; | |
| import { describe, expect, it } from "vitest"; | |
| describe("self-hosted proxy routes", () => { | |
| it("allows browser-based CLI authorization pages", () => { | |
| const source = readFileSync(join(process.cwd(), "proxy.ts"), "utf8"); | |
| expect(source).toContain('path.startsWith("/cli/")'); | |
| }); | |
| }); | |
| import { readFileSync } from "node:fs"; | |
| import { describe, expect, it } from "vitest"; | |
| describe("self-hosted proxy routes", () => { | |
| it("allows browser-based CLI authorization pages", () => { | |
| const source = readFileSync(new URL("../../proxy.ts", import.meta.url), "utf8"); | |
| expect(source).toContain('path.startsWith("/cli/")'); | |
| }); | |
| }); |
|
I found one security-sensitive gap in the new agent confirmation model. The API advertises many write/destructive capabilities as requiring If confirmation is intended as a security boundary, I think this needs a server-verifiable proof of confirmation instead of a spoofable header: e.g. a short-lived confirmation challenge minted by the dashboard/native client and bound to |
|
hey @greptileai, please re-review the PR |
|
|
||
| it("ignores tombstones during organization creation", () => { | ||
| const source = readFileSync( | ||
| join(process.cwd(), "app/api/v1/[...route]/route.ts"), |
There was a problem hiding this comment.
process.cwd() can vary depending on how vitest is invoked, so these tests can get a bit flaky. Using import.meta.url keeps the path stable.
| join(process.cwd(), "app/api/v1/[...route]/route.ts"), | |
| const source = readFileSync( | |
| new URL("../../app/api/v1/[...route]/route.ts", import.meta.url), | |
| "utf8", | |
| ); |
| try { | ||
| const url = new URL(value); | ||
| return ( | ||
| (url.protocol === "http:" || url.protocol === "https:") && |
There was a problem hiding this comment.
Nice to have a centralized URL guard. One extra hardening that tends to pay off: reject embedded credentials (https://user:pass@...) so we don’t end up storing/leaking them (logs, dashboards, etc.).
| (url.protocol === "http:" || url.protocol === "https:") && | |
| (url.protocol === "http:" || url.protocol === "https:") && | |
| url.hostname.length > 0 && | |
| url.username.length === 0 && | |
| url.password.length === 0 |
| return ((hours * 60 + minutes) * 60 + seconds) * 1000 + milliseconds; | ||
| }; | ||
|
|
||
| const stripVttCueTags = (value: string) => { |
There was a problem hiding this comment.
Small perf win: most cue text probably has no tags. An early return avoids per-character allocations in that case.
| const stripVttCueTags = (value: string) => { | |
| const stripVttCueTags = (value: string) => { | |
| if (!value.includes("<")) return value; | |
| const text: string[] = []; | |
| let insideTag = false; | |
| for (const character of value) { | |
| if (character === "<") { | |
| insideTag = true; | |
| continue; | |
| } | |
| if (insideTag) { | |
| if (character === ">") insideTag = false; | |
| continue; | |
| } | |
| text.push(character); | |
| } | |
| return text.join(""); | |
| }; |
3 participants
Adds an additive Effect /api/v1, comprehensive Cap CLI and local stdio MCP surface, secure browser/PKCE authentication with OS-backed credentials, Agent Skill installers, server-enforced idempotency, lifecycle cleanup, and dashboard-equivalent account, library, organization, and developer operations. Includes release-binary Docker E2E coverage across MySQL, MinIO, media processing, permissions, authentication, CLI/MCP parity, security, and regression gates.
Greptile Summary
This PR adds a comprehensive agent API surface to Cap: a new
/api/v1Effect-based HTTP handler, PKCE browser authentication, OS-backed CLI credential storage, an MCP stdio interface, server-enforced idempotency via MySQLFOR UPDATEtransactions, and a full suite of account/library/organization/developer operations.lastUsedAtis now updated in the auth middleware (with a 5-minute throttle), the Rust callback listener returns a 405 response for non-GET requests, and LIKE search terms are now escaped withescapeAgentLikePatternandESCAPE '!'.timingSafeEqualcan diverge from byte length for multi-byte UTF-8 characters (causing a throw instead of a 401), and theagentApiOperationscleanup only deletes terminal states, leaving any workflow-crashed records in\"running\"state indefinitely.Confidence Score: 5/5
Safe to merge — the three previously-flagged defects are all addressed, auth and idempotency logic are sound, and the two remaining observations are non-blocking edge cases.
The core security paths (PKCE flow, token hashing, scope enforcement, idempotency correctness) are well-implemented. The cron timingSafeEqual edge case can only produce a 500 for malformed Authorization headers and does not allow authentication bypass. The stuck-operation cleanup gap requires a double failure scenario before it manifests operationally.
apps/web/lib/agent-api-cleanup.ts and apps/web/app/api/cron/cleanup-agent-api/route.ts contain the two minor issues noted in the comments.
Important Files Changed
Reviews (5): Last reviewed commit: "fix: validate developer app logo URLs" | Re-trigger Greptile