fix(extensions): refuse web UI install on local edits (swamp-club#129)#1191
fix(extensions): refuse web UI install on local edits (swamp-club#129)#1191
Conversation
Port the #126 filesChecksum anchor + local-edits-check pattern into a shared libswamp helper (`src/libswamp/extensions/local_edits.ts`, exporting `detectLocalEditsForExtension`, `LocalEditsStatus`, and `LocalEditsError`) and consult it from the `swamp open` web UI install callback before the force-pull. When the on-disk per-extension subtree diverges from the stored anchor, the callback throws `LocalEditsError` instead of silently overwriting; the HTTP handler at `handleExtensionInstall` maps it to 409 Conflict with a body directing the user to run `swamp extension pull <name> --force` from the terminal. Third in the data-loss family (sibling of #121 and #126). Auto-update and the web UI are the two implicit force-overwrite surfaces; explicit `swamp extension pull --force`, lockfile-restore (`extension_install.ts`), and `swamp extension update` remain opt-in force paths and are not touched. `resolve_datastore.ts`'s inline detectLocalEdits closure from #126 collapses to a one-line call to the new shared helper; behavior is identical and the #126 auto-update tests stay green as a regression fence. Filed as follow-up: systeminit/swamp-uat#150 (end-to-end UAT for the web UI refuse flow). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
CLI UX Review
Blocking
None.
Suggestions
None.
Verdict
PASS — No user-facing CLI surface changes. The only UX impact is the new LocalEditsError message, which is clear, names the affected extension, and includes the exact remediation command (swamp extension pull <name> --force). The HTTP 409 response body uses the same { error: { message } } shape as every other error response in http.ts. LocalEditsError extends UserError so CLI layers render it without a stack trace.
There was a problem hiding this comment.
Code Review
Blocking Issues
None.
Suggestions
-
Comment length in
open.ts:174-182: The 8-line comment explaining the defense-in-depth rationale is justified (the WHY is non-obvious since there's no current UI click path), but could be trimmed to ~3 lines while preserving the key information (why force:true exists, why only top-level is checked, issue reference). -
Pre-existing import boundary violation in
resolve_datastore.ts:44:maybeAutoUpdateDatastoreExtensionis imported directly from../libswamp/extensions/datastore_auto_update.tsrather than through../libswamp/mod.ts. Not introduced by this PR (and this PR actually fixed the equivalent violation fordetectLocalEdits), but worth noting as a follow-up cleanup.
What looks good
- DDD layering:
detectLocalEditsForExtensionis correctly placed as an application-service function in libswamp, orchestrating domain concepts (extension identity) with infrastructure readers (lockfile, digest).LocalEditsErrorproperly extendsUserErroras a typed domain error, and the HTTP handler acts as a clean anti-corruption layer translating it to 409. - Import boundaries: All CLI/presentation imports (
open.ts,http.ts,http_test.ts) go throughlibswamp/mod.ts. Internal libswamp imports (datastore_auto_update.ts → local_edits.ts) correctly use direct paths. - Test coverage: 6 unit tests cover all three
LocalEditsStatusoutcomes plus edge cases (missing lockfile, missing extension dir) and the error shape. 2 HTTP handler tests verify 409 vs 500 routing. - Error degradation: The
catch → "no-anchor"pattern ensures a safety-check bug never wedges a user command — correct defensive design. - License headers: Both new files include the AGPLv3 header.
- Type safety: No
anytypes, properreadonlyonextensionName,typeimport forLocalEditsStatus.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
None.
Low
-
TOCTOU gap between check and pull —
src/cli/commands/open.ts:182-190:detectLocalEditsForExtensionruns, thenpullExtensionruns withforce: true. A file modified in the gap between those two awaits would be silently overwritten. The window is sub-millisecond in practice and this endpoint isn't under concurrent use by the same user, so it's not exploitable — but it's worth noting that the protection is best-effort, not atomic. No fix needed; just a property of the check-then-act pattern. -
Fail-open design on infrastructure errors —
src/libswamp/extensions/local_edits.ts:62-64: The catch-allcatch { return "no-anchor"; }means a corrupt lockfile (valid JSON structure but bad data types) or a permission error on the extension directory would degrade to "no-anchor" and allow the force-pull. This is documented as intentional ("callers never wedge a user command on a safety-check bug"), and it's the right tradeoff for defense-in-depth — just noting that it's a deliberate fail-open, not fail-closed.
Verdict
PASS — Clean, well-structured PR. The refactoring from inline closure to shared helper is behavior-preserving (verified line-by-line). The new local-edits guard in the web UI install path is correct: it checks before the force-pull, throws a typed LocalEditsError, and the HTTP handler maps it to 409 with actionable remediation. Type export changes in mod.ts are non-breaking. Test coverage hits all three states (match, mismatch, no-anchor) plus the error-class shape and the HTTP 409/500 split. No logic errors, no resource leaks, no security regressions.
Summary
Third in the data-loss family (sibling of #121/#1187 and #126/#1190).
swamp open's web UI install callback callspullExtensionwithforce: trueand, pre-fix, had no local-edits check — so hittingPOST /api/extensions/installfor an already-installed extension would silently overwrite anything the user had edited under.swamp/pulled-extensions/<name>/.Port #126's
filesChecksumanchor + detectLocalEdits pattern into a shared libswamp helper and consult it from the web UI before the force-pull. On mismatch, throw a typedLocalEditsError; the HTTP handler maps it to 409 Conflict with a body that names the extension and the opt-in remediation (swamp extension pull <name> --force).The ui.ts Install button only renders for uninstalled extensions today, so there's no UI click path that reaches this bug — the fix is defense-in-depth for the HTTP endpoint itself, which is reachable by curl, scripts, or any future UI change (e.g. an "Update / Reinstall" button). Scope and fix-shape intentionally mirror #126.
What changes
src/libswamp/extensions/local_edits.ts— shared helper (detectLocalEditsForExtension,LocalEditsStatus,LocalEditsError), re-exported throughlibswamp/mod.tssrc/cli/resolve_datastore.ts— Issue 11: Remove deprecated artifact entities and repositories #126's inlinedetectLocalEditsclosure collapses to a one-line call into the new helper (pure DRY)src/cli/commands/open.ts—installExtensioncallback runs the local-edits check before force-pull; throwsLocalEditsErroron mismatchsrc/serve/open/http.ts—handleExtensionInstallmapsLocalEditsError→ 409, unchanged 500 fallthrough for other errorsWhat's out of scope
extension_update.ts:118andextension_install.ts:114still useforce:true; the former is an explicit user command, the latter is lockfile-restore. Refusing them would break their contracts.upstream_extensions.jsonbut present on disk with edits) — matches Issue 11: Remove deprecated artifact entities and repositories #126's scope. Fixing it would require pushing the check intoinstallExtensionitself, which would wrongly refuse the opt-in paths above.Test Plan
deno check,deno lint,deno fmt, fulldeno run test(4455 passed),deno run compilesrc/libswamp/extensions/local_edits_test.ts(match, mismatch, no-anchor × 3 conditions, LocalEditsError message shape)src/serve/open/http_test.ts(409 onLocalEditsError, 500 on plainErroras a regression fence)swamp extension pull @stack72/system-extensions→ edit a file →curl POST /api/extensions/install→ got 409 with the expected remediation body.swamp extension pull --force→ curl again → 200.datastore_auto_update_test.tsandresolve_datastore_test.tsboth still green after the helper refactor🤖 Generated with Claude Code