Record Stage 11 Promptkit security audit

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.