← back to Cli Printing Press
fix(cli): harden manifest-gen remote spec loader against hangs and silent truncation (#1699)
aecf495af1b8be4394d0f41253f3b2cdc639e370 · 2026-05-19 22:41:12 -0700 · Trevin Chow
* fix(cli): harden manifest-gen remote spec loader against hangs and silent truncation
loadSpec in cmd/manifest-gen had two defects when fetching remote specs:
1. No timeout on the HTTP request. http.Get with the default client has no
overall request timeout, so a remote host that flushes headers and then
blocks on the body would hang manifest-gen indefinitely.
2. Silent truncation. io.LimitReader(body, 50MiB) followed by io.ReadAll
returned the first 50 MiB of any oversized response with no error, so the
caller could not tell a real 49 MiB spec from a multi-gigabyte stream
truncated mid-document.
Fix: route the request through http.NewRequestWithContext with a 60s timeout,
and read limit+1 bytes so we can return an explicit "spec exceeds N bytes"
error when the response is over the limit. The 50 MiB cap is preserved.
Adds main_test.go covering happy path, local file, non-200, stalled server
(with a short timeout override so the test runs in well under a second),
oversized response, and exactly-at-limit response.
* fix(cli): eliminate data race in manifest-gen loadSpec tests
The previous test helpers `withTimeout` / `withMaxBytes` mutated the
package-level `remoteSpecTimeout` and `maxRemoteSpecBytes` vars while
the `httptest.Server` handler goroutine read those same vars to size
its response. There was no happens-before relationship between the
test-goroutine write and the handler-goroutine read, so `go test
-race ./cmd/manifest-gen/...` could flag a data race.
Refactor `loadSpec` to take its byte limit and timeout as explicit
parameters. The single production caller in `main()` passes
`maxRemoteSpecBytes` and `remoteSpecTimeout` directly; tests pass
their own local values. No package-level state is mutated, so the
handler closures close over local vars and the race is gone by
construction.
Restore the package-level identifiers to `const` since tests no
longer override them. Production behavior is unchanged (same 50 MiB
limit, same 60s timeout).
Files touched
M cmd/manifest-gen/main.goA cmd/manifest-gen/main_test.go
Diff
commit aecf495af1b8be4394d0f41253f3b2cdc639e370
Author: Trevin Chow <trevin@trevinchow.com>
Date: Tue May 19 22:41:12 2026 -0700
fix(cli): harden manifest-gen remote spec loader against hangs and silent truncation (#1699)
* fix(cli): harden manifest-gen remote spec loader against hangs and silent truncation
loadSpec in cmd/manifest-gen had two defects when fetching remote specs:
1. No timeout on the HTTP request. http.Get with the default client has no
overall request timeout, so a remote host that flushes headers and then
blocks on the body would hang manifest-gen indefinitely.
2. Silent truncation. io.LimitReader(body, 50MiB) followed by io.ReadAll
returned the first 50 MiB of any oversized response with no error, so the
caller could not tell a real 49 MiB spec from a multi-gigabyte stream
truncated mid-document.
Fix: route the request through http.NewRequestWithContext with a 60s timeout,
and read limit+1 bytes so we can return an explicit "spec exceeds N bytes"
error when the response is over the limit. The 50 MiB cap is preserved.
Adds main_test.go covering happy path, local file, non-200, stalled server
(with a short timeout override so the test runs in well under a second),
oversized response, and exactly-at-limit response.
* fix(cli): eliminate data race in manifest-gen loadSpec tests
The previous test helpers `withTimeout` / `withMaxBytes` mutated the
package-level `remoteSpecTimeout` and `maxRemoteSpecBytes` vars while
the `httptest.Server` handler goroutine read those same vars to size
its response. There was no happens-before relationship between the
test-goroutine write and the handler-goroutine read, so `go test
-race ./cmd/manifest-gen/...` could flag a data race.
Refactor `loadSpec` to take its byte limit and timeout as explicit
parameters. The single production caller in `main()` passes
`maxRemoteSpecBytes` and `remoteSpecTimeout` directly; tests pass
their own local values. No package-level state is mutated, so the
handler closures close over local vars and the race is gone by
construction.
Restore the package-level identifiers to `const` since tests no
longer override them. Production behavior is unchanged (same 50 MiB
limit, same 60s timeout).
---
cmd/manifest-gen/main.go | 31 +++++++--
cmd/manifest-gen/main_test.go | 144 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 171 insertions(+), 4 deletions(-)
diff --git a/cmd/manifest-gen/main.go b/cmd/manifest-gen/main.go
index a04bef82..d156159c 100644
--- a/cmd/manifest-gen/main.go
+++ b/cmd/manifest-gen/main.go
@@ -14,6 +14,7 @@
package main
import (
+ "context"
"flag"
"fmt"
"io"
@@ -21,6 +22,7 @@ import (
"os"
"path/filepath"
"strings"
+ "time"
"github.com/mvanhorn/cli-printing-press/v4/internal/graphql"
"github.com/mvanhorn/cli-printing-press/v4/internal/openapi"
@@ -28,6 +30,13 @@ import (
"github.com/mvanhorn/cli-printing-press/v4/internal/spec"
)
+// Tunables for remote spec fetches. Production callers pass these to loadSpec
+// explicitly; tests pass their own values so no package-level state is mutated.
+const (
+ maxRemoteSpecBytes int64 = 50 * 1024 * 1024
+ remoteSpecTimeout time.Duration = 60 * time.Second
+)
+
func main() {
specFlag := flag.String("spec", "", "URL or local path to the API spec")
outputFlag := flag.String("output", ".", "Output directory for tools-manifest.json")
@@ -40,7 +49,7 @@ func main() {
}
// Load spec data.
- data, err := loadSpec(*specFlag)
+ data, err := loadSpec(*specFlag, maxRemoteSpecBytes, remoteSpecTimeout)
if err != nil {
fmt.Fprintf(os.Stderr, "error loading spec: %v\n", err)
os.Exit(1)
@@ -110,9 +119,15 @@ func main() {
fmt.Fprintf(os.Stderr, "Spec format: %s\n", format)
}
-func loadSpec(source string) ([]byte, error) {
+func loadSpec(source string, maxBytes int64, timeout time.Duration) ([]byte, error) {
if openapi.IsRemoteSpecSource(source) {
- resp, err := http.Get(source)
+ ctx, cancel := context.WithTimeout(context.Background(), timeout)
+ defer cancel()
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, source, nil)
+ if err != nil {
+ return nil, fmt.Errorf("fetching %s: %w", source, err)
+ }
+ resp, err := http.DefaultClient.Do(req)
if err != nil {
return nil, fmt.Errorf("fetching %s: %w", source, err)
}
@@ -120,7 +135,15 @@ func loadSpec(source string) ([]byte, error) {
if resp.StatusCode != 200 {
return nil, fmt.Errorf("fetching %s: HTTP %d", source, resp.StatusCode)
}
- return io.ReadAll(io.LimitReader(resp.Body, 50*1024*1024))
+ // Read limit+1 bytes so we can distinguish "exactly at limit" from "exceeds limit".
+ data, err := io.ReadAll(io.LimitReader(resp.Body, maxBytes+1))
+ if err != nil {
+ return nil, fmt.Errorf("fetching %s: %w", source, err)
+ }
+ if int64(len(data)) > maxBytes {
+ return nil, fmt.Errorf("fetching %s: spec exceeds %d bytes", source, maxBytes)
+ }
+ return data, nil
}
return os.ReadFile(source)
}
diff --git a/cmd/manifest-gen/main_test.go b/cmd/manifest-gen/main_test.go
new file mode 100644
index 00000000..86ce63ed
--- /dev/null
+++ b/cmd/manifest-gen/main_test.go
@@ -0,0 +1,144 @@
+package main
+
+import (
+ "bytes"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+ "time"
+)
+
+func TestLoadSpec_HappyPath(t *testing.T) {
+ body := "openapi: 3.0.0\ninfo:\n title: tiny\n"
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "application/yaml")
+ _, _ = w.Write([]byte(body))
+ }))
+ t.Cleanup(srv.Close)
+
+ got, err := loadSpec(srv.URL, maxRemoteSpecBytes, remoteSpecTimeout)
+ if err != nil {
+ t.Fatalf("loadSpec returned error: %v", err)
+ }
+ if string(got) != body {
+ t.Fatalf("loadSpec returned %q, want %q", string(got), body)
+ }
+}
+
+func TestLoadSpec_LocalFile(t *testing.T) {
+ dir := t.TempDir()
+ path := filepath.Join(dir, "spec.yaml")
+ body := []byte("base_url: https://example.com\nresources: {}\n")
+ if err := os.WriteFile(path, body, 0o600); err != nil {
+ t.Fatalf("write fixture: %v", err)
+ }
+
+ got, err := loadSpec(path, maxRemoteSpecBytes, remoteSpecTimeout)
+ if err != nil {
+ t.Fatalf("loadSpec returned error: %v", err)
+ }
+ if !bytes.Equal(got, body) {
+ t.Fatalf("loadSpec returned %q, want %q", string(got), string(body))
+ }
+}
+
+func TestLoadSpec_Non200(t *testing.T) {
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ http.Error(w, "nope", http.StatusInternalServerError)
+ }))
+ t.Cleanup(srv.Close)
+
+ _, err := loadSpec(srv.URL, maxRemoteSpecBytes, remoteSpecTimeout)
+ if err == nil {
+ t.Fatalf("loadSpec returned nil error for 500 response")
+ }
+ if !strings.Contains(err.Error(), "HTTP 500") {
+ t.Fatalf("error %q does not mention HTTP 500", err.Error())
+ }
+}
+
+// TestLoadSpec_StalledServer verifies the request times out instead of hanging
+// forever when the server flushes headers but never sends the body.
+func TestLoadSpec_StalledServer(t *testing.T) {
+ shortTimeout := 200 * time.Millisecond
+
+ released := make(chan struct{})
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "application/yaml")
+ w.WriteHeader(http.StatusOK)
+ if f, ok := w.(http.Flusher); ok {
+ f.Flush()
+ }
+ // Block until the test ends so the body never arrives.
+ select {
+ case <-released:
+ case <-r.Context().Done():
+ }
+ }))
+ t.Cleanup(func() {
+ close(released)
+ srv.Close()
+ })
+
+ start := time.Now()
+ _, err := loadSpec(srv.URL, maxRemoteSpecBytes, shortTimeout)
+ elapsed := time.Since(start)
+
+ if err == nil {
+ t.Fatalf("loadSpec returned nil error for stalled server")
+ }
+ // Allow generous slack (~3x the configured timeout) for scheduler jitter.
+ if elapsed > 3*shortTimeout+500*time.Millisecond {
+ t.Fatalf("loadSpec took %v, expected to error within ~3x timeout (%v)", elapsed, shortTimeout)
+ }
+}
+
+// TestLoadSpec_Oversized verifies that responses larger than the byte limit
+// produce an explicit oversized-spec error instead of silently truncating.
+func TestLoadSpec_Oversized(t *testing.T) {
+ const smallLimit int64 = 1024
+
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "application/yaml")
+ // Stream smallLimit + 1 bytes so the read sees one byte past the limit.
+ buf := bytes.Repeat([]byte{'a'}, int(smallLimit)+1)
+ _, _ = w.Write(buf)
+ }))
+ t.Cleanup(srv.Close)
+
+ _, err := loadSpec(srv.URL, smallLimit, remoteSpecTimeout)
+ if err == nil {
+ t.Fatalf("loadSpec returned nil error for oversized response")
+ }
+ want := fmt.Sprintf("%d bytes", smallLimit)
+ if !strings.Contains(err.Error(), want) {
+ t.Fatalf("error %q does not mention byte limit %q", err.Error(), want)
+ }
+ if !strings.Contains(err.Error(), "exceeds") {
+ t.Fatalf("error %q does not mention 'exceeds'", err.Error())
+ }
+}
+
+// TestLoadSpec_AtLimit verifies a response exactly at the byte limit succeeds.
+func TestLoadSpec_AtLimit(t *testing.T) {
+ const smallLimit int64 = 1024
+
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "application/yaml")
+ buf := bytes.Repeat([]byte{'a'}, int(smallLimit))
+ _, _ = w.Write(buf)
+ }))
+ t.Cleanup(srv.Close)
+
+ got, err := loadSpec(srv.URL, smallLimit, remoteSpecTimeout)
+ if err != nil {
+ t.Fatalf("loadSpec returned error for at-limit response: %v", err)
+ }
+ if int64(len(got)) != smallLimit {
+ t.Fatalf("loadSpec returned %d bytes, want %d", len(got), smallLimit)
+ }
+}
← 9eff1e20 fix(cli): resolve shipcheck CLI binary name from .printing-p
·
back to Cli Printing Press
·
(newest)