fix: unbreak cold install and harden /api/config#14
Open
us wants to merge 1 commit intofirecrawl:mainfrom
Open
fix: unbreak cold install and harden /api/config#14us wants to merge 1 commit intofirecrawl:mainfrom
us wants to merge 1 commit intofirecrawl:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes fresh-clone install/build issues across agent-core and templates, restores CI, and adds authentication + input hardening around the Next template’s /api/config endpoint that manages API keys.
Changes:
- Unblocks cold installs by adding missing Node type deps, aligning Next template ESLint to v9 +
eslint-config-nextv16, and regenerating stale template lockfiles. - Hardens Next
/api/configread/write withCONFIG_ADMIN_TOKENauth plus dotenv-injection guards; updates settings UI to manage the admin token. - Restores CI workflow and improves sync script behavior (exclude synced lockfile).
Reviewed changes
Copilot reviewed 14 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| agent-templates/next/pnpm-lock.yaml | Updates Next template lockfile for dependency/version alignment (ESLint 9, Next config, etc.). |
| agent-templates/next/package.json | Adds typecheck script and aligns ESLint/Next lint deps for Next 16 + ESLint 9. |
| agent-templates/next/eslint.config.mjs | Introduces ESLint 9 flat config using FlatCompat for Next presets. |
| agent-templates/next/app/(agent)/api/config/route.ts | Adds hosted auth gating + env-value validation for config read/write. |
| agent-templates/next/app/(agent)/_lib/config/keys.ts | Implements constant-time auth compare, hosted read/write guards, and env-value safety helpers. |
| agent-templates/next/app/(agent)/_components/settings-panel.tsx | Adds admin-token UX and attaches bearer token to config requests. |
| agent-templates/next/agent-core/package.json | Adds @types/node to unblock template vendored agent-core type builds. |
| agent-templates/library/agent-core/package.json | Adds @types/node to unblock template vendored agent-core type builds. |
| agent-templates/express/server.ts | Makes CORS opt-in and reports corsOrigin: null when unset. |
| agent-templates/express/pnpm-lock.yaml | Regenerates lockfile to remove stale papaparse specifiers. |
| agent-templates/express/agent-core/package.json | Adds @types/node to unblock template vendored agent-core type builds. |
| agent-templates/express/.env.example | Documents new opt-in CORS default behavior. |
| agent-core/pnpm-lock.yaml | Adds Node types to lockfile to support clean installs/builds. |
| agent-core/package.json | Adds missing @types/node dev dependency to fix cold type builds. |
| .internal/scripts/sync-agent-core.mjs | Excludes pnpm-lock.yaml from vendored sync to reduce diff noise. |
| .github/workflows/ci.yml | Restores CI (agent-core matrix + template cold-install matrix + sync-check). |
| .env.example | Adds root-level env docs pointer to per-template env examples + hosted token note. |
Files not reviewed (3)
- agent-core/pnpm-lock.yaml: Language not supported
- agent-templates/express/pnpm-lock.yaml: Language not supported
- agent-templates/next/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
agent-templates/next/app/(agent)/_components/settings-panel.tsx:511
- Config writes (
saveKey/removeKey/saveValue/removeValue) don’t handle 401/403 the wayfetchKeysdoes (clearing a stale token and switching the UI into the auth-required state). As-is, a rejected token remains in localStorage and the UI just shows the raw error, leaving users stuck until they manually clear it.
const res = await fetch("/api/config", { method: "POST", headers: configHeaders(), body: JSON.stringify({ keys: { [id]: value } }) });
const data = await res.json();
if (res.ok) { setKeyStatuses(data.keys); setKeyDrafts((prev) => ({ ...prev, [id]: "" })); setSaveMsg(hosted ? "Saved for this session" : "Saved to .env.local"); setTimeout(() => setSaveMsg(""), 3000); }
else setSaveMsg(data.error || "Failed to save");
} catch { setSaveMsg("Network error"); }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
136
to
170
| @@ -40,7 +156,7 @@ function readEnvFile(): Record<string, string> { | |||
| const trimmed = line.trim(); | |||
| if (!trimmed || trimmed.startsWith("#")) continue; | |||
| const eq = trimmed.indexOf("="); | |||
| if (eq > 0) vars[trimmed.slice(0, eq)] = trimmed.slice(eq + 1); | |||
| if (eq > 0) vars[trimmed.slice(0, eq)] = stripQuotes(trimmed.slice(eq + 1)); | |||
| } | |||
| return vars; | |||
| } catch { | |||
| @@ -53,9 +169,13 @@ function writeEnvFile(vars: Record<string, string>) { | |||
| fs.writeFileSync(ENV_FILE, lines.join("\n") + "\n", "utf-8"); | |||
| } | |||
There was a problem hiding this comment.
stripQuotes() removes balanced surrounding quotes from values read out of .env.local, but writeEnvFile() always writes values back unquoted. This will corrupt any pre-existing quoted env var containing spaces or other characters that require quoting (e.g. FOO="a b" becomes FOO=a b after any config update), potentially breaking unrelated environment variables.
Comment on lines
+488
to
+490
| // Skills endpoint isn't auth-guarded today, but sending the header | ||
| // costs nothing and keeps us consistent if it ever becomes guarded. | ||
| const res = await fetch("/api/skills", { headers: configHeaders(false) }); |
There was a problem hiding this comment.
The admin token is attached to /api/skills requests as well. This increases the token’s exposure surface (e.g., accidental server/proxy logging or future route changes) without being required for the endpoint. Prefer only attaching the Authorization header to /api/config calls (and add it to /api/skills only if/when that endpoint is actually guarded).
Suggested change
| // Skills endpoint isn't auth-guarded today, but sending the header | |
| // costs nothing and keeps us consistent if it ever becomes guarded. | |
| const res = await fetch("/api/skills", { headers: configHeaders(false) }); | |
| // Skills endpoint is not auth-guarded, so avoid sending the admin token. | |
| const res = await fetch("/api/skills"); |
Comment on lines
+39
to
+44
| // Reject payloads where any value would inject newlines into .env.local. | ||
| // Empty string means "delete this key" and is legal. | ||
| for (const value of Object.values(keys ?? {})) { | ||
| if (value !== "" && !isSafeEnvValue(value)) { | ||
| return Response.json({ error: "Invalid characters in key value" }, { status: 400 }); | ||
| } |
There was a problem hiding this comment.
The comment says this validation only rejects values that would inject newlines into .env.local, but isSafeEnvValue() also rejects quotes, backslashes, #, $, and all whitespace. Update the comment to match the actual guard (or narrow the guard if the intent is truly newline-only).
Comment on lines
30
to
37
| @@ -17,6 +36,19 @@ export async function POST(req: Request) { | |||
| return Response.json({ error: "Invalid request body" }, { status: 400 }); | |||
| } | |||
There was a problem hiding this comment.
await req.json() will throw on invalid JSON, which will currently bubble up as a 500. Since this is a public API route, consider wrapping the JSON parse in a try/catch and returning a 400 with a clear error when the body isn’t valid JSON.
Cold install (pnpm install --frozen-lockfile) was failing across
agent-core and the three templates; agent-core dts build was also
broken on a fresh clone. Knock down each blocker and harden the
Next template's BYOK config endpoint, which became a public write
surface on hosted deploys without an auth gate.
agent-core
- Add @types/node devDep so the dts build resolves Node globals
on a clean install.
agent-templates/next
- Align lint stack with Next 16: ESLint 9 flat config via FlatCompat,
matching eslint-config-next.
- Add typecheck script.
- /api/config: gate POST behind CONFIG_ADMIN_TOKEN with a constant-
time Bearer compare; gate GET on hosted platforms (Vercel, Railway,
Fly, Render) so masked-key disclosure isn't public. Empty token
short-circuits to 500 instead of silently opening the endpoint.
- Reject env values containing characters that would break out of
their .env.local line or trip dotenv parsing: \r \n \0 " ' \ # $
and whitespace. Covers variable expansion (${VAR}) and unquoted-
space line corruption.
- Round-trip safe writes: stripQuotes on read, re-quote on write
whenever a value would otherwise be mis-parsed.
- Wrap req.json() in try/catch to return 400 on malformed bodies.
- settings-panel: admin-token UX with localStorage persistence,
Bearer header on every config write, and a shared handleAuthFailure
helper so 401/403 on saveKey/removeKey/saveValue/removeValue clears
the stale token and flips the UI back to the auth-required state
(mirroring fetchKeys's recovery path).
- /api/skills stays unauthenticated; the token is no longer attached
there to keep its exposure minimal.
agent-templates/express
- CORS opt-in: emit Access-Control-* only when CORS_ORIGIN is set.
Default is no CORS (server-to-server only); document in .env.example.
- Regenerate lockfile to drop stale papaparse specifiers that broke
--frozen-lockfile.
.internal/scripts/sync-agent-core.mjs
- Exclude pnpm-lock.yaml from rsync; templates don't install from the
vendored package.json so the lockfile is dead weight in diffs.
- Drop the three vendored template lockfiles already in the tree.
root
- .env.example pointing at per-template env examples plus a hosted-
deploy note for CONFIG_ADMIN_TOKEN.
us
force-pushed
the
fix/cold-install-blockers
branch
from
11e4bbb to
da8c666
Compare
us
changed the title
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
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.
Summary
Fresh-clone developer experience was broken on multiple fronts — this PR makes
pnpm install --frozen-lockfile+pnpm buildwork end-to-end acrossagent-coreand all three templates, and hardens the Next/api/configendpoint that holds user API keys.What changed
Cold-install blockers
agent-core— add missing@types/nodedevDep. dts build failed on a clean install because pnpm's isolated store doesn't hoist transitive types.agent-templates/next— align eslint9+ eslint-config-next16.2.1(Next 16 dropped the legacy eslint 8 preset); add flateslint.config.mjsvia@eslint/eslintrcFlatCompat; addtsc --noEmittypecheck script.agent-templates/express— regeneratepnpm-lock.yamlto drop stalepapaparsespecifiers that made--frozen-lockfilerefuse to install.Config endpoint hardening (next template)
/api/configGET/POST gated behindCONFIG_ADMIN_TOKENon hosted deployments — constant-time compare viacrypto.timingSafeEqual, empty-token hard-deny, andisSafeEnvValuevalue guard rejecting\r \n \0 " ' \ # $and whitespace (all known dotenv-injection vectors including${VAR}variable expansion).Authorization: Beareron every config write. 401/403 from the server drops the stale token via a sharedhandleAuthFailurehelper used by every save/remove path; the UI distinguishes rejected-token vs network-error feedback.Express CORS
Access-Control-*headers whenCORS_ORIGINis explicitly set./v1/configreportsnullinstead of"*"when CORS is unset. Safer default for an endpoint that holds API keys.Sync hygiene
sync-agent-core.mjsexcludespnpm-lock.yamlfrom rsync; templates don't install from the vendored agent-corepackage.jsonso the vendored lockfile was dead weight in every diff. Three stale copies removed.Docs
.env.exampleas a documentation-only pointer to per-template env files (repo root has no code that reads.env).Verified locally
agent-core(node 22): 91 tests ✅agent-templates/express:pnpm install --frozen-lockfile✅agent-templates/library:pnpm install --frozen-lockfile✅agent-templates/next:pnpm install --frozen-lockfile✅sync-agent-core.mjs --check✅Test plan
pnpm install --frozen-lockfilesucceeds inagent-coreand each templateCONFIG_ADMIN_TOKEN, exercise the settings panel — save key, wrong token, clear token/v1/configreturnscorsOrigin: nullwithoutCORS_ORIGIN