Audit artifact paths and filesystem safety
This commit is contained in:
@@ -100,7 +100,7 @@ These are prioritization signals, not findings:
|
|||||||
| 0: baseline | complete | Revision/environment pinned; graph refreshed; inventories and every prescribed baseline command recorded. `TST-001` owns the non-blocking race limitation. |
|
| 0: baseline | complete | Revision/environment pinned; graph refreshed; inventories and every prescribed baseline command recorded. `TST-001` owns the non-blocking race limitation. |
|
||||||
| 1: contract and boundary map | complete | Canonical contracts and focused internal docs read; ownership, stage-contract, lifecycle, scenario, area, and preliminary risk-to-test matrices recorded below. |
|
| 1: contract and boundary map | complete | Canonical contracts and focused internal docs read; ownership, stage-contract, lifecycle, scenario, area, and preliminary risk-to-test matrices recorded below. |
|
||||||
| 2: runner and manifest | complete | Full/single-stage entry paths, every lifecycle outcome, both manifest models/transitions, save disagreement states, canonical invalidation boundaries, and runner lock lifetime reviewed. Focused app/manifest test and race commands passed. Confirmed `COR-001` and `RSK-001`; assigned `DUP-001`, `SIM-001`, `COM-001`, `TST-002`, and lock-release details to later stages. |
|
| 2: runner and manifest | complete | Full/single-stage entry paths, every lifecycle outcome, both manifest models/transitions, save disagreement states, canonical invalidation boundaries, and runner lock lifetime reviewed. Focused app/manifest test and race commands passed. Confirmed `COR-001` and `RSK-001`; assigned `DUP-001`, `SIM-001`, `COM-001`, `TST-002`, and lock-release details to later stages. |
|
||||||
| 3: paths and filesystem | not_started | Assigned path, lock, artifact, and mutation questions below. |
|
| 3: paths and filesystem | complete | Canonical local/remote paths, every artifact source family, filesystem mutations, directory promotion, cleanup confinement, permissions, and lock mechanics reviewed. Focused normal and race commands passed. Confirmed `COR-002`, `COR-003`, `RSK-002`, `RSK-003`, and `RSK-004`; added `DUP-002` and `TST-003`, and refined `DUP-001`. |
|
||||||
| 4: publish and cleanup | not_started | Assigned remote commit and cleanup scenarios below. |
|
| 4: publish and cleanup | not_started | Assigned remote commit and cleanup scenarios below. |
|
||||||
| 5: restore and previous state | not_started | Assigned restore and previous-cache scenarios below. |
|
| 5: restore and previous state | not_started | Assigned restore and previous-cache scenarios below. |
|
||||||
| 6: configuration and composition | not_started | Assigned configuration, CLI composition, and process-boundary areas below. |
|
| 6: configuration and composition | not_started | Assigned configuration, CLI composition, and process-boundary areas below. |
|
||||||
@@ -128,9 +128,9 @@ has been mapped but not behaviorally audited.
|
|||||||
| Analyze and artifact dependencies | `internal/stage`, `internal/artifacts`, `internal/artifactpolicy` | 10 | assigned |
|
| Analyze and artifact dependencies | `internal/stage`, `internal/artifacts`, `internal/artifactpolicy` | 10 | assigned |
|
||||||
| Publish and cleanup | `internal/stage`, `internal/app` | 4 | assigned |
|
| Publish and cleanup | `internal/stage`, `internal/app` | 4 | assigned |
|
||||||
| Manifest state | `internal/manifest`, transition policy in `internal/app` | 2 | reviewed |
|
| Manifest state | `internal/manifest`, transition policy in `internal/app` | 2 | reviewed |
|
||||||
| Artifacts, paths, and policy | `internal/artifacts`, `internal/artifactpolicy`, `internal/pathsafe` | 3 (resolution consumption revisited in 10) | assigned |
|
| Artifacts, paths, and policy | `internal/artifacts`, `internal/artifactpolicy`, `internal/pathsafe` | 3 (resolution consumption revisited in 10) | reviewed |
|
||||||
| Restore | `internal/app`, `internal/artifacts`, `internal/previouscache`, `internal/audio` | 5 | assigned |
|
| Restore | `internal/app`, `internal/artifacts`, `internal/previouscache`, `internal/audio` | 5 | assigned |
|
||||||
| File operations | `internal/fileops`, `internal/pathsafe`, local artifact store | 3 (promotion vertical slice in 9) | assigned |
|
| File operations | `internal/fileops`, `internal/pathsafe`, local artifact store | 3 (promotion vertical slice in 9) | reviewed |
|
||||||
| External adapters and storage | `internal/adapters`, `internal/audio` | 7 | assigned |
|
| External adapters and storage | `internal/adapters`, `internal/audio` | 7 | assigned |
|
||||||
| Shared models and diagnostics | `internal/artifactmodel`, `internal/contracts`, `internal/logging` | 7 (maintainability revisited in 11) | assigned |
|
| Shared models and diagnostics | `internal/artifactmodel`, `internal/contracts`, `internal/logging` | 7 (maintainability revisited in 11) | assigned |
|
||||||
| Tests, examples, and automation | package test owners, `examples/`, `.woodpecker/` | 12 | assigned |
|
| Tests, examples, and automation | package test owners, `examples/`, `.woodpecker/` | 12 | assigned |
|
||||||
@@ -361,6 +361,127 @@ Stage 2 test observations for this matrix:
|
|||||||
| Interruption and restart | Non-succeeded action logic and retry tests indirectly prove `running` is rerunnable. | No kill/reload normalization, reconciliation, or abandoned-run status test; confirmed `RSK-001`. |
|
| Interruption and restart | Non-succeeded action logic and retry tests indirectly prove `running` is rerunnable. | No kill/reload normalization, reconciliation, or abandoned-run status test; confirmed `RSK-001`. |
|
||||||
| Same-session concurrency | Artifact store has focused lock tests. | No assembled concurrent runner or release-failure test; Stage 3 owns the mechanism and sufficiency decision. |
|
| Same-session concurrency | Artifact store has focused lock tests. | No assembled concurrent runner or release-failure test; Stage 3 owns the mechanism and sufficiency decision. |
|
||||||
|
|
||||||
|
## Path, Artifact, Filesystem, And Lock Conclusions
|
||||||
|
|
||||||
|
### Canonical ownership and normalization
|
||||||
|
|
||||||
|
`internal/artifacts/paths.go` owns the campaign/session layout, run-local
|
||||||
|
layout, previous cache, Notarius bundle, spool, and audio-cache constructors.
|
||||||
|
`internal/artifacts/s3_keys.go` owns session/run/current and published-output
|
||||||
|
keys. `internal/pathsafe` is the shared lexical boundary for slash-normalized
|
||||||
|
relative destinations: it rejects empty, absolute, drive-qualified, traversal,
|
||||||
|
and leading-backslash forms, normalizes mixed separators, and verifies a
|
||||||
|
joined destination remains lexically under its root. `internal/artifactpolicy`
|
||||||
|
adds configured source/destination policy without performing filesystem I/O.
|
||||||
|
|
||||||
|
Most production callers consume those owners directly. Previous-cache planning
|
||||||
|
normalizes configured and manifest-derived relative paths before calling the
|
||||||
|
path constructor, and restore derives a normalized relative path before
|
||||||
|
installing it. One ad hoc reconstruction remains in publish:
|
||||||
|
`resolvePublishRunManifestSource` joins the literal `manifest.json` to an
|
||||||
|
already-derived run root instead of using the canonical run-manifest helper;
|
||||||
|
`DUP-002` assigns that maintainability decision to Stage 11.
|
||||||
|
|
||||||
|
The constructors themselves do not enforce that campaign, session, run, or
|
||||||
|
artifact-relative components are safe opaque segments. Configuration currently
|
||||||
|
checks session identifiers and campaign identity only for presence. As a
|
||||||
|
result, traversal-bearing operator identity reaches both local `filepath.Join`
|
||||||
|
and remote `path.Join`; `COR-002` records the concrete namespace escape. The
|
||||||
|
exported previous-artifact helper also promises a path under
|
||||||
|
`previous/artifacts` while accepting a traversal value, although its current
|
||||||
|
production callers validate first. That helper-level contract should be fixed
|
||||||
|
with the same boundary rather than treated as a separate defect.
|
||||||
|
|
||||||
|
### Artifact resolution
|
||||||
|
|
||||||
|
The complete resolution chain is deterministic and matches the documented
|
||||||
|
source-family distinctions:
|
||||||
|
|
||||||
|
- built-ins normalize against a fixed registry, prefer matching producer
|
||||||
|
outputs from the manifest, validate content shape, then use a canonical
|
||||||
|
session fallback; missing state returns `SessionArtifactNotFoundError`;
|
||||||
|
- configured sources resolve only through explicit runtime-catalog
|
||||||
|
availability, preserve generated-versus-disabled-file provenance, validate
|
||||||
|
non-empty text, and return the same typed missing condition when planned but
|
||||||
|
unavailable;
|
||||||
|
- extraction hydration requires a successful current extract record, exact
|
||||||
|
configured definitions, producer/receipt identity, compatible contract and
|
||||||
|
external provenance, confined regular files, a complete index/lane set, and
|
||||||
|
matching checksums before making any source available;
|
||||||
|
- previous-session planning sorts requirements and records, validates relative
|
||||||
|
cache destinations, loads current remote state with expected campaign,
|
||||||
|
session, and run identity, and hydrates only explicit remote objects into the
|
||||||
|
local cache. Analyze-time resolution is then local-only, preferring a
|
||||||
|
manifest input path before the documented filesystem fallback and returning
|
||||||
|
a typed missing error; and
|
||||||
|
- current-state loading has distinct typed missing-pointer and missing-manifest
|
||||||
|
errors, rejects empty or malformed state, and validates expected campaign,
|
||||||
|
session, and run identity or pointer/manifest run consistency.
|
||||||
|
|
||||||
|
Only extraction claims checksum/contract/external-provenance enforcement at
|
||||||
|
resolution time, and its implementation enforces all three. Built-in,
|
||||||
|
configured, and previous-cache resolvers enforce their documented content and
|
||||||
|
availability rules but do not re-hash ordinary manifest records. That is not a
|
||||||
|
Stage 3 contract mismatch; Stages 5 and 10 remain responsible for deciding
|
||||||
|
whether restore or analyze threat models require stronger trust than the
|
||||||
|
documented previous-cache fallback and manifest-aware built-in behavior.
|
||||||
|
|
||||||
|
### Filesystem mutation mechanics
|
||||||
|
|
||||||
|
The mutation inventory separates lexical policy from explicit-path mechanics:
|
||||||
|
|
||||||
|
| Mechanism | Normal-process atomicity and cleanup | Confinement and durability conclusion |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| `WriteFileAtomic` | Same-directory temporary file; write, file sync, close, chmod, rename; removes a failed temporary file; replaces an existing destination where the platform rename permits it. | Accepts an explicit destination as intended, but follows symlinked destination ancestors and does not sync the parent after rename. |
|
||||||
|
| `CopyFileAtomicWithChecksum` / `CopyFileAtomic` | Opens the explicit source, copies and hashes through a same-directory temporary file, syncs/closes/chmods, then renames; prior destination survives failures before rename. | Does not require a regular source, follows source and destination symlinks, and does not sync the destination parent. Current canonical materialization callers validate content but inherit destination confinement and crash-durability findings. |
|
||||||
|
| `InstallDownloadedTempFile` | Chmods and renames a caller-supplied sibling temporary file; same-filesystem placement is established by current audio, previous-cache, and restore callers. | Does not sync the downloaded file or destination directory. The object-store downloads close their path-based files but expose no completed-data sync guarantee. |
|
||||||
|
| `PromoteDirectory` | Copies to a temporary sibling, permits only regular files/directories, uses confined source handles and identity checks, syncs files/directories, atomically installs without replacement, syncs the destination parent, cleans failures, and reports unsupported platforms before creating the promotion tree. | Source replacement, source symlinks/non-regular entries, existing or concurrently-created destinations, same-filesystem placement, and platform behavior are strongly handled. Destination ancestors remain path-based and can be symlinked or replaced. |
|
||||||
|
| Cleanup removal | Refuses empty roots/targets, lexical root deletion and traversal, wrong final-node type, and a final target that is a symlink. Missing targets are idempotent. | It does not inspect root or ancestor components and performs `RemoveAll` after a path-based check, so a symlinked ancestor can redirect deletion outside the root. |
|
||||||
|
|
||||||
|
Low-level file and storage helpers correctly accept explicit destinations and
|
||||||
|
do not infer stage, workspace, or cleanup policy. Policy belongs in their
|
||||||
|
callers. The current caller-side checks are lexical and final-node checks,
|
||||||
|
however, so they cannot prevent destination-ancestor symlink traversal or
|
||||||
|
replacement. `COR-003` consolidates the write, promotion-destination, and
|
||||||
|
destructive-cleanup variants under that single root cause; later stage and
|
||||||
|
adapter audits should reference it rather than duplicate it.
|
||||||
|
|
||||||
|
Single-file writers preserve the prior visible file on failures before rename,
|
||||||
|
but unlike directory promotion they do not sync the containing directory after
|
||||||
|
rename. Download installs additionally do not establish a file-sync boundary.
|
||||||
|
`RSK-002` records the resulting crash/power-loss durability gap for canonical
|
||||||
|
files and manifests. Stage 3 also confirmed that `DUP-001` duplicates this
|
||||||
|
same incomplete persistence sequence across both manifest save paths.
|
||||||
|
|
||||||
|
Runtime layout and copied/generated files use fixed requested modes `0755` and
|
||||||
|
`0644` subject to process umask, including transcripts, artifacts, logs,
|
||||||
|
manifests, and the lock. No operating contract requires a restrictive umask or
|
||||||
|
documents an ownership/mode boundary. `RSK-004` records the private-data
|
||||||
|
exposure risk rather than assuming a deployment-specific parent directory is
|
||||||
|
always restrictive.
|
||||||
|
|
||||||
|
### Lock scenario and focused tests
|
||||||
|
|
||||||
|
The local lock is an atomically created `O_CREATE|O_EXCL` sentinel. This is
|
||||||
|
sufficient to serialize two live contenders before either reads a manifest,
|
||||||
|
and the existing focused conflict test exercises that mechanism. It is not an
|
||||||
|
OS-owned advisory lock: any existing sentinel conflicts without interpreting
|
||||||
|
the recorded PID/time, process death cannot remove it, and recovery is manual.
|
||||||
|
Release closes then unlinks, but the runner discards its error. An unlink
|
||||||
|
failure can therefore let a command report success while every later invocation
|
||||||
|
is blocked. `RSK-003` resolves Scenario 10 and the Stage 2 release question.
|
||||||
|
|
||||||
|
Existing focused tests strongly cover lexical mixed-slash/traversal handling,
|
||||||
|
artifact-source ordering and typed missing states, extraction trust checks,
|
||||||
|
current-state identity, ordinary atomic replacement/temp cleanup, source-side
|
||||||
|
promotion races and special files, no-replace installation, permissions, and
|
||||||
|
basic lock acquire/conflict/release. They do not cover unsafe identity segments,
|
||||||
|
destination-ancestor symlinks or replacement, cleanup through a symlinked
|
||||||
|
ancestor, directory-sync/file-sync guarantees, stale-lock ownership/recovery,
|
||||||
|
release failure, or assembled concurrent runners. `TST-003` assigns a narrow
|
||||||
|
cross-package regression set to Stage 12; each confirmed finding below also
|
||||||
|
identifies its smallest behavior-level test.
|
||||||
|
|
||||||
## Confirmed Findings
|
## Confirmed Findings
|
||||||
|
|
||||||
### `COR-001`: session identity initialization preserves stale invocation paths and accepts conflicting identity
|
### `COR-001`: session identity initialization preserves stale invocation paths and accepts conflicting identity
|
||||||
@@ -402,6 +523,88 @@ Stage 2 test observations for this matrix:
|
|||||||
6 owns configuration identity validation. They should reference this root
|
6 owns configuration identity validation. They should reference this root
|
||||||
finding rather than create duplicates.
|
finding rather than create duplicates.
|
||||||
|
|
||||||
|
### `COR-002`: unsafe identity components escape canonical local and remote namespaces
|
||||||
|
|
||||||
|
- Category: confirmed correctness/security defect.
|
||||||
|
- Locations/invariant: `internal/artifacts/paths.go` canonical constructors,
|
||||||
|
`internal/artifacts/s3_keys.go` in `S3SessionPrefix`, and
|
||||||
|
`internal/config/validate.go` in `validateCampaign`, `validateSession`, and
|
||||||
|
`validateSessionIdentifier`. Canonical identities must be opaque safe
|
||||||
|
segments, and traversal must not broaden local or remote operation scope.
|
||||||
|
- Evidence: campaign, session, and previous-session identifiers are checked
|
||||||
|
only for non-emptiness. Path/key constructors clean and join the raw values
|
||||||
|
without rejecting `.`/`..`, separators, drive forms, or traversal. A value
|
||||||
|
such as `../../outside` therefore changes the cleaned local workspace/spool
|
||||||
|
destination and the S3 namespace. `EnsureLayoutFor` also validates only
|
||||||
|
presence before creating the derived directories. The exported
|
||||||
|
`SessionPreviousArtifactPath*` helpers similarly claim an under-root result
|
||||||
|
but accept traversal; current production callers happen to validate their
|
||||||
|
relative artifact values first.
|
||||||
|
- Realistic scenario: an operator typo, generated session config, or untrusted
|
||||||
|
restored identity containing traversal makes prepare create/copy files
|
||||||
|
outside the intended workspace or spool root. Publish can read or write a
|
||||||
|
different campaign/session prefix, potentially colliding with unrelated
|
||||||
|
remote state.
|
||||||
|
- Impact/likelihood/confidence: high local and remote integrity/confidentiality
|
||||||
|
impact; low-to-moderate likelihood because ordinary date-like IDs are safe
|
||||||
|
but configuration is operator-controlled; high confidence from direct data
|
||||||
|
flow into canonical joins.
|
||||||
|
- Estimated remediation scope and owner: small-to-medium shared
|
||||||
|
config/artifacts change. Define one strict opaque-segment contract, validate
|
||||||
|
identities before layout/key construction, and make canonical helpers fail
|
||||||
|
closed rather than return an escaped path. Decide compatibility for existing
|
||||||
|
non-segment IDs explicitly in Stage 6.
|
||||||
|
- Test changes: table-test separator, traversal, absolute/drive, dot, and mixed-
|
||||||
|
slash identities at the configuration boundary; add artifacts tests proving
|
||||||
|
no local directory or S3 key can escape its expected namespace. Add a direct
|
||||||
|
unsafe-relative test for the exported previous-artifact constructor if it
|
||||||
|
remains public.
|
||||||
|
- Dependencies: Stage 6 owns configuration compatibility and validation.
|
||||||
|
`COR-001` separately owns conflicts among otherwise valid identities;
|
||||||
|
`COR-003` owns symlink-based escape after lexical identities are safe.
|
||||||
|
|
||||||
|
### `COR-003`: filesystem mutation confinement follows symlinked destination ancestors
|
||||||
|
|
||||||
|
- Category: confirmed correctness/security defect.
|
||||||
|
- Locations/invariant: lexical joins in `internal/pathsafe`, path-based writers
|
||||||
|
in `internal/fileops/fileops.go`, destination setup in
|
||||||
|
`internal/fileops/directory.go`, local layout/copy operations in
|
||||||
|
`internal/artifacts/local.go`, and cleanup validation/removal in
|
||||||
|
`internal/app/cleanup_targets.go`, `clean.go`, and
|
||||||
|
`post_publish_cleanup.go`. Writes, replacements, promotions, and deletions
|
||||||
|
must remain beneath an explicit root despite symlinks or replacement races.
|
||||||
|
- Evidence: lexical `Rel` checks cannot observe filesystem links. File writers
|
||||||
|
call `MkdirAll`/`CreateTemp`/`Rename` through destination paths. Promotion
|
||||||
|
strongly confines and identity-checks its source, but only path-checks the
|
||||||
|
destination parent before creating/installing the sibling tree. Cleanup
|
||||||
|
`Lstat`s only the final target and then calls `RemoveAll`; it neither rejects
|
||||||
|
a symlinked root/ancestor nor holds a confined directory handle. A target
|
||||||
|
such as `root/campaign/session`, where `campaign` is a symlink to an outside
|
||||||
|
tree, passes the lexical and final-node checks and deletes the outside
|
||||||
|
session directory. The analogous ancestor redirects writes and promotion.
|
||||||
|
- Realistic scenario: a stale, user-created, restored, or concurrently replaced
|
||||||
|
workspace component redirects prepare/materialization into another tree; a
|
||||||
|
later `clean session` or post-publish cleanup recursively removes data there.
|
||||||
|
The same gap permits a time-of-check/time-of-use replacement of a destination
|
||||||
|
ancestor.
|
||||||
|
- Impact/likelihood/confidence: critical destructive and confidentiality impact;
|
||||||
|
low-to-moderate likelihood depending on workspace ownership and multi-user
|
||||||
|
exposure; high confidence from the path-based checks and standard symlink
|
||||||
|
resolution semantics.
|
||||||
|
- Estimated remediation scope and owner: medium shared filesystem change. Use
|
||||||
|
root-relative directory handles/no-follow component traversal (with explicit
|
||||||
|
platform behavior) for mutations and deletion, and carry validated handles
|
||||||
|
through install/remove where feasible. Keep `fileops` policy-neutral by
|
||||||
|
passing an explicit root/destination capability rather than inferring stage
|
||||||
|
policy.
|
||||||
|
- Test changes: real-filesystem tests for a symlinked root, intermediate
|
||||||
|
component, destination parent replacement, and cleanup ancestor; assert an
|
||||||
|
outside sentinel survives and no outside temporary/output is created. Retain
|
||||||
|
the existing promotion source-race suite as the model for narrow hooks.
|
||||||
|
- Dependencies: Stages 4, 5, 7-9 must reference this finding for their concrete
|
||||||
|
cleanup, restore/download, adapter, stage-materialization, and extraction
|
||||||
|
consequences. `COR-002` owns lexical identity traversal separately.
|
||||||
|
|
||||||
### `RSK-001`: invocation audit records can remain indefinitely `running`
|
### `RSK-001`: invocation audit records can remain indefinitely `running`
|
||||||
|
|
||||||
- Category: confirmed correctness/operational risk.
|
- Category: confirmed correctness/operational risk.
|
||||||
@@ -438,10 +641,113 @@ Stage 2 test observations for this matrix:
|
|||||||
status/restore presents abandoned runs; Stage 13 should decide whether true
|
status/restore presents abandoned runs; Stage 13 should decide whether true
|
||||||
process interruption is accepted risk after handled errors are fixed.
|
process interruption is accepted risk after handled errors are fixed.
|
||||||
|
|
||||||
|
### `RSK-002`: single-file atomic replacement is not crash-durable
|
||||||
|
|
||||||
|
- Category: confirmed data-durability risk.
|
||||||
|
- Locations/invariant: `internal/fileops.WriteFileAtomic`,
|
||||||
|
`CopyFileAtomicWithChecksum`, `InstallDownloadedTempFile`, and both atomic
|
||||||
|
manifest-save sequences in `internal/manifest/store.go`. Successful canonical
|
||||||
|
files and durable ledgers must survive a crash/power-loss boundary consistent
|
||||||
|
with reported success.
|
||||||
|
- Evidence: writers that create their own temporary file sync its data before
|
||||||
|
rename, but none syncs the containing directory after rename. Download
|
||||||
|
callers close an initially empty sibling temp before object-store download,
|
||||||
|
and neither the download interface nor `InstallDownloadedTempFile` syncs the
|
||||||
|
completed file before rename. Directory promotion already demonstrates the
|
||||||
|
stronger sequence by syncing copied files, temporary directories, and the
|
||||||
|
destination parent after no-replace rename.
|
||||||
|
- Realistic scenario: a command reports a saved session/run manifest or
|
||||||
|
materialized canonical output, then the host loses power. The directory entry
|
||||||
|
rename is not durable and can disappear or expose filesystem-dependent state;
|
||||||
|
a downloaded restore/previous/audio file has an additional unsynced-data
|
||||||
|
window.
|
||||||
|
- Impact/likelihood/confidence: high integrity/recovery impact; low likelihood
|
||||||
|
per invocation but cumulative operational exposure; high confidence that the
|
||||||
|
sync calls are absent, with exact failure manifestation filesystem-dependent.
|
||||||
|
- Estimated remediation scope and owner: small-to-medium shared fileops and
|
||||||
|
manifest change. Centralize the durable temp-file install sequence, sync
|
||||||
|
completed downloads before install, then sync the parent directory with the
|
||||||
|
same explicit platform policy used by directory promotion.
|
||||||
|
- Test changes: introduce a narrow injectable sync/rename seam or ordered
|
||||||
|
filesystem-operation fake to assert file-sync-before-rename and directory-
|
||||||
|
sync-after-rename for write, copy, download install, and both manifest types;
|
||||||
|
retain real-filesystem overwrite/temp-cleanup tests for visible atomicity.
|
||||||
|
- Dependencies: `DUP-001` is the maintainability multiplier. Stages 5 and 7
|
||||||
|
should reference this root for restore/audio/storage download behavior rather
|
||||||
|
than create new durability findings.
|
||||||
|
|
||||||
|
### `RSK-003`: stale sentinel locks can block a session indefinitely and release failures are hidden
|
||||||
|
|
||||||
|
- Category: confirmed availability/operational risk.
|
||||||
|
- Locations/invariant: `internal/artifacts/local.go` lock acquisition/release,
|
||||||
|
the ignored deferred release in `internal/app/runner.go`, and the manual stale
|
||||||
|
lock procedure in `docs/troubleshooting.md`. A live same-session invocation
|
||||||
|
must exclude competitors, while completed or dead ownership must have a safe,
|
||||||
|
observable recovery path.
|
||||||
|
- Evidence: `O_CREATE|O_EXCL` correctly serializes live contenders, but any
|
||||||
|
existing `.lock` conflicts without checking whether its recorded PID/time is
|
||||||
|
live. Process death leaves the file forever. `ReleaseSessionLock` can report
|
||||||
|
close or unlink failure, but the runner defers it as `_ = ...`; an unlink
|
||||||
|
failure can therefore be reported as command success while the next run is
|
||||||
|
blocked. Recovery requires the operator to inspect process state and manually
|
||||||
|
delete the file.
|
||||||
|
- Realistic scenario: the process is killed or the filesystem rejects unlink
|
||||||
|
after a successful long run. Every later invocation for the session fails at
|
||||||
|
acquisition until an operator notices and safely removes the sentinel.
|
||||||
|
- Impact/likelihood/confidence: medium-to-high availability impact; moderate
|
||||||
|
lifetime likelihood for interruption and low likelihood for unlink failure;
|
||||||
|
high confidence. Mutual exclusion itself is sound in the ordinary live-
|
||||||
|
process case.
|
||||||
|
- Estimated remediation scope and owner: medium artifact-store/application
|
||||||
|
change. Prefer an OS-released lock while retaining useful metadata, or define
|
||||||
|
a conservative ownership/lease protocol; surface release failures without
|
||||||
|
obscuring an earlier command error and document automated versus manual
|
||||||
|
recovery.
|
||||||
|
- Test changes: add process/concurrency coverage for live exclusion and death
|
||||||
|
recovery, an injectable close/unlink failure proving the command cannot
|
||||||
|
silently succeed, and a subsequent-acquisition assertion. Do not encode
|
||||||
|
unsafe PID reuse assumptions in a unit test.
|
||||||
|
- Dependencies: `RSK-001` covers abandoned invocation audit state, not
|
||||||
|
exclusion. Stage 5 should check operator status presentation; Stage 12 owns
|
||||||
|
the smallest durable assembled-runner case.
|
||||||
|
|
||||||
|
### `RSK-004`: default runtime modes can expose private campaign material to other local users
|
||||||
|
|
||||||
|
- Category: confirmed security/operational risk.
|
||||||
|
- Locations/invariant: layout and mutation modes across
|
||||||
|
`internal/artifacts/local.go`, `internal/fileops`, stage/download writers, and
|
||||||
|
manifest persistence; security contract in `docs/policy/architecture.md`.
|
||||||
|
Transcripts, prompts, artifacts, reports, logs, and manifests are private
|
||||||
|
campaign material.
|
||||||
|
- Evidence: runtime directories request `0755` and files request `0644`, subject
|
||||||
|
only to ambient process umask. The default workspace is `/var/lib/narratio`;
|
||||||
|
Narratio can create its layout with those modes, and no operations contract
|
||||||
|
requires a restrictive umask, private parent, service-user ownership, or
|
||||||
|
configurable mode policy. Fixed-mode promotion also normalizes bundle files
|
||||||
|
to `0644` and directories to `0755`.
|
||||||
|
- Realistic scenario: a service or operator runs with the common `0022` umask
|
||||||
|
on a multi-user host. Other local accounts can traverse the workspace and
|
||||||
|
read transcripts, prompts, generated artifacts, diagnostics, and manifests.
|
||||||
|
- Impact/likelihood/confidence: high confidentiality impact; environment-
|
||||||
|
dependent but realistic likelihood; high confidence in requested modes and
|
||||||
|
documentation absence, moderate confidence in exposure on any particular
|
||||||
|
deployment because parent ACLs can mitigate it.
|
||||||
|
- Estimated remediation scope and owner: medium operations/configuration and
|
||||||
|
shared-writer change. Establish secure directory/file defaults, preserve
|
||||||
|
deliberate executability where needed, define ownership/ACL/umask
|
||||||
|
expectations, and provide an explicit compatibility/migration story.
|
||||||
|
- Test changes: assert privacy-oriented effective modes under a controlled
|
||||||
|
permissive umask for representative layout, manifest, artifact, log, and
|
||||||
|
promoted-bundle paths; document platform/ACL limitations instead of assuming
|
||||||
|
POSIX bits are universal.
|
||||||
|
- Dependencies: Stage 6 owns configuration/default compatibility, Stage 7 owns
|
||||||
|
adapter diagnostics, and Stage 13 should rank the risk against deployment
|
||||||
|
assumptions.
|
||||||
|
|
||||||
## Candidate Register
|
## Candidate Register
|
||||||
|
|
||||||
The remaining candidates require inspection by their named owners. Stage 2
|
The remaining candidates require inspection by their named owners. Stages 2
|
||||||
promoted `COR-001` and `RSK-001` into the confirmed register above.
|
and 3 have promoted their confirmed defects and risks into the register above.
|
||||||
|
|
||||||
### `ARC-001`: `IODecl` is not a complete or consistently classified stage contract
|
### `ARC-001`: `IODecl` is not a complete or consistently classified stage contract
|
||||||
|
|
||||||
@@ -513,12 +819,48 @@ promoted `COR-001` and `RSK-001` into the confirmed register above.
|
|||||||
- Evidence: `LocalStore.Save` contains its own temp/create/write/sync/close/
|
- Evidence: `LocalStore.Save` contains its own temp/create/write/sync/close/
|
||||||
context-check/rename sequence, while `SaveRun` delegates the same mechanism
|
context-check/rename sequence, while `SaveRun` delegates the same mechanism
|
||||||
to `writeJSONAtomically`. Error prefixes differ, but the durability mechanism
|
to `writeJSONAtomically`. Error prefixes differ, but the durability mechanism
|
||||||
is otherwise repeated.
|
is otherwise repeated. Stage 3 confirmed that both copies omit the same
|
||||||
|
parent-directory sync required by `RSK-002`.
|
||||||
- Realistic risk: a future durability, cleanup, permission, or platform fix may
|
- Realistic risk: a future durability, cleanup, permission, or platform fix may
|
||||||
reach only one manifest type, creating different guarantees for the two
|
reach only one manifest type, creating different guarantees for the two
|
||||||
ledgers.
|
ledgers.
|
||||||
- Confirmation owners: Stage 3 should compare filesystem guarantees and Stage
|
- Confirmation owners: Stage 3 should compare filesystem guarantees and Stage
|
||||||
11 should decide whether sharing the helper preserves useful error context.
|
11 should decide whether sharing the helper preserves useful error context.
|
||||||
|
The filesystem comparison is complete; the simplification decision remains
|
||||||
|
assigned to Stage 11.
|
||||||
|
|
||||||
|
### `DUP-002`: publish reconstructs the canonical run-manifest path
|
||||||
|
|
||||||
|
- Category: duplicated path-ownership candidate.
|
||||||
|
- Evidence: `internal/stage/publish.go` in
|
||||||
|
`resolvePublishRunManifestSource` joins the literal `manifest.json` to an
|
||||||
|
already-derived run root, while `internal/artifacts` owns
|
||||||
|
`SessionRunManifestPath*` and the run-manifest path model. The same literal is
|
||||||
|
repeated when constructing the upload-relative record.
|
||||||
|
- Realistic risk: a layout/name change can update canonical path construction
|
||||||
|
without updating publish discovery, causing a completed run to fail publish
|
||||||
|
or upload the wrong record. Current values agree, so this is not a correctness
|
||||||
|
defect at the audited revision.
|
||||||
|
- Confirmation owner: Stage 11 should decide whether publish should receive the
|
||||||
|
canonical run-manifest path from its caller/model or use an artifacts helper;
|
||||||
|
preserve the explicit remote relative name separately if it is a protocol
|
||||||
|
constant.
|
||||||
|
|
||||||
|
### `TST-003`: filesystem safety tests omit destination and lock-recovery boundaries
|
||||||
|
|
||||||
|
- Category: test-suite sufficiency candidate.
|
||||||
|
- Evidence: focused path/file tests cover lexical traversal, mixed slashes,
|
||||||
|
ordinary atomic overwrite/cleanup, promotion source symlinks and replacement,
|
||||||
|
no-replace installation, and basic lock conflict/release. No test covers an
|
||||||
|
unsafe identity component, symlinked destination ancestor, destination-parent
|
||||||
|
replacement, cleanup through an ancestor symlink, file/directory sync order,
|
||||||
|
stale lock recovery, lock release failure, or concurrent assembled runners.
|
||||||
|
- Realistic risk: the confirmed `COR-002`, `COR-003`, `RSK-002`, and `RSK-003`
|
||||||
|
mechanisms can regress or be only partially repaired while a broad focused
|
||||||
|
suite remains green.
|
||||||
|
- Confirmation owner: Stage 12 should select one narrow behavior-level case per
|
||||||
|
distinct invariant and reuse shared low-level tests across callers. Avoid
|
||||||
|
duplicating every path spelling or persistence call sequence.
|
||||||
|
|
||||||
### `SIM-001`: runner terminalization and persistence ordering lack a narrow owner
|
### `SIM-001`: runner terminalization and persistence ordering lack a narrow owner
|
||||||
|
|
||||||
@@ -557,8 +899,11 @@ promoted `COR-001` and `RSK-001` into the confirmed register above.
|
|||||||
| Coverage percentages | deferred diagnostic signals, not findings | Stage 12 must reason from risk and test ownership, not a numeric target. |
|
| Coverage percentages | deferred diagnostic signals, not findings | Stage 12 must reason from risk and test ownership, not a numeric target. |
|
||||||
| Session `last_error` survives a later stage success | rejected as a current-state defect at Stage 2 | No production reader was found; current status and per-stage error are authoritative, so the field can serve as historical context. Reopen only if an operator surface treats it as the active error. |
|
| Session `last_error` survives a later stage success | rejected as a current-state defect at Stage 2 | No production reader was found; current status and per-stage error are authoritative, so the field can serve as historical context. Reopen only if an operator surface treats it as the active error. |
|
||||||
| Minimal loaded-manifest status/timestamp validation | rejected as a standalone Stage 2 finding | Unknown/non-succeeded statuses fail conservatively into execution, nil maps/records are normalized, and no realistic unsafe caller was established. Configured-versus-persisted identity conflict is separately confirmed in `COR-001`. |
|
| Minimal loaded-manifest status/timestamp validation | rejected as a standalone Stage 2 finding | Unknown/non-succeeded statuses fail conservatively into execution, nil maps/records are normalized, and no realistic unsafe caller was established. Configured-versus-persisted identity conflict is separately confirmed in `COR-001`. |
|
||||||
| Ignored runner lock-release error | assigned to Stage 3 | The application discards the error, but whether close/unlink failure preserves an OS lock or merely a harmless lock file depends on the local-store implementation and platform semantics. |
|
| Ignored runner lock-release error | confirmed as `RSK-003` at Stage 3 | The lock is an exclusive-create sentinel, not an OS-released lock. An unlink failure leaves the conflicting file while the runner suppresses the error; process death does the same without a release attempt. |
|
||||||
| Two durable meanings of “skip” | runner portion confirmed, stage-contract conclusion deferred | Run action/status distinguishes idempotent skip from executed self-skip. Stages 4 and 8 must determine whether successful disabled outcomes and their wording are intentional. |
|
| Two durable meanings of “skip” | runner portion confirmed, stage-contract conclusion deferred | Run action/status distinguishes idempotent skip from executed self-skip. Stages 4 and 8 must determine whether successful disabled outcomes and their wording are intentional. |
|
||||||
|
| Exported previous-artifact helper accepts traversal | consolidated into `COR-002` | Current production callers normalize first, but the helper's under-root contract is false in isolation. The identity/relative-segment boundary should be repaired once rather than as separate caller bugs. |
|
||||||
|
| Built-in and previous resolvers do not re-hash manifest records | documented trust distinction; deferred to Stages 5 and 10 | Extraction explicitly requires checksum/contract/provenance validation and enforces it. Other source families explicitly use content validation and a previous-cache filesystem fallback; consumer/restore threat models must establish a stronger requirement before this becomes a finding. |
|
||||||
|
| Promotion destination is path-based while source is handle-confined | consolidated into `COR-003` | Source hardening is strong, but destination ancestors share the same symlink/replacement root cause as ordinary writers and cleanup. |
|
||||||
|
|
||||||
## Unresolved Questions And Follow-Up
|
## Unresolved Questions And Follow-Up
|
||||||
|
|
||||||
@@ -566,8 +911,17 @@ promoted `COR-001` and `RSK-001` into the confirmed register above.
|
|||||||
them interrupted, or retain them as an explicitly accepted audit limitation?
|
them interrupted, or retain them as an explicitly accepted audit limitation?
|
||||||
- Do Stage 3 path mechanics and Stage 4 publish/cleanup behavior confirm every
|
- Do Stage 3 path mechanics and Stage 4 publish/cleanup behavior confirm every
|
||||||
concrete consequence of the stale run-derived fields in `COR-001`?
|
concrete consequence of the stale run-derived fields in `COR-001`?
|
||||||
- Can any `ReleaseSessionLock` error retain exclusion or make a subsequent
|
- Which cross-platform root-relative filesystem primitive should own the
|
||||||
acquisition unsafe, and how should the runner surface such a deferred error?
|
symlink-safe mutation capability required by `COR-003` without moving stage
|
||||||
|
policy into low-level `fileops`?
|
||||||
|
- Should sentinel locking be replaced by an OS-released lock, or can a
|
||||||
|
conservative ownership/recovery protocol satisfy `RSK-003` across supported
|
||||||
|
platforms?
|
||||||
|
- Which existing identifier spellings constrain the strict segment validation
|
||||||
|
required by `COR-002`?
|
||||||
|
- Do deployment packaging and service definitions already enforce an umask,
|
||||||
|
ownership, or ACL that reduces `RSK-004`, and where should that contract be
|
||||||
|
documented and tested?
|
||||||
- Are disabled render/analyze/publish outcomes intentionally successful so
|
- Are disabled render/analyze/publish outcomes intentionally successful so
|
||||||
pipeline continuation and optional outputs work, and do all operator views
|
pipeline continuation and optional outputs work, and do all operator views
|
||||||
describe that distinction accurately?
|
describe that distinction accurately?
|
||||||
@@ -640,3 +994,42 @@ No accepted risks or final audit conclusions are recorded yet.
|
|||||||
intentionally retain diagnosis data while running/failure/skip clear it;
|
intentionally retain diagnosis data while running/failure/skip clear it;
|
||||||
session authority makes all enumerated disagreement states retry or reuse
|
session authority makes all enumerated disagreement states retry or reuse
|
||||||
conservatively; the runner's two skip forms are durably distinguishable.
|
conservatively; the runner's two skip forms are durably distinguishable.
|
||||||
|
|
||||||
|
### Stage 3
|
||||||
|
|
||||||
|
- Contracts and code reviewed: architecture/path/security policy; internal
|
||||||
|
artifacts, workspace, manifest, operations, and troubleshooting contracts;
|
||||||
|
all canonical local/S3/cache constructors; pathsafe and artifactpolicy;
|
||||||
|
built-in, configured, extraction, previous, and current-state resolution;
|
||||||
|
local-store layout/copy/lock code; atomic write/copy/download installation;
|
||||||
|
directory promotion and platform-specific no-replace/directory-sync support;
|
||||||
|
restore/audio/previous download callers; manual and post-publish cleanup.
|
||||||
|
- Graph/source evidence: canonical-helper and direct-mutation inventories;
|
||||||
|
callers of path/key, artifact-resolution, fileops, current-state, previous-
|
||||||
|
cache, and lock helpers; focused test-function inventories; fallback text
|
||||||
|
search for non-code policy and direct OS mutation sites where graph results
|
||||||
|
were insufficient.
|
||||||
|
- Validation: `go test -count=1 ./internal/artifacts
|
||||||
|
./internal/artifactpolicy ./internal/pathsafe ./internal/fileops` passed (0.98
|
||||||
|
s wall time). `go test -race -count=1 ./internal/artifacts
|
||||||
|
./internal/fileops` passed (2.25 s wall time).
|
||||||
|
- Conclusions: canonical owners and artifact resolution order are recorded
|
||||||
|
above; lexical normalization handles mixed separators, traversal, absolute,
|
||||||
|
and drive forms when callers invoke it; extraction source trust and source-
|
||||||
|
side promotion are strong; low-level helpers correctly consume explicit
|
||||||
|
destinations. Scenario 10 is resolved: live contenders are excluded, while
|
||||||
|
stale sentinel/release behavior is unsafe operationally.
|
||||||
|
- Findings: confirmed `COR-002`, `COR-003`, `RSK-002`, `RSK-003`, and
|
||||||
|
`RSK-004`; added `DUP-002` and `TST-003`; refined `DUP-001` with the shared
|
||||||
|
durability gap.
|
||||||
|
- Explicit no-finding conclusions: current-state helpers have typed missing
|
||||||
|
cases and strict identity checks; artifact resolution is deterministic and
|
||||||
|
matches its documented source-specific validation; promotion preserves an
|
||||||
|
existing/concurrent destination and rejects unsafe source trees; temporary
|
||||||
|
files/trees are cleaned on ordinary failures; unsupported promotion
|
||||||
|
platforms fail before creating a durable bundle; fileops does not infer
|
||||||
|
higher-level policy.
|
||||||
|
- Follow-up: Stages 4-10 should cite the shared confinement/durability roots for
|
||||||
|
concrete callers. Stages 5 and 10 must decide whether ordinary manifest and
|
||||||
|
previous-cache checksum trust is sufficient. Stages 6, 11, and 12 own
|
||||||
|
compatibility, simplification, and durable regression coverage respectively.
|
||||||
|
|||||||
Reference in New Issue
Block a user