Treat the server and the filesystem as untrusted input - #22
Merged
Conversation
Two functions in internal/api carry the policy, so it cannot drift apart at the two dozen sites that need it. Printable escapes with Go's own rules, which cover the control characters a terminal acts on and the bidirectional overrides that reorder what is around them, while leaving accented letters, other scripts, emoji and spaces byte-identical. Decode bounds a body at 32 MiB before reading it, which is set against the server's caps of 100 fleets per user and 100 fleet keys per fleet. A device name is renamable by any member of the fleet, so a name carrying an erase-line sequence used to erase its own row and hide the device from the owner who pays for it. All five tables, both browser links, the one-time code, the fleet key and the relayed server sentence now go through Printable. Every success body was decoded unbounded while every error body was already bounded, and twelve sites returned encoding/json's own words to the user. A captive portal now reads as a sentence rather than "invalid character '<'". The same was true of the filesystem. Seven sites returned an os path error raw, and two of them fired after an irreversible act had already succeeded: deleting an account whose stored login could not then be removed swallowed "Account deleted." entirely. Both say what happened and what is left to do. The stored login lands by rename from a temporary file in the same folder, so an interrupted write cannot destroy a working login and a file already at a wider mode cannot keep it. Four smaller holes on the same theme: key create could print an empty fleet key and exit zero, logout looped forever on an empty stored login rather than treating it as logged out, one unreadable last-seen time hid the whole device table behind Go's layout string, and three fleet-name lookups rendered a blank cell for a fleet that had gone. account delete now reads the status of the probe it already makes, so an invalid login is reported before the question rather than after it. Fifteen of the sixteen new guards were checked against the mutation that breaks them. The exception is the scheme check in openBrowser, which cannot be exercised without launching a real handler on the machine running the tests. No version change, and nothing under .github, flake.nix, .goreleaser.yaml or go.mod is touched.
siliconwitch
added a commit
that referenced
this pull request
Aug 21, 2026
The third and last of the cleanup cycles, plus the five findings the review of #22 confirmed. ## Coding rules The `--json` path of every list command assigned nothing before it returned, two commands built an HTTP client above the guards that can refuse the call, and three constants had one use each. The two five-second values in the login poll are literals now, each carrying the RFC 8628 section it comes from. `Request` and `AuthenticatedRequest` take a reader named `reader`; the session keeps `In` and `Out`, which `docs/cli.md` records as the one place a reader is not called `reader`. `takeServerFlag` had one production call site and was kept alive by a unit test written against it, which is both of the rules that say not to. Its thirty lines now run where they always ran, and the table that covered them drives `Dispatch` instead: a synthetic command records the base it was handed and the arguments left over. ## Tests Nothing invoked `main`, so none of its exit paths were covered. A subprocess table runs six of them, including the two that must exit zero. `fleet list`, `key list` and `account balance` had no server-refusal case, `FetchDevices` had no failure test of its own, `member remove` never reached its unknown-fleet guard, and nothing read the four lines `login` prints. ## Carried over from the review of #22 The `account delete` probe asked `GET /`, which `superstack-server/internal/api/auth.go:25-29` exempts from authentication, so the not-logged-in guard added last cycle could never fire against the real server and its test built a server shape that does not exist. It asks `GET /fleets` now. `logout` treated an already-removed login as a failure while `account delete` tolerated it, so a second logout printed an instruction naming a file that was not there. `maximumBody` had one use. The comment on the bound named only the device list as uncapped, and the member list is uncapped too. Two error sentences said the login had "nowhere to live", which is a metaphor that names no action. ## The flake guard `CLAUDE.md` states that `vendorHash = null` holds only while the module has no third-party dependencies, and nothing checked it: adding one leaves `gofmt`, `go mod tidy` and the GoReleaser snapshot green while `nix build` fails on inconsistent vendoring. `TestTheModuleStaysDependencyFree` reads that condition out of `go.mod` and `go.sum`, so it costs nothing and runs everywhere the suite does. The new `flake` workflow covers what a Go build cannot see, the version regex in `flake.nix` and the `postInstall` rename, by building the package and checking that the version `main.go` declares, the version the flake reads out of it, and the version the built binary reports all agree. It runs on tags and on pull requests touching `flake.nix`, `flake.lock`, `main.go` or `go.mod`, not on every pull request, because a cold runner downloads a 257 MiB Go toolchain to do it against a `build` job that finishes in about a minute. Measured locally: 45 seconds of compile once the store is warm. The paths list includes the workflow itself, so this pull request runs it rather than merging it untested. ## Evidence - `gofmt -l .` clean, `go mod tidy` leaves the module files unchanged, `go vet ./...` clean, `go test -count=1 ./...` all packages pass with no skips. - Every new guard was checked against the mutation that breaks it. Two came back missed on the first attempt and both were fixed rather than reported as caught: the `fs.ErrNotExist` tolerance in `logout` had no test, and the member-remove row served an empty fleet list so the loop never ran under mutation. - The workflow's script was run against a real build before being written down. All three versions agreed and the binary landed at `bin/superstack`, which is the rename working. - The pinned action SHA was resolved with `gh api`, not guessed. `actionlint`, which carries shellcheck, passes on all three workflows. - A stray 10 MB `api.test` binary was sitting untracked in the tree, so `*.test` is ignored now. An adversarial review over this diff is still running. If it confirms anything, the fix lands as a further commit on this branch before merge. No version change, and nothing under `flake.nix`, `.goreleaser.yaml` or `go.mod` is touched. Paired with a documentation change in `superstack-server`.
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
1 participant
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.
The second of three cycles closing findings from the two architecture reviews. This one is a single idea applied at every boundary, which is why roughly two dozen sites are one pull request and not three.
The two functions
api.Printableescapes with Go's own rules throughstrconv.QuoteToGraphic, so it covers the control characters a terminal acts on and the bidirectional overrides that reorder what is around them, while leaving accented letters, other scripts, emoji and spaces byte-identical.api.Decodebounds a body at 32 MiB before reading it, set against the server's caps of 100 fleets per user and 100 fleet keys per fleet.They live in
internal/apifor the same reasonTakeJsonFlagandFormatBalancedo: more than one part needs them and no part may call another. This is policy rather than procedure, and the failure mode of letting it drift apart at two dozen sites is a hole rather than a style wobble.What was wrong
A device name is renamable by any member of the fleet, and neither side restricts the characters. A name carrying an erase-line sequence erased its own row and hid the device from the owner who pays for it.
Every success body was decoded unbounded while every error body was already bounded, and twelve sites returned
encoding/json's own words to the user.The same was true of the filesystem. Seven sites returned an
ospath error raw, and two fired after an irreversible act had already succeeded: deleting an account whose stored login could not then be removed swallowedAccount deleted.entirely, leaving the user unable to tell whether the account was gone.The stored login was written with
os.WriteFile, which truncates first, so a failed write destroyed a working login, and0600applied only on creation, so a file already at a wider mode kept it.Four smaller holes on the same theme:
key createcould print an empty fleet key and exit zero,logoutlooped forever on an empty stored login, one unreadable last-seen time hid the whole device table behind Go's layout string, and three fleet-name lookups rendered a blank cell for a fleet that had gone.Evidence
gofmt -l .clean,go mod tidyleaves the module files unchanged,go vet ./...clean,go test -count=1 ./...all packages pass with no skips.\x1b[2K\rhidden-from-the-owner, the row stays visible, and no live escape byte reaches stdout. A captive-portal HTML answer now reads as a sentence rather thaninvalid character '<'.Printablecannot panic:strconv.QuoteToGraphicalways returns a value quoted at both ends, checked against an empty string, a lone continuation byte, truncated multi-byte sequences and all-high bytes.os.CreateTempcreates at0600, the temporary file shares the target's directory so the rename cannot cross a filesystem, and renaming over a pre-existing0644file narrows it to0600and leaves exactly one file behind.Gaps, stated rather than softened
openBrowser's scheme check has no test. Exercising it would launch a real handler on the machine running the tests.CLAUDE.mdrecords both.An adversarial review pass over this diff is still running. If it confirms anything, the fix lands as a second commit on this branch before merge.
No version change, and nothing under
.github,flake.nix,.goreleaser.yamlorgo.modis touched. Paired with a documentation change insuperstack-server.