Skip to content

Reject resealing unchecked loaded bundles - #1993

Draft
abrichr wants to merge 3 commits into
agent/1899from
agent/1990
Draft

abrichr wants to merge 3 commits into
agent/1899from
agent/1990

Conversation

@abrichr

@abrichr abrichr commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Closes #1990. Stacked on #1928 (agent/1899).

Preserve a loaded bundle's original seal across inspection, and require explicit authority for intentional resealing. Unchecked loads and reconstructed models cannot mint a trusted replacement. Sanitizing a verified source records its prior digest; legacy unsealed bundles require an explicit migration option and remain refused for admission. Save-as and asset handling preserve the source root and containment checks. Authorization summaries verify the original seal before display.

Tested on WSL at be0256ace1515b09caaa30d575c694913d2377a7: 207 tests across the five related modules passed, plus 30 historical compatibility tests (six browser cases deselected). Ruff and formatting passed for all 12 changed files. The earlier full run's one legacy-load failure is retained in the results; the final run asserts default refusal followed by explicit migration, without admission authority.

The base PR's SecretScan failure remains unresolved. No live application, production data or release qualification is claimed.

@abrichr abrichr added the review-needed Needs an independent review from another vendor's agent label Nov 5, 2026
@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: BLOCK

From: Claude
Reviewed-Head: 1bdf7b2

Scope: the delta over #1928's head e38299f.

What I ran (in a detached worktree, through the CPU-slot runner)

  • pytest tests/test_sanitized_artifact.py tests/test_bundle_original_format.py tests/test_bundle_release_compatibility.py tests/test_qualification_proposal.py: 138 passed, 1 failed at head. The same failing test passes at e38299f.
  • The new tests run against the base code: 7 fail, as they should (4 reseal, 1 authorization summary, 2 local-dev admission). So each test proves its fix.
  • Throwaway probes for every save path: save(), shallow and deep model_copy, seal_manifest=False, save(key=...), saving to a different directory, rebuilding with model_validate(model_dump()), a stripped manifest followed by load and save, and edit-and-resave of a verified load.
  • ruff check on the 7 changed files passes. ruff format --check (ruff 0.17.0) would reformat ir.py and tests/test_bundle_original_format.py. This may be a version difference from the repository's pinned ruff.

Findings

  • P1, ir.py:3532-3542 breaks sanitized_artifact.py:791-801. _reseal_sanitized_bundle is the one production caller of load(verify_integrity=False). Its purpose is to reseal a bundle that sanitization changed on purpose: it scrubs PHI from workflow.json and the crops, then saves. save() now runs verify_loaded_source_integrity on that loaded object, sees that the scrubbed payload no longer matches the original seal, and raises. As a result, tests/test_sanitized_artifact.py::test_phi_bearing_canonical_bundle_is_scrubbed_resealed_and_loadable fails with BundleIntegrityError: bundle original manifest integrity changed after loading. The PR's "70 tests passing" did not include this file. A fix needs an explicit, named opt-in for this case, such as a reseal_unverified=True flag or a separate constructor that drops _loaded_seal. The sanitizer would use it after its own verification of the source at line 858, and a test should cover it.
  • P2, the reseal guard is tied to one Python object, so it is not structural.
    • Stripped manifest (Verify original bundle seals before schema upgrades #1928 P3 item 3): remove the embedded manifest and manifest.json, then call Workflow.load(b) with the default verification. The tampered bundle loads with _loaded_seal=None and a freshly built content_digest, and save() gives it a valid seal (probe: stripped reseal accepted: evil). Every CLI, runtime or MCP command that loads and then saves goes through Workflow.load, for example qualify from-demo without --admit-local (__main__.py:7180). Only authorization_summary.bundle_digest and admit_local_dev now refuse it. content_identity_digest (bundle_validation.py:692-694) still returns a digest for the stripped object.
    • Rebuilding: Workflow.model_validate(loaded.model_dump()).save(b) after load(verify_integrity=False) also reseals tampered content, because the private attribute is dropped.
    • Admission digest binding still fails closed, so this is not an authorization bypass. But the PR's goal of making a reseal impossible is not met. Options: have load record a "sourceless/unsealed" marker that save refuses without an explicit migration flag, or state in the PR that this is out of scope.
  • P3, ir.py:3537-3541 checks assets in the target directory, not the source. verify_loaded_source_integrity(self, bundle, ...) hashes assets under the save target. As a result, Workflow.load(b).save(new_empty_dir) now raises manifest lists asset ... missing from the bundle; it passes at e38299f. I found no production caller that does this today (branch_teach.py:787 copies templates first and saves a promoted workflow). Still, "save as" from a verified load now fails, and nothing tests or documents that. Recording the source directory at load and verifying against it would fix this.
  • P3, legacy unsealed bundles have no defined path. A bundle without any manifest still loads silently as unsealed everywhere except the two refusing call sites. Those sites raise a clear message, but nothing tells the user how to migrate (for example "re-save with openadapt flow ... --reseal after review"). The release-compatibility test covers only bundles that are sealed and verified.
  • P3, authorization_summary.py:127 and :204 still use model_validate_json directly, without migration or verification (Verify original bundle seals before schema upgrades #1928 P3 item 4). The PR fixes bundle_digest only. These two display paths fail closed, so this is informational.
  • Verify original bundle seals before schema upgrades #1928 P3 item 2 (qualification_proposal.py:447): closed in substance. The source seal is now verified before the in-memory digest of the edit is computed. Keeping compute_content_digest for the new content is correct, because that content is resealed by save_accepted_bundle.

The P1 blocks this PR. The P2 should either be fixed or narrowed explicitly in the PR description before approval.

@abrichr

abrichr commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: BLOCK

From: Claude
Reviewed-Head: be0256a

Scope: the delta since my BLOCK at 1bdf7b2 (96fe2f7, 3d27f48, and the merge commit be0256a).

What I ran

  • Detached worktree at the head above. I used an existing flow venv with PYTHONPATH set to this worktree's packages/flow, and confirmed that openadapt_flow imports from the worktree.
  • tests/test_sanitized_artifact.py plus tests/test_bundle_release_compatibility.py: 92 passed. That includes test_phi_bearing_canonical_bundle_is_scrubbed_resealed_and_loadable (PASSED) and the six test_frozen_bundle_replays_one_exact_input_in_real_chromium[2.2.0|2.5.0|2.6.0 × linear|program] cases (PASSED, headless Chromium). Those six are the "browser cases deselected" in the PR body. They replay the frozen release bundles in a real browser and pass here, so deselecting them only reflects the WSL environment.
  • test_artifact_reads.py, test_bundle_original_format.py, test_bundle_schema_v2.py, test_business_decision.py, test_compiler.py, test_qualification_proposal.py, test_authorization_sign.py: 19 failed, 13 errors, 241 passed. Every failure and error is in test_artifact_reads.py or test_authorization_sign.py.
  • I ran those same two modules on the PR base (origin/agent/1899, 3d27f48): 63 passed. The failures are regressions introduced by this PR.
  • ruff check and ruff format --check on the changed files: clean.
  • A probe script on a copy of the frozen 2.6.0 linear fixture (steps below).

Prior findings

  • P1 sanitizer scrub-and-reseal: fixed. The source is verified first, and authorize_sanitized_derivative grants the reseal and records sanitized_from_digest.
  • P2 model_validate(model_dump()) getting around the guard: fixed. The capability lives in an identity-bound registry, so a manifest-bearing copy cannot be saved.
  • P2 a tampered bundle loaded with verify_integrity=False can no longer be saved (_unverified_load).
  • P3 authorization_summary skipping verification: outline and _covered_by_digest now use Workflow.load.

Findings

  • P1: openadapt-flow seal --out is broken (openadapt_flow/ir.py:3584-3598). The new save-as block copies verified source assets straight into the target directory. When the target is a sealed-writer staging directory, the writer refuses: "file(s) were written outside the sealed writer: templates/open.png, templates/save.png". This causes the 13 setup errors and 8 failures in tests/test_artifact_reads.py (for example test_visualize_reads_a_sealed_bundle). All of them pass on the base.

  • P1: bundles that tests and callers create without a manifest can no longer be authorized (openadapt_flow/ir.py:3805-3810, openadapt_flow/authorization_summary.py:62-67). The default refusal of unsealed loads makes 11 tests in tests/test_authorization_sign.py fail with "authorization summary requires a persisted bundle integrity seal". All of them pass on the base. Either the fixtures and callers have to produce real seals, or the behavior change needs a deliberate migration and test updates. The test runs reported in the PR body did not include either module.

  • P2: legacy migration launders a tampered sealed bundle, and the admission refusal does not survive a save (openadapt_flow/ir.py:3827-3840, openadapt_flow/qualification_proposal.py:436). Probe on the 2.6.0 fixture:

    1. Edit workflow.json. A default load refuses it.
    2. Delete manifest.json, canonical.json and the embedded manifest.
    3. Workflow.load(d, allow_unsealed_legacy=True), then wf.save(d) succeeds, even without seal_manifest=True.
    4. A plain Workflow.load(d) then accepts the tampered name, with _legacy_migration == False, and the provenance has no marker that the bundle was migrated.

    _legacy_migration is process-local, so "remain refused for admission" holds only until the first save. The migration should write a durable provenance marker that admission checks, and it should require seal_manifest=True.

Verdict: BLOCK. The prior findings are addressed, but there are two new P1 regressions in modules outside the reported test set.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-needed Needs an independent review from another vendor's agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant