Repository navigation
Conversation
A repo can now ship a `codebay.json` at its workspace root:
{
"overrides": { "githubToken": "MOCHI_GITHUB_AUTH" },
"ports": { "6969": "web", "8123:5173": "vite" }
}
`overrides` points `claudeCodeToken`, `githubToken`, `gitUserName` and
`gitUserEmail` at the NAME of a host-side variable, resolved against the
Custom-env-vars setting first and then the manager's own environment. The
three credential injections consult it inside `apply()`, so an edit refreshes
on rebuild; `auth.status()` stays host-level since it has no instance context.
Git name and email resolve independently.
`ports` merges with the project's devcontainer.json `forwardPorts`/`appPort`:
a shared port gets a display name on the dashboard and instance page, a port
only in codebay.json is forwarded too, and a `host:container` key pins the
host side (falling back to allocation, with a log line, when it's taken).
Port seeding moves from `boot()` to `provision()` so a restart re-reads both
project files; `instances.seeded_ports` is the high-water mark that keeps a
re-seed from resurrecting a port removed by hand.
Log redaction moves to `secrets.server.ts` and gains `registerSecretValue`, so
a token resolved out of host env is masked like a Settings one.
Nothing cascades off `instances`, so `deleteInstanceRow` left the seeded forwards behind; `db.isolated.test.ts` asserts on `usedPorts()`, a global UNION over both tables, and failed whenever readdir put this file first.
`projectOverride` registered every resolved value as a redaction secret, so a repo naming its git identity masked that name in every sandbox's boot log — including the line reporting the identity it just injected. `claimPinnedPort` consulted neither `reservedPorts` nor populated it, so a concurrent `allocatePort()` boot could hand out the same host port and leave the second container unable to bind.
Renumber the named-ports migration to 0012 (main took 0011) and add seeded_ports to the new test fixtures from main.
khromov
left a comment
There was a problem hiding this comment.
Verdict: Changes requested
The merge with main is clean and CI is green on all three OSes. I found four correctness bugs in the new code, each one reproduced (details inline):
- A hostile or broken
codebay.json(for example a symlink to/dev/zerocommitted to a repo) hangs the provision forever. The parse warnings also echo the file's contents into the boot log. - Two instances that pin the same host port and boot at the same time can both claim it.
- On the first restart after upgrade, a pre-existing instance brings back declared ports the user had removed by hand.
- A per-field git identity override does nothing when the host has only one half of the identity.
I checked these and found nothing wrong: invalid port values (0, 65536, floats, whitespace, 1:2:3), the reserved 8080/7681 ports, __proto__ keys, the moved stripJsonc, and migration 0012 (it was renumbered from 0011 in the merge because main took 0011).
| const file = join(workspaceDir, PROJECT_CONFIG_FILE); | ||
| if (!existsSync(file)) return emptyConfig(); | ||
| try { | ||
| return parseProjectConfig(await readFile(file, 'utf8')); |
There was a problem hiding this comment.
codebay.json is read without checking the file type or size, so a symlink can hang the boot or leak host files.
readFile follows symlinks and reads until EOF. codebay.json comes from repo content, and git can check out a symlink.
Failure scenarios:
- A repo commits
codebay.json -> /dev/zero.seedProjectPortsnever returns, the row stayscreatingfor good, and the manager's memory keeps growing. The same hang hits everyprojectOverride()call inside the credential injections'apply(), which have no timeout. - A repo commits
codebay.json -> ~/.aws/credentialsor any other host file. The parse error lands in the boot log verbatim, for examplecould not be parsed: JSON Parse error: Unexpected identifier "AKIA...". MCPget_logsreturns that log too.
I reproduced both: a /dev/zero symlink hung readProjectConfig for more than 3s, and a text file had its first token echoed in warnings.
Suggested fix: lstat the file and require isFile() and not a symlink (or realpath it and confirm it stays inside the workspace), cap the size at around 64 KiB, and don't quote raw file content or keys in warnings.
There was a problem hiding this comment.
Fixed in 5c0d703. readProjectConfig now opens the file with O_NOFOLLOW | O_NONBLOCK and fstats the same handle, so it never follows a symlink and never blocks on a FIFO, and there is no TOCTOU gap. It refuses anything that is not a regular file, and refuses files over 64 KiB, checked both before the read and on the bytes actually read. A parse failure now logs a generic "could not be parsed as JSON" instead of the parser message that quoted the file. New regression tests in project-config.isolated.test.ts cover a symlink to a secret, a symlink to /dev/zero, a FIFO, an oversized file, and a parse error that must not echo the file. These are POSIX-only where they need symlinks or mkfifo.
| ); | ||
| return null; | ||
| } | ||
| reservedPorts.set(pinned, Date.now()); |
There was a problem hiding this comment.
Two boots can both claim the same pinned host port.
The comment on line 478 says concurrent boots are handled, but the check and the reservation are separated by two awaits (hostPortsInUse() and isHostPortBindable()). A second seedProjectPorts checking reservedPorts.has(pinned) during that window sees nothing.
Failure scenario: two instances of the same repo, whose codebay.json has "9123:3000", provision at the same time (for example two quick creates). Both insert a port_forwards row with host_port 9123. The second devcontainer up then fails with "port is already allocated", or on VM-backed daemons its forward is silently dead. This reproduced with Promise.all([seedProjectPorts(a), seedProjectPorts(b)]) and a docker stub whose listContainers takes 20 ms.
Suggested fix: do the synchronous checks (usedPorts(), reservedPorts) and reservedPorts.set(pinned, …) before any await. If the async Docker or bind probes then reject the port, reservedPorts.delete(pinned) and fall back.
There was a problem hiding this comment.
Fixed in 87d7047. claimPinnedPort now does the synchronous checks (usedPorts() and reservedPorts) and reserves the port before its first await. It releases the reservation if the Docker or bind probe then rejects the port. Regression test in project-ports.isolated.test.ts: two concurrent seedProjectPorts calls pin the same host port against a slow Docker stub, and only one of them gets it.
| // Without this a port the user removed by hand would come back on the next restart; a row from | ||
| // before the column has no record, so treat whatever it already forwards as seeded. | ||
| const seeded = new Set<number>( | ||
| row.seeded_ports ? (JSON.parse(row.seeded_ports) as number[]) : [...existing.keys()] |
There was a problem hiding this comment.
Existing instances bring back declared ports the user removed by hand.
Seeding used to run only in boot(). It now runs on every provision(). For a row that predates seeded_ports, this fallback counts only the ports it currently forwards as seeded. A devcontainer.json port the user deleted by hand is by definition not in existing, so the loop below re-inserts it.
Failure scenario: an instance created on 0.15 declares forwardPorts: [3000, 4000], and the user removed 4000 in the Ports box. After upgrading, the first Restart logs Forwarding declared port 4000 → … and publishes it again. That contradicts the README's "A port you remove by hand stays removed". The reviewer's repro showed [3000, 4000] after the restart.
Suggested fix: when seeded_ports is NULL, seed the set with existing.keys() plus declared, since the old boot path already seeded every declared port once. Only ports that appear solely in codebay.json should count as new. A lifecycle test for this legacy case would cover it.
There was a problem hiding this comment.
Fixed in 13a42a7. New rows are now created with seeded_ports = '[]', so NULL only ever means a row from before the column. For those rows the seeded set is existing ∪ declared, because their first boot already seeded every declared port. Ports that appear only in codebay.json still arrive. Regression test in project-ports.isolated.test.ts: a legacy row declaring 3000 and 4000 with only 3000 still forwarded keeps 4000 removed and still picks up the new codebay.json port.
| projectOverride(workspaceDir, 'gitUserName'), | ||
| projectOverride(workspaceDir, 'gitUserEmail'), | ||
| hostIdentity() | ||
| ]); |
There was a problem hiding this comment.
Per-field override doesn't work when the host has only half an identity.
The docstring says the two halves resolve independently, but hostIdentity() still returns null unless the host has both user.name and user.email. So host?.name and host?.email are both empty whenever either one is missing.
Failure scenario: the host has only git config --global user.email set (common on CI boxes and fresh machines), and codebay.json sets overrides.gitUserName. The expected result is the repo's name plus the host's email. Instead readGitIdentity returns null, and the boot log says "No global git identity found on host; skipped git identity injection". The reviewer reproduced this with a HOME whose gitconfig had only user.email.
Suggested fix: resolve the host name and email per field. Use the Settings pair if both are set, else each git config --global key on its own. Then combine field by field here.
There was a problem hiding this comment.
Fixed in 9e505fb. hostIdentity() now returns each half on its own (the Settings pair when both are set, otherwise each git config --global key separately), and readGitIdentity combines them field by field. readGitConfig now passes process.env explicitly, because Bun.spawn ignores later env edits; that lets the test point GIT_CONFIG_GLOBAL at a fixture. Regression test in injections.isolated.test.ts: a host gitconfig with only user.email plus a gitUserName override gives the full identity, and with no override the result is still null.
…bay.json A repo could commit codebay.json as a symlink to /dev/zero (hanging the boot) or to a host file (leaking it through the parse warning).
Two boots pinning the same host port could both pass the availability check across its awaits and claim the port.
A row from before seeded_ports had every declared port seeded by its first boot, so NULL now counts them as seeded; new rows start at [].
hostIdentity() returned null unless the host had both user.name and user.email, so overriding just one was skipped on a half-configured host.
A repo can ship a
codebay.jsonat its workspace root to override which credentials Codebay injects into its sandbox, and to name its ports.{ // Each value is the NAME of a variable, never a secret — safe to commit. "overrides": { "claudeCodeToken": "MOCHI_CLAUDE_TOKEN", "githubToken": "MOCHI_GITHUB_AUTH", "gitUserName": "MOCHI_GIT_NAME", "gitUserEmail": "MOCHI_GIT_EMAIL" }, "ports": { "6969": "web", "8123:5173": "vite" } }Motivating case: the Mochi repo wants its own robot GitHub account and commit author, without changing what every other sandbox gets.
Overrides
Names resolve against Settings → Custom environment variables first, then the manager process's own environment. Unresolved → the injection falls back to whatever it would have used anyway.
gitUserName/gitUserEmailare independent, so a repo can rename the author and keep the host's email.The three injections consult
projectOverride(target.instance.workspace_path, key)insideapply(), so an edit refreshes on rebuild with no cache to invalidate.auth.status()deliberately does not — it has no target and is a global preflight of what the host offers; the boot log now names the source that won for each injection instead.Named ports
Key is the container port, or
"<host>:<container>"to pin the host side; value is the display name. Merges with the project's ownforwardPorts/appPort— a port declared there gets a name, a port only here is forwarded too. A taken pin falls back to an allocated port with a warning rather than failing the boot.PortsBox(dashboard card) shows the name in place of the number, with both in the tooltip; the instance page's panel shows a name chip before the port.Notable changes beyond the feature
boot()toprovision(), so a Restart re-readsdevcontainer.jsonandcodebay.json. Previously a port newly added toforwardPortswas never picked up. That in turn needsinstances.seeded_ports(migration0011) as a high-water mark — without it, a re-seed would resurrect a port the user removed with the×.stripJsoncextracted tosrc/lib/jsonc.tssoproject-config.server.tsdoesn't pull indevcontainer.server.ts's imports (and would close a cycle: injections import project-config).src/lib/secrets.server.tsand gainedregisterSecretValue, so a token resolved out of host env is masked in the boot log like a Settings one.redactSecrets/invalidateSecretValuesare re-exported frominstances.server.ts, so no call site changed.WRITESclobber-safety table is unchanged.Security posture — please read
A repo can name any environment variable set on the Codebay host, and its value is injected into that repo's container, where an agent running with permission prompts bypassed can read it. The four override keys do not contain the blast radius. It's also read from the instance's copy of the workspace, so an agent can write a
codebay.jsonand grant itself a different variable on the next restart.This was the explicit choice over an allowlist, and the README says it plainly. If you'd rather narrow it, the cheapest guard is a small deny-set in
resolveHostVarfor the manager's own secrets (BASIC_AUTH_PASSWORD,MOCHI_KEY) — say the word and I'll add it.Known limits (documented)
ghtoken:git.server.tsreads it before the workspace exists.claudeCodeTokendoesn't apply when Settings → Custom endpoint swaps the Claude slot, which uses its own token.Bun.envis case-insensitive, so stick to SCREAMING_SNAKE.Verification
bun run checksclean: 522 pass, 0 fail, 0 typecheck errors.project-config.isolated.test.ts(parser + resolver),project-ports.isolated.test.ts(merge, pinning, no-resurrect, re-seed),jsonc.test.ts; extendedinjections.isolated.test.ts(all four overrides, scoping, fall-through),db.isolated.test.ts(label round-trip),instances-lifecycle.isolated.test.ts(name serialization, registered-secret redaction).0011— existing forwards survive withlabel: null./debugrenders the named chip (web) beside unnamed ones, tooltip intact.docker infofails), sodevcontainer upwith a realcodebay.jsonis untested end to end.