Complete configuration and secrets audit
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
# Repository Audit Ledger
|
||||
|
||||
Status: In progress; Stages 1-3 complete.
|
||||
Status: In progress; Stages 1-4 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
|
||||
@@ -20,8 +20,14 @@ defect in wall-clock window construction on daylight-saving transition days
|
||||
and one medium-severity test gap around Daily run-ID uniqueness. Report
|
||||
registry identities, command and configuration lookup policies, batch
|
||||
membership, civil-day periods, and rolling-hour periods otherwise match their
|
||||
documented contracts. Subsystem conclusions and final disposition remain
|
||||
pending the later stages.
|
||||
documented contracts. Stage 4 found three medium-severity validation defects:
|
||||
unsupported missing-source keys are silently accepted, two single-report
|
||||
notification identities may render blank, and programmatically constructed
|
||||
report overrides are silently ignored. It also found one low-severity secret
|
||||
loading atomicity defect. Defaults, file and CLI precedence, strict YAML field
|
||||
handling, active Promptkit settings, output-path preservation, and maintained
|
||||
examples otherwise match their contracts. Subsystem conclusions and final
|
||||
disposition remain pending the later stages.
|
||||
|
||||
## Baseline Metadata
|
||||
|
||||
@@ -151,7 +157,7 @@ inventory commands, graph index refresh, and graph architecture inspection.
|
||||
| 1 | Establish the baseline and audit ledger | Complete |
|
||||
| 2 | Audit architecture and dependency direction | Complete |
|
||||
| 3 | Audit report identity and time foundations | Complete |
|
||||
| 4 | Audit configuration, secrets, and validation | Pending |
|
||||
| 4 | Audit configuration, secrets, and validation | Complete |
|
||||
| 5 | Audit CLI parsing, wiring, and output contracts | Pending |
|
||||
| 6 | Audit weather data acquisition and collection | Pending |
|
||||
| 7 | Audit forecast and fact derivation | Pending |
|
||||
@@ -186,7 +192,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication.
|
||||
| --- | --- | ---: | --- |
|
||||
| Architecture and dependency direction | Assembled app and CLI tests; graph traces | 2 | Sufficient at the architectural boundary: direct imports are acyclic, external dependency types remain adapter-local, and representative workflow ordering and publication boundaries have focused tests. `AUD-001` records unused persistence APIs that do not participate in normal execution. |
|
||||
| Report identity, periods, dates, and timezones | `internal/report`, `internal/timeutil`, plus focused `internal/app` and `internal/forecast` callers | 3 | Partly insufficient. Registry identities, alias policies, batch membership, civil-day boundaries, and elapsed six-hour periods are coherent and exercised. `AUD-002` records incorrect wall-clock windows on daylight-saving transition days, and `AUD-003` records the missing regression test for Daily run-ID uniqueness across dates. |
|
||||
| Configuration, validation, and secrets | `internal/config` | 4 | Pending |
|
||||
| Configuration, validation, and secrets | `internal/config`, focused `internal/cli` and Promptkit adapter mapping, maintained examples | 4 | Partly insufficient. Defaults, precedence, known-field parsing, active URL/duration/template checks, output-path preservation, profile-source rules, secret redaction, and examples are exercised. `AUD-004` through `AUD-007` record unsupported missing-source keys, blank notification identities, constructed-report override inconsistency, and non-atomic secret environment mutation. |
|
||||
| CLI parsing, output, and exit behavior | `internal/cli` | 5 | Pending |
|
||||
| Weather transport and normalized collection | `internal/adapters/weatherapi`, `internal/collect`, `internal/weatherdata` | 6 | Pending |
|
||||
| Forecast and fact derivation | `internal/forecast`, `internal/facts` | 7 | Pending |
|
||||
@@ -322,6 +328,155 @@ with evidence about meaningful risks, test ownership, gaps, and duplication.
|
||||
- Related findings: none
|
||||
- Remediation reference: pending
|
||||
|
||||
### AUD-004: Unsupported missing-source keys are silently accepted
|
||||
|
||||
- Stage: 4
|
||||
- Status: candidate
|
||||
- Severity: medium
|
||||
- Confidence: high
|
||||
- Category: correctness
|
||||
- Area: `internal/config.Validate`, `MissingSourceConfig.Sources`, and
|
||||
`internal/adapters/weatherapi.policyFor`
|
||||
- Evidence: The configuration contract lists seven optional source keys and
|
||||
states that hourly forecast data is always required. `Validate` checks only
|
||||
that each map key is nonblank and its policy is one of `error`, `warn`, or
|
||||
`none`; it never checks the key against the supported set. The Weather API
|
||||
adapter looks up policies by its own literal source names and otherwise uses
|
||||
the default. Its required-hourly path returns an error before consulting the
|
||||
policy. Consequently a typo such as `alert: error`, an arbitrary key, or an
|
||||
ineffective `hourly: none` loads successfully. Focused config tests cover an
|
||||
invalid policy value but no unsupported key.
|
||||
- Contract at risk: `missing_source.sources` accepts only the documented
|
||||
optional source identities, and invalid configuration must fail loading
|
||||
rather than silently changing the effective failure policy.
|
||||
- Impact: An operator can believe a source-specific `error`, `warn`, or `none`
|
||||
rule is active while runtime collection ignores it and applies the default;
|
||||
the mistake is visible only when that source is missing or malformed.
|
||||
- Recommendation: Give `internal/config` one authoritative supported-source
|
||||
set and reject every other key, including `hourly`. Keep adapter source names
|
||||
aligned with that owner without introducing a second independently maintained
|
||||
list.
|
||||
- Test implications: Add loaded and constructed configuration cases for every
|
||||
supported key plus representative typo, unknown, blank, and required-hourly
|
||||
keys. Assert behavior through `Validate` or `LoadFile`, not private map
|
||||
iteration.
|
||||
- Validation: The new unsupported-key cases fail against the current validator
|
||||
and pass when validation rejects keys that cannot affect optional-source
|
||||
policy; focused config and Weather API policy tests remain green.
|
||||
- Related findings: none
|
||||
- Remediation reference: pending
|
||||
|
||||
### AUD-005: Single-report notification identities may render blank
|
||||
|
||||
- Stage: 4
|
||||
- Status: candidate
|
||||
- Severity: medium
|
||||
- Confidence: high
|
||||
- Category: correctness
|
||||
- Area: `validateDistributorNotify`, `RenderDistributorBundleID`, and
|
||||
`RenderDistributorIdempotencyKey`
|
||||
- Evidence: Enabled single-report notification validation rejects an empty
|
||||
template and unknown variables, and `RenderDistributorPipelineID` rejects a
|
||||
whitespace-only result. In contrast, the bundle-ID and idempotency-key
|
||||
renderers return whitespace unchanged and validation does not perform a
|
||||
nonblank result check. Batch renderers consistently reject whitespace-only
|
||||
pipeline, bundle, and idempotency identities. The Distributor adapter checks
|
||||
only equality with the empty string, so whitespace values also pass its
|
||||
local request checks. Focused tests cover the single pipeline case and all
|
||||
three batch cases, but omit the two single-report cases.
|
||||
- Contract at risk: Enabled notification requires usable pipeline, bundle, and
|
||||
idempotency identities; configuration validation must reject invalid active
|
||||
templates before report execution.
|
||||
- Impact: A whitespace bundle ID or idempotency key can reach Distributor after
|
||||
report publication, risking rejected uploads or collisions between otherwise
|
||||
distinct report deliveries while configuration preflight reports success.
|
||||
- Recommendation: Apply the same trimmed-nonempty invariant to all three
|
||||
single-report identity renderers and exercise those renderers during active
|
||||
configuration validation.
|
||||
- Test implications: Extend the existing single-report validation table with
|
||||
whitespace-rendered bundle and idempotency identities, and retain the batch
|
||||
table as the parallel contract owner.
|
||||
- Validation: Both new cases fail against the current single-report renderers
|
||||
and pass when all active rendered identities are nonblank; focused config,
|
||||
app notification, and Distributor adapter tests pass.
|
||||
- Related findings: none
|
||||
- Remediation reference: pending
|
||||
|
||||
### AUD-006: Constructed report overrides depend on inaccessible presence flags
|
||||
|
||||
- Stage: 4
|
||||
- Status: candidate
|
||||
- Severity: medium
|
||||
- Confidence: high
|
||||
- Category: correctness
|
||||
- Area: `ReportConfig`, `ReportDistributorConfig`,
|
||||
`Config.ReportModuleOverrides`, and
|
||||
`Config.ReportDistributorPathOverrides`
|
||||
- Evidence: YAML unmarshalling sets private `deterministicModulesSet` and
|
||||
`pathTemplatesSet` flags to distinguish omission from an explicit list.
|
||||
Both public override accessors skip exported `DeterministicModules` and
|
||||
`PathTemplates` values unless the corresponding private flag is true. Code
|
||||
outside package `config` therefore cannot construct an equivalent override:
|
||||
setting a nonempty exported slice in `Config.Reports` is silently ignored,
|
||||
and `Validate` skips its contents as well. Tests named for loaded/constructed
|
||||
consistency reside inside package `config` and manually set the private
|
||||
flags, so they do not exercise the configuration shape available to app
|
||||
callers. Graph traces show the accessors feed app report resolution and
|
||||
single/batch Distributor path rendering.
|
||||
- Contract at risk: Loaded and programmatically constructed configurations
|
||||
with the same exported values should validate and produce the same report
|
||||
module and path overrides.
|
||||
- Impact: Repository callers and tests that construct a `config.Config` can
|
||||
silently run default modules or Distributor paths instead of their requested
|
||||
values, while invalid constructed overrides can pass validation unused.
|
||||
- Recommendation: Represent presence through an API available to downstream
|
||||
callers, such as constructors/setters or an optional-list representation
|
||||
that preserves omitted versus explicitly empty YAML without private state
|
||||
that exported values cannot establish.
|
||||
- Test implications: Move or add behavior tests from an external config test
|
||||
package or an app boundary so they can use only exported construction APIs;
|
||||
assert both nonempty overrides and explicit-empty rejection.
|
||||
- Validation: The external-boundary test fails with direct exported-field
|
||||
construction today and passes when loaded and constructed forms yield the
|
||||
same overrides and validation result.
|
||||
- Related findings: none
|
||||
- Remediation reference: pending
|
||||
|
||||
### AUD-007: Failed configuration loads can leave secrets applied
|
||||
|
||||
- Stage: 4
|
||||
- Status: candidate
|
||||
- Severity: low
|
||||
- Confidence: high
|
||||
- Category: security
|
||||
- Area: `internal/config.Load` and `loadSecrets`
|
||||
- Evidence: `Load` calls `loadSecrets` before `Validate`. The secret loader
|
||||
validates and applies directory entries one at a time with `os.Setenv`, with
|
||||
no preflight of all entries and no restoration of prior environment values.
|
||||
A valid secret is therefore installed even when later configuration
|
||||
validation fails, and an earlier secret remains installed when a later
|
||||
invalid entry, unreadable value, or failed environment assignment aborts the
|
||||
directory load. Existing tests cover successful replacement, line-ending
|
||||
trimming, rejection diagnostics, and omission of secret values from errors,
|
||||
but do not assert failure atomicity.
|
||||
- Contract at risk: A rejected configuration should not partially apply its
|
||||
secret-bearing environment interaction, and repeatable in-process loads
|
||||
should not inherit state from an unsuccessful attempt.
|
||||
- Impact: The normal CLI process exits after a load failure, limiting the
|
||||
operational effect, but embedded runners, repeated actions, and tests can
|
||||
observe or consume credentials from a configuration that was never accepted.
|
||||
- Recommendation: Validate non-secret configuration before secret application,
|
||||
preflight and read the complete secret directory before mutation, and either
|
||||
apply environment changes atomically with rollback or return a scoped lookup
|
||||
rather than mutating process-global state incrementally.
|
||||
- Test implications: Add cases for a valid secret followed by an invalid entry
|
||||
and for valid secrets paired with a later-invalid configuration, asserting
|
||||
that prior set/unset environment state is restored.
|
||||
- Validation: Both failure cases leave the environment exactly as it was before
|
||||
loading; successful overwrite and single-line-ending behavior remain intact.
|
||||
- Related findings: none
|
||||
- Remediation reference: pending
|
||||
|
||||
## Retained Decisions
|
||||
|
||||
### RET-001: Keep the application package as the explicit composition owner
|
||||
@@ -401,6 +556,40 @@ period calculations would conflate calendar and elapsed-time semantics.
|
||||
Reconsider only if a report's product contract changes which kind of time it
|
||||
represents.
|
||||
|
||||
### RET-008: Keep configured output paths declarative during loading
|
||||
|
||||
Configuration preserves a nonblank `output.directory` exactly as supplied and
|
||||
does not clean, absolutize, inspect, create, expand, or rebase it. App and
|
||||
publication owners later resolve it against the invocation working directory
|
||||
and perform destination-specific preflight. This keeps configuration loading
|
||||
free of working-directory and filesystem lifecycle policy while still rejecting
|
||||
a meaningless whitespace-only setting. Reconsider only if a new configuration
|
||||
contract requires an intrinsic path property that can be checked without
|
||||
assuming an execution destination.
|
||||
|
||||
### RET-009: Keep inactive optional integrations free of active-field validation
|
||||
|
||||
Distributor templates and delivery settings are validated only when
|
||||
notification is enabled, and batch templates only when both Distributor and
|
||||
batch notification are enabled. Likewise, a blank Promptkit local endpoint
|
||||
means the backend is unregistered and its configured default concurrency is
|
||||
dormant; CLI mapping forwards the limit only with an endpoint, while the
|
||||
adapter rejects an active limit without one. This lets complete defaults and
|
||||
examples describe optional integrations without making unused credentials or
|
||||
templates mandatory. Reconsider only if dormant configuration must become an
|
||||
installation-time lint contract distinct from runtime configuration loading.
|
||||
|
||||
### RET-010: Keep Promptkit profile source precedence in its adapter boundary
|
||||
|
||||
Configuration owns mutual exclusion of `profile_file` and `profile_dir`, URL
|
||||
and concurrency shape, and the selected profile ID. The Promptkit adapter owns
|
||||
opening configured sources, complete-definition precedence, embedded and
|
||||
built-in fallbacks, and classification of malformed profile errors before
|
||||
collection. Maintained profile tests exercise that boundary offline. Moving
|
||||
profile parsing into config would duplicate dependency-specific behavior and
|
||||
leak Promptkit mechanics. Reconsider only if Weatherreporter adopts a
|
||||
repository-owned profile format independent of Promptkit.
|
||||
|
||||
## Open Questions
|
||||
|
||||
No Stage 1 open questions or unexplained baseline failures remain.
|
||||
@@ -433,6 +622,24 @@ Stage 3 routed these investigation leads to their assigned later stages:
|
||||
referenced registries, embedded assets, generated-text schemas, and render
|
||||
compatibility rather than duplicating those audits here.
|
||||
|
||||
Stage 4 routed these investigation leads to their assigned later stages:
|
||||
|
||||
- Configuration accepts any absolute URL scheme with a host, matching the
|
||||
documented term “absolute URL.” Stages 6, 11, and 16 should assess transport
|
||||
scheme support at the Weather API, Promptkit, and Distributor boundaries
|
||||
before narrowing the configuration contract.
|
||||
- `weather_api.base_url` may be empty in a valid configuration because only
|
||||
collection workflows require it. Stages 5-6 should verify action-specific
|
||||
failure timing and diagnostics rather than making every non-collection
|
||||
configuration require an endpoint.
|
||||
- Report config-key normalization accepts trimmed and case-folded spellings in
|
||||
addition to the documented hyphen/underscore equivalence. Stage 24 should
|
||||
decide whether the canonical reference should describe that compatibility
|
||||
or the executable contract should be narrowed.
|
||||
- Distributor delivery redaction, runtime template consumption, and notification
|
||||
idempotency remain assigned to Stage 16. Stage 4 accounts only for active
|
||||
configuration and safe identity/path rendering before execution.
|
||||
|
||||
## Stage Log
|
||||
|
||||
### Stage 1: Establish The Baseline And Audit Ledger
|
||||
@@ -560,3 +767,58 @@ Stage 3 routed these investigation leads to their assigned later stages:
|
||||
- Retained decisions: `RET-005`, `RET-006`, and `RET-007`.
|
||||
- Open questions: the three leads recorded above are routed to their assigned
|
||||
later stages.
|
||||
|
||||
### Stage 4: Audit Configuration, Secrets, And Validation
|
||||
|
||||
- Status: Complete.
|
||||
- Scope reviewed: all production and focused-test code in `internal/config`;
|
||||
the configuration-to-executor mapping in `internal/cli`; Promptkit adapter
|
||||
configuration and focused tests; all three maintained examples;
|
||||
`docs/config.md`; and configuration-related architecture, operations,
|
||||
documentation, and testing rules.
|
||||
- Exclusions: CLI parsing and action-specific diagnostics remain assigned to
|
||||
Stage 5. Weather API transport behavior remains assigned to Stage 6.
|
||||
Promptkit source loading and sensitive execution diagnostics remain assigned
|
||||
to Stage 11. Output publication preflight remains assigned to Stages 15 and
|
||||
17, Distributor delivery remains assigned to Stage 16, and repository-wide
|
||||
documentation coherence remains assigned to Stage 24.
|
||||
|
||||
#### Precedence And Validation Map
|
||||
|
||||
| Contract or risk | Implementation and consumer evidence | Disposition |
|
||||
| --- | --- | --- |
|
||||
| Discovery and precedence | `Load` starts with `Defaults`, merges the explicit or default YAML path, tolerates only an absent implicit default file, applies nonempty units/timezone overrides, normalizes report modules, loads configured secrets, and validates. Representative CLI coverage proves explicit units/timezone beat file values while unrelated file values remain. | Matches the documented defaults → file → CLI order. Environment variables do not override config fields. |
|
||||
| YAML schema and migrations | `mergeFile` uses `KnownFields(true)`; custom notification, report, Distributor-path, and module decoders reject their own unknown fields. Retired `scriptorium`, recent-change, and workspace keys have focused migration/rejection tests. | Sufficient for known struct fields and retired top-level contracts. Unsupported keys inside the semantic missing-source map are `AUD-004`. |
|
||||
| Defaults and partial merges | Exact documented defaults are built once in `Defaults`; YAML decoding merges partial nested structs over them, and focused tests cover default literals, omitted optional sections, batch partial configuration, and both maintained config examples. | Matches the canonical reference. Exact-default assertions protect user-visible contracts rather than private constants. |
|
||||
| Output directory | Config accepts empty, relative, and absolute values, preserves them byte-for-byte, and rejects only nonempty whitespace. Graph traces place path resolution, inspection, creation, and publication below app/filesystem owners. | Matches policy; retained as `RET-008`. |
|
||||
| Weather and daypart validation | Config checks absolute base URLs when present, positive timeout, nonnegative precision, nonempty units/timezone, resolvable timezone, JSON format, nonempty dayparts, names, and parseable clocks. Collection separately requires a base URL for collection actions. | Matches the documented split. The Stage 3 DST defect remains `AUD-002`. |
|
||||
| Missing-source policy | Policy values and nonblank keys are validated, and the adapter consumes source-specific values by source name with a default fallback. Required hourly failures bypass optional-source policy. | Unsupported and ineffective keys pass silently; `AUD-004`. |
|
||||
| Report/module overrides | Loaded aliases, duplicate normalized reports, module identities/order, compatibility, duplicate modules/stanzas, typed options, explicit empty lists, and Distributor path templates are validated through report/module registries. | Loaded YAML behavior is well covered. Exported constructed values cannot establish private presence state; `AUD-006`. Detailed module semantics remain for Stages 8-9. |
|
||||
| Promptkit settings | Config validates mutually exclusive profile sources, positive timeout, optional absolute local endpoint, and nonnegative concurrency. CLI maps a dormant endpoint-less limit to the adapter zero value; adapter construction owns file/directory loading, backend registration, fallbacks, and safe error classification. | Matches the documented inactive/active split; retained as `RET-009` and `RET-010`. |
|
||||
| Secret directory | Empty disables loading. A configured directory accepts only directly contained regular files with environment-variable basenames, rejects symlinks/directories/nonregular/unreadable entries, removes one LF or CRLF, overwrites the named environment value, and reports only directory/file names and operation context. | Values are absent from tested diagnostics, but failure application is not atomic; `AUD-007`. |
|
||||
| Distributor configuration | Disabled notification skips active-field checks. Enabled notification validates endpoint, token variable name, timeout, failure policy, allowed template variables, batch activation, and report path safety/uniqueness. Templates render from repository-owned metadata rather than environment values. | Mostly sufficient. Two single-report rendered identities omit the nonblank invariant; `AUD-005`. |
|
||||
| Filesystem and URL timing | Config validates intrinsic string/URL shape. Output destinations are inspected by app/publication owners, Promptkit sources by its adapter, and secret entries by the config loader. No audit command contacted an external service. | Matches ownership boundaries for Stage 4; transport-specific scheme support is routed to later stages. |
|
||||
| Constructed configuration | `Validate`, report override accessors, and focused tests attempt to provide loaded/constructed consistency without mutating caller-owned option values. | General scalar validation is consistent, but report override presence is not externally constructible; `AUD-006`. |
|
||||
| Maintained examples | Both YAML configuration examples load through the strict config suite. The endpoint-only `weather-light` profile is inspected through the real Promptkit adapter with embedded assets and no network or credential. All examples contain synthetic endpoints and no secrets. | Matches documentation and testing policy. |
|
||||
|
||||
#### Commands And Evidence
|
||||
|
||||
- Used graph search, source snippets, call traces, and graph-augmented test
|
||||
searches for defaults, `Load`, YAML merging and migration rejection,
|
||||
`Validate`, secret loading, report/module/path traversal, notification
|
||||
renderers, CLI executor mapping, Promptkit adapter construction, Weather API
|
||||
missing-source policy consumption, and app notification consumers.
|
||||
- Compared executable fields, defaults, precedence, templates, paths, profile
|
||||
settings, source keys, and secret behavior with `docs/config.md`, maintained
|
||||
examples, and the configuration-related operations and architecture rules.
|
||||
- Ran the production CLI with `examples/minimal-config.yml`. It reached offline
|
||||
profile credential preflight and returned `missing_credential` before weather
|
||||
collection, confirming that the endpoint-less local concurrency default is
|
||||
intentionally dormant rather than rejected by adapter construction.
|
||||
- Ran
|
||||
`go test ./internal/config ./internal/adapters/promptkit ./internal/cli`;
|
||||
all focused packages passed.
|
||||
- Findings: `AUD-004`, `AUD-005`, `AUD-006`, and `AUD-007`.
|
||||
- Retained decisions: `RET-008`, `RET-009`, and `RET-010`.
|
||||
- Open questions: the four leads recorded above are routed to their assigned
|
||||
later stages.
|
||||
|
||||
Reference in New Issue
Block a user