Repository navigation
Verify the staged project tree once during creation - #151
Conversation
Same codes, messages, JSON, and publication states for every reachable input; fewer lines and about a third less wall time per creation. - Nest the immutable plan once (_plan_tree) and check paths, modes, and file/directory collisions before anything is written. - Verify the stage once, after its final chmod, instead of three full read-backs plus an in-memory comparison of the plan against a second copy of itself. - Bind the stage by identity at the parent reopen and after publication; the interleaved identity checks were subsumed by those two. - Fold the release lookup, artifact-length check, strict JSON hooks, and per-call failure messages into fewer helpers and shared constants. - Map renameatx_np/renameat2 through one table. Tests that patched the removed _validate_staged_project now wrap _materialize_project. Removed: the two tests that fed the builder output it cannot produce (a manifest on an unlisted pair, a corrupted core file compared against itself) and the container/content type cases.
The previous commit dropped two checks that main still performs. An unlisted toolchain pair must not publish a lake-manifest.json. The builder adds one only for catalog releases, but _scaffold_plan passes a root manifest from the templates through unchanged. Refuse after the final tree verification, as main does, so the populated stage is kept for recovery. Release descriptors must name their manifest resource with characters below U+D800. Names at or above U+E000 were reaching the manifest loader and failing there with the manifest message instead of the descriptor message.
- Reuse the strict JSON hooks from claims.py; both callers discard the error text, so only the parse rules matter. - Let _load_release_bundle map JSON errors from _parse_release_bundle; its handler catches the same exceptions with the same message. - Every descriptor passed to _descriptor_identity is opened with O_DIRECTORY, and an entry with the same device and inode is that directory, so the S_ISDIR checks are dead. The owner check stays. - The target name is already checked before absolute(), which cannot change it, and _open_parent always receives an absolute path. - The injected validation failure test raised an error that the code does not raise at that point. The unlisted template manifest test now pins the same stage-preserving branch with a real refusal.
meta-cla
Bot
added
the
CLA Signed
|
One fix is required before this is ready. The unlisted-template-manifest refusal now happens after Move the manifest-membership check immediately after materialization and before the root chmod, while keeping the same error code/message and populated-stage recovery behavior. Add an assertion that the retained stage is |
Deicyde
added
the
awaiting author
The first commit dropped the identity check main runs between opening the stage and materializing it. That left the first owner check in the final verification and the first entry check just before publication. _open_parent accepts a parent that another uid owns, as when root creates the project in a user's directory, and that owner can rename the stage entry and put its own directory in its place before the open. Creation then wrote the whole tree into the replacement before refusing it; main refuses before writing anything. Restore main's _require_stage_identity call after the open. It raises main's code and message, and the existing handler adds the stage notice. The test swaps the stage just after the open, which the check also catches. An unprivileged test cannot hand the creator a directory owned by another uid, and a same-uid directory swapped in before the open matches its descriptor. The test asserts that neither directory receives a file.
The second commit placed this refusal after the final tree verification, saying main does the same. Main refuses from _validate_staged_project, before its chmod, so the populated stage it keeps for recovery is still 0700. Here the kept stage was already 0755. Move the refusal to just after materialization, before the chmod. It keeps its code, its message, and the populated stage. Its condition now checks one direction only: with a catalog release the plan always holds lake-manifest.json, and _plan_tree refuses any template entry that collides with it. The message, which the roadmap check shares, becomes _STAGED_MESSAGE beside _CONTRACTS_MESSAGE. The template-manifest test now requires the kept stage to be 0700.
The first commit dropped main's comparison of the staged core files with a freshly built core plan. A template at a core path still fails, because _plan_tree refuses two plan entries at one path, but nothing pinned that. Add a case per core path, each refused with the contracts message before a stage exists.
_unsafe_parent_metadata trusts a parent from its mode bits and owner. On macOS an inheritable ACL on an accepted 0755 parent is copied onto the 0700 stage and survives its chmod, so another uid can change the stage. Main has the same exposure. Say so where the trust rule is stated, rather than leave the comment implying that only the invoking user and root can change the stage.
The first commit rewrote both without changing their results, so the rewrites only add diff. plugin_pin returns strings and _normalize_autoform_source returns None or a non-empty string, so main's _resolve_workflow_pin tail picks the same source and ref. In _safe_https_git_url the widened try covers only urlsplit property reads, which do not raise on Python 3.10 or 3.13.
Main checked the stage's identity again after materialization, before the chmod. Without that check, a stage renamed during the write reached fchmod(0o755) and the manifest refusal first, so the renamed tree became readable and a template manifest changed the error code.
Deicyde
removed
the
awaiting author
|
The review blocker is fixed at |
|
Final exact-head review at |
Main refused a directory renamed into the stage name before the open, but only after writing the tree into it: one holding an entry failed the listing check at the end of materialization, and one with another mode failed the 0700 root check in its staged validation. This branch verifies once, after the chmod, so an empty 0755 directory passed and was published, and a 0700 one holding a file was refused only after the chmod had made it 0755. Check before the first write that the opened stage is 0700 and empty.
No test failed when the owner clause in _entry_matches_descriptor was removed. The manifest test now pins the exact message, not a substring.
The workspace-substitution test gains an unlisted pair with a template manifest. The identity check after the write runs first, so the code is main's project-create-failed; with the refusal moved above that check it becomes project-create-validation-failed. The template-manifest test gains a template directory named lake-manifest.json, which main published and this branch refuses. The swapped-in stage cases get readable IDs.
|
Published the independently audited fast-forward at |
|
Final gate at exact head |
1 participant
Follow-up to #96. Before publishing,
project newread the staged tree back three times and compared the plan against a second copy of itself. With this change it verifies the stage once, after its final chmod. It checks the stage root's identity where main does, minus two checks: the one after main's staged validation, which no longer exists, and the one after the final verification, which the check at the parent reopen right after it covers. Main's per-directory checks during materialization are dropped too (see the security claim).Error codes, messages, the
autoform-project-creation/v1JSON and the publication states are unchanged, except for the accepted trades and the other differences listed under the security claim below.Also in the first commit:
_plan_treebefore anything is written; main nested it again in_materialize_projectand in each_verify_project_planrun.renameatx_npandrenameat2share one call path; a conditional picks the function name and flag.statandS_ISDIRcheck of the stage entry before its open is gone. TheO_DIRECTORY | O_NOFOLLOWopen refuses the same entries, with the same code and message.Security claim
Claim. Against a process running under a different uid, this change keeps every defence main has that affects what gets published. Such a process can at most rename the stage entry, or remove it while it is still empty, which fails creation as on main. The only renames this change misses that main could catch are undone before the parent reopen, so the published tree is the planned one either way. The other detections it drops all need a process running as the same user or as root, and that process can edit the published project after the rename on main too. On macOS, main and this change are both weaker, because neither reads ACLs (see the first reasoning bullet).
Reasoning.
_open_parentaccepts only if its mode bits are not group- or world-writable, or it is sticky and owned by us or root. On Linux, an ACL that grants write on the parent also sets its group write bit, which the check sees, and entries the stage inherits are masked by its 0700 and later 0755 mode. So there, only our uid or root can chmod the stage root or create, replace or remove anything inside it. Any other uid can at most rename the stage entry, or remove it while it is still empty, and only if it owns the parent. On macOS, ACLs are not read. An inheritable entry on an accepted 0755 parent is copied onto the stage and everything created in it, and it survives the chmod, so the uids it names can change the stage, including between_verify_project_treeand the rename. Main has the same exposure. Refusing a parent that carries an ACL, or checking that the stage has none, is left for a separate change._require_stage_identitychecks the entry right after the stage is opened, again after the tree is written, and at the parent reopen, and_publication_statecovers the rename itself. The codes are main's. A directory owned by another uid fails the first check before anything is written. Before the first write, the opened stage must also be 0700 and empty, so a directory of ours renamed into its place before the open is refused before anything is written into it; main refused it too, before its chmod, but only after writing into it whatever of the tree it could. Only a directory that cannot be told apart from a fresh stage gets through, on main as here, and it is published like one._verify_project_treeafter the last write and sync, and that check gates publication. It is the same strength as main's_verify_project_plan. Main ran it twice more and, during materialization, checked each new subdirectory's identity and every directory's listing; those extra checks are what this change drops.Accepted trades. Apart from the last, each one needs the same uid or root, per the reasoning above.
fchmod(0o755)of the stage root overwrites a mode change made to it during materialization. Main detected that change.Other differences from main. In each one, this change refuses to publish.
project-create-validation-failedwhere main reportedproject-create-failed. A stage renamed before the check that follows the write, or one that was not 0700 and empty when opened, keeps main's code, because those checks run first.lake-manifest.json. Main's check looked only at planned file paths, so with an unlisted pair it published that directory. It is now refused like a template manifest file.project-parent-changedorproject-parent-unverifiable, where main's identity check after its staged validation or final verification reportedproject-create-failedfirst. Both keep the stage.The descriptor message for release resource names at or above U+E000 was a regression in the first commit and is fixed below, not traded.
Fixes
The second commit restores two checks that the first commit dropped:
_scaffold_plankeeps a rootlake-manifest.jsonfrom the templates, rendered like any other text template, and with no catalog release nothing collides with it. That commit put the refusal after the final verification and called the order main's; main refuses before its chmod, and a later commit moves it there.After merging main at 71b251c, eight commits address review findings:
_plan_treerefuses a duplicate. The message is the shared_STAGED_MESSAGE._unsafe_parent_metadata, matching the first reasoning bullet._resolve_workflow_pin's tail and_safe_https_git_urlare main's code again. The first commit's rewrites returned the same results and only added diff.project-create-validation-failed.In the meantime, 45fffc4 on this PR moved the manifest refusal before the chmod, as 2a9137c does; 2439944 merges it and keeps this branch's tree. The last commit, Test the manifest refusal's order and its directory case, adds the cases listed under Test changes.
Smaller removals
A third commit reuses the strict-JSON hooks from
claims.pyand drops checks that cannot fire:tryin_parse_release_bundlerepeated its caller's handler;S_ISDIRchecks on descriptors opened withO_DIRECTORYand on entries that match such a descriptor by device and inode;parent.absolute()on a path that is already absolute.Size
create.py goes from 1493 lines to 1034.
Test changes
_validate_staged_projectnow wrap_materialize_project.test_unlisted_plan_must_not_carry_a_manifest. The first commit removed it, saying the builder cannot produce that plan. That premise was false: the real builder produces it from a template tree with a root manifest. The second commit replaces it withtest_an_unlisted_pair_never_publishes_a_template_manifest. The new test uses the real builder and asserts the code, the exact message with the stage notice, that the target is absent, and that exactly one stage remains, holds the manifest and is still 0700. It runs with a manifest file and with a template directory of that name.test_corrupt_core_plan_is_rejected_without_path_based_inspection. Removed. On main,_build_project_plantakes the core files from_core_project_plan, and the staged check re-derives them from the same function. Both versions refuse a template file at a core path as a_plan_treecollision before staging. So only a patched builder could reach that check, and this test patched the builder.test_a_template_cannot_replace_a_core_project_filenow pins the collision with the real builder: a template atlean-toolchain,lakefile.toml,lake-manifest.json(with a catalog release) orsrc/Project.leanis refused with the contracts message before a stage exists.test_injected_validation_failure_preserves_stage_for_safe_recovery. Removed. Two tests now pin the same stage-preserving branch with real errors, and both assert the stage notice:test_requested_parent_rebind_before_publish_preserves_the_stageand the new manifest test.test_release_metadata_names_its_manifest_below_the_surrogate_rangecovers U+E000 and an astral character.test_plan_requires_safe_paths_and_file_modes_before_writing.test_mutation_after_validation_is_not_publishedis nowtest_mutated_stage_is_not_published.test_stage_substitution_is_refused_before_writingswaps the stage entry just after the open, and asserts that neither the opened stage nor its replacement received a file;test_swapped_in_stage_directory_is_refused_before_writingrenames either a 0700 directory holding a file or an empty 0755 one (casesnonempty-0700andempty-0755) into the stage name before the open, and asserts that each is refused unchanged and that the original stage stays empty;test_foreign_owned_stage_is_refused_before_writingmakes the stage look owned by another uid, and asserts that it is refused while still empty. An unprivileged test cannot hand the creator a directory owned by another uid, so it patchesos.geteuidonce the stage exists.test_workspace_substitution_fails_before_publicationalso asserts that the renamed stage is still 0700. A second case runs it with an unlisted pair and a template manifest, where the code must stayproject-create-failed.This is independent of the other #96 follow-ups. The branch includes main at 71b251c, with the test-sharing PR (#152). #162 edits the same test file, and the two merge cleanly.
Tests: These ran on e0f31f2 with Python 3.13.14 and pytest 9.1.0 from a venv built from
uv.lock;uv runcould not resolve the workspace in this checkout. The rest of the suite was left to CI, where all 9 checks pass on e0f31f2.ruff check autoform_cli servers tests(ruff 0.15.17): clean.tests/test_project_create.py: 257 passed, and 257 passed merged with Use _refused in the remaining refusal tests and drop a dead hash-length branch #162.test_project_create.py,test_project_inspect.py,test_skill_examples.pyandtest_plugin_runtime.py, 514 passed.project-create-validation-failed);project-create-validation-failed);_entry_matches_descriptor;_plan_tree's duplicate check (creation succeeds).