diff --git a/.gitignore b/.gitignore index 7f616cf98..d073e9d90 100644 --- a/.gitignore +++ b/.gitignore @@ -39,3 +39,10 @@ devlog/**/security-advisory-draft* # Test-generated artifacts tests/.tmp-*/ .claude/ + +# Retired Go native-runtime experiment. `go/` is not part of the build, the +# typecheck, or the test path, and nothing in `src/` imports it. A single file +# from it (go/internal/cli/config_parity.go) has now been committed by a broad +# `git add` three separate times and reached `dev` once — see +# tests/repo-hygiene.test.ts, which fails if any path here becomes tracked again. +go/ diff --git a/AGENTS.md b/AGENTS.md index 5911d4b97..7f4fcfbd3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,6 +130,16 @@ with regression coverage in `tests/startup-prompt.test.ts`, another action that spends the user's identity, credits, or reputation, gate it the same way rather than relying on a prompt an agent can answer. +**Be clear about what that enforcement is and is not.** The management endpoint +requires a dashboard session, which stops the casual path — an agent that would +have POSTed there because the endpoint existed, and one holding only the admin +token. It is not a technical barrier against a determined local agent: a process +running as the user can mint its own session from the loopback dashboard +bootstrap, and can skip the proxy entirely by running `gh` itself. Every local +credential is equally reachable by both the browser and the agent, so no check +inside this process can tell them apart. The real boundary is the rule above, and +it binds you regardless of which mechanism is within reach. + ## Commands ```bash diff --git a/go/internal/cli/config_parity.go b/go/internal/cli/config_parity.go deleted file mode 100644 index dbbf24caf..000000000 --- a/go/internal/cli/config_parity.go +++ /dev/null @@ -1,682 +0,0 @@ -package cli - -import ( - "bytes" - "context" - "encoding/json" - "errors" - "fmt" - "io" - "os" - "path/filepath" - "sort" - "strings" - - "github.com/lidge-jun/opencodex-go/internal/config" -) - -const configUsage = `Usage: - ocx config [show] [--json] [--source] - ocx config get [--json] - ocx config set [--json] - ocx config unset [--json] - ocx config validate [path|-] [--json] - ocx config export - ocx config import --yes [--json]` - -// configDocument is the config as a generic tree, which is what a dot path -// walks. The typed struct cannot represent an arbitrary path. -type configDocument map[string]any - -// readConfigDocument loads the config file as a generic tree plus its -// diagnostics, mirroring readConfigDiagnostics: the config plus where it came -// from and, when the file could not be used, why. -type configDiagnostics struct { - document configDocument - source string - failure string - warnings []string - // order is the key sequence the document should print in. A Go map has - // none, and the oracle prints the order it parsed. - order documentOrder -} - -func readConfigDiagnostics() (configDiagnostics, error) { - path, err := configPath() - if err != nil { - return configDiagnostics{}, err - } - fallback := func(reason string) configDiagnostics { - // The oracle discards an unusable file and hands back defaults, so - // show/get/export never surface its contents. That matters beyond - // tidiness: exporting an unvalidated file would copy whatever - // credentials it holds into a new location. - return configDiagnostics{document: defaultConfigDocument(), source: "fallback", failure: reason, order: defaultDocumentOrder} - } - raw, readErr := os.ReadFile(path) - if readErr != nil { - if os.IsNotExist(readErr) { - return configDiagnostics{document: defaultConfigDocument(), source: "default", order: defaultDocumentOrder}, nil - } - return configDiagnostics{}, readErr - } - // A BOM is stripped the way the oracle does before parsing. - trimmed := strings.TrimPrefix(string(raw), "\ufeff") - var decoded any - if json.Unmarshal([]byte(trimmed), &decoded) != nil { - return fallback("invalid_json"), nil - } - record, isObject := decoded.(map[string]any) - if !isObject { - return fallback("invalid_json"), nil - } - // Degrade before validating: the oracle's schema drops these fields rather - // than rejecting, so a single bad optional value must not send an - // otherwise-good file to fallback. - warnings := degradeInvalidFields(configDocument(record)) - normalized, normalizeErr := normalizeConfigDocument(configDocument(record)) - if normalizeErr != nil { - return fallback(normalizeErr.Error()), nil - } - // The order comes from the SOURCE bytes, not the normalized map, so a - // user's own field sequence survives a round trip through show. - return configDiagnostics{document: normalized, source: "file", warnings: warnings, order: orderOfDocument([]byte(trimmed))}, nil -} - -// readConfigDocument is the common case: the effective config and its origin. -func readConfigDocument() (configDocument, string, error) { - diagnostics, err := readConfigDiagnostics() - if err != nil { - return nil, "", err - } - return diagnostics.document, diagnostics.source, nil -} - -// validateConfigDocument runs the same validation a write would, without -// persisting, so `set` and `import` can refuse an invalid candidate. -func validateConfigDocument(document configDocument) error { - // Structural rules the typed decode cannot express. A missing `providers` - // unmarshals to a nil map and a dangling `defaultProvider` decodes fine, - // so without these an import would write `"providers": null` that the - // oracle rejects outright. - providersValue, hasProviders := document["providers"] - if !hasProviders || providersValue == nil { - return usageError("", "schema_invalid: providers: Invalid input: expected record, received undefined") - } - providers, isObject := providersValue.(map[string]any) - if !isObject { - return usageError("", "schema_invalid: providers: Invalid input: expected record") - } - if selected, present := document["defaultProvider"]; present { - name, isString := selected.(string) - if !isString { - return usageError("", "schema_invalid: defaultProvider: expected string") - } - // No exemption for "openai": the oracle rejects it too when it is - // absent from providers. - if _, known := providers[name]; !known { - return usageError("", "schema_invalid: defaultProvider: defaultProvider must exist in providers") - } - } - encoded, err := json.Marshal(document) - if err != nil { - return err - } - // Decode ONTO the defaults, not onto a zero value. The oracle's schema - // supplies a hostname when the document omits one, so validating a - // zero-valued struct rejected ordinary TypeScript-written configs with - // "hostname: must not be blank" -- a config the TS CLI calls valid. - candidate := config.FreshInstall() - candidate.Providers = nil - candidate.Combos = nil - if err := json.Unmarshal(encoded, &candidate); err != nil { - return usageError("", "%s", err.Error()) - } - return candidate.Validate() -} - -// normalizeConfigDocument validates and returns the document with schema -// defaults MATERIALIZED, the way the oracle's validateConfigCandidate hands -// back a normalized config rather than the raw input. -// -// Without this, a file that legitimately omits `port` validates but then -// `config get port` reports the path as missing, even though the oracle -// resolves it to 10100. -// -// Defaults are layered UNDER the document rather than over it, so a key the -// user actually wrote always wins, and unknown members survive untouched. -func normalizeConfigDocument(document configDocument) (configDocument, error) { - if err := validateConfigDocument(document); err != nil { - return nil, err - } - base := map[string]any(defaultConfigDocument()) - for key, value := range document { - base[key] = value - } - return configDocument(base), nil -} - -// saveConfigDocument writes the VALIDATED GENERIC document, not a typed -// round-trip of it. -// -// Marshalling through config.Config loses any unknown member of a known -// nested object: the root and provider structs carry passthrough fields, but -// something like visionSidecar does not, so `config set port 13000` would -// silently delete visionSidecar.futureNested. Editing one key must never -// discard a setting the user wrote. -// -// The write mirrors config.Save's durability: private temp file in the same -// directory, fsync, atomic rename. -func saveConfigDocument(document configDocument) error { - path, err := configPath() - if err != nil { - return err - } - if err := validateConfigDocument(document); err != nil { - return err - } - encoded, err := json.MarshalIndent(map[string]any(document), "", " ") - if err != nil { - return err - } - encoded = append(encoded, '\n') - - dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o700); err != nil { - return fmt.Errorf("create config directory: %w", err) - } - temp, err := os.CreateTemp(dir, ".config-*.tmp") - if err != nil { - return fmt.Errorf("create temporary config: %w", err) - } - tempPath := temp.Name() - committed := false - defer func() { - _ = temp.Close() - if !committed { - _ = os.Remove(tempPath) - } - }() - if err := temp.Chmod(0o600); err != nil { - return fmt.Errorf("protect temporary config: %w", err) - } - if _, err := temp.Write(encoded); err != nil { - return fmt.Errorf("write temporary config: %w", err) - } - if err := temp.Sync(); err != nil { - return fmt.Errorf("sync temporary config: %w", err) - } - if err := temp.Close(); err != nil { - return fmt.Errorf("close temporary config: %w", err) - } - if err := os.Rename(tempPath, path); err != nil { - return fmt.Errorf("replace config: %w", err) - } - committed = true - return nil -} - -// readConfigInput reads a candidate from a file or, for "-", from stdin. -func readConfigInput(source string, stdin io.Reader) (configDocument, error) { - var raw []byte - var err error - if source == "-" { - if stdin == nil { - stdin = os.Stdin - } - raw, err = io.ReadAll(stdin) - } else { - raw, err = os.ReadFile(source) - } - if err != nil { - return nil, err - } - var decoded any - if json.Unmarshal([]byte(strings.TrimPrefix(string(raw), "\ufeff")), &decoded) != nil { - return nil, usageError("", "invalid JSON in %s", source) - } - record, isObject := decoded.(map[string]any) - if !isObject { - return nil, usageError("", "invalid JSON in %s", source) - } - return configDocument(record), nil -} - -// runConfigParity implements the oracle's config surface. The legacy -// fixed-key form stays reachable through runConfig for compatibility. -func runConfigParity(ctx context.Context, args []string, streams IO) error { - rest := append([]string{}, args...) - action := "show" - if len(rest) > 0 { - action = strings.ToLower(rest[0]) - rest = rest[1:] - } - wantsJSON := takeFlag(&rest, "--json") - - switch action { - case "show": - source := takeFlag(&rest, "--source") - if err := rejectArgs(rest, configUsage, false); err != nil { - return err - } - diagnostics, err := readConfigDiagnostics() - if err != nil { - return err - } - redacted, _ := redactConfigValue(map[string]any(diagnostics.document), "").(map[string]any) - if !source { - // show always prints JSON: the oracle passes true for wantsJson. - // It goes through the ordered marshaller so the printed sequence is - // the file's, not Go's map iteration order. - encoded, marshalErr := marshalDocumentInOrder(configDocument(redacted), diagnostics.order) - if marshalErr != nil { - return marshalErr - } - _, writeErr := fmt.Fprintln(streams.Out, string(encoded)) - return writeErr - } - // `error` is present either way, null on success, so a consumer can - // read one shape rather than test for the key. - var failure any - if diagnostics.failure != "" { - failure = diagnostics.failure - } - return printData(streams, map[string]any{ - "config": redacted, - "source": diagnostics.source, - "error": failure, - "warnings": warningList(diagnostics.warnings), - }, true, nil) - - case "get": - if len(rest) == 0 { - return usageError(configUsage, "config path is required") - } - path := rest[0] - rest = rest[1:] - if err := rejectArgs(rest, configUsage, false); err != nil { - return err - } - document, _, err := readConfigDocument() - if err != nil { - return err - } - value, err := getConfigPath(map[string]any(document), path) - if err != nil { - return err - } - segments, err := configPathSegments(path) - if err != nil { - return err - } - value = redactConfigValue(value, segments[len(segments)-1]) - if wantsJSON { - return printData(streams, value, true, nil) - } - text, err := formatConfigValue(value) - if err != nil { - return err - } - _, err = fmt.Fprintln(streams.Out, text) - return err - - case "set", "unset": - if len(rest) == 0 { - return usageError(configUsage, "config path and value are required") - } - path := rest[0] - rest = rest[1:] - var parsed any - if action == "set" { - if len(rest) == 0 { - return usageError(configUsage, "config path and value are required") - } - parsed = parseConfigValue(rest[0]) - rest = rest[1:] - } - if err := rejectArgs(rest, configUsage, false); err != nil { - return err - } - document, _, err := readConfigDocument() - if err != nil { - return err - } - if err := setConfigPath(map[string]any(document), path, parsed, action == "unset"); err != nil { - return err - } - if err := validateConfigDocument(document); err != nil { - return err - } - if err := saveConfigDocument(document); err != nil { - return err - } - var saved any - if action == "set" { - if value, getErr := getConfigPath(map[string]any(document), path); getErr == nil { - segments, _ := configPathSegments(path) - saved = redactConfigValue(value, segments[len(segments)-1]) - } - } - verb := "Set" - if action == "unset" { - verb = "Unset" - } - return printData(streams, map[string]any{"ok": true, "path": path, "value": saved}, - wantsJSON, []string{fmt.Sprintf("%s %s.", verb, path)}) - - case "validate": - source := "" - if len(rest) > 0 { - source = rest[0] - rest = rest[1:] - } - if err := rejectArgs(rest, configUsage, false); err != nil { - return err - } - document := configDocument{} - if source != "" { - loaded, err := readConfigInput(source, streams.In) - if err != nil { - return err - } - document = loaded - } else { - loaded, _, err := readConfigDocument() - if err != nil { - return err - } - document = loaded - } - if err := validateConfigDocument(document); err != nil { - // Invalid config is a reported result, not a crash: the oracle - // prints the reason and exits 1. - if printErr := printData(streams, map[string]any{"ok": false, "error": err.Error()}, - wantsJSON, []string{"Config is invalid: " + err.Error()}); printErr != nil { - return printErr - } - return errSilentFailure - } - reported := source - if reported == "" { - reported, _ = configPath() - } - return printData(streams, map[string]any{"ok": true, "source": reported}, - wantsJSON, []string{"Config is valid."}) - - case "export": - if len(rest) == 0 { - return usageError(configUsage, "export path is required") - } - target := rest[0] - rest = rest[1:] - if err := rejectArgs(rest, configUsage, false); err != nil { - return err - } - document, _, err := readConfigDocument() - if err != nil { - return err - } - // Export is a BACKUP, so it is deliberately not redacted -- a masked - // copy could not be imported back. It is written 0600 for that reason. - encoded, err := json.MarshalIndent(map[string]any(document), "", " ") - if err != nil { - return err - } - encoded = append(encoded, '\n') - if target == "-" { - _, err = streams.Out.Write(encoded) - return err - } - // WriteFile's mode applies only when it CREATES the file, so exporting - // over an existing world-readable path would leave credentials - // readable. Chmod unconditionally. - if err := os.WriteFile(target, encoded, 0o600); err != nil { - return err - } - if err := os.Chmod(target, 0o600); err != nil { - return fmt.Errorf("protect exported config: %w", err) - } - _, err = fmt.Fprintf(streams.Out, "Exported config to %s.\n", target) - return err - - case "import": - if len(rest) == 0 { - return usageError(configUsage, "import path is required") - } - source := rest[0] - rest = rest[1:] - yes := takeFlag(&rest, "--yes") - if !yes { - return usageError(configUsage, "import requires --yes") - } - if err := rejectArgs(rest, configUsage, false); err != nil { - return err - } - document, err := readConfigInput(source, streams.In) - if err != nil { - return err - } - if err := validateConfigDocument(document); err != nil { - return err - } - if err := saveConfigDocument(document); err != nil { - return err - } - return printData(streams, map[string]any{"ok": true, "source": source}, wantsJSON, - []string{fmt.Sprintf("Imported config from %s. Restart or run ocx sync if needed.", source)}) - } - return usageError(configUsage, "unknown config command %s", action) -} - -// errSilentFailure marks a failure the command has ALREADY reported, so Run -// exits non-zero without printing a second "Error:" line over the top of it. -var errSilentFailure = errors.New("reported failure") - -// defaultConfigDocument is the generic form of the built-in default config. -// -// The oracle answers an absent or unusable config with getDefaultConfig() -// rather than an empty object, so `validate` succeeds on a fresh home and -// `get providers.openai.adapter` resolves before the user has written anything. -func defaultConfigDocument() configDocument { - // Built from FreshInstall, then reconciled with the oracle's - // getDefaultConfig() SHAPE. - // - // The two are not the same document. Go's struct marshals hostname, debug - // and log that the oracle omits, and the oracle carries websockets:false - // that Go's zero value drops. Serving or persisting the Go shape would - // write a config the TypeScript CLI did not produce, so the extras are - // removed and the missing key restored. - defaults := config.FreshInstall() - encoded, err := json.Marshal(defaults) - if err != nil { - return configDocument{} - } - var document map[string]any - if json.Unmarshal(encoded, &document) != nil { - return configDocument{} - } - for _, goOnly := range []string{"hostname", "debug", "log", "streamMode"} { - delete(document, goOnly) - } - if _, present := document["websockets"]; !present { - document["websockets"] = false - } - return configDocument(document) -} - -// degradableFields are the schema entries the oracle declares with -// `.catch(undefined)`: an invalid value is DROPPED with a warning rather than -// rejecting the whole file, so one hand-edited typo cannot hide every provider -// and account the user has configured. -var degradableFields = map[string]string{ - "injectionModel": "a string", - "injectionEffort": "a string", - "streamMode": "a string", - "syncCodexSubagentDefaults": "a boolean", -} - -// degradeInvalidFields removes malformed optional fields and reports what it -// dropped, in the oracle's wording. -func degradeInvalidFields(document configDocument) []string { - warnings := []string{} - for _, field := range []string{"injectionModel", "injectionEffort", "streamMode", "syncCodexSubagentDefaults"} { - value, present := document[field] - if !present || value == nil { - continue - } - expected := degradableFields[field] - valid := false - switch typed := value.(type) { - case string: - valid = expected == "a string" - if field == "streamMode" && valid { - valid = typed == "auto" || typed == "legacy-tee" || typed == "eager-relay" - } - case bool: - valid = expected == "a boolean" - } - if !valid { - delete(document, field) - warnings = append(warnings, field+" ignored: expected "+expected) - } - } - return warnings -} - -// warningList renders warnings as a JSON array, empty rather than null when -// there are none. -func warningList(warnings []string) []any { - out := make([]any, 0, len(warnings)) - for _, warning := range warnings { - out = append(out, warning) - } - return out -} - -// documentOrder is the ordered form of a whole config document. -// -// A Go map has no key order and JSON.stringify preserves the one it parsed, so -// `config show` printed alphabetically where the oracle prints file order. The -// order is tracked beside the document rather than inside it, because every -// dot-path walk in this file relies on plain map lookup. -type documentOrder struct { - value orderedValue - ok bool -} - -// orderOfDocument records the key sequence, at every depth, from the source -// bytes. -func orderOfDocument(raw []byte) documentOrder { - value, err := decodeOrdered(raw) - if err != nil || value.kind != 'o' { - return documentOrder{} - } - return documentOrder{value: value, ok: true} -} - -// defaultDocumentOrder is the oracle's getDefaultConfig() literal order, used -// when there is no file to read an order from. -var defaultDocumentOrder = orderOfDocument([]byte(`{ - "port": 0, - "openaiProviderTierVersion": 0, - "providers": {"openai": {"adapter": "", "baseUrl": "", "authMode": "", "codexAccountMode": ""}}, - "defaultProvider": "", - "subagentModels": [], - "multiAgentGuidanceEnabled": false, - "websockets": false, - "codexAutoStart": false, - "codexShimAutoRestore": false -}`)) - -// marshalDocumentInOrder renders the document following the recorded key order -// at each level, appending any key the order does not mention in sorted order -// so the output stays deterministic. -func marshalDocumentInOrder(document configDocument, order documentOrder) ([]byte, error) { - var reference *orderedValue - if order.ok { - reference = &order.value - } - compact, err := orderedJSONBytes(map[string]any(document), reference) - if err != nil { - return nil, err - } - var indented bytes.Buffer - if err := json.Indent(&indented, compact, "", " "); err != nil { - return nil, err - } - return indented.Bytes(), nil -} - -// orderedJSONBytes serializes value, taking key order from reference when the -// two line up and falling back to sorted keys when they do not. -func orderedJSONBytes(value any, reference *orderedValue) ([]byte, error) { - record, isObject := value.(map[string]any) - if !isObject { - if items, isArray := value.([]any); isArray { - out := []byte{'['} - for index, item := range items { - if index > 0 { - out = append(out, ',') - } - var childReference *orderedValue - if reference != nil && reference.kind == 'a' && index < len(reference.values) { - childReference = &reference.values[index] - } - encoded, err := orderedJSONBytes(item, childReference) - if err != nil { - return nil, err - } - out = append(out, encoded...) - } - return append(out, ']'), nil - } - return json.Marshal(jsSafe(value)) - } - - keys := make([]string, 0, len(record)) - seen := make(map[string]struct{}, len(record)) - if reference != nil && reference.kind == 'o' { - for _, key := range reference.keys { - if _, present := record[key]; present { - keys = append(keys, key) - seen[key] = struct{}{} - } - } - } - remaining := make([]string, 0, len(record)) - for key := range record { - if _, already := seen[key]; !already { - remaining = append(remaining, key) - } - } - sort.Strings(remaining) - keys = append(keys, remaining...) - - out := []byte{'{'} - for index, key := range keys { - if index > 0 { - out = append(out, ',') - } - encodedKey, err := json.Marshal(key) - if err != nil { - return nil, err - } - var childReference *orderedValue - if reference != nil && reference.kind == 'o' { - for position, candidate := range reference.keys { - if candidate == key { - childReference = &reference.values[position] - break - } - } - } - encodedValue, err := orderedJSONBytes(record[key], childReference) - if err != nil { - return nil, err - } - out = append(out, encodedKey...) - out = append(out, ':') - out = append(out, encodedValue...) - } - return append(out, '}'), nil -} diff --git a/src/lib/bounded-body.ts b/src/lib/bounded-body.ts index 3ff852246..745833c79 100644 --- a/src/lib/bounded-body.ts +++ b/src/lib/bounded-body.ts @@ -39,6 +39,19 @@ export interface BoundedBodyResult { const TOTAL_TIMEOUT = Symbol("bounded body total timeout"); const INACTIVITY_TIMEOUT = Symbol("bounded body inactivity timeout"); +/** + * Test-only instrumentation: how many times the retained buffer was reallocated + * during the most recent read. The accumulator grows geometrically, so this is + * logarithmic in the body size and independent of how many chunks the peer sends. + * The per-chunk array it replaced retained one object per chunk instead, which a + * fragmenting peer can inflate far past the payload ceiling — a property no + * correctness assertion can see, which is why it is observable here. + */ +let bufferGrowthsForTests = 0; +export function boundedBodyBufferGrowthsForTests(): number { + return bufferGrowthsForTests; +} + function timeoutPromise(ms: number, value: symbol): { promise: Promise; clear: () => void } { let timer: ReturnType | undefined; const promise = new Promise((resolve) => { @@ -105,6 +118,7 @@ export async function readBoundedResponseBody( // beyond the payload ceiling on large budgets. let retained = new Uint8Array(Math.min(maxBytes, 64 * 1024)); let retainedBytes = 0; + bufferGrowthsForTests = 0; let mustCancel = false; let cancelReason: unknown; const total = timeoutPromise(options.totalTimeoutMs ?? BOUNDED_BODY_TIMEOUT_MS, TOTAL_TIMEOUT); @@ -197,6 +211,7 @@ export async function readBoundedResponseBody( ); grown.set(retained.subarray(0, retainedBytes)); retained = grown; + bufferGrowthsForTests += 1; } retained.set(value, retainedBytes); retainedBytes += value.byteLength; diff --git a/src/server/index.ts b/src/server/index.ts index 0b1e323d8..5220a4d17 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -157,7 +157,7 @@ import { handleImages } from "./images"; import { handleLive, logLiveSidebandFrame, parseLiveSidebandTarget, resolveLiveSidebandUpgrade } from "./live"; import { handleSearch } from "./search"; import { fetchAllModels, handleManagementAPI, VERSION } from "./management-api"; -import { initializeManagementAuthState, issueGuiSession, requireManagementAuth } from "./management-auth"; +import { initializeManagementAuthState, issueGuiSession, managementPrincipal, requireManagementAuth } from "./management-auth"; const MAX_WS_FRAME_BYTES = 50 * 1024 * 1024; const WEBSOCKET_IDLE_TIMEOUT_SECONDS = 0; @@ -448,7 +448,11 @@ export function startServer(port?: number) { if (url.pathname.startsWith("/api/")) { const apiAuthError = requireManagementAuth(req, managementAuth, config); if (apiAuthError) return withManagementCors(apiAuthError, req, config); - const mgmtResponse = await handleManagementAPI(req, url, config); + // Which credential passed the gate, resolved from the same session table the + // gate used. Consent-bearing routes need this: request headers are forgeable + // by anything holding the admin token, the credential is not. + const principal = managementPrincipal(req, managementAuth, config) ?? undefined; + const mgmtResponse = await handleManagementAPI(req, url, config, {}, principal); if (mgmtResponse) return withManagementCors(mgmtResponse, req, config); return withManagementCors(formatErrorResponse(404, "not_found", `Unknown endpoint: ${req.method} ${url.pathname}`), req, config); } diff --git a/src/server/management-api.ts b/src/server/management-api.ts index ad5ebc896..af4b6bf9a 100644 --- a/src/server/management-api.ts +++ b/src/server/management-api.ts @@ -68,6 +68,7 @@ import { handleSystemRoutes } from "./management/system-routes"; import { handleSidebarRoutes } from "./management/sidebar-routes"; import { handleIntegrationRoutes } from "./management/integration-routes"; import type { ManagementContext } from "./management/context"; +import type { ManagementPrincipal } from "./management-auth"; export type { ManagementApiDeps } from "./management/context"; import { fetchAllModels } from "./management/shared"; import { CatalogGatherBusyError } from "../codex/catalog/provider-fetch"; @@ -82,7 +83,13 @@ export const VERSION = (() => { } })(); -export async function handleManagementAPI(req: Request, url: URL, config: OcxConfig, deps: ManagementApiDeps = {}): Promise { +export async function handleManagementAPI( + req: Request, + url: URL, + config: OcxConfig, + deps: ManagementApiDeps = {}, + principal?: ManagementPrincipal, +): Promise { if (!isAllowedManagementOrigin(req, config)) { return jsonResponse({ error: "cross-origin request blocked" }, 403, req, config); } @@ -125,7 +132,7 @@ export async function handleManagementAPI(req: Request, url: URL, config: OcxCon } } catch { /* best-effort */ } } - const ctx: ManagementContext = { req, url, config, deps, refreshCodexCatalogBestEffort, syncClaudeAgentDefsBestEffort }; + const ctx: ManagementContext = { req, url, config, deps, principal, refreshCodexCatalogBestEffort, syncClaudeAgentDefsBestEffort }; let routed: Response | null; try { routed = (await handleConfigRoutes(ctx)) diff --git a/src/server/management-auth.ts b/src/server/management-auth.ts index 813179636..1f1d72b53 100644 --- a/src/server/management-auth.ts +++ b/src/server/management-auth.ts @@ -237,6 +237,38 @@ export function issueGuiSession( return { token, ...session }; } +/** + * Which credential actually authorized a management request. + * + * `admin-token` is the raw token from disk/env: anything running as the user can + * read it, including a coding agent. `gui-session` is a session token this process + * minted for a browser, and it only authorizes a mutation after the origin and the + * per-session CSRF token match. Consent-bearing routes must key off this value + * rather than off request headers, which the token holder can forge freely. + */ +export type ManagementPrincipal = "admin-token" | "gui-session"; + +/** + * The principal for a request that already passed `requireManagementAuth`. Kept as a + * separate resolution (rather than a changed return type) so every existing caller + * keeps its `Response | null` contract; the value is derived from the same session + * table and the same CSRF comparison the gate uses, so the two cannot disagree. + */ +export function managementPrincipal( + req: Request, + state: ManagementAuthState, + config?: OcxConfig, +): ManagementPrincipal | null { + if (!state.available) return null; + const actual = req.headers.get("x-opencodex-api-key")?.trim() + || req.headers.get("authorization")?.replace(/^Bearer\s+/i, "").trim(); + if (!actual) return null; + if (equalSecret(actual, state.token)) return "admin-token"; + if (!config) return null; + removeExpiredSessions(state); + return state.sessions.has(actual) ? "gui-session" : null; +} + export function requireManagementAuth( req: Request, state: ManagementAuthState, diff --git a/src/server/management/context.ts b/src/server/management/context.ts index 61d37e7a7..aee70b9ee 100644 --- a/src/server/management/context.ts +++ b/src/server/management/context.ts @@ -1,5 +1,6 @@ import type { OcxConfig } from "../../types"; import type { StartupInstallAction } from "../startup-action-control"; +import type { ManagementPrincipal } from "../management-auth"; export interface ManagementApiDeps { toggleCodexMultiAgentV2?: (enabled: boolean) => void; @@ -26,6 +27,15 @@ export interface ManagementContext { url: URL; config: OcxConfig; deps: ManagementApiDeps; + /** + * Which credential authorized this request, resolved by the auth gate before + * dispatch. Routes that spend the USER's identity (not just the proxy's) must + * branch on this instead of on request headers: the admin token is readable by + * anything running as the user, so a token holder can forge any header a route + * might otherwise treat as browser evidence. Undefined only in direct-dispatch + * tests, which are treated as the untrusted `admin-token` case. + */ + principal?: ManagementPrincipal; refreshCodexCatalogBestEffort: () => Promise; syncClaudeAgentDefsBestEffort: () => Promise; } diff --git a/src/server/management/sidebar-routes.ts b/src/server/management/sidebar-routes.ts index fa9366918..8eb50e1c8 100644 --- a/src/server/management/sidebar-routes.ts +++ b/src/server/management/sidebar-routes.ts @@ -9,40 +9,54 @@ * this surface only learns the yes/no answer. `gh` writes the authenticated account * name to stderr, so that output is discarded at the source rather than forwarded. * - * The star POST additionally refuses agent-driven programmatic callers. Management - * auth proves the caller reached the admin token, not that a person chose to star: - * a coding agent runs on the user's machine and can read that token from disk, so - * the CLI's "ask the user" deferral would be bypassable with one `curl` here. + * The star POST additionally requires a dashboard session. Management auth proves + * the caller reached the admin token, not that a person chose to star: a coding + * agent runs on the user's machine and can read that token from disk, so the CLI's + * "ask the user" deferral would be bypassable with one `curl` here. * - * The dashboard button must keep working even when the proxy itself was started by - * an agent, which is the common case — the person is at the browser, not at the - * spawning shell. A GUI click is therefore distinguished by its browser session - * evidence (a same-origin `Origin` plus the minted CSRF header, both already - * verified by the management auth gate) rather than by the proxy's own env. + * The requirement is unconditional, and that is the point. It used to apply only + * when `isAgentDriven()` was true — but that function reads the SERVER's + * environment, not the caller's, so a proxy already running as a service (no agent + * markers, the normal remote setup) accepted a raw-token star from anyone who could + * read the token, which includes every agent on the machine. The provenance of the + * HTTP caller is not knowable from the server's env; only the credential is. So the + * mutation asks for a GUI session this process minted for a browser, which the auth + * gate accepts only after matching origin and the per-session CSRF token. */ import { jsonResponse } from "../auth-cors"; -import { agentDrivenMarkers, isAgentDriven } from "../../cli/agent-driven"; +import { agentDrivenMarkers } from "../../cli/agent-driven"; import type { ManagementContext } from "./context"; /** - * True when this request carries the browser-session evidence a dashboard click - * always has: an `Origin` (only a browser sends one) plus the per-session CSRF - * token, which `requireManagementAuth` has already matched against the minted - * session before dispatch. A shell/HTTP caller holding only the admin token has - * neither, which is exactly the case the agent guard is aimed at. + * True only when a minted GUI session authorized this request. + * + * The previous version of this check looked for an `Origin` plus the CSRF headers + * and reasoned that the auth gate had already validated them. It had not: the gate + * accepts a raw admin token BEFORE it ever consults the session table, so a caller + * holding that token (any process running as the user, a coding agent included) + * could add three nonempty headers of its choosing and satisfy this check without + * a browser ever being involved. The credential itself is the only part of the + * request an agent cannot fabricate, so that is what this now reads. */ -function hasBrowserSessionEvidence(req: Request): boolean { - return !!req.headers.get("Origin")?.trim() - && !!req.headers.get("x-opencodex-csrf-token")?.trim() - && !!req.headers.get("x-opencodex-gui-origin")?.trim(); +function hasBrowserSessionEvidence(ctx: ManagementContext): boolean { + return ctx.principal === "gui-session"; } -// Known edge, deliberately fail-closed: a non-loopback operator dashboard signs in -// with the raw admin token instead of a minted GUI session, so its clicks carry no -// CSRF header. If that proxy was *also* started from an agent shell, the button is -// refused and the response names the one-line `gh` command to run by hand. A -// service-run proxy (`OCX_SERVICE`, the usual remote setup) is not agent-driven and -// is unaffected. +// Known edge, deliberately fail-closed: a non-loopback operator dashboard that signs +// in with the raw admin token instead of a minted GUI session gets its click refused, +// and the response names the one-line `gh` command to run by hand. That is the +// correct trade — an endpoint reachable with a readable token cannot establish that +// a human chose to spend their own GitHub identity. +// +// The honest limit of this guard: a local process running AS THE USER can mint its +// own GUI session (the dashboard bootstrap is served to any loopback GET) and can +// equally just run `gh api -X PUT /user/starred/...` itself, which needs no proxy at +// all. No check inside this process can distinguish that caller from the browser, +// because both hold every local credential. So this endpoint is not a technical +// barrier against a determined local agent — it removes the CASUAL path (an agent +// that would have POSTed here because the endpoint existed) and makes the refusal +// legible. The actual boundary is normative and lives in AGENTS.md: an agent must +// not spend the user's identity, whichever mechanism is at hand. export async function handleSidebarRoutes(ctx: ManagementContext): Promise { const { req, url } = ctx; @@ -55,10 +69,11 @@ export async function handleSidebarRoutes(ctx: ManagementContext): Promise = new Promise(resolve => { - if (upstream.signal.aborted) resolve("aborted"); - else upstream.signal.addEventListener("abort", () => resolve("aborted"), { once: true }); - }); + // implementation's, not the stream's), so abort must break a parked read on + // a silent upstream. Cancelling the reader does that: the pending read + // settles and the loop observes the abort. This is deliberately NOT a + // shared `Promise.race([reader.read(), aborted])` companion — racing every + // read against one never-settled promise retains a reaction per chunk, and + // that is the exact retention class relay.ts avoids at its own drain. + const wakeParkedRead = () => { reader.cancel(upstream.signal.reason).catch(() => {}); }; + if (upstream.signal.aborted) wakeParkedRead(); + else upstream.signal.addEventListener("abort", wakeParkedRead, { once: true }); try { for (;;) { - const result = await Promise.race([reader.read(), aborted]); - if (result === "aborted") break; + const result = await reader.read(); const { done: upstreamDone, value } = result; + // A chunk that already settled is INSPECTED before abort is honored. A read + // can settle with a real chunk in the same tick the signal fires (post-cancel + // drain: the terminal frame arrives, then the drain timer aborts upstream). + // Checking the signal first discarded that frame, so the terminal was never + // recorded and the turn was accounted as a plain cancel. + if (!upstreamDone && value !== undefined) hooks.inspectChunk(value); + if (upstream.signal.aborted) break; if (upstreamDone) { hooks.finishInspection(); if (rewrite) { @@ -220,7 +228,6 @@ export function relaySseEagerBounded( } break; } - hooks.inspectChunk(value); if (cancelled) { // Discard-drain: inspection only, nothing queued. Stop at terminal // or when the bounded window expires. diff --git a/tests/bounded-body.test.ts b/tests/bounded-body.test.ts index 93ca2d5e7..41584c643 100644 --- a/tests/bounded-body.test.ts +++ b/tests/bounded-body.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import { BOUNDED_BODY_MAX_BYTES, + boundedBodyBufferGrowthsForTests, readBoundedResponseBody, } from "../src/lib/bounded-body"; @@ -105,6 +106,109 @@ describe("readBoundedResponseBody", () => { expect(cancelled).toBe(true); }); + /** + * The `maxBytes` option exists so one caller — the non-streaming upstream JSON + * read in responses/core.ts — can accept a whole completion (32 MiB ceiling) + * while the other eight callers keep the 64 KiB error-body default. Without + * these three tests the option was mutation-surviving: ignoring `maxBytes` + * entirely left the suite green, because the only oversize test used a body + * that exceeds BOTH ceilings. + */ + describe("an explicit maxBytes budget", () => { + const CUSTOM_CAP = BOUNDED_BODY_MAX_BYTES * 4; + + test("accepts a body larger than the default but within the custom cap", async () => { + const size = BOUNDED_BODY_MAX_BYTES * 2; + const response = responseFromChunks(new Uint8Array(size).fill(0x61)); + + const result = await readBoundedResponseBody(response, { maxBytes: CUSTOM_CAP }); + + expect(result.text.length).toBe(size); + expect(result.oversized).toBe(false); + expect(result.truncated).toBe(false); + expect(result.displaySafe).toBe(true); + }); + + test("accepts exactly the custom cap", async () => { + const response = responseFromChunks(new Uint8Array(CUSTOM_CAP).fill(0x61)); + + const result = await readBoundedResponseBody(response, { maxBytes: CUSTOM_CAP }); + + expect(result.text.length).toBe(CUSTOM_CAP); + expect(result.oversized).toBe(false); + }); + + test("rejects one byte past the custom cap and discards the prefix", async () => { + const response = responseFromChunks(new Uint8Array(CUSTOM_CAP + 1).fill(0x61)); + + const result = await readBoundedResponseBody(response, { maxBytes: CUSTOM_CAP }); + + expect(result.text).toBe(""); + expect(result.oversized).toBe(true); + expect(result.displaySafe).toBe(false); + }); + + test("a highly fragmented body under the cap is reassembled exactly", async () => { + // Guards the geometric single-buffer accumulation: the previous per-chunk + // array retained one object per transport chunk, which a peer can inflate + // far beyond the payload ceiling. Correctness here is the observable part — + // 20k one-byte chunks must still decode to exactly their content. + const chunkCount = 20_000; + const chunks = Array.from({ length: chunkCount }, () => new Uint8Array([0x61])); + const response = responseFromChunks(...chunks); + + const result = await readBoundedResponseBody(response, { maxBytes: CUSTOM_CAP }); + + expect(result.text.length).toBe(chunkCount); + expect(result.text).toBe("a".repeat(chunkCount)); + expect(result.oversized).toBe(false); + }); + + test("retention is logarithmic in the body, not linear in the chunk count", async () => { + // Growth accounting for the single buffer: it doubles a handful of times no + // matter how the peer fragments the body. This catches an exact-fit + // reallocation mutation; the per-chunk ARRAY shape is caught structurally in + // the test below, because that implementation never touches this counter. + const fine = Array.from({ length: 20_000 }, () => new Uint8Array([0x61])); + await readBoundedResponseBody(responseFromChunks(...fine), { maxBytes: CUSTOM_CAP }); + const fineGrowths = boundedBodyBufferGrowthsForTests(); + + const coarse = [new Uint8Array(20_000).fill(0x61)]; + await readBoundedResponseBody(responseFromChunks(...coarse), { maxBytes: CUSTOM_CAP }); + const coarseGrowths = boundedBodyBufferGrowthsForTests(); + + // 20k one-byte chunks fit inside the 64 KiB seed: no growth at all, and the + // same body delivered as one chunk behaves identically. + expect(fineGrowths).toBe(coarseGrowths); + expect(fineGrowths).toBeLessThanOrEqual(2); + + // Past the seed, growth stays logarithmic: doubling from 64 KiB to 256 KiB is + // two reallocations no matter how the peer fragments it. + const big = Array.from({ length: 256 }, () => new Uint8Array(1024).fill(0x61)); + await readBoundedResponseBody(responseFromChunks(...big), { maxBytes: CUSTOM_CAP }); + expect(boundedBodyBufferGrowthsForTests()).toBeLessThanOrEqual(4); + }); + + test("the accumulator never retains one object per transport chunk", async () => { + // The retained-object shape is the actual security property and no behavioral + // assertion can see it: a `Uint8Array[]` of chunks reassembles byte-identically + // while holding one reference per chunk, which a fragmenting peer inflates far + // past the payload ceiling. It also never increments the growth counter above, + // so that test alone cannot catch it. Pin the shape, the same instrument this + // repository uses for the relay retention rule and the star-consent guard. + const source = (await Bun.file(new URL("../src/lib/bounded-body.ts", import.meta.url)).text()) + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/(^|[^:])\/\/.*$/gm, "$1"); + + // No per-chunk collection: the reader must accumulate into one buffer. + expect(source).not.toMatch(/chunks\s*\.\s*push\s*\(/); + expect(source).not.toMatch(/const\s+chunks\s*:\s*Uint8Array\[\]/); + // And that buffer must be the geometric one this module documents. + expect(source).toMatch(/let\s+retained\s*=\s*new\s+Uint8Array\(/); + expect(source).toMatch(/retained\.set\(value,\s*retainedBytes\)/); + }); + }); + test("parent abort rejects with the exact reason object", async () => { const controller = new AbortController(); const reason = { code: "parent-stopped" }; diff --git a/tests/google-antigravity-replay.test.ts b/tests/google-antigravity-replay.test.ts index 00a586b1b..64962ee3d 100644 --- a/tests/google-antigravity-replay.test.ts +++ b/tests/google-antigravity-replay.test.ts @@ -413,6 +413,25 @@ describe("antigravity replay fixed-size key identities", () => { expect(antigravityCanonicalJsonBoundedForTests({ a: [1, "x"] }, 1024)).toBe('{"a":[1,"x"]}'); }); + test("pathological nesting is refused by the depth cap, not by a stack overflow", () => { + // The byte and key budgets do not bound RECURSION: a deeply nested argument + // shape is tiny on the wire. Without the depth cap this walk exhausts the + // stack and throws RangeError out of a replay observation, which is a crash + // path rather than a skipped replay. Removing MAX_CANONICAL_DEPTH left every + // other antigravity test green, so this is the only case that pins it. + const nest = (levels: number): unknown => { + let value: unknown = 1; + for (let i = 0; i < levels; i++) value = { n: value }; + return value; + }; + + // Comfortably inside the cap: canonicalizes normally. + expect(antigravityCanonicalJsonBoundedForTests(nest(120), 1024 * 1024)).toContain('{"n":'); + // Past the cap: a null refusal, never a thrown RangeError. + expect(antigravityCanonicalJsonBoundedForTests(nest(200), 1024 * 1024)).toBeNull(); + expect(antigravityCanonicalJsonBoundedForTests(nest(50_000), 8 * 1024 * 1024)).toBeNull(); + }); + test("overflow aborts the walk near the cap, proven by scan instrumentation", () => { resetCanonicalScanUnitsForTests(); const hugeString = "y".repeat(10 * 1024 * 1024); diff --git a/tests/relay-eager.test.ts b/tests/relay-eager.test.ts index 2084ea8c1..a66b759d1 100644 --- a/tests/relay-eager.test.ts +++ b/tests/relay-eager.test.ts @@ -111,6 +111,51 @@ async function readAll(stream: ReadableStream): Promise { } describe("relaySseEagerBounded — inline payload rewrite (#864)", () => { + test("neither relay races reads against a shared abort promise", async () => { + // Retention shape, not behavior: racing every read against ONE never-settled + // promise attaches a reaction per completed read and holds it until abort, so a + // long stream retains O(chunk-count) callbacks. Both relays relay identically + // either way, which is exactly why no behavioral assertion catches a regression + // here — relay.ts already states the rule in prose at its own drain, and this + // pins it for both files. The sanctioned shape is: cancel the reader on abort. + const eager = await Bun.file(new URL("../src/server/relay-eager.ts", import.meta.url)).text(); + const relay = await Bun.file(new URL("../src/server/relay.ts", import.meta.url)).text(); + + // Strip comments first: both files DESCRIBE the banned shape in prose, and the + // rule is about the code, not the explanation of why the code avoids it. + const stripComments = (source: string): string => + source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/(^|[^:])\/\/.*$/gm, "$1"); + + for (const [name, source] of [["relay-eager.ts", eager], ["relay.ts", relay]] as const) { + const racesReads = /Promise\.race\(\s*\[\s*reader\.read\(\)/.test(stripComments(source)); + expect(`${name} races reads: ${racesReads}`).toBe(`${name} races reads: false`); + } + // And the eager producer must keep the reader-cancel wake-up that replaced it. + expect(eager).toMatch(/reader\.cancel\(upstream\.signal\.reason\)/); + }); + + test("a terminal frame settling in the same tick as abort is still recorded", async () => { + // Post-cancel drain: the terminal arrives, and the drain deadline aborts + // upstream in the same tick. Honoring the signal before examining the settled + // read discarded that frame, so the turn was accounted as a cancel instead of + // the completion it actually reached. + const up = controlledUpstream(); + const { hooks, rec } = makeHooks(); + const upstream = new AbortController(); + const relayed = relaySseEagerBounded(up.stream, upstream, hooks); + const reading = readAll(relayed); + + up.push(sse(DELTA)); + await settle(); + // Enqueue the terminal and abort without yielding in between. + up.push(enc.encode(`event: response.completed\ndata: ${COMPLETED}\n\n`)); + upstream.abort(new Error("drain window expired")); + up.close(); + await reading; + + expect(rec.terminals.map(t => t.status)).toContain("completed"); + }); + test("rewrites complete blocks across fragmented chunks and flushes the tail at EOF", async () => { const up = controlledUpstream(); const { hooks } = makeHooks(); diff --git a/tests/repo-hygiene.test.ts b/tests/repo-hygiene.test.ts index ad5f87ec5..7f699fc9c 100644 --- a/tests/repo-hygiene.test.ts +++ b/tests/repo-hygiene.test.ts @@ -19,6 +19,16 @@ const FORBIDDEN_TRACKED_DIRS = [".codexclaw", ".omo", ".claude", "node_modules", const FORBIDDEN_TRACKED_FILENAMES = [".DS_Store", "Thumbs.db"]; +/** + * The retired Go native-runtime experiment. Nothing in `src/`, the build, the + * typecheck, or the test path reads from `go/`, so a tracked file there is always + * an accident — and this specific one is a repeat offender: `git add -A` pulled + * `go/internal/cli/config_parity.go` back into the index three times during the + * #820 campaign, and the third one rode a merge into `dev`. `.gitignore` cannot + * catch that on its own, because an already-tracked path ignores the rule. + */ +const RETIRED_TRACKED_DIRS = ["go"]; + function trackedFiles(): string[] { const result = Bun.spawnSync(["git", "ls-files"], { cwd: repoRoot }); if (result.exitCode !== 0) { @@ -64,12 +74,24 @@ describe("repository hygiene", () => { expect(offenders).toEqual([]); }); + test("the retired Go runtime stays untracked", () => { + const offenders = trackedFiles().filter((path) => + RETIRED_TRACKED_DIRS.some((dir) => path === dir || path.startsWith(`${dir}/`)), + ); + + expect(offenders).toEqual([]); + }); + test("gitignore still declares the agent-state directories", async () => { const ignore = await Bun.file(new URL("../.gitignore", import.meta.url)).text(); for (const dir of FORBIDDEN_TRACKED_DIRS) { expect(ignore).toContain(`${dir}/`); } + + for (const dir of RETIRED_TRACKED_DIRS) { + expect(ignore).toContain(`${dir}/`); + } }); }); diff --git a/tests/sidebar-routes.test.ts b/tests/sidebar-routes.test.ts index 36d4bbeab..4a311e112 100644 --- a/tests/sidebar-routes.test.ts +++ b/tests/sidebar-routes.test.ts @@ -19,13 +19,14 @@ async function call( method: string, pathname: string, headers: Record = {}, + principal?: "admin-token" | "gui-session", ): Promise<{ status: number; body: unknown; raw: string; routed: boolean }> { // `isAllowedManagementOrigin` derives the expected origin from the Host header and // rejects the request outright when it is missing, so Host is required here. Omitting // Origin models the GUI's own same-origin fetch. const url = new URL(`http://127.0.0.1:10100${pathname}`); const req = new Request(url, { method, headers: { host: "127.0.0.1:10100", ...headers } }); - const res = await handleManagementAPI(req, url, config); + const res = await handleManagementAPI(req, url, config, {}, principal); if (!res) return { status: 404, body: null, raw: "", routed: false }; const raw = await res.text(); return { status: res.status, body: raw ? JSON.parse(raw) : null, raw, routed: true }; @@ -183,27 +184,90 @@ describe("route surface", () => { async runGh(args) { calls.push(args); return { status: 0 }; }, }, async () => { invalidateStarStatusCache(); - // Browser-session evidence: same-origin Origin plus the minted CSRF/GUI-origin - // headers the management auth gate has already verified before dispatch. + // Browser-session evidence is the CREDENTIAL, not the headers: the auth gate + // resolved a minted GUI session, which it only issues to a browser and only + // accepts for a mutation after matching origin and the per-session CSRF token. const { status, body } = await call("POST", "/api/github/star", { origin: "http://127.0.0.1:10100", "x-opencodex-gui-origin": "http://127.0.0.1:10100", "x-opencodex-csrf-token": "csrf-token", - }); + }, "gui-session"); expect(status).toBe(200); expect((body as Record).ok).toBe(true); })); expect(calls.some(args => args.includes("PUT"))).toBe(true); }); - test("a hand-typed run is not blocked by the agent guard", async () => { + test("forged dashboard headers on an admin-token call cannot star", async () => { + // The consent guard used to read the request's Origin/CSRF/GUI-origin headers and + // trust them as proof of a browser click. The auth gate accepts a raw admin token + // BEFORE it consults the session table, so an agent that can read that token — any + // process running as the user — could send exactly these headers with arbitrary + // values and star the repository with the user's identity. + const calls: string[][] = []; + await withEnv({ ...NO_AGENT_ENV, CODEX_THREAD_ID: "019fbc94" }, () => withStarDeps({ + nowMs: () => 0, + async runGh(args) { calls.push(args); return { status: 0 }; }, + }, async () => { + invalidateStarStatusCache(); + const { status, body } = await call("POST", "/api/github/star", { + origin: "http://127.0.0.1:10100", + "x-opencodex-gui-origin": "http://127.0.0.1:10100", + "x-opencodex-csrf-token": "forged-by-the-token-holder", + }, "admin-token"); + expect(status).toBe(403); + expect((body as Record).code).toBe("agent_consent_required"); + })); + expect(calls).toEqual([]); + }); + + test("a direct dispatch with no resolved principal is treated as untrusted", async () => { + // Defense in depth for callers that bypass the HTTP gate (route-level tests, future + // internal dispatchers): an unknown principal must never satisfy the consent check. + const calls: string[][] = []; + await withEnv({ ...NO_AGENT_ENV, CODEX_THREAD_ID: "019fbc94" }, () => withStarDeps({ + nowMs: () => 0, + async runGh(args) { calls.push(args); return { status: 0 }; }, + }, async () => { + invalidateStarStatusCache(); + const { status } = await call("POST", "/api/github/star", { + origin: "http://127.0.0.1:10100", + "x-opencodex-gui-origin": "http://127.0.0.1:10100", + "x-opencodex-csrf-token": "csrf-token", + }); + expect(status).toBe(403); + })); + expect(calls).toEqual([]); + }); + + test("a clean-environment server still refuses a raw-token star", async () => { + // The guard used to fire only when isAgentDriven() was true — but that reads + // the SERVER's environment, not the caller's. A proxy running as a service has + // no agent markers, so this exact request (raw admin token, no dashboard + // session) starred the repository for anyone who could read the token, which + // is every agent on the machine. Caller provenance is not knowable here; the + // credential is, so the dashboard session is required unconditionally. + const calls: string[][] = []; + await withEnv(NO_AGENT_ENV, () => withStarDeps({ + nowMs: () => 0, + async runGh(args) { calls.push(args); return { status: 0 }; }, + }, async () => { + invalidateStarStatusCache(); + const { status, body } = await call("POST", "/api/github/star", {}, "admin-token"); + expect(status).toBe(403); + expect((body as Record).code).toBe("agent_consent_required"); + })); + expect(calls).toEqual([]); + }); + + test("a dashboard click on a clean-environment server still stars", async () => { const calls: string[][] = []; await withEnv(NO_AGENT_ENV, () => withStarDeps({ nowMs: () => 0, async runGh(args) { calls.push(args); return { status: 0 }; }, }, async () => { invalidateStarStatusCache(); - const { status } = await call("POST", "/api/github/star"); + const { status } = await call("POST", "/api/github/star", {}, "gui-session"); expect(status).toBe(200); })); expect(calls.some(args => args.includes("PUT"))).toBe(true); diff --git a/tests/startup-prompt.test.ts b/tests/startup-prompt.test.ts index 52208e502..8674124b8 100644 --- a/tests/startup-prompt.test.ts +++ b/tests/startup-prompt.test.ts @@ -115,12 +115,18 @@ describe("startup star prompt", () => { // The CLI deferral is worthless if an agent can reach the same write over // HTTP: it runs on the user's machine and can read the admin token from disk. - expect(routes).toContain("isAgentDriven()"); expect(routes).toContain("agent_consent_required"); - // A dashboard click must still work when an agent started the proxy, so the - // refusal is conditioned on the absence of browser-session evidence. expect(routes).toContain("hasBrowserSessionEvidence"); - expect(routes).toMatch(/isAgentDriven\(\)\s*&&\s*!hasBrowserSessionEvidence\(req\)/); + // The requirement is UNCONDITIONAL. Gating it on isAgentDriven() reads the + // server's environment, not the caller's, so a service-run proxy (no agent + // markers) accepted a raw-token star from any agent on the machine. + expect(routes).toMatch(/if\s*\(!hasBrowserSessionEvidence\(ctx\)\)/); + expect(routes).not.toMatch(/isAgentDriven\(\)\s*&&/); + // And that evidence must be the authenticating CREDENTIAL, never a request + // header: the admin token is readable by anything running as the user, so a + // header-shaped check is forgeable by the exact caller this guard refuses. + expect(routes).toMatch(/principal === "gui-session"/); + expect(routes).not.toMatch(/hasBrowserSessionEvidence[\s\S]*?headers\.get\("x-opencodex-csrf-token"\)/); }); test("the consent rule is written down where agents and users read it", async () => { diff --git a/tests/windows-elevation-spawn.test.ts b/tests/windows-elevation-spawn.test.ts index 40b7243ae..0c7bab727 100644 --- a/tests/windows-elevation-spawn.test.ts +++ b/tests/windows-elevation-spawn.test.ts @@ -22,6 +22,7 @@ import { import { evaluateSchedulerInstallRestartReconciliation, finalizeWindowsSchedulerServiceRegistration, + schedulerVerificationMaySettle, setFinalizeWindowsSchedulerHooksForTests, } from "../src/service"; import type { WindowsSchedulerInstallVerification } from "../src/service"; @@ -987,6 +988,43 @@ describe("finalizeWindowsSchedulerServiceRegistration", () => { expect(parentRollbackLaunches).toBe(0); }); + test("the settle predicate refuses every unproven state on its own terms", () => { + // The end-to-end unknown-SCM test above cannot isolate this: its fixture has + // taskInstalled and registrationHealthy both true, so the FINAL clause is + // already false and deleting an earlier guard leaves it green. Exercising the + // predicate directly with a transient-looking tail (invisible task) is what + // proves each guard carries its own weight. + const transientTail: WindowsSchedulerInstallVerification = { + taskInstalled: false, + registrationHealthy: false, + registrationInvalid: false, + assetsHealthy: true, + nativeServiceAbsent: true, + nativeStatusUnknown: false, + conflict: false, + ok: false, + detail: "Task Scheduler task is not installed.", + }; + + // Baseline: a scheduler view that has genuinely not caught up may settle. + expect(schedulerVerificationMaySettle(transientTail)).toBe(true); + + // Each of these is unproven or permanent, and must refuse even though the + // transient tail below it still looks retryable. + expect(schedulerVerificationMaySettle({ ...transientTail, ok: true })).toBe(false); + expect(schedulerVerificationMaySettle({ ...transientTail, conflict: true })).toBe(false); + expect(schedulerVerificationMaySettle({ ...transientTail, assetsHealthy: false })).toBe(false); + expect(schedulerVerificationMaySettle({ ...transientTail, nativeServiceAbsent: false })).toBe(false); + expect(schedulerVerificationMaySettle({ ...transientTail, registrationInvalid: true })).toBe(false); + + // And a fully healthy-but-not-ok view has nothing left to wait for. + expect(schedulerVerificationMaySettle({ + ...transientTail, + taskInstalled: true, + registrationHealthy: true, + })).toBe(false); + }); + test("ownership lost during a settle delay stops without rollback or state write", async () => { mockParentRollbackSpawn(); let owned = true; diff --git a/tests/windows-secret-acl.test.ts b/tests/windows-secret-acl.test.ts index cea08c21a..ce6e4edb1 100644 --- a/tests/windows-secret-acl.test.ts +++ b/tests/windows-secret-acl.test.ts @@ -626,6 +626,12 @@ describe("ephemeral ACL memo release (#840 refinement)", () => { test("ephemeral release clears temp-keyed timeout memos in BOTH namespaces", () => { setPlatformForTests("win32"); setIcaclsRunnerForTests(() => timeout); + // Own the environment this test needs. It used to inherit USERNAME from an + // earlier describe's `??=`, which never restores it: run this file's blocks in + // another order, or this test alone, and the harden fails before it ever + // reaches the memo behavior under test. + const previousUsername = process.env.USERNAME; + process.env.USERNAME = "ocx-test-user"; const tempA = join(testDir, "dest.ocx.1.1.tmp"); const tempB = join(testDir, "dest.ocx.1.2.tmp"); writeFileSync(tempA, "a", "utf-8"); @@ -642,6 +648,8 @@ describe("ephemeral ACL memo release (#840 refinement)", () => { } finally { setIcaclsRunnerForTests(null); setPlatformForTests(null); + if (previousUsername === undefined) delete process.env.USERNAME; + else process.env.USERNAME = previousUsername; } });