diff --git a/.agent-runtime/goals/held-path-identity.json b/.agent-runtime/goals/held-path-identity.json new file mode 100644 index 0000000..c3c33ba --- /dev/null +++ b/.agent-runtime/goals/held-path-identity.json @@ -0,0 +1,141 @@ +{ + "schema_version": "v1alpha1", + "revision": 11, + "goal": { + "id": "held-path-identity", + "intent": "The JSONL sink writes to the file it read, and every remaining check-then-use gap is stated rather than implied", + "acceptance": [ + { + "id": "jsonl-single-open", + "acceptance": "The sink scans and appends through one descriptor", + "status": "complete", + "evidence": [ + { + "type": "file", + "reference": "observability/sink_jsonl.go OpenJSONLSink; readJSONLFile", + "result": "One descriptor, opened O_RDWR|O_APPEND, validated and then read from and appended to." + } + ] + }, + { + "id": "stated-boundary", + "acceptance": "The security model names the remaining check-then-use gaps and what exploiting them requires", + "status": "complete", + "evidence": [ + { + "type": "file", + "reference": "SECURITY.md", + "result": "Workspace and executable check-then-use are named, with what an actor needs and why executable_path records the inspected file." + } + ] + }, + { + "id": "revisit-trigger", + "acceptance": "The deferral records the decision that would re-open it", + "status": "complete", + "evidence": [ + { + "type": "file", + "reference": "SECURITY.md", + "result": "States that executing commands which are not fully trusted makes both real, and names descriptor-anchored traversal as the fix." + } + ] + } + ], + "non_goals": [ + "Descriptor-anchor workspace traversal in this pass", + "Redesign the sandbox model", + "Defend against a compromised host" + ], + "state": "active", + "current_phase": "closure", + "receipts": { + "completeness_omission_audit": { + "phase": "completeness_omission_audit", + "summary": "Checked the other direction as well: a destination whose name resolves elsewhere, such as a symlink, is refused rather than followed, because a caller configured a destination and not whatever currently answers to it.", + "evidence": [ + { + "type": "test", + "reference": "observability TestJSONLRefusesAPathThatResolvedElsewhere", + "result": "A symlinked destination is rejected." + } + ], + "recorded_at": "2026-08-14T23:50:11.199764768Z" + }, + "execute": { + "phase": "execute", + "summary": "Split readJSONL into a path form and a descriptor form, opened the sink once with O_RDWR|O_APPEND|O_CREATE, validated that descriptor, and read the history from it. Removed the now-dead scanJSONL wrapper.", + "evidence": [ + { + "type": "file", + "reference": "observability/sink_jsonl.go", + "result": "One open for the sink's lifetime; the remaining os.Open is the separate read-only ReplayJSONL entry point." + } + ], + "recorded_at": "2026-08-14T23:50:11.112800073Z" + }, + "gap_plan": { + "phase": "gap_plan", + "summary": "Hold one descriptor for the sink's lifetime and read the history from it. Leave workspace and executable resolution as they are, and state them in the security model: both need concurrent write access to the machine already running the Task, which the stated trust boundary excludes.", + "evidence": [ + { + "type": "file", + "reference": "SECURITY.md trust boundary", + "result": "Manifests and their commands are trusted reviewed inputs, so a party who can rewrite a PATH directory can do so with or without a race." + } + ], + "recorded_at": "2026-08-14T23:50:11.081411822Z" + }, + "orient": { + "phase": "orient", + "summary": "Three surfaces validate a pathname and act on it later. The JSONL sink was the only one where the split is a plain correctness defect rather than a race against an actor: it opened the path to scan, closed it, and opened it again to append, so recovered state and writes could describe different files with no attacker involved beyond a rename.", + "evidence": [ + { + "type": "file", + "reference": "observability/sink_jsonl.go OpenJSONLSink", + "result": "scanJSONL opened and closed the path, then os.OpenFile opened it again; the SameFile check proved path equals descriptor at the second open, not that it was the file scanned." + } + ], + "recorded_at": "2026-08-14T23:50:11.050697736Z" + }, + "reconcile": { + "phase": "reconcile", + "summary": "The behavioural test first written to cover this did not discriminate: it renamed the path after the sink was open, which both the old and the new code survive because both held a descriptor by then. Its comment was corrected to say what it actually pins.", + "evidence": [ + { + "type": "test", + "reference": "observability TestJSONLAppendsToTheFileItRead", + "result": "It fails if the single-descriptor construction is ever undone, and it does not claim to reproduce the original window." + } + ], + "recorded_at": "2026-08-14T23:50:11.142761962Z" + }, + "self_review": { + "phase": "self_review", + "summary": "The window itself is closed by construction rather than by a test. Reproducing it from outside would need a hook that exists only to be hooked, which is the kind of control this repository is trying to remove, so the limitation is stated instead of dressed up.", + "evidence": [ + { + "type": "file", + "reference": "observability/sink_jsonl_test.go", + "result": "The test comment states plainly that it does not reproduce the defect it accompanies." + } + ], + "recorded_at": "2026-08-14T23:50:11.171830052Z" + }, + "verify": { + "phase": "verify", + "summary": "gofmt, go vet, staticcheck, full tests under umask 002, race, and all six contract checkers.", + "evidence": [ + { + "type": "test", + "reference": "local gate transcript 2026-08-15", + "result": "All green." + } + ], + "recorded_at": "2026-08-14T23:50:11.227703174Z" + } + }, + "created_at": "2026-08-14T23:50:11.022035863Z", + "updated_at": "2026-08-14T23:50:11.311924048Z" + } +} diff --git a/.agent-runtime/goals/runtime-bounds-truth.json b/.agent-runtime/goals/runtime-bounds-truth.json index b76d4d5..ced9115 100644 --- a/.agent-runtime/goals/runtime-bounds-truth.json +++ b/.agent-runtime/goals/runtime-bounds-truth.json @@ -1,6 +1,6 @@ { "schema_version": "v1alpha1", - "revision": 14, + "revision": 15, "goal": { "id": "runtime-bounds-truth", "intent": "Task and Goal contracts enforce the bounds and outcomes they declare", @@ -82,9 +82,33 @@ "Sandbox untrusted commands", "Add Windows support" ], - "state": "active", + "state": "completed", "current_phase": "closure", "receipts": { + "closure": { + "phase": "closure", + "summary": "Merged as PR #56 with all four required checks green.", + "evidence": [ + { + "type": "link", + "reference": "https://github.com/NDDev-it-com/agent-runtime/pull/56", + "result": "Merged to main." + } + ], + "recorded_at": "2026-08-14T23:45:44.259561919Z", + "closure": { + "achieved_outcome": "The Task and Goal contracts enforce the bounds and outcomes they declare, and the wall-clock ceiling is stated rather than implied.", + "cleanup": "Task branch deleted locally and on origin.", + "remaining": [], + "next_work": [ + { + "type": "issue", + "reference": "held path identity", + "result": "Close the JSONL sink's split between the file it reads and the file it writes, and state the remaining check-then-use gaps rather than implying they are closed." + } + ] + } + }, "completeness_omission_audit": { "phase": "completeness_omission_audit", "summary": "Owning the process group also removed an undocumented overshoot: a 300ms timeout returned at 2.302s because the orphaned descendant held the inherited pipes for the full WaitDelay. It now returns at 301ms. The grace period is documented rather than removed, because a terminated process still needs bounded time to release its pipes.", @@ -171,6 +195,6 @@ } }, "created_at": "2026-08-14T23:40:47.471824358Z", - "updated_at": "2026-08-14T23:41:15.508130592Z" + "updated_at": "2026-08-14T23:45:44.259561919Z" } } diff --git a/CHANGELOG.md b/CHANGELOG.md index 62df919..1296898 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,23 @@ contract. ### Fixed +- A JSONL sink holds one descriptor for its lifetime. It used to open the path + to scan the existing history, close it, and open it again to append, so a + rename in between left the recovered duplicate-identity and size state + describing the first file while every write went to the second. A destination + whose name no longer resolves to the object just opened is refused rather than + followed. + +### Changed + +- The security model records the two check-then-use gaps that remain rather than + implying they are closed. Workspace resolution and executable resolution both + validate a pathname and act on it a moment later, so an actor with concurrent + write access to the running machine can move the used object away from the + checked one — which also means `executable_path` records what was inspected. + Both sit outside the stated trust boundary, and the entry says what would make + them real and what the fix would be if it ever changes. + - Malformed evidence can no longer be staged on a pending checklist item. `Validate` checked evidence only on completed items, but acceptance evidence is append-only for every item, so a bad record staged on a pending one could diff --git a/SECURITY.md b/SECURITY.md index f40b4e0..c816b7f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -34,6 +34,28 @@ caller. `Result.executable_path` records the file that ran. Use an external sandbox for untrusted code and provide only short-lived, least-privilege credentials. +### Paths validated and paths used + +Two runtime checks resolve a pathname and act on it a moment later rather than +on the object they examined. Workspace resolution returns a validated string +that `Open`, `Stat` and the child's working directory consume afterwards, and a +command name is resolved by inspecting a candidate file that `exec` opens +later. An actor able to rename or replace a directory or an executable in that +interval can move the used object away from the checked one — so +`Result.executable_path` records what was inspected, which under such a race is +not necessarily what ran. + +Both require concurrent write access to the machine already running the Task, +which is outside the trust boundary above: manifests and their commands are +trusted, reviewed inputs, and a party who can rewrite a directory on the PATH +can do so with or without a race. They are recorded here rather than closed +because stating a gap is worth more than a control that does not remove it. + +This holds only while that trust boundary does. If this runtime is ever used to +execute commands that are not fully trusted, these become real and the fix is +descriptor-anchored traversal and execution — the mechanism the JSONL sink and +the release publisher already use. + Avoid placing secrets in manifests, instructions, command arguments, logs, or issue reports. Captured command output may contain sensitive data and should be handled accordingly by callers. @@ -73,7 +95,10 @@ are re-evaluated key by key, bounded, copied, and JSON encoded; custom errors an stringers are never invoked. Redaction metadata contains only typed reasons and aggregate counts, not paths, hashes, lengths, or removed values. -JSONL paths are trusted caller configuration. Existing files must be regular, -owner-only, supported-version canonical JSONL. Symlinks, partial records, +JSONL paths are trusted caller configuration. A sink opens its destination once +and holds that descriptor for its lifetime, so the history it recovers and the +records it appends describe one object; a name that no longer resolves to the +object just opened is refused rather than followed. Existing files must be +regular, owner-only, supported-version canonical JSONL. Symlinks, partial records, duplicates, unsupported versions, oversize files, and corruption fail closed. The sink does not encrypt data or protect against a compromised host. diff --git a/observability/sink_jsonl.go b/observability/sink_jsonl.go index 6ea140c..ed953a1 100644 --- a/observability/sink_jsonl.go +++ b/observability/sink_jsonl.go @@ -45,20 +45,34 @@ func OpenJSONLSink(path string, options JSONLOptions) (*JSONLSink, error) { if maxBytes < MaxEnvelopeBytes || maxBytes > MaximumMaxFileBytes { return nil, &SinkError{Code: SinkFailure} } - history, err := scanJSONL(path, maxBytes) - if err != nil { - return nil, err - } - file, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) + // One descriptor for the whole life of the sink. O_APPEND puts every write + // at the end regardless of where reading left the offset, so the history + // this sink trusts and the file it extends are the same object. Opening to + // scan and reopening to append let a rename between the two split them: + // duplicate-identity and size state would describe the file that was read + // while the writes went somewhere else. + file, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_RDWR, 0o600) if err != nil { return nil, &SinkError{Code: SinkUnavailable, Retryable: true} } - info, statErr := os.Lstat(path) openedInfo, openedErr := file.Stat() - if statErr != nil || openedErr != nil || !info.Mode().IsRegular() || info.Mode().Perm()&0o077 != 0 || !os.SameFile(info, openedInfo) { + if openedErr != nil || !openedInfo.Mode().IsRegular() || openedInfo.Mode().Perm()&0o077 != 0 { + _ = file.Close() + return nil, &SinkError{Code: SinkFailure} + } + // The name must still resolve to the object now held. A path that resolved + // elsewhere is refused rather than followed, because a caller configured a + // destination, not whatever currently answers to it. + named, namedErr := os.Lstat(path) + if namedErr != nil || !os.SameFile(named, openedInfo) { _ = file.Close() return nil, &SinkError{Code: SinkFailure} } + history, err := readJSONLFile(file, maxBytes, nil) + if err != nil { + _ = file.Close() + return nil, err + } return &JSONLSink{name: options.Name, file: file, syncEveryWrite: options.SyncEveryWrite, ids: history.ids, maxFileBytes: maxBytes, size: history.size}, nil } func newJSONLSinkWriter(name string, file syncWriteCloser, syncEveryWrite bool) (*JSONLSink, error) { @@ -156,11 +170,6 @@ type jsonlHistory struct { size int64 } -// scanJSONL validates an existing history without retaining its envelopes. -func scanJSONL(path string, maxBytes int64) (jsonlHistory, error) { - return readJSONL(path, maxBytes, nil) -} - // readJSONL is the single definition of a valid JSONL history: canonical // single-line envelopes, a newline-terminated final record, unique event // identity, and a strictly increasing sequence within each subject stream. The @@ -168,15 +177,24 @@ func scanJSONL(path string, maxBytes int64) (jsonlHistory, error) { // so one file can never be acceptable to append to yet impossible to replay. // When collect is non-nil every decoded envelope is appended to it in file order. func readJSONL(path string, maxBytes int64, collect *[]Envelope) (jsonlHistory, error) { - history := jsonlHistory{ids: map[string]bool{}, sequences: map[string]uint64{}} file, err := os.Open(path) if os.IsNotExist(err) { - return history, nil + return jsonlHistory{ids: map[string]bool{}, sequences: map[string]uint64{}}, nil } if err != nil { return jsonlHistory{}, &SinkError{Code: SinkUnavailable, Retryable: true} } defer file.Close() + return readJSONLFile(file, maxBytes, collect) +} + +// readJSONLFile reads the history of an already-open file. Taking the +// descriptor rather than the path is what lets the sink validate and then +// append to one object: reopening by name between the two left the recovered +// identity and size describing the file that was read while every append went +// to whatever the name resolved to afterwards. +func readJSONLFile(file *os.File, maxBytes int64, collect *[]Envelope) (jsonlHistory, error) { + history := jsonlHistory{ids: map[string]bool{}, sequences: map[string]uint64{}} info, err := file.Stat() if err != nil { return jsonlHistory{}, &SinkError{Code: SinkUnavailable, Retryable: true} diff --git a/observability/sink_jsonl_test.go b/observability/sink_jsonl_test.go index 879f776..1c20ad3 100644 --- a/observability/sink_jsonl_test.go +++ b/observability/sink_jsonl_test.go @@ -360,3 +360,76 @@ func sinkCode(err error, code SinkErrorCode) bool { var typed *SinkError return errors.As(err, &typed) && typed.Code == code } + +// TestJSONLAppendsToTheFileItRead pins the guarantee that the recovered history +// and the appends describe one object: replace the path after opening and the +// sink keeps extending the file it recovered, leaving the impostor untouched. +// +// It does not reproduce the defect it accompanies. That was a window inside +// OpenJSONLSink — the path was opened once to scan, closed, and opened again to +// append, so a rename in between left duplicate-identity and size state +// describing the first file while writes went to the second. The window is +// closed by construction, by holding a single descriptor for the life of the +// sink, and hitting it from outside would require a hook that exists only to be +// hooked. What this test can do is fail if that construction is ever undone. +func TestJSONLAppendsToTheFileItRead(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "events.jsonl") + sink, err := OpenJSONLSink(path, JSONLOptions{Name: "file", SyncEveryWrite: true}) + if err != nil { + t.Fatal(err) + } + emitter := testEmitter(t, sink) + if _, report, err := emitter.Emit(context.Background(), testDraft()); err != nil || !report.Succeeded() { + t.Fatalf("err=%v report=%#v", err, report) + } + + // The name now resolves to a different, empty file. + held := filepath.Join(dir, "held.jsonl") + if err := os.Rename(path, held); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, nil, 0o600); err != nil { + t.Fatal(err) + } + + if _, _, err := emitter.Emit(context.Background(), acceptedTaskDraft()); err != nil { + t.Fatal(err) + } + if results := emitter.Close(context.Background()); !results[0].Delivered { + t.Fatalf("close=%#v", results) + } + + impostor, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if len(impostor) != 0 { + t.Fatalf("the sink wrote into the file that took over the path: %q", impostor) + } + events, _, err := ReplayJSONL(held) + if err != nil { + t.Fatalf("the file the sink actually held is unreadable: %v", err) + } + if len(events) != 2 { + t.Fatalf("the held file has %d events, want both writes", len(events)) + } +} + +// TestJSONLRefusesAPathThatResolvedElsewhere covers the other direction: a name +// that no longer refers to the object just opened is a configuration the caller +// did not make, so it is refused rather than followed. +func TestJSONLRefusesAPathThatResolvedElsewhere(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target.jsonl") + if err := os.WriteFile(target, nil, 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.jsonl") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if _, err := OpenJSONLSink(link, JSONLOptions{Name: "file"}); err == nil { + t.Fatal("a symlinked destination was accepted") + } +}