fix: harden archival state boundaries#90
Merged
Conversation
…in ingest - Validate a payload's filename (via bag.py's existing _reject_unsafe_relpath) before writing its encrypted bytes into the SEALED-payload staging directory. Previously the write happened before any traversal check, so a payload key containing `..` or an absolute path (reachable via oralhistory.apply_session_manifest's payload_filename, which is not otherwise sanitized) could write ciphertext outside the intended temp directory. - Move the bag-collision check ahead of SEALED-field encryption, matching the ordering already used for identity sealing: a colliding ingest no longer mutates the caller's in-memory Record fields (plaintext -> ciphertext, in place, unrecoverable) before discovering the ingest will fail anyway. - Write the per-record .versions.json history index via the module's own atomic_write_text helper instead of a plain write_text call, so a crash mid-write can no longer corrupt it into something _read_versions silently reads back as "no history" (the method's own docstring already claims every write here is atomic; this one wasn't). Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
…rruption IdentityVault.add/revoke/rekey mutated the in-memory store and persisted to disk with no locking at all, unlike every sibling JSON store in the codebase (ConsentRequestStore, ProposalStore, TombstoneStore, ...). Under the threaded browse server, two concurrent calls (e.g. an ingest and a takedown) open the *same* temp file -- its name is derived only from os.getpid(), identical across threads in one process -- so the second thread's truncating open can corrupt the first thread's in-flight write before either renames it into place. This is the archive's single most sensitive file. Wrapped each method's mutate-then-persist step in the existing ledger._filelock.file_lock, the same primitive already used throughout the codebase for exactly this class of race. AttestStore._record_attested had the identical unlocked read-modify-write gap on attested-conditions.json (two approvals crossing a 2-of-N threshold for different conditions near-simultaneously could race and drop one). Locked it the same way. Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
_record_event read-modify-wrote logs/lockdown.premis.json with no lock,
so a lockdown and stand-up event (or two lockdown attempts) racing under
the threaded server could lose one. Locked it like every other archive
log store.
execute_stand_up() always constructed its LockdownResult with
disclosure_stopped=False, even after a real, successful stand-up that
unlinked lockdown.flag and resumed disclosure -- so LockdownResult.summary()
printed "stand-up executed -- disclosure unchanged", the opposite of what
happened, directly to the steward running `ledger stand-up --execute`
(cli.py prints this string verbatim). The PREMIS audit event had the same
bug: its detail hardcoded "flag_removed=True" regardless of whether a flag
was actually present to remove. Both now reflect whether the freeze was
actually lifted, and summary() renders stand-up's own wording ("resumed" /
"was already open") instead of reusing lockdown's "stopped"/"unchanged"
phrasing for an action it was never written to describe.
Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
atom_feed_xml sorted entries by each record's raw, un-normalized dc:date
string. Dublin Core dates in this app are free text of varying
granularity/padding ("1994", "2021-5-1", "2021-12-01", a full timestamp),
and comparing them as plain strings is lexicographic, not chronological
-- an unpadded date can sort after a later, zero-padded one purely
because of character ordering, breaking the feed's documented "most
recent first" guarantee. The sort key now runs through the same
_atom_timestamp widening already used to render each entry's <updated>
value, so the order a reader sees always agrees with the dates displayed.
Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
Both modules' escape() helper wrapped xml.sax.saxutils.escape, which only escapes &, <, and > by default -- not the quote character delimiting an XML attribute. Both modules interpolate escaped values directly into double-quoted attributes built from contributor-suppliable text (mets.py: LABEL="...", xlink:href="...", xlink:title="..." built from record.title and payload.filename; ead.py: id="c-..." built from record.record_id). A literal `"` in that text breaks the exported document's well-formedness. record_id is always a system-generated uuid4 hex today so that specific call site isn't reachable yet, but record.title is genuine free text from the web contribute form or CLI, so this was a real, reachable correctness bug. Both helpers now also escape `"` and `'`. Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
All eight commits carry Signed-off-by.