mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-25 03:42:06 +00:00
xarmian
47a4448afc
* chore(import-bundle): audit + harden bundle import validation (TASK-891) Re-reviewed handlers_import_bundle.go before exposing the bundle import flow through the web UI under PLAN-890. The audit doc lives at DOC-895; this commit lands the small inline fixes. Findings + actions: - Duplicate pad-export.json now rejected (was: silently ran ImportWorkspace twice, stranding the first workspace as an orphan with no attachments). - Duplicate attachments/manifest.json now rejected (was: silently overwrote manifestByPath, dropping prior entries). - Defense-in-depth path-traversal guard added via isSafeBundleEntryName — rejects entries with `..` segments, absolute paths, or NUL bytes BEFORE the switch. Storage was already hash-keyed and safe, but the silent-skip behavior on malicious tar names was a poor audit story. - Auth/permissions now documented on the handler — RequireAuth middleware gates the endpoint; no per-workspace role check applies because the request creates a new workspace (mirrors handleCreateWorkspace). Tests added: TestImportBundle_RejectsDuplicateExport, TestImportBundle_RejectsDuplicateManifest, TestImportBundle_RejectsPathTraversal, TestIsSafeBundleEntryName. Two larger gaps deferred as their own tasks: - TASK-896 (partial-import orphan workspace on mid-stream failure — needs design discussion). - TASK-897 (per-user storage quota enforcement on import — gated on Phase 2 quota work; matches upload handler's warn-only Phase 1 policy today). Parent: PLAN-890. * fix(import-bundle): roll back partial workspace on validation reject per Codex review (round 1) Codex P1 on PR #308: when the duplicate-pad-export.json or duplicate-manifest.json guards fire, the workspace from the first occurrence has already been inserted by ImportWorkspace. The handler returned 400 but the orphan workspace stayed in the destination DB. A malformed/malicious bundle could repeatedly POST and pile up half-imported workspaces. Fix: when importBundle returns an importStatusError after creating a workspace, the handler now soft-deletes that workspace via DeleteWorkspace before returning the 400. Mid-stream errors that are NOT importStatusError (e.g. manifest decode after items inserted) intentionally keep the partial workspace — that's the existing design tracked under TASK-896 (partial-import design discussion). Tests extended: TestImportBundle_RejectsDuplicateExport and TestImportBundle_RejectsDuplicateManifest now also list the destination workspaces after the rejected import and assert the partial workspace does NOT appear. Parent: PLAN-890. * fix(import-bundle): cascade attachment tombstone on rollback per Codex review (round 2) Codex P1 round 2 on PR #308: when the duplicate-manifest guard fires AFTER blobs have already been rehydrated (e.g. bundle layout [pad-export, manifest, blob1, blob2, duplicate-manifest]), the previous fix soft-deleted the workspace but left the attachment rows live. Live rows pin blobs from orphan-GC and continue counting toward per-user storage usage even though the workspace is gone. Fix: added Store.SoftDeleteWorkspaceAttachments(workspaceID), a single bulk UPDATE that tombstones every live attachment row (originals AND thumbnails — both carry the same workspace_id) under a workspace. The handler's rollback path now calls this BEFORE DeleteWorkspace so orphan-GC reclaims the blobs after the grace window. Best-effort: any error in either op is logged with workspace context but the original 400 still flows. New test: TestImportBundle_RollbackTombstonesAttachments builds a real export bundle from a source workspace with one attachment, surgically appends a duplicate manifest.json AFTER the real entries, posts it, and asserts (a) 400, (b) workspace gone from listings, (c) zero live attachment rows on the destination. The pre-fix code left rows=1 live; the new path tombstones them. Parent: PLAN-890. * fix(import-bundle): return ws on path-traversal reject so rollback fires (Codex round 3) Codex P1 round 3 on PR #308: the path-traversal early-return at the top of the import loop returned (nil, importStatusError) instead of (ws, importStatusError). When a malicious path-traversal entry follows a valid pad-export.json, the workspace was already created — but because the handler saw ws == nil, it skipped the rollback cascade, leaving the workspace and any rehydrated attachments behind. Fix: return ws (which is nil before pad-export.json is processed, so the no-workspace cleanup path still works for first-entry-bad bundles, and non-nil after, so the cascade runs). One-line change keyed off the existing rollback flow. New test: TestImportBundle_PathTraversalAfterExportRollsBack hand-builds a tar with a valid pad-export.json followed by a "attachments/../../etc/passwd" entry, posts it, asserts 400, and asserts the partial workspace is GONE from listings. Pre-fix this test would have shown the workspace leaking through. Parent: PLAN-890.