From c046e0685108ca9edc2b82072da5a755657e01ab Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Wed, 15 Jul 2026 22:52:51 +0200 Subject: [PATCH 1/4] fix(receipt): finalize and guard all egress paths --- cmd_receipt.go | 16 +- config_command.go | 12 +- exit.go | 31 ++++ exit_test.go | 30 +++ internal/agent/opencode_config.go | 6 +- internal/agent/opencode_config_test.go | 22 ++- internal/exposure/exposure.go | 7 + internal/exposure/exposure_test.go | 1 + internal/httpx/guard_test.go | 225 +++++++++++++---------- internal/httpx/httpx.go | 100 ++++++++-- internal/httpx/httpx_test.go | 62 +++++++ internal/setup/interactive.go | 11 +- internal/sysutil/error_reporting.go | 8 +- internal/sysutil/error_reporting_test.go | 58 ++++++ internal/vnc/stream.go | 17 +- main.go | 59 +++--- 16 files changed, 499 insertions(+), 166 deletions(-) create mode 100644 exit.go create mode 100644 exit_test.go create mode 100644 internal/sysutil/error_reporting_test.go diff --git a/cmd_receipt.go b/cmd_receipt.go index 75edf55..f13a218 100644 --- a/cmd_receipt.go +++ b/cmd_receipt.go @@ -29,7 +29,7 @@ func handleReceiptCommand(args []string) { receiptVerify(argAt(args, 1)) default: fmt.Fprintln(os.Stderr, "Usage: qmax-code receipt [id|latest]") - os.Exit(1) + exitWithReceipt(1) } } @@ -44,7 +44,7 @@ func receiptList() { paths, err := receipt.List() if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } if len(paths) == 0 { fmt.Println("No exposure receipts found.") @@ -65,7 +65,7 @@ func receiptShow(idOrLatest string) { data, err := json.MarshalIndent(r, "", " ") if err != nil { fmt.Fprintf(os.Stderr, "Error: cannot marshal receipt: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } fmt.Println(string(data)) } @@ -74,7 +74,7 @@ func receiptVerify(idOrLatest string) { r := mustResolveReceipt(idOrLatest) if err := receipt.Verify(r); err != nil { fmt.Fprintf(os.Stderr, "✗ INVALID: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } fmt.Printf("✓ VALID — run %s (%s), %d request(s) across %v\n", r.RunID, r.RunKind, r.Summary.TotalRequests, r.Destinations) @@ -86,25 +86,25 @@ func mustResolveReceipt(idOrLatest string) *receipt.Receipt { dir, err := receipt.ReceiptsDir() if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } if idOrLatest == "" || idOrLatest == "latest" { paths, err := receipt.List() if err != nil || len(paths) == 0 { fmt.Fprintln(os.Stderr, "No exposure receipts found.") - os.Exit(1) + exitWithReceipt(1) } r, err := receipt.Load(paths[len(paths)-1]) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } return r } r, err := receipt.Load(filepath.Join(dir, idOrLatest+".json")) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } return r } diff --git a/config_command.go b/config_command.go index eb7095f..0f0d1ce 100644 --- a/config_command.go +++ b/config_command.go @@ -44,22 +44,22 @@ func handleConfigCommand(args []string) { case "set": if len(args) < 3 { fmt.Fprintln(os.Stderr, "Usage: qmax-code config set KEY VALUE") - os.Exit(2) + exitWithReceipt(2) } if err := setConfigField(args[1], args[2]); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } fmt.Printf(" Set %s = %s\n", args[1], configSetDisplayValue(args[1], args[2])) case "unset": if len(args) < 2 { fmt.Fprintln(os.Stderr, "Usage: qmax-code config unset KEY") - os.Exit(2) + exitWithReceipt(2) } if err := setConfigField(args[1], ""); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } fmt.Printf(" Cleared %s\n", args[1]) @@ -67,14 +67,14 @@ func handleConfigCommand(args []string) { cfg := api.DefaultConfig() if err := cfg.Save(); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } fmt.Println(" api.Config reset to defaults") default: fmt.Fprintf(os.Stderr, "Unknown config command: %s\n", args[0]) fmt.Fprintln(os.Stderr, "Try: qmax-code config [show|set|unset|reset]") - os.Exit(2) + exitWithReceipt(2) } } diff --git a/exit.go b/exit.go new file mode 100644 index 0000000..275e340 --- /dev/null +++ b/exit.go @@ -0,0 +1,31 @@ +package main + +import ( + "os" + "sync" +) + +// exitWithReceipt is the only process-exit path used after main has installed +// the session receipt. os.Exit skips deferred functions, so finalize must run +// explicitly for command failures that occur after an outbound request. +var ( + exitMu sync.RWMutex + finalizeBeforeExit func() + processExit = os.Exit +) + +func configureExitFinalizer(finalize func()) { + exitMu.Lock() + finalizeBeforeExit = finalize + exitMu.Unlock() +} + +func exitWithReceipt(code int) { + exitMu.RLock() + finalize := finalizeBeforeExit + exitMu.RUnlock() + if finalize != nil { + finalize() + } + processExit(code) +} diff --git a/exit_test.go b/exit_test.go new file mode 100644 index 0000000..7b83338 --- /dev/null +++ b/exit_test.go @@ -0,0 +1,30 @@ +package main + +import "testing" + +func TestExitWithReceiptFinalizesBeforeProcessExit(t *testing.T) { + exitMu.Lock() + oldFinalize := finalizeBeforeExit + oldProcessExit := processExit + exitMu.Unlock() + t.Cleanup(func() { + exitMu.Lock() + finalizeBeforeExit = oldFinalize + processExit = oldProcessExit + exitMu.Unlock() + }) + + var order []string + configureExitFinalizer(func() { order = append(order, "finalize") }) + processExit = func(code int) { + if code != 7 { + t.Errorf("exit code = %d, want 7", code) + } + order = append(order, "exit") + } + + exitWithReceipt(7) + if len(order) != 2 || order[0] != "finalize" || order[1] != "exit" { + t.Fatalf("order = %v, want [finalize exit]", order) + } +} diff --git a/internal/agent/opencode_config.go b/internal/agent/opencode_config.go index 0d65f21..088a53c 100644 --- a/internal/agent/opencode_config.go +++ b/internal/agent/opencode_config.go @@ -38,6 +38,10 @@ func userOpenCodeConfigDir() string { // literal is a plaintext key sitting on disk. var literalAPIKeyRe = regexp.MustCompile(`"apiKey"\s*:\s*"([^"]*)"`) +// loadProviderKey is replaceable in tests so provider-env construction does +// not depend on a developer's real OS keychain entries. +var loadProviderKey = api.LoadProviderKey + // PlaintextKeyInUserOpenCodeConfig reports whether the user's OWN opencode // config files contain a plaintext apiKey (not an {env:}/{file:} reference), // and the path where one was found. Used to warn a user — when they move a @@ -211,7 +215,7 @@ func WriteOpenCodeConfig(cfg *api.Config, sctx *api.SessionContext, permissionMo func OpenCodeProviderEnv(cfg *api.Config) map[string]string { env := map[string]string{} for _, p := range cfg.ActiveProviders() { - key := api.LoadProviderKey(p.ID) + key := loadProviderKey(p.ID) if key == "" { continue } diff --git a/internal/agent/opencode_config_test.go b/internal/agent/opencode_config_test.go index 40cc50e..1ebc0fa 100644 --- a/internal/agent/opencode_config_test.go +++ b/internal/agent/opencode_config_test.go @@ -119,15 +119,25 @@ func TestOpenCodeModelsPassesProviderEnv(t *testing.T) { } func TestOpenCodeProviderEnvInjectsKeys(t *testing.T) { - t.Setenv("QMAX_PC_ZAI_CODING_PLAN", "zai-secret") - t.Setenv("GROQ_API_KEY", "gsk_secret") + oldLoadProviderKey := loadProviderKey + t.Cleanup(func() { loadProviderKey = oldLoadProviderKey }) + loadProviderKey = func(providerID string) string { + switch providerID { + case "zai-coding-plan": + return "test-zai-key" + case "groq": + return "test-groq-key" + default: + return "" + } + } cfg := &api.Config{EnabledProviders: []string{"zai-coding-plan", "groq"}} env := OpenCodeProviderEnv(cfg) - if env["QMAX_PC_ZAI_CODING_PLAN"] != "zai-secret" { - t.Errorf("zai env not injected: %v", env) + if env["QMAX_PC_ZAI_CODING_PLAN"] != "test-zai-key" { + t.Error("zai env not injected") } - if env["GROQ_API_KEY"] != "gsk_secret" { - t.Errorf("groq env not injected: %v", env) + if env["GROQ_API_KEY"] != "test-groq-key" { + t.Error("groq env not injected") } } diff --git a/internal/exposure/exposure.go b/internal/exposure/exposure.go index 9ef5eb8..420cab2 100644 --- a/internal/exposure/exposure.go +++ b/internal/exposure/exposure.go @@ -33,6 +33,8 @@ const ( CatLLMCompletion = "llm-completion" // reserved: response-side LLM accounting CatCloudAPI = "cloud-api" // QualityMax cloud REST API (projects, scripts, integrations) CatMCPTraffic = "mcp-traffic" // reserved: outbound MCP-over-HTTP egress + CatTelemetry = "telemetry" // opt-in error-reporting envelope + CatVNCControl = "vnc-control" // noVNC WebSocket handshake/control channel CatControl = "control" // metadata only: auth check, login poll, health/reachability probes CatUncategorized = "uncategorized" // unknown host/path — flagged so it can't hide ) @@ -62,6 +64,11 @@ func Classify(host, path string) string { strings.Contains(p, "/api/job-health/"): return CatControl + // Sentry-compatible telemetry is opt-in. The request body is still hashed + // by httpx, but it is categorized separately from QualityMax cloud REST. + case strings.Contains(p, "/envelope"): + return CatTelemetry + // QualityMax cloud REST API — projects, scripts, crawl, integrations, etc. case strings.Contains(p, "/api/"): return CatCloudAPI diff --git a/internal/exposure/exposure_test.go b/internal/exposure/exposure_test.go index c2f1fa6..dadb8bb 100644 --- a/internal/exposure/exposure_test.go +++ b/internal/exposure/exposure_test.go @@ -20,6 +20,7 @@ func TestClassify(t *testing.T) { {"cli poll", "app.qualitymax.io", "/api/auth/cli-poll", CatControl}, {"ollama tags probe", "localhost:11434", "/api/tags", CatControl}, {"job health", "app.qualitymax.io", "/api/job-health/background/abc", CatControl}, + {"telemetry envelope", "sentry.example", "/api/42/envelope/", CatTelemetry}, {"projects list", "app.qualitymax.io", "/api/projects", CatCloudAPI}, {"script fetch", "app.qualitymax.io", "/api/automation/scripts/1234", CatCloudAPI}, diff --git a/internal/httpx/guard_test.go b/internal/httpx/guard_test.go index 880e71d..940983a 100644 --- a/internal/httpx/guard_test.go +++ b/internal/httpx/guard_test.go @@ -1,56 +1,39 @@ package httpx import ( + "go/ast" + "go/parser" + "go/token" "os" "path/filepath" - "strconv" "strings" "testing" ) -// forbiddenSymbols are the egress-creating constructs. Their only legitimate -// home is this package. -var forbiddenSymbols = []string{ - "http.Client{", - "http.NewRequest(", - "http.NewRequestWithContext(", - "http.Get(", - "http.Post(", - "http.PostForm(", - "http.Head(", - "http.DefaultClient", - "http.DefaultTransport", +// forbiddenLibraryImports are client libraries that create egress outside the +// standard library. New entries require a dedicated httpx wrapper. +var forbiddenLibraryImports = map[string]bool{ + "github.com/go-resty/resty": true, + "github.com/levigross/grequests": true, + "github.com/h2non/gentleman": true, + "github.com/valyala/fasthttp": true, + "github.com/imroc/req": true, + "github.com/jmcvetta/napping": true, } -// forbiddenImports are third-party HTTP/WebSocket clients that would bypass -// net/http entirely. -var forbiddenImports = []string{ - "go-resty", "levigross/grequests", "h2non/gentleman", - "valyala/fasthttp", "imroc/req", "jmcvetta/napping", - "coder/websocket", -} - -// sanctionedCarveOuts are packages allowed to bypass the guard. Each must have -// a documented justification in its package doc. -// httpx — the chokepoint itself (creates the clients). -// vnc — documented WebSocket carve-out (see internal/vnc/stream.go package -// doc): live-browser screen streaming carries pixels, never -// source/prompts. Reviewed and accepted on QUA-1316. -var sanctionedCarveOuts = map[string]bool{"httpx": true, "vnc": true} - -// scanForEgressViolations walks root and returns any forbidden egress symbols or -// third-party HTTP imports outside sanctioned carve-out packages. It is -// extracted from the guard test so the injection test can exercise the same -// logic against a temp directory. +// scanForEgressViolations parses production Go source rather than matching +// strings. It catches import aliases, zero-value http.Client declarations, +// new(http.Client), raw transports, default clients, direct WebSocket dials, +// and net.Dial calls. func scanForEgressViolations(root string) ([]string, error) { var violations []string - err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err + err := filepath.Walk(root, func(path string, info os.FileInfo, walkErr error) error { + if walkErr != nil { + return walkErr } if info.IsDir() { name := info.Name() - if sanctionedCarveOuts[name] || name == "vendor" || name == "build" || strings.HasPrefix(name, ".") { + if name == "httpx" || name == "vendor" || name == "build" || strings.HasPrefix(name, ".") { if path != root { return filepath.SkipDir } @@ -60,86 +43,140 @@ func scanForEgressViolations(root string) ([]string, error) { if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { return nil } - data, rerr := os.ReadFile(path) - if rerr != nil { - return rerr + + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + return err } rel, _ := filepath.Rel(root, path) - for i, line := range strings.Split(string(data), "\n") { - code := line - if idx := strings.Index(code, "//"); idx >= 0 { - code = code[:idx] + violations = append(violations, scanFileForEgressViolations(file, fset, rel)...) + return nil + }) + return violations, err +} + +func scanFileForEgressViolations(file *ast.File, fset *token.FileSet, rel string) []string { + imports := map[string]string{} + var violations []string + for _, spec := range file.Imports { + path := strings.Trim(spec.Path.Value, `"`) + name := filepath.Base(path) + if spec.Name != nil { + name = spec.Name.Name + } + imports[name] = path + if forbiddenLibraryImports[path] { + violations = append(violations, violation(rel, fset, spec.Pos(), "forbidden egress library "+path)) + } + } + + ast.Inspect(file, func(node ast.Node) bool { + switch n := node.(type) { + case *ast.ValueSpec: + if selectorMatches(n.Type, imports, "net/http", "Client") || selectorMatches(n.Type, imports, "net/http", "Transport") { + violations = append(violations, violation(rel, fset, n.Pos(), "raw net/http client or transport declaration")) } - for _, sym := range forbiddenSymbols { - if strings.Contains(code, sym) { - violations = append(violations, - rel+":"+strconv.Itoa(i+1)+" forbidden egress symbol "+sym) - } + case *ast.CompositeLit: + if selectorMatches(n.Type, imports, "net/http", "Client") || selectorMatches(n.Type, imports, "net/http", "Transport") { + violations = append(violations, violation(rel, fset, n.Pos(), "raw net/http client or transport construction")) } - for _, imp := range forbiddenImports { - if strings.Contains(line, imp) { - violations = append(violations, - rel+":"+strconv.Itoa(i+1)+" forbidden HTTP library import "+imp) - } + case *ast.CallExpr: + if isNewHTTPClientOrTransport(n, imports) { + violations = append(violations, violation(rel, fset, n.Pos(), "new(net/http Client or Transport)")) + } + if callMatches(n, imports, "net/http", map[string]bool{ + "NewRequest": true, "NewRequestWithContext": true, "Get": true, + "Post": true, "PostForm": true, "Head": true, + }) { + violations = append(violations, violation(rel, fset, n.Pos(), "raw net/http request creation")) + } + if callMatches(n, imports, "net", map[string]bool{ + "Dial": true, "DialTimeout": true, "DialTCP": true, "DialUDP": true, + }) { + violations = append(violations, violation(rel, fset, n.Pos(), "raw net dial")) + } + if callMatches(n, imports, "github.com/coder/websocket", map[string]bool{"Dial": true}) { + violations = append(violations, violation(rel, fset, n.Pos(), "direct WebSocket dial; use httpx.DialWebSocket")) + } + case *ast.SelectorExpr: + if selectorMatches(n, imports, "net/http", "DefaultClient") || selectorMatches(n, imports, "net/http", "DefaultTransport") { + violations = append(violations, violation(rel, fset, n.Pos(), "raw net/http default client or transport")) } } - return nil + return true }) - return violations, err + return violations +} + +func selectorMatches(expr ast.Expr, imports map[string]string, packagePath, name string) bool { + selector, ok := expr.(*ast.SelectorExpr) + if !ok || selector.Sel.Name != name { + return false + } + ident, ok := selector.X.(*ast.Ident) + return ok && imports[ident.Name] == packagePath +} + +func callMatches(call *ast.CallExpr, imports map[string]string, packagePath string, names map[string]bool) bool { + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok || !names[selector.Sel.Name] { + return false + } + ident, ok := selector.X.(*ast.Ident) + return ok && imports[ident.Name] == packagePath +} + +func isNewHTTPClientOrTransport(call *ast.CallExpr, imports map[string]string) bool { + ident, ok := call.Fun.(*ast.Ident) + if !ok || ident.Name != "new" || len(call.Args) != 1 { + return false + } + return selectorMatches(call.Args[0], imports, "net/http", "Client") || + selectorMatches(call.Args[0], imports, "net/http", "Transport") +} + +func violation(rel string, fset *token.FileSet, pos token.Pos, message string) string { + return rel + ":" + fset.Position(pos).String()[len(fset.Position(pos).Filename)+1:] + " " + message } -// TestNoEgressOutsideHttpx is the static Egress Guard. It walks the whole -// qmax-code module and fails if any package other than a sanctioned carve-out -// constructs an HTTP client/request/transport or imports a third-party HTTP -// library. This makes an un-receipted egress path impossible to merge — the -// load-bearing half of the "Receipts, not promises" guarantee. -// -// net/http may still be IMPORTED elsewhere for types and constants (http.Request, -// http.MethodPost, http.StatusOK, http.StatusText); only the egress-CREATING -// symbols are forbidden. Route all outbound HTTP through -// httpx.NewClient / httpx.NewRequest. -func TestNoEgressOutsideHttpx(t *testing.T) { - root := filepath.Join("..", "..") // module root: qmax-code/ +func TestNoEgressOutsideHTTPX(t *testing.T) { + root := filepath.Join("..", "..") violations, err := scanForEgressViolations(root) if err != nil { - t.Fatalf("walk failed: %v", err) + t.Fatalf("scan egress: %v", err) } if len(violations) > 0 { - t.Fatalf("egress guard: %d violation(s) outside sanctioned carve-outs — route all outbound HTTP through httpx.NewClient/NewRequest:\n%s", - len(violations), strings.Join(violations, "\n")) + t.Fatalf("egress guard: %d violation(s) outside httpx:\n%s", len(violations), strings.Join(violations, "\n")) } } -// TestEgressGuardDetectsInjection proves the guard actually catches a raw -// http.Client construction and a forbidden import — so the guard's detection -// logic itself is validated in CI, not just the absence of violations today. -func TestEgressGuardDetectsInjection(t *testing.T) { +func TestEgressGuardDetectsAliasAndConstructorBypasses(t *testing.T) { dir := t.TempDir() - // Simulate a package that bypasses the chokepoint. - if err := os.WriteFile(filepath.Join(dir, "bad.go"), []byte( - `package bad + bad := `package bad + +import ( + h "net/http" + "net" + ws "github.com/coder/websocket" +) -import "net/http" +var client h.Client +var transport = new(h.Transport) -var client = &http.Client{} -`), 0644); err != nil { +func f() { + _, _ = net.Dial("tcp", "example.com:80") + _, _, _ = ws.Dial(nil, "ws://example.com", nil) +} +` + if err := os.WriteFile(filepath.Join(dir, "bad.go"), []byte(bad), 0o644); err != nil { t.Fatal(err) } - violations, err := scanForEgressViolations(dir) if err != nil { - t.Fatalf("scan failed: %v", err) - } - if len(violations) == 0 { - t.Fatal("guard failed to detect an injected raw http.Client{} — detection logic is broken") - } - found := false - for _, v := range violations { - if strings.Contains(v, "forbidden egress symbol http.Client{") { - found = true - } + t.Fatalf("scan: %v", err) } - if !found { - t.Fatalf("guard did not flag http.Client{}: %v", violations) + if len(violations) != 4 { + t.Fatalf("violations = %d, want 4: %v", len(violations), violations) } } diff --git a/internal/httpx/httpx.go b/internal/httpx/httpx.go index 0b7d46a..7db1b10 100644 --- a/internal/httpx/httpx.go +++ b/internal/httpx/httpx.go @@ -19,16 +19,17 @@ package httpx import ( - "bytes" "context" "crypto/sha256" "encoding/hex" "hash" "io" "net/http" + "sync" "time" receipt "github.com/Quality-Max/qmax-receipt" + "github.com/coder/websocket" "github.com/qualitymax/qmax-code/internal/exposure" ) @@ -61,6 +62,10 @@ func NewRequest(ctx context.Context, method, url string, body io.Reader) (*http. // unset and the recorded Entry.Model stays nil. type modelKey struct{} +// categoryKey carries an explicit category for egress protocols whose URL path +// alone cannot identify their purpose (for example a WebSocket handshake). +type categoryKey struct{} + // WithModel annotates ctx with the LLM model id for requests built from it. // LLM call sites (Anthropic, Cerebras, Ollama) wrap their context with this so // the receipt records model attribution; all other traffic omits it. @@ -71,6 +76,18 @@ func WithModel(ctx context.Context, model string) context.Context { return context.WithValue(ctx, modelKey{}, model) } +// WithCategory annotates ctx with an explicit receipt category. Most HTTP +// callers rely on exposure.Classify; use this only for protocol handshakes. +func WithCategory(ctx context.Context, category string) context.Context { + if ctx == nil { + ctx = context.Background() + } + if category == "" { + return ctx + } + return context.WithValue(ctx, categoryKey{}, category) +} + func modelFromContext(ctx context.Context) string { if ctx == nil { return "" @@ -81,6 +98,34 @@ func modelFromContext(ctx context.Context) string { return "" } +func categoryFromContext(ctx context.Context) string { + if ctx == nil { + return "" + } + if category, ok := ctx.Value(categoryKey{}).(string); ok { + return category + } + return "" +} + +// DialWebSocket performs a WebSocket handshake through the recording HTTP +// client. The WebSocket protocol itself uses the upgraded connection, while +// the destination and handshake egress are captured in the session receipt. +func DialWebSocket(ctx context.Context, url string, opts *websocket.DialOptions) (*websocket.Conn, *http.Response, error) { + if ctx == nil { + ctx = context.Background() + } + ctx = WithCategory(ctx, exposure.CatVNCControl) + + var dialOpts websocket.DialOptions + if opts != nil { + dialOpts = *opts + dialOpts.Subprotocols = append([]string(nil), opts.Subprotocols...) + } + dialOpts.HTTPClient = NewClient(0) + return websocket.Dial(ctx, url, &dialOpts) +} + type receiptTransport struct{ base http.RoundTripper } // hashingBody hashes and counts bytes as the transport reads the request body @@ -89,19 +134,35 @@ type hashingBody struct { rc io.ReadCloser h hash.Hash n int64 + mu sync.Mutex + // complete is true only when the transport consumed the body through EOF. + // A server can respond before consuming a request body, in which case a + // prefix hash would be misleading and must not be signed as complete. + complete bool } func (b *hashingBody) Read(p []byte) (int, error) { n, err := b.rc.Read(p) + b.mu.Lock() + defer b.mu.Unlock() if n > 0 { _, _ = b.h.Write(p[:n]) // hash.Hash.Write never errors b.n += int64(n) } + if err == io.EOF { + b.complete = true + } return n, err } func (b *hashingBody) Close() error { return b.rc.Close() } +func (b *hashingBody) snapshot() (bytes int64, sha256 string, complete bool) { + b.mu.Lock() + defer b.mu.Unlock() + return b.n, hex.EncodeToString(b.h.Sum(nil)), b.complete +} + func (t *receiptTransport) RoundTrip(req *http.Request) (*http.Response, error) { rec := receipt.FromContext(req.Context()) @@ -116,28 +177,36 @@ func (t *receiptTransport) RoundTrip(req *http.Request) (*http.Response, error) if m := modelFromContext(req.Context()); m != "" { entry.Model = &m } + if category := categoryFromContext(req.Context()); category != "" { + entry.Category = category + } // Wrap the body so it is hashed+counted as it streams to the wire. var hb *hashingBody if req.Body != nil { hb = &hashingBody{rc: req.Body, h: sha256.New()} req.Body = hb - } else { - hb = &hashingBody{rc: io.NopCloser(bytes.NewReader(nil)), h: sha256.New()} } resp, err := t.base.RoundTrip(req) - // Read the accumulated hash/size after RoundTrip returns. This is accurate - // because every current endpoint (Anthropic, Cerebras, Ollama, cloud REST) - // reads the full request body before sending response headers — so the - // transport has fully drained hashingBody by the time we get here. An - // early-responding server (rare; HTTP/2 with a 4xx before body drain) would - // see a partial count; acceptable given the no-buffering design constraint. - entry.ReqBytes = hb.n - entry.ReqSHA256 = hex.EncodeToString(hb.h.Sum(nil)) + // A body hash is only trustworthy after EOF. If a peer responds before the + // body is drained, report the metadata as unavailable rather than signing a + // hash of a prefix as if it represented the complete request. + if hb == nil { + empty := sha256.Sum256(nil) + entry.ReqSHA256 = hex.EncodeToString(empty[:]) + } else { + bytes, digest, complete := hb.snapshot() + if complete { + entry.ReqBytes = bytes + entry.ReqSHA256 = digest + } else { + entry.Note = "request-body-incomplete: byte count and hash unavailable" + } + } if err != nil { - entry.Note = "transport-error: " + err.Error() + entry.Note = appendNote(entry.Note, "transport-error: "+err.Error()) } else if resp != nil { entry.RespStatus = resp.StatusCode entry.RespBytes = resp.ContentLength @@ -145,3 +214,10 @@ func (t *receiptTransport) RoundTrip(req *http.Request) (*http.Response, error) rec.Record(entry) return resp, err } + +func appendNote(existing, next string) string { + if existing == "" { + return next + } + return existing + "; " + next +} diff --git a/internal/httpx/httpx_test.go b/internal/httpx/httpx_test.go index 704403f..2de6626 100644 --- a/internal/httpx/httpx_test.go +++ b/internal/httpx/httpx_test.go @@ -12,8 +12,15 @@ import ( "time" receipt "github.com/Quality-Max/qmax-receipt" + "github.com/coder/websocket" ) +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + // TestRecordsRequestHashAndSize drives a real request through the recording // client and asserts the receipt captured the method, category, byte size and // content hash — and never the content itself. @@ -88,6 +95,61 @@ func TestTransportErrorStillRecorded(t *testing.T) { } } +func TestIncompleteRequestBodyIsNotSignedAsACompleteHash(t *testing.T) { + rec := receipt.NewCurrent("test:early-response") + req, err := NewRequest(context.Background(), http.MethodPost, "http://example.test/api/projects", io.NopCloser(strings.NewReader("abcdef"))) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + + transport := &receiptTransport{base: roundTripperFunc(func(req *http.Request) (*http.Response, error) { + buf := make([]byte, 3) + if _, err := req.Body.Read(buf); err != nil { + t.Fatalf("read partial body: %v", err) + } + return &http.Response{StatusCode: http.StatusRequestEntityTooLarge, Body: io.NopCloser(strings.NewReader(""))}, nil + })} + if _, err := transport.RoundTrip(req); err != nil { + t.Fatalf("RoundTrip: %v", err) + } + + if rec.EntryCount() != 1 { + t.Fatalf("entries = %d, want 1", rec.EntryCount()) + } + e := rec.Entries[0] + if e.ReqBytes != 0 || e.ReqSHA256 != "" { + t.Errorf("incomplete metadata = bytes:%d hash:%q, want unavailable", e.ReqBytes, e.ReqSHA256) + } + if !strings.Contains(e.Note, "request-body-incomplete") { + t.Errorf("note = %q, want incomplete-body marker", e.Note) + } +} + +func TestWebSocketHandshakeIsRecorded(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + conn, err := websocket.Accept(w, r, nil) + if err == nil { + _ = conn.Close(websocket.StatusNormalClosure, "") + } + })) + defer srv.Close() + + rec := receipt.NewCurrent("test:vnc") + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + conn, _, err := DialWebSocket(context.Background(), wsURL, nil) + if err != nil { + t.Fatalf("DialWebSocket: %v", err) + } + defer conn.Close(websocket.StatusNormalClosure, "") + + if rec.EntryCount() != 1 { + t.Fatalf("entries = %d, want 1", rec.EntryCount()) + } + if got := rec.Entries[0].Category; got != "vnc-control" { + t.Errorf("category = %q, want vnc-control", got) + } +} + // TestFinalizeAndVerifyRoundTrip proves qmax-code can produce a signed receipt // and verify it offline through the shared module. func TestFinalizeAndVerifyRoundTrip(t *testing.T) { diff --git a/internal/setup/interactive.go b/internal/setup/interactive.go index b3e687d..242be54 100644 --- a/internal/setup/interactive.go +++ b/internal/setup/interactive.go @@ -144,9 +144,10 @@ func LoginViaBrowser() (*api.AuthConfig, error) { return nil, fmt.Errorf("timed out waiting for browser authorization") } -// RunInteractiveSetup guides a first-time user through login and project selection. -// Returns the api.AuthConfig and selected project ID. -func RunInteractive() (*api.AuthConfig, int) { +// RunInteractive guides a first-time user through login and project selection. +// It returns failures to main so the session receipt can be finalized before +// the process exits. +func RunInteractive() (*api.AuthConfig, int, error) { fmt.Println() tui.AnimateMax(tui.MoodWaving, tui.GetMaxGreeting()) fmt.Println() @@ -184,7 +185,7 @@ func RunInteractive() (*api.AuthConfig, int) { tui.AnimateMax(tui.MoodSad, "Login failed: "+err.Error()) fmt.Println() fmt.Println(" Try again with: qmax-code login") - os.Exit(1) + return nil, 0, fmt.Errorf("interactive login: %w", err) } // Show success @@ -266,7 +267,7 @@ func RunInteractive() (*api.AuthConfig, int) { fmt.Println(" \"review the latest PR for security issues\"") fmt.Println() - return auth, projectID + return auth, projectID, nil } // loginWithKeyPrompt asks the user to paste their API key. diff --git a/internal/sysutil/error_reporting.go b/internal/sysutil/error_reporting.go index 2a31e09..2f65a07 100644 --- a/internal/sysutil/error_reporting.go +++ b/internal/sysutil/error_reporting.go @@ -8,6 +8,7 @@ import ( "github.com/getsentry/sentry-go" + "github.com/qualitymax/qmax-code/internal/httpx" "github.com/qualitymax/qmax-code/internal/security" ) @@ -28,6 +29,8 @@ var telemetryDeniedTagPrefixes = []string{ "response", "body", "text", "data", } +var initSentry = sentry.Init + // InitErrorReporting sets up Sentry-compatible error reporting when explicitly // enabled via QMAX_CODE_TELEMETRY=1 and QMAX_CODE_TELEMETRY_DSN. Public builds // must not report anything unless the user opts in. version is reported as the @@ -42,12 +45,15 @@ func InitErrorReporting(version string) { return } - err := sentry.Init(sentry.ClientOptions{ + err := initSentry(sentry.ClientOptions{ Dsn: dsn, Release: fmt.Sprintf("qmax-code@%s", version), Environment: "production", AttachStacktrace: true, BeforeSend: sanitizeEvent, + // Sentry's buffered transport uses this client for envelope uploads, + // keeping opt-in telemetry in the session Exposure Receipt. + HTTPClient: httpx.NewClient(10 * time.Second), }) if err != nil { // Silently fail — error reporting is best-effort diff --git a/internal/sysutil/error_reporting_test.go b/internal/sysutil/error_reporting_test.go new file mode 100644 index 0000000..38d4310 --- /dev/null +++ b/internal/sysutil/error_reporting_test.go @@ -0,0 +1,58 @@ +package sysutil + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + receipt "github.com/Quality-Max/qmax-receipt" + "github.com/getsentry/sentry-go" +) + +func TestErrorReportingUsesReceiptClient(t *testing.T) { + oldInit := initSentry + t.Cleanup(func() { initSentry = oldInit }) + t.Setenv(telemetryEnabledEnv, "1") + t.Setenv(telemetryDSNEnv, "https://public@example.test/1") + + var client *http.Client + initSentry = func(options sentry.ClientOptions) error { + client = options.HTTPClient + return nil + } + InitErrorReporting("test") + if client == nil { + t.Fatal("InitErrorReporting did not configure an HTTP client") + } + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.Copy(io.Discard, r.Body) + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + rec := receipt.NewCurrent("test:telemetry") + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, srv.URL+"/api/42/envelope/", nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + resp, err := client.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + resp.Body.Close() + + deadline := time.Now().Add(time.Second) + for rec.EntryCount() == 0 && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } + if rec.EntryCount() != 1 { + t.Fatalf("entries = %d, want 1", rec.EntryCount()) + } + if got := rec.Entries[0].Category; got != "telemetry" { + t.Errorf("category = %q, want telemetry", got) + } +} diff --git a/internal/vnc/stream.go b/internal/vnc/stream.go index 3cfc8ee..99bd4da 100644 --- a/internal/vnc/stream.go +++ b/internal/vnc/stream.go @@ -1,13 +1,9 @@ package vnc -// EGRESS CARVE-OUT (QUA-1316): this package is the sole non-httpx egress path -// in qmax-code. It opens a WebSocket (coder/websocket) to the cloud-sandbox -// noVNC endpoint to stream the live browser framebuffer. This traffic carries -// rendered pixels (screenshots), never source code, prompts, or API keys, so it -// is outside the Exposure Receipt's content-accountability scope. The static -// egress guard (internal/httpx/guard_test.go) allowlists this package -// explicitly — any *new* WebSocket or raw-socket egress must be reviewed and -// added to the carve-out list, not silently merged. +// VNC connects to the cloud-sandbox noVNC endpoint through httpx, which +// records its WebSocket handshake as vnc-control traffic in the Exposure +// Receipt. The upgraded channel carries only VNC framing and framebuffer data; +// no application payload is sent over it by qmax-code. // // Minimal RFB 3.8 client over WebSocket, tailored for QM Cloud Sandbox // noVNC endpoints. Decodes Raw + CopyRect into a 32-bit BGRX framebuffer @@ -28,6 +24,7 @@ import ( "time" "github.com/coder/websocket" + "github.com/qualitymax/qmax-code/internal/httpx" ) // VNCFrame is a snapshot of the framebuffer after one or more updates. @@ -89,13 +86,13 @@ func DialVNC(ctx context.Context, rawURL string, fps int) (*VNCStream, error) { // dialOnce attempts a single WebSocket upgrade (subprotocol then bare). // Returns the live conn or (nil, err) on hard failure. dialOnce := func(dctx context.Context) (*websocket.Conn, error) { - c, _, e := websocket.Dial(dctx, wsURL, &websocket.DialOptions{ + c, _, e := httpx.DialWebSocket(dctx, wsURL, &websocket.DialOptions{ Subprotocols: []string{"binary"}, }) if e == nil { return c, nil } - c, _, e2 := websocket.Dial(dctx, wsURL, nil) + c, _, e2 := httpx.DialWebSocket(dctx, wsURL, nil) if e2 == nil { return c, nil } diff --git a/main.go b/main.go index 22001fc..de06856 100644 --- a/main.go +++ b/main.go @@ -47,11 +47,6 @@ func main() { return } - // Initialize error reporting only when explicitly enabled. - sysutil.InitErrorReporting(Version) - defer sysutil.FlushErrorReporting() - defer sysutil.RecoverPanic() - // Exposure Receipt: record every outbound request this session makes (LLM // prompts, cloud-API calls, integration connects) into a signed, offline- // verifiable manifest under ~/.qmax-code/receipts/. Installed before any @@ -66,8 +61,22 @@ func main() { finalizeReceipt := func() { receiptOnce.Do(func() { finalizeSessionReceipt(sessionRec) }) } + // The receipt must remain active while telemetry flushes: that flush is an + // outbound HTTP request when opt-in error reporting is enabled. + finalizeForExit := func() { + sysutil.FlushErrorReporting() + finalizeReceipt() + } + configureExitFinalizer(finalizeForExit) defer finalizeReceipt() + // Initialize error reporting only when explicitly enabled. These defers are + // registered after the receipt finalizer, therefore they run first on a + // normal return (LIFO) and are included in the session receipt. + sysutil.InitErrorReporting(Version) + defer sysutil.FlushErrorReporting() + defer sysutil.RecoverPanic() + // `receipt` inspects prior manifests offline and does no egress of its own. if len(os.Args) > 1 && os.Args[1] == "receipt" { handleReceiptCommand(os.Args[2:]) @@ -82,7 +91,7 @@ func main() { return } fmt.Fprintln(os.Stderr, "Usage: qmax-code serve --mcp") - os.Exit(2) + exitWithReceipt(2) } // Handle "config" subcommand — lets users change persisted settings @@ -100,7 +109,7 @@ func main() { if len(os.Args) > 1 && os.Args[1] == "codex" { if err := handleCodexCommand(os.Args[2:]); err != nil { fmt.Fprintln(os.Stderr, "Error:", err) - os.Exit(1) + exitWithReceipt(1) } return } @@ -109,7 +118,7 @@ func main() { if len(os.Args) > 1 && os.Args[1] == "cc" { if err := handleCCCommand(os.Args[2:]); err != nil { fmt.Fprintln(os.Stderr, "Error:", err) - os.Exit(1) + exitWithReceipt(1) } return } @@ -135,7 +144,7 @@ func main() { if err != nil { tui.AnimateMax(tui.MoodSad, "Login failed: "+err.Error()) fmt.Fprintf(os.Stderr, "\n Try: qmax-code login --api-key qm-YOUR-API-KEY\n") - os.Exit(1) + exitWithReceipt(1) } tui.AnimateMax(tui.MoodHappy, fmt.Sprintf("Logged in as %s", cfg.Email)) return @@ -145,7 +154,7 @@ func main() { sessions, err := session.ListSessions(20) if err != nil || len(sessions) == 0 { fmt.Println("No sessions found.") - os.Exit(0) + exitWithReceipt(0) } fmt.Printf("%-10s %-18s %-6s %-8s %s\n", "ID", "Updated", "Turns", "Tokens", "Project") for _, s := range sessions { @@ -164,7 +173,7 @@ func main() { fmt.Fprintln(os.Stderr, " For non-interactive use, pass a prompt:") fmt.Fprintln(os.Stderr, " qmax-code -p \"\"") fmt.Fprintln(os.Stderr, " qmax-code \"\"") - os.Exit(2) + exitWithReceipt(2) } // Load persistent user config @@ -192,7 +201,7 @@ func main() { } default: fmt.Fprintf(os.Stderr, "Error: --backend must be cc, codex, cerebras, opencode, or api\n") - os.Exit(2) + exitWithReceipt(2) } } @@ -208,7 +217,7 @@ func main() { if !isValidModelName(effectiveModel) { fmt.Fprintf(os.Stderr, "Error: --model %q is not recognized.\n", *model) fmt.Fprintf(os.Stderr, " Valid: %s.\n", api.ValidClaudeModelsHelp()) - os.Exit(2) + exitWithReceipt(2) } // Resolve Anthropic API key: flag > env > keychain @@ -240,7 +249,11 @@ func main() { // If no qmax CLI and no API client, run full interactive setup if qmaxBin == "" && apiClient == nil { - setupAuth, setupProjectID := setup.RunInteractive() + setupAuth, setupProjectID, setupErr := setup.RunInteractive() + if setupErr != nil { + fmt.Fprintln(os.Stderr, "Setup failed:", setupErr) + exitWithReceipt(1) + } auth = setupAuth apiClient = api.NewAPIClient(auth) appConfig.DefaultProject = setupProjectID @@ -264,7 +277,7 @@ func main() { fmt.Fprintln(os.Stderr, "\nError: backend=cc but 'claude' CLI was not found.") fmt.Fprintln(os.Stderr, " Install Claude Code: https://claude.ai/download") fmt.Fprintln(os.Stderr, " Or switch backend: qmax-code config set backend api") - os.Exit(1) + exitWithReceipt(1) } consent := setup.PromptOrchConsent(appConfig, "cc") if !consent.Proceed { @@ -283,7 +296,7 @@ func main() { fmt.Fprintln(os.Stderr, "\nError: backend=codex but 'codex' CLI was not found.") fmt.Fprintln(os.Stderr, " Install Codex CLI: npm install -g @openai/codex") fmt.Fprintln(os.Stderr, " Or switch backend: qmax-code config set backend api") - os.Exit(1) + exitWithReceipt(1) } consent := setup.PromptOrchConsent(appConfig, "codex") if !consent.Proceed { @@ -324,7 +337,7 @@ func main() { fmt.Fprintln(os.Stderr, "\nError: Cerebras API key required for backend=cerebras.") fmt.Fprintln(os.Stderr, " export CEREBRAS_API_KEY=csk-...") fmt.Fprintln(os.Stderr, " Or switch backend: qmax-code config set backend api") - os.Exit(1) + exitWithReceipt(1) } anthropicKey = "__cerebras_mode__" // skip Anthropic key gate below } else if cliBackend == "opencode" { @@ -333,7 +346,7 @@ func main() { fmt.Fprintln(os.Stderr, "\nError: backend=opencode but 'opencode' CLI was not found.") fmt.Fprintln(os.Stderr, " Install opencode: https://opencode.ai (curl -fsSL https://opencode.ai/install | bash)") fmt.Fprintln(os.Stderr, " Or switch backend: qmax-code config set backend api") - os.Exit(1) + exitWithReceipt(1) } consent := setup.PromptOrchConsent(appConfig, "opencode") if !consent.Proceed { @@ -369,7 +382,7 @@ func main() { fmt.Fprintln(os.Stderr, " Or use a CLI backend (no API key needed):") fmt.Fprintln(os.Stderr, " qmax-code config set backend cc # Claude Code login / Agent SDK credit") fmt.Fprintln(os.Stderr, " qmax-code config set backend codex # OpenAI/Codex subscription") - os.Exit(1) + exitWithReceipt(1) } // Detect project from cwd if not set via flag; fall back to saved config @@ -531,7 +544,7 @@ func main() { if *oneShot != "" { if err := runOneShot(*oneShot); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } return } @@ -540,7 +553,7 @@ func main() { if remaining := flag.Args(); len(remaining) > 0 { if err := runOneShot(strings.Join(remaining, " ")); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + exitWithReceipt(1) } return } @@ -556,7 +569,7 @@ func main() { } if loadErr != nil { fmt.Fprintf(os.Stderr, "Cannot resume session %q: %v\n", *resumeID, loadErr) - os.Exit(1) + exitWithReceipt(1) } ag.History = sess.Messages ag.Usage = sess.Usage @@ -575,7 +588,7 @@ func main() { agent.CleanupStaleMCPConfigs() // Interactive REPL - repl.Run(ag, cliAgent, *quiet, Version, finalizeReceipt) + repl.Run(ag, cliAgent, *quiet, Version, finalizeForExit) } // resolveModel expands shorthand model names to full model IDs. From e3936ce9e1b0b224a3f02b356eb8f5da17c81976 Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Wed, 15 Jul 2026 23:32:20 +0200 Subject: [PATCH 2/4] fix(receipt): address egress review findings --- exit_test.go | 46 ++++++++++++++++++++- internal/httpx/guard_test.go | 23 +++++++++++ internal/httpx/httpx.go | 11 ++++- internal/httpx/httpx_test.go | 79 ++++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 2 deletions(-) diff --git a/exit_test.go b/exit_test.go index 7b83338..54aba85 100644 --- a/exit_test.go +++ b/exit_test.go @@ -1,6 +1,10 @@ package main -import "testing" +import ( + "sync" + "sync/atomic" + "testing" +) func TestExitWithReceiptFinalizesBeforeProcessExit(t *testing.T) { exitMu.Lock() @@ -28,3 +32,43 @@ func TestExitWithReceiptFinalizesBeforeProcessExit(t *testing.T) { t.Fatalf("order = %v, want [finalize exit]", order) } } + +func TestExitWithReceiptConcurrentFinalizerReconfiguration(t *testing.T) { + exitMu.Lock() + oldFinalize := finalizeBeforeExit + oldProcessExit := processExit + processExit = func(int) {} + exitMu.Unlock() + t.Cleanup(func() { + exitMu.Lock() + finalizeBeforeExit = oldFinalize + processExit = oldProcessExit + exitMu.Unlock() + }) + + var finalizations atomic.Int32 + configureExitFinalizer(func() { finalizations.Add(1) }) + + const calls = 100 + start := make(chan struct{}) + var wg sync.WaitGroup + for range calls { + wg.Add(2) + go func() { + defer wg.Done() + <-start + configureExitFinalizer(func() { finalizations.Add(1) }) + }() + go func() { + defer wg.Done() + <-start + exitWithReceipt(0) + }() + } + close(start) + wg.Wait() + + if got := finalizations.Load(); got != calls { + t.Errorf("finalizations = %d, want %d", got, calls) + } +} diff --git a/internal/httpx/guard_test.go b/internal/httpx/guard_test.go index 940983a..c57cc70 100644 --- a/internal/httpx/guard_test.go +++ b/internal/httpx/guard_test.go @@ -66,6 +66,9 @@ func scanFileForEgressViolations(file *ast.File, fset *token.FileSet, rel string name = spec.Name.Name } imports[name] = path + if name == "." && (path == "net/http" || path == "net" || path == "github.com/coder/websocket") { + violations = append(violations, violation(rel, fset, spec.Pos(), "dot-import of egress package "+path)) + } if forbiddenLibraryImports[path] { violations = append(violations, violation(rel, fset, spec.Pos(), "forbidden egress library "+path)) } @@ -180,3 +183,23 @@ func f() { t.Fatalf("violations = %d, want 4: %v", len(violations), violations) } } + +func TestEgressGuardRejectsDotImports(t *testing.T) { + dir := t.TempDir() + bad := `package bad + +import . "net/http" + +var client = Client{} +` + if err := os.WriteFile(filepath.Join(dir, "bad.go"), []byte(bad), 0o644); err != nil { + t.Fatal(err) + } + violations, err := scanForEgressViolations(dir) + if err != nil { + t.Fatalf("scan: %v", err) + } + if len(violations) != 1 || !strings.Contains(violations[0], "dot-import") { + t.Fatalf("violations = %v, want dot-import violation", violations) + } +} diff --git a/internal/httpx/httpx.go b/internal/httpx/httpx.go index 7db1b10..a805716 100644 --- a/internal/httpx/httpx.go +++ b/internal/httpx/httpx.go @@ -66,6 +66,8 @@ type modelKey struct{} // alone cannot identify their purpose (for example a WebSocket handshake). type categoryKey struct{} +const webSocketHandshakeTimeout = 15 * time.Second + // WithModel annotates ctx with the LLM model id for requests built from it. // LLM call sites (Anthropic, Cerebras, Ollama) wrap their context with this so // the receipt records model attribution; all other traffic omits it. @@ -122,7 +124,11 @@ func DialWebSocket(ctx context.Context, url string, opts *websocket.DialOptions) dialOpts = *opts dialOpts.Subprotocols = append([]string(nil), opts.Subprotocols...) } - dialOpts.HTTPClient = NewClient(0) + // coder/websocket converts this client timeout into a handshake context + // deadline, then clears the client timeout before returning the upgraded + // connection. This bounds a stalled upgrade without imposing a lifetime on + // the VNC stream itself. + dialOpts.HTTPClient = NewClient(webSocketHandshakeTimeout) return websocket.Dial(ctx, url, &dialOpts) } @@ -216,6 +222,9 @@ func (t *receiptTransport) RoundTrip(req *http.Request) (*http.Response, error) } func appendNote(existing, next string) string { + if next == "" { + return existing + } if existing == "" { return next } diff --git a/internal/httpx/httpx_test.go b/internal/httpx/httpx_test.go index 2de6626..5796f24 100644 --- a/internal/httpx/httpx_test.go +++ b/internal/httpx/httpx_test.go @@ -21,6 +21,19 @@ func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } +type failingReadCloser struct{ read bool } + +func (b *failingReadCloser) Read(p []byte) (int, error) { + if b.read { + return 0, io.ErrUnexpectedEOF + } + b.read = true + copy(p, "abc") + return 3, io.ErrUnexpectedEOF +} + +func (b *failingReadCloser) Close() error { return nil } + // TestRecordsRequestHashAndSize drives a real request through the recording // client and asserts the receipt captured the method, category, byte size and // content hash — and never the content itself. @@ -125,6 +138,72 @@ func TestIncompleteRequestBodyIsNotSignedAsACompleteHash(t *testing.T) { } } +func TestRequestBodyReadErrorIsRecordedAsIncomplete(t *testing.T) { + rec := receipt.NewCurrent("test:body-read-error") + req, err := NewRequest(context.Background(), http.MethodPost, "http://example.test/api/projects", &failingReadCloser{}) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + + transport := &receiptTransport{base: roundTripperFunc(func(req *http.Request) (*http.Response, error) { + buf := make([]byte, 8) + _, err := req.Body.Read(buf) + return nil, err + })} + if _, err := transport.RoundTrip(req); err == nil { + t.Fatal("RoundTrip error = nil, want body read error") + } + + if rec.EntryCount() != 1 { + t.Fatalf("entries = %d, want 1", rec.EntryCount()) + } + e := rec.Entries[0] + if e.ReqBytes != 0 || e.ReqSHA256 != "" { + t.Errorf("incomplete metadata = bytes:%d hash:%q, want unavailable", e.ReqBytes, e.ReqSHA256) + } + if !strings.Contains(e.Note, "request-body-incomplete") || !strings.Contains(e.Note, "transport-error") { + t.Errorf("note = %q, want incomplete-body and transport-error markers", e.Note) + } +} + +func TestRequestWithoutReceiptContextIsRecorded(t *testing.T) { + req, err := NewRequest(context.Background(), http.MethodGet, "http://example.test/api/projects", nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + rec := receipt.FromContext(req.Context()) + before := rec.EntryCount() + transport := &receiptTransport{base: roundTripperFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(""))}, nil + })} + if _, err := transport.RoundTrip(req); err != nil { + t.Fatalf("RoundTrip: %v", err) + } + if got := rec.EntryCount(); got != before+1 { + t.Errorf("entries = %d, want %d", got, before+1) + } +} + +func TestAppendNote(t *testing.T) { + tests := []struct { + existing string + next string + want string + }{ + {want: ""}, + {next: "next", want: "next"}, + {existing: "existing", want: "existing"}, + {existing: "first", next: "second", want: "first; second"}, + } + for _, tt := range tests { + t.Run(tt.want, func(t *testing.T) { + if got := appendNote(tt.existing, tt.next); got != tt.want { + t.Errorf("appendNote(%q, %q) = %q, want %q", tt.existing, tt.next, got, tt.want) + } + }) + } +} + func TestWebSocketHandshakeIsRecorded(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { conn, err := websocket.Accept(w, r, nil) From c01e3d5c99c1db294ccd28f92958502940d82367 Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Wed, 15 Jul 2026 23:47:26 +0200 Subject: [PATCH 3/4] fix(receipt): redact transport error notes --- internal/httpx/httpx.go | 10 +++++++++- internal/httpx/httpx_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/internal/httpx/httpx.go b/internal/httpx/httpx.go index a805716..4759aff 100644 --- a/internal/httpx/httpx.go +++ b/internal/httpx/httpx.go @@ -22,6 +22,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "fmt" "hash" "io" "net/http" @@ -212,7 +213,7 @@ func (t *receiptTransport) RoundTrip(req *http.Request) (*http.Response, error) } } if err != nil { - entry.Note = appendNote(entry.Note, "transport-error: "+err.Error()) + entry.Note = appendNote(entry.Note, transportErrorNote(err)) } else if resp != nil { entry.RespStatus = resp.StatusCode entry.RespBytes = resp.ContentLength @@ -221,6 +222,13 @@ func (t *receiptTransport) RoundTrip(req *http.Request) (*http.Response, error) return resp, err } +// transportErrorNote records a stable diagnostic category without copying an +// arbitrary transport error into a customer-held receipt. Error strings may +// contain URLs, proxy details, or credentials supplied by a custom transport. +func transportErrorNote(err error) string { + return fmt.Sprintf("transport-error: %T", err) +} + func appendNote(existing, next string) string { if next == "" { return existing diff --git a/internal/httpx/httpx_test.go b/internal/httpx/httpx_test.go index 5796f24..9c70e09 100644 --- a/internal/httpx/httpx_test.go +++ b/internal/httpx/httpx_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "errors" "io" "net/http" "net/http/httptest" @@ -108,6 +109,32 @@ func TestTransportErrorStillRecorded(t *testing.T) { } } +func TestTransportErrorNoteExcludesErrorText(t *testing.T) { + rec := receipt.NewCurrent("test:private-transport-error") + req, err := NewRequest(context.Background(), http.MethodGet, "http://example.test/api/projects", nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + transportErr := errors.New("not-for-receipt") + transport := &receiptTransport{base: roundTripperFunc(func(*http.Request) (*http.Response, error) { + return nil, transportErr + })} + if _, err := transport.RoundTrip(req); !errors.Is(err, transportErr) { + t.Fatalf("RoundTrip error = %v, want %v", err, transportErr) + } + + if rec.EntryCount() != 1 { + t.Fatalf("entries = %d, want 1", rec.EntryCount()) + } + note := rec.Entries[0].Note + if !strings.HasPrefix(note, "transport-error: ") { + t.Errorf("note = %q, want transport error category", note) + } + if strings.Contains(note, transportErr.Error()) { + t.Errorf("note = %q, must not include the transport error text", note) + } +} + func TestIncompleteRequestBodyIsNotSignedAsACompleteHash(t *testing.T) { rec := receipt.NewCurrent("test:early-response") req, err := NewRequest(context.Background(), http.MethodPost, "http://example.test/api/projects", io.NopCloser(strings.NewReader("abcdef"))) From fed30eef05cd7017237e6f2ddb5c0bc1ab7ec607 Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Wed, 15 Jul 2026 23:57:54 +0200 Subject: [PATCH 4/4] fix(receipt): make transport notes opaque --- internal/httpx/httpx.go | 11 +++++------ internal/httpx/httpx_test.go | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/internal/httpx/httpx.go b/internal/httpx/httpx.go index 4759aff..a66549a 100644 --- a/internal/httpx/httpx.go +++ b/internal/httpx/httpx.go @@ -22,7 +22,6 @@ import ( "context" "crypto/sha256" "encoding/hex" - "fmt" "hash" "io" "net/http" @@ -222,11 +221,11 @@ func (t *receiptTransport) RoundTrip(req *http.Request) (*http.Response, error) return resp, err } -// transportErrorNote records a stable diagnostic category without copying an -// arbitrary transport error into a customer-held receipt. Error strings may -// contain URLs, proxy details, or credentials supplied by a custom transport. -func transportErrorNote(err error) string { - return fmt.Sprintf("transport-error: %T", err) +// transportErrorNote deliberately omits arbitrary transport details. Error +// strings and concrete types can disclose URLs, proxy configuration, or +// implementation details in a customer-held receipt. +func transportErrorNote(_ error) string { + return "transport-error" } func appendNote(existing, next string) string { diff --git a/internal/httpx/httpx_test.go b/internal/httpx/httpx_test.go index 9c70e09..c76016b 100644 --- a/internal/httpx/httpx_test.go +++ b/internal/httpx/httpx_test.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "errors" "io" + "net" "net/http" "net/http/httptest" "strings" @@ -127,8 +128,8 @@ func TestTransportErrorNoteExcludesErrorText(t *testing.T) { t.Fatalf("entries = %d, want 1", rec.EntryCount()) } note := rec.Entries[0].Note - if !strings.HasPrefix(note, "transport-error: ") { - t.Errorf("note = %q, want transport error category", note) + if note != "transport-error" { + t.Errorf("note = %q, want generic transport error category", note) } if strings.Contains(note, transportErr.Error()) { t.Errorf("note = %q, must not include the transport error text", note) @@ -256,6 +257,37 @@ func TestWebSocketHandshakeIsRecorded(t *testing.T) { } } +func TestFailedWebSocketHandshakeIsRecorded(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + addr := listener.Addr().String() + if err := listener.Close(); err != nil { + t.Fatalf("close listener: %v", err) + } + + rec := receipt.NewCurrent("test:vnc-failed-handshake") + conn, _, err := DialWebSocket(context.Background(), "ws://"+addr, nil) + if err == nil { + if conn != nil { + _ = conn.Close(websocket.StatusNormalClosure, "") + } + t.Fatal("DialWebSocket error = nil, want connection failure") + } + + if rec.EntryCount() != 1 { + t.Fatalf("entries = %d, want 1", rec.EntryCount()) + } + e := rec.Entries[0] + if e.Category != "vnc-control" { + t.Errorf("category = %q, want vnc-control", e.Category) + } + if e.Note != "transport-error" { + t.Errorf("note = %q, want generic transport error", e.Note) + } +} + // TestFinalizeAndVerifyRoundTrip proves qmax-code can produce a signed receipt // and verify it offline through the shared module. func TestFinalizeAndVerifyRoundTrip(t *testing.T) {