diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 490f83e..1c76e46 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-10 complete. +Status: In progress; Stages 1-11 complete. This temporary roadmap document is the evidence ledger for the staged audit defined by [the audit plan](audit-plan.md). It records audit evidence and @@ -75,8 +75,15 @@ accepts several malformed package shapes, and stanza serialization failures lack a focused regression test. Prompt/profile/schema identities, deterministic category ordering, neutral execution lifecycle and error categories, safe bounded errors, debug opt-in, and output-copy isolation -otherwise match their contracts. Subsystem -conclusions and final disposition remain pending the later stages. +otherwise match their contracts. Stage 11 found two medium-severity security +defects in the explicit debug path: common credential aliases and URL forms +survive heuristic redaction, and pathname-based symlink checks can be raced +before sensitive files are created or replaced. Promptkit type containment, +source precedence, exact inline execution, credential preflight, cancellation, +safe error classification, callback ordering, restrictive modes, atomic file +replacement, and ordinary concurrent execution otherwise match their +contracts. Subsystem conclusions and final disposition remain pending the +later stages. ## Baseline Metadata @@ -213,7 +220,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 8 | Audit module contracts, registry, and source-facing briefing modules | Complete | | 9 | Audit derived, planning, formatting, and SPC briefing modules | Complete | | 10 | Audit prompt inputs, assets, and neutral execution contracts | Complete | -| 11 | Audit Promptkit adaptation and secure prompt debugging | Pending | +| 11 | Audit Promptkit adaptation and secure prompt debugging | Complete | | 12 | Audit generated-text validation and catalog contracts | Pending | | 13 | Audit render contexts, templates, and Markdown rendering | Pending | | 14 | Audit application preparation and prompt preflight | Pending | @@ -247,7 +254,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | Forecast and fact derivation | `internal/forecast`, `internal/facts` | 7 | Partly insufficient. Half-open selection, stable ordering, alert parsing and clipping, precipitation-window grouping, report-family selection, and SPC checked-empty behavior have focused owners. `AUD-016` through `AUD-019` record unit collapse, overnight alert omission, invalid percentage acceptance, and incomplete ice-text classification. | | Module and briefing contracts | `internal/module`, `internal/briefing` | 8-9 | Partly insufficient. Registry/default composition, report support, missing optional sources, checked-empty alerts, output identity, ordering, current/hourly prompt curation, precipitation timing, and SPC selection have focused owners. `AUD-020` through `AUD-028` record inconsistent pointer options, false weather-story availability, permissive snapshot schema validation, apparent-temperature mislabeling, daypart-key collisions, inconsistent planning identities, incomplete outdoor hazard scoring, incorrect subzero phrases, and stale official SPC descriptions. | | Prompt inputs, embedded assets, and execution contracts | `internal/promptinput`, `internal/promptassets`, `internal/promptexec` | 10 | Partly insufficient. Prompt/profile/schema identities, deterministic grouping, validation/execution result categories, safe bounded errors, explicit debug, and copy isolation have focused owners. `AUD-029` through `AUD-033` record raw warning types and transport paths crossing the boundary, stale embedded prompt paths, incorrect SPC locality instructions, permissive YAML loading, and missing serialization-failure coverage. | -| Promptkit boundary and sensitive debug output | `internal/adapters/promptkit`, `internal/promptdebug` | 11 | Pending | +| Promptkit boundary and sensitive debug output | `internal/adapters/promptkit`, `internal/promptdebug` | 11 | Partly insufficient. Exact prompt/profile inspection, source precedence, inline execution, credential checks, cancellation, safe classified errors, callback ordering, debug opt-in, static symlink rejection, restrictive modes, atomic replacement, and distinct concurrent captures have focused owners. `AUD-034` and `AUD-035` record incomplete credential redaction and a check/use symlink race. | | Generated-text validation | `internal/generatedtext` | 12 | Pending | | Render contexts and templates | `internal/generatedtext`, `internal/reporttemplate` | 13 | Pending | | Prompt preflight and prepared inputs | `internal/app` | 14 | Pending | @@ -1460,6 +1467,85 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: none - Remediation reference: pending +### AUD-034: Prompt-debug credential redaction misses common aliases and URL forms + +- Stage: 11 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: security +- Area: `internal/promptdebug.safePromptDebugEndpoint`, + `redactPromptDebugParameters`, and `isPromptDebugSecretKey` +- Evidence: Debug preparation deliberately persists the resolved provider + endpoint and effective extra parameters after key-based redaction. The key + normalizer recognizes strings containing credential, secret, password, + token, API key, or authorization. A deterministic temporary probe confirmed + that common synthetic aliases such as access-key, signature, auth, and cookie + retain their marker values in endpoint queries or nested parameters. URL + user information and the existing API-key fixture are redacted, but URL path + credentials and unrecognized query names are also retained. The probe was + removed after reproduction and no sensitive fixture value was recorded. +- Contract at risk: Prompt-debug artifacts may contain content-rich execution + detail, but their explicit safe mapping and focused tests promise that + provider credentials are not persisted. +- Impact: An operator profile or backend using an unrecognized credential + alias or signed URL can write a live credential into `preparation.json`. + Files are mode `0600` beneath a mode `0700` root, limiting exposure, but the + credential becomes durable and subject to backup, retention, or later + permission mistakes despite the artifact claiming redaction. +- Recommendation: Stop trying to prove arbitrary provider maps and complete + URLs safe through a denylist. Persist an allowlisted endpoint projection and + allowlisted non-secret execution parameters, or replace all provider-specific + extras with names/type summaries. Define how signed paths and queries are + omitted rather than expanding an indefinitely incomplete alias list. +- Test implications: Add table-driven synthetic markers across URL user info, + paths, common query aliases, case/separator variants, and nested maps/lists; + assert that no marker reaches serialized debug bytes while reviewed ordinary + parameters remain useful. Do not use real credential-like fixture values. +- Validation: Every supported debug projection is allowlisted, adversarial + marker tests find no value leak, and normal summaries remain content-free. +- Related findings: none +- Remediation reference: pending + +### AUD-035: Prompt-debug symlink checks can be raced before writes + +- Stage: 11 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: security +- Area: `internal/promptdebug.ensureSecureDirectory`, `runDirectory`, and + `writeSecureJSON` +- Evidence: Directory validation walks path strings with `os.Lstat` and rejects + every symlink visible at that moment. `runDirectory` then joins and creates + paths by name, and `writeSecureJSON` later creates a temporary file and + renames it through the same pathname. No directory handle pins the validated + root or descendants, so a process able to replace a component in a writable + ancestor can swap it for a symlink after validation and before `Mkdir`, + `CreateTemp`, or `Rename`. Static root/component symlink tests and concurrent + missing-ancestor tests pass, but they cannot protect this check/use window. +- Contract at risk: Sensitive debug artifacts must remain beneath the + explicitly authorized root, and symlink rejection must hold at the file + operation rather than only during an earlier observation. +- Impact: Under an explicitly enabled debug run and a replaceable ancestor, a + local filesystem attacker can race sensitive rendered prompts or model + output into a directory outside the authorized root. Restrictive final modes + do not restore path containment after redirection. +- Recommendation: Anchor traversal and creation to opened directory handles + and use no-follow, directory-only operations for every component and final + replacement, with platform-specific handling where necessary. Alternatively + require and document a pre-existing trusted root while still pinning it for + each write; string containment and repeated `Lstat` are not sufficient. +- Test implications: Add a deterministic filesystem seam or supported-platform + race harness that replaces a component between validation and creation and + proves no artifact appears outside the root. Retain the cheaper static + symlink, mode, collision, and concurrent-distinct-run cases. +- Validation: Debug creation and atomic replacement remain relative to the + originally validated root handle across component swaps; attempted + redirection fails closed without an external file or partial target. +- Related findings: none +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -1715,6 +1801,35 @@ anemic duplicate of the dependency API. Reconsider only if a second executor implementation demonstrates a missing neutral semantic; Promptkit-specific cancellation and diagnostic translation remain Stage 11. +### RET-023: Keep Promptkit source resolution and prepared handles in the adapter + +The adapter supplies embedded prompt/schema/profile filesystems and configured +profile sources directly to one Promptkit engine, then maps exact inspections +and one-use prepared executions into repository-owned values. It does not +reparse Promptkit YAML, merge profile fields, duplicate backend capacity, or +expose dependency handles. Focused tests cover complete-source precedence, +malformed matching definitions, built-in fallback, exact inline input, +callback-before-provider ordering, credential checks, cancellation, and all +stable error categories. Promptkit v0.5.0 explicitly documents its engine as +concurrency-safe and has its own concurrent prepare/run contract test, while +Weatherreporter's race-enabled adapter/app suites pass. Reconsider only if the +dependency contract changes or Weatherreporter adopts an independent profile +format requiring repository-owned resolution. + +### RET-024: Keep explicit prompt-debug storage separate from normal publication + +The debug writer is disabled without an absolute operator root and otherwise +owns versioned per-report/run JSON records, restrictive directory/file modes, +validated path segments, atomic same-file replacement, and distinct comparison +run directories. Application callbacks make a requested preparation-write +failure stop before provider generation, and execution-write failure prevents +report publication. These authorization, sensitivity, and failure semantics +differ materially from ordinary Markdown and comparison publication, so a +generic filesystem writer would obscure the contract. Retain the separate +owner while remediating `AUD-034` and `AUD-035`; reconsider shared mechanics +only after a common primitive can preserve every artifact's security and +transaction boundary. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -1886,6 +2001,24 @@ Stage 10 routed these investigation leads to their assigned later stages: prevents provider work belongs to Stage 14 with `AUD-033` as its focused package-level prerequisite. +Stage 11 routed these investigation leads to their assigned later stages: + +- Debug writer construction creates or tightens the explicitly requested root + before prompt inspection and collection. This is authorized diagnostic state, + not an ordinary-workflow violation, but Stage 24 should make the possibility + of an empty root after later preflight failure explicit if operators need that + lifecycle detail. +- `marshalDebugParameters` and structured-schema capture discard impossible- + under-current-Promptkit JSON marshal errors. Promptkit v0.5.0 constrains those + values to JSON-compatible copies, so Stage 11 recorded no defect. Stage 23 + should avoid generalizing the adapter around that assumption unless the + dependency contract changes. +- Application-level inspection ordering, exact prompt/profile consistency, and + prepared-input reuse remain Stage 14; comparison scheduling and cancellation + aggregation remain Stage 18. Stage 11 accounted for adapter context + propagation and Promptkit's concurrent-engine contract without duplicating + those orchestration findings. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -2394,3 +2527,60 @@ Stage 10 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-021` and `RET-022`. - Open questions: the three leads recorded above are routed to their assigned later stages. + +### Stage 11: Audit Promptkit Adaptation And Secure Prompt Debugging + +- Status: Complete. +- Scope reviewed: all production and focused-test code in + `internal/adapters/promptkit` and `internal/promptdebug`; Promptkit integration + and adapter internals; relevant operations, architecture, documentation, and + testing rules; Promptkit v0.5.0's local public concurrency/prepared-execution + contract; and immediate app/CLI consumers needed to trace credential checks, + debug authorization, callback failure, and result projection. +- Exclusions: Generated-text schema semantics remain Stage 12; application + prompt inspection/preparation order remains Stage 14; comparison execution + scheduling remains Stage 18; suite-wide test hygiene and cross-cutting + cleanup remain Stages 19-23; documentation-wide coherence remains Stage 24; + and broader adversarial diagnostics remain Stage 25. + +#### Adapter And Debug-Security Accounting + +| Contract or risk | Adapter, filesystem, caller, and test evidence | Disposition | +| --- | --- | --- | +| Dependency containment and configuration | Promptkit production imports occur only in its adapter. CLI-owned settings map to profile directory/file, local endpoint/capacity, and timeout; mutually exclusive sources and inactive local capacity fail construction. | Matches the architecture boundary. Profile resolution remains dependency-owned under `RET-023`. | +| Profile source precedence | Explicit in-memory definitions, configured file/directory, embedded fallback, and built-ins provide complete definitions in the documented order. Absence falls through; a malformed matching configured definition fails. | Coherent with offline focused tests and retained as part of `RET-023`. | +| Exact inspection and credential preflight | Prompt inspection returns exact ID/version/hash, one input, and output contract. Profile inspection returns only logical ID, backend/model, direct-key requirement, and environment-variable name; app checks direct keys and nonblank environment values before collection. | Safe values and missing-credential categories match. Environment names are identifiers, not secret values. | +| Prepared inline execution | The adapter copies the YAML bytes into one inline artifact, prepares once, invokes the callback before generation, and runs the same opaque one-use handle. Callback failure prevents the provider call and deferred discard clears unused private state. | Matches the integration contract with exact-provenance and callback tests. | +| Cancellation and concurrency | Preparation and execution receive caller context independently; canceled/deadline causes map before generic generation errors. Promptkit documents `Engine` as concurrency-safe with capacity owned per engine, and comparison uses one adapter concurrently without wrapper state. | Coherent; race-enabled adapter/app tests pass. No redundant application-wide limit or adapter mutex is warranted. | +| Error classification and normal redaction | Promptkit configuration, prompt/profile, credential, artifact/render, capacity, generation, validation, request, cancellation, and deadline errors map to bounded repository categories. Underlying causes remain discoverable but their content does not enter `Error()`, summaries, or logs. | Sufficient at this boundary; focused marker and category tests protect normal output. | +| Debug authorization and isolation | Empty root disables all filesystem access; an enabled root must be absolute and not `/`. Report/date/run segments reject separators and traversal, comparison derives distinct deterministic run segments, and content-rich values appear only when capture is requested. | Matches explicit opt-in and per-run isolation. Separate ownership retained as `RET-024`. | +| Debug credential projection | User information, fragments, known secret query keys, and known nested parameter keys are removed or redacted before preparation JSON is written. | The denylist misses common aliases, signed/path credentials, and arbitrary provider vocabulary; `AUD-034`. | +| Directory and symlink containment | Static symlinks in the root or any visible descendant component are rejected; directory modes are tightened to `0700`, and unsafe references fail before a file write. | Path checks are separated from later name-based creation/replacement and can be raced; `AUD-035`. | +| File collision, modes, and partial writes | JSON is marshaled before creation; same-name artifacts use a mode-`0600` temporary in the final directory and atomic rename; non-regular existing targets are rejected; distinct concurrent run directories and shared missing ancestors are tested. | Coherent for non-adversarial paths. A failed requested callback stops generation, and a failed execution write stops publication. | +| Complexity and test ownership | Adapter mapping/classification is straight-line or small linear copying. Debug recursion follows bounded JSON-compatible parameter trees; directory traversal is linear in path components; realistic artifact sizes are dominated by intentionally captured prompt/output content. Adapter tests own dependency translation; debug tests own filesystem/security behavior; app tests own callback consequences. | No Stage 11 efficiency or duplication finding. Redaction and race gaps are attached to their narrow owners rather than inferred from coverage. | + +#### Commands And Evidence + +- Used graph architecture, symbol and augmented-code search, snippets, + inbound/data-flow traces, and hotspot inspection for adapter construction, + exact inspection, preparation/execution, classification, debug writes, + path validation, redaction, application callbacks, and CLI debug-root flow. + Text inspection was limited to scoped documents, tests, string-based + security policy, and the locally installed Promptkit v0.5.0 contract. +- Confirmed Promptkit's public engine concurrency guarantee, one-use prepared + handle state, independent preparation/execution contexts, credential-redacted + details, and upstream concurrent engine contract test without contacting a + provider. +- Ran a temporary deterministic redaction probe using only synthetic markers; + it proved that common endpoint-query and nested-parameter aliases survive + serialization, then was removed without retaining source/test changes. +- Ran + `go test -race -coverprofile=/tmp/weatherreporter-stage11-cover.out ./internal/adapters/promptkit ./internal/promptdebug ./internal/app`; + all focused packages passed with 90.0%, 73.6%, and 76.0% statement coverage + respectively. Coverage guided branch inspection but was not itself a + finding. Ran `go test ./...`, `go vet ./...`, and `git diff --check`; all + passed. +- Findings: `AUD-034` and `AUD-035`. +- Retained decisions: `RET-023` and `RET-024`. +- Open questions: the three leads recorded above are routed to their assigned + later stages.