Skip to content

idp: HTML form for self-service account deletion (#392)#393

Merged
melvincarvalho merged 8 commits into
gh-pagesfrom
issue-392-delete-account-form
May 9, 2026
Merged

idp: HTML form for self-service account deletion (#392)#393
melvincarvalho merged 8 commits into
gh-pagesfrom
issue-392-delete-account-form

Conversation

@melvincarvalho

@melvincarvalho melvincarvalho commented May 9, 2026

Copy link
Copy Markdown
Contributor

Closes #392. Builds on #391 (JSON endpoint).

Summary

  • Public unauthenticated form at /idp/account/delete — matches the existing /idp and /idp/register pattern.
  • Auth at submission time: form posts username + password, server validates credentials and uses them as proof-of-possession for the delete (side-effect-free via findByUsername + verifyPassword per pass 5).
  • No JavaScript required. Form-encoded POST works in any browser.
  • Single-user mode returns 403 with HTML disabled-message body (consistent with /idp/register and the JSON DELETE endpoint's 403).

Form

  • Username (no email — JSS doesn't actually do email anywhere user-facing; data layer is the only place email exists, auto-generated as <username>@jss)
  • Current password (re-entry as proof)
  • Type your username again to confirm (destructive-action UX guard)
  • ☐ Keep my pod data on this server — opt-out checkbox; the form's default is purge-on (the user is leaving the server, so wiping their pod data with the account is the expected shape). Checking this box opts out of the purge but the account is still deleted.

Form vs JSON endpoint defaults — deliberately different

Surface Default Field Rationale
POST /idp/account/delete (form) purge ON keepData (opt-out) User is leaving — total cleanup is the natural expectation
DELETE /idp/account (JSON) purge off purgeData (opt-in) Programmatic / CLI parity — operator scripts shouldn't accidentally wipe data
jss account delete (CLI) purge off --purge flag Operator path; same shape as JSON

Routes

GET /idp/account/delete Render the form (or single-user disabled page with 403)
POST /idp/account/delete Process the submission. 5/min/IP rate-limit (same as JSON endpoint)

Internal refactor

Factored "delete + optional purge" into deleteAccountAndOptionallyPurge(request, account, purgeData) shared between the JSON endpoint and the form endpoint — same code path, same case-sensitivity fix (#391 pass 2: uses account.podName), same path-relative safety check (#391 pass 3).

Tests added (9)

GET renders form HTML ✓ all required fields present, including name="keepData"
POST happy path ✓ default purges pod data + success page + login fails 401
POST with keepData=on ✓ account deleted but pod data preserved
POST mismatched confirmUsername ✓ form re-rendered with error, account untouched
POST wrong password ✓ form re-rendered with error, account untouched
POST missing fields ✓ form re-rendered with error
Single-user GET 403 with disabled message
Single-user POST 403 + account still functional
Existing 9 DELETE-endpoint tests ✓ unchanged

17/17 delete-account tests pass; 636/636 full suite passes.

Files

  • src/idp/credentials.jshandleAccountDeleteForm + shared deleteAccountAndOptionallyPurge helper, ~120 LOC
  • src/idp/views.jsaccountDeletePage template (form + single-user disabled variant + success variant), ~165 LOC
  • src/idp/index.js — two routes (GET + POST) with 403 handling for single-user mode, ~24 LOC
  • test/idp-delete-account.test.js — 9 new tests, ~270 LOC

Test plan

  • CI green
  • Manual: open https://test-pod.example/idp/account/delete in a browser → form renders
  • Manual: submit with valid credentials and leave the keep-data box unchecked (default) → success page, account gone, pod data gone
  • Manual: submit with the keep-data box checked → success page, account gone, pod data preserved
  • Manual: visit on a single-user pod → 403 with disabled message instead of form

Refs

Human-friendly UI on top of the JSON DELETE /idp/account endpoint
(#391). Public unauthenticated page at /idp/account/delete; auth
happens at submission time via username + password. Matches the
existing /idp landing and /idp/register pattern — no JavaScript
required, accessibility-first form-encoded POST.

## Form fields
- Username or email
- Current password (re-entry as proof of possession)
- Type your username again to confirm (destructive-action UX guard)
- Optional checkbox: also delete pod data

## Routes added
- GET  /idp/account/delete  → render the form (or single-user disabled message)
- POST /idp/account/delete  → process submission

Same 5/min/IP rate-limit as the JSON endpoint. Single-user mode shows
the disabled-message page instead of the form on both GET and POST,
matching the JSON endpoint's 403 policy. Operator path remains the
CLI (`jss account delete`).

## Internal refactor
Factored the shared "delete + optional purge" logic into
`deleteAccountAndOptionallyPurge(request, account, purgeData)` so the
JSON endpoint and form endpoint share the same path. Pod-data purge
uses `account.podName` (per #391 pass 2) and path.relative (per #391
pass 3) so the case-sensitivity fix and FS-root edge case carry over.

## Tests added (8)
- GET renders form HTML with all required fields
- POST happy path → success page + login fails after
- POST with purgeData=on wipes pod tree
- POST with mismatched confirmUsername → form re-rendered with error
  + username pre-filled for retry, account untouched
- POST with wrong password → form re-rendered with error, account
  untouched
- POST with missing fields → form re-rendered with error
- Single-user mode: GET renders disabled message (no form)
- Single-user mode: POST also renders disabled message — does NOT
  delete (verified: account still functional after the POST)

636/636 full suite passes.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a human-friendly, no-JavaScript HTML account-deletion flow to the IdP at /idp/account/delete, complementing the existing JSON DELETE /idp/account endpoint and aligning with the existing /idp + /idp/register UI patterns.

Changes:

  • Introduces a public GET page that renders an account-deletion form (or a single-user-mode disabled page).
  • Adds a POST handler that validates username/password + confirmation and performs account deletion with optional pod-data purge.
  • Expands the IdP delete-account test suite to cover the new HTML GET/POST flows and single-user behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
src/idp/credentials.js Adds form POST handler and a shared delete+optional-purge helper.
src/idp/views.js Adds the accountDeletePage HTML template (form, disabled, success variants).
src/idp/index.js Registers GET/POST routes for /idp/account/delete with rate limiting on POST.
test/idp-delete-account.test.js Adds tests for HTML form rendering, submission outcomes, purge behavior, and single-user mode.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/idp/credentials.js
Comment on lines +420 to +424
/**
* Internal: delete an account record + optional pod-data purge.
* Shared between the JSON endpoint (handleDeleteAccount) and the
* form-driven endpoint (handleAccountDeletePost in #392).
*

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real catch. Fixed in b8a2afc: `handleDeleteAccount` now calls `deleteAccountAndOptionallyPurge` instead of carrying its own inline purge block. Both endpoints share the same code path verbatim — no divergence risk.

Comment thread src/idp/credentials.js Outdated
/**
* Internal: delete an account record + optional pod-data purge.
* Shared between the JSON endpoint (handleDeleteAccount) and the
* form-driven endpoint (handleAccountDeletePost in #392).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b8a2afc: JSDoc now references `handleAccountDeleteForm` (the actual exported name) and notes that both GET and POST methods live in the same handler.

Comment thread src/idp/credentials.js Outdated
Comment on lines +468 to +472
* endpoint. Returns HTML — success page on completion, redirect back
* to the GET form with an error message on failure.
*
* Single-user mode: redirects to GET which renders the disabled
* message instead of the form. Same policy as the JSON endpoint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b8a2afc: doc now says the failure path re-renders the form in-place at status 200 (single response), not a redirect. The implementation hasn't changed; just describing it accurately.

Comment thread src/idp/credentials.js Outdated
Comment on lines +530 to +531
await deleteAccountAndOptionallyPurge(request, account, purgeData);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real UX bug. Fixed in b8a2afc: form handler now passes the `purged` value from the helper through to the view. When the user requested `purgeData` but the purge didn't run (fs throw, path-relative check rejected, etc.), the success page surfaces 'your account was deleted but the pod-data purge did not complete on this server'. Account deletion still succeeds (it can't be rolled back) but the user knows files may remain.

Comment thread src/idp/views.js Outdated
Comment on lines +653 to +656
<strong>This is permanent.</strong> Your account record, all credentials, and any active
sessions will be removed. Anyone holding your WebID URI will see it become a
tombstone — federated references (ActivityPub, Nostr, type indexes) cannot be
retracted from this server.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b8a2afc: warning copy softened to 'future sign-ins with this username will fail' + an italic note that already-issued access tokens may remain usable until their expiry. JSS doesn't currently revoke tokens on deletion (noted as deferred follow-up in #391's PR description) and the page no longer claims it does.

Comment thread src/idp/views.js Outdated
<div class="container">
<div class="logo">${solidLogo}</div>
<h1>Account deleted</h1>
<p>Your account has been permanently removed. Any active sessions are now invalid.</p>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fix in b8a2afc: success copy now says 'Future sign-ins with this username will fail' with the italic token-expiry note. No longer claims session invalidation the server doesn't perform.

Six fixes per the review:

1. handleDeleteAccount now actually uses the new
   deleteAccountAndOptionallyPurge helper. The helper was added in the
   prior commit but the original function still had its own inline
   purge block — divergence risk. Now both endpoints share the same
   path verbatim.

2. JSDoc reference \`handleAccountDeletePost\` → \`handleAccountDeleteForm\`
   (the actual exported name). Two route methods (GET + POST) live in
   the same handler; doc updated to reflect that.

3. JSDoc claimed failures "redirect back to the GET form with an
   error message" — the implementation actually re-renders the form
   in-place with status 200. Doc corrected to match (single response,
   no redirect).

4. Form handler now respects the \`purged\` return from the helper.
   When the user requested purgeData but the purge didn't run (fs
   threw, path-relative check rejected), the success page surfaces a
   "your account was deleted but the pod-data purge did not complete"
   notice. Account deletion succeeded — that part can't be undone —
   but the operator may need to finish cleanup.

5. Warning copy on the form softened: was "any active sessions will
   be removed", which the server doesn't actually implement.
   Now: "future sign-ins with this username will fail" + an italic
   note that already-issued access tokens may remain usable until
   their expiry.

6. Same correction on the success-page copy.

17/17 delete-account tests still pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Comment thread src/idp/credentials.js Outdated
/**
* Internal: delete an account record + optional pod-data purge.
* Shared between the JSON endpoint (handleDeleteAccount) and the
* form-driven endpoint (handleAccountDeletePost in #392).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8448f94: one stray reference to handleAccountDeletePost in the helper's JSDoc that the pass-1 fix missed. Now correctly says handleAccountDeleteForm.

Comment thread src/idp/views.js Outdated
Comment on lines +547 to +551
* @param {string|null} opts.username - Pre-fill on redirect-back-with-error
* @param {boolean} opts.singleUser - When true, render a disabled message
* instead of the form. Deletion via HTTP is blocked in single-user mode
* (would brick the IdP until re-seed); operator path stays the CLI.
* @param {boolean} opts.success - When true, render the post-delete confirmation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8448f94: JSDoc no longer says 'redirect-back-with-error' (the implementation re-renders in place, status 200, no redirect) and the `purgeFailed` param added in pass 1 is now documented.

Comment thread src/idp/views.js
Comment on lines +688 to +690
<label for="confirmUsername">Type your username again to confirm</label>
<input type="text" id="confirmUsername" name="confirmUsername" required
placeholder="Must match the username above">

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real UX confusion. Investigated and confirmed by the maintainer: JSS doesn't actually do email anywhere user-facing — no SMTP, no verification flow, no recovery flow, the HTML register form doesn't even have an email input. The 'email' field in accounts.js is purely a data-layer artifact (auto-generates `@jss` if absent). So 'Username or email' on this form mismatched reality. Fixed in 8448f94 by simplifying the label to 'Username' (placeholder: 'alice'). Now consistent with the 'Type your username again' confirmation. The server's authenticate() still tries findByUsername then findByEmail for back-compat, but the email-fallback path is unreachable from this form.

Comment thread src/idp/credentials.js
Comment on lines +459 to +475
const confirmUsername = (body?.confirmUsername || '').trim();
const purgeData = body?.purgeData === 'on' || body?.purgeData === true;

if (!username || !currentPassword || !confirmUsername) {
return reply.type('text/html').send(accountDeletePage({
error: 'All fields are required.',
username,
}));
}

// Destructive-action UX guard: typed username must match. Compare
// case-sensitively against the *typed* form value, not the resolved
// account — the user shouldn't be able to typo their way to a delete
// that hits a different (lowercased) record.
if (username !== confirmUsername) {
return reply.type('text/html').send(accountDeletePage({
error: 'Confirmation does not match the username you entered.',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fix as the previous comment in 8448f94 — dropping email from the form UI removed the inconsistency. The variable / comments still say 'username' which now accurately reflects what the form accepts. Renaming to 'identifier' would only make sense if we re-introduce email at the form layer; for now 'username' is honest.

…ername UX confusion

Four polish fixes:

1. handleAccountDeleteForm reference in deleteAccountAndOptionallyPurge's
   JSDoc was still mis-named handleAccountDeletePost (one stray spot the
   pass-1 fix missed).

2. accountDeletePage's JSDoc updated: "redirect-back-with-error" was
   wrong (we re-render in place); also documented the new purgeFailed
   param added in pass 1.

3. + 4. Form label simplified from "Username or email" → "Username".
   Confirms a user point: JSS doesn't really do email — no SMTP, no
   verification flow, no recovery flow, the HTML register form doesn't
   ask for email at all. The "email" field in accounts.js is a
   data-layer artifact (auto-generates <username>@jss if absent) and
   isn't surfaced anywhere user-facing. Saying "username or email" on
   this form mismatched reality and confused the destructive-action
   confirm field ("Type your username again to confirm"). Now both
   fields consistently say username, matching what users will actually
   enter.

Server-side authenticate() still tries findByUsername then
findByEmail for back-compat with anyone who registered programmatically
with a real email; that fallback is just unreachable from this form
since the field never accepts email anymore. Cleaning that up across
the wider auth surface is a separate PR.

17/17 delete-account tests still pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread src/idp/credentials.js Outdated
Comment on lines +416 to +423
/**
* Handle GET / POST /idp/account/delete (#392) — form-driven account deletion.
*
* Public unauthenticated endpoint that takes a form-encoded body with
* username + currentPassword + confirmUsername (+ optional purgeData
* checkbox). Authenticates the user via password directly (no Bearer
* token round-trip required), validates the destructive-action UX
* guard, then calls into the same delete logic as the JSON endpoint

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 253e730: JSDoc now says POST-only and points at index.js for the GET route (which just renders the form via accountDeletePage). The 'GET / POST' wording from pass 1 was misleading — only POST flows through this handler.

Comment thread src/idp/views.js Outdated
Comment on lines +541 to +552
* supplies an identifier (username or email) + password, which the
* server validates and uses as proof-of-possession for the delete.
* The "type your identifier to confirm" field is the destructive-action
* UX guard.
*
* On any failure (wrong password, mismatched confirmation, etc.) the
* handler re-renders this same form in place at status 200 with an
* error message and the identifier field pre-filled — no redirect.
*
* @param {object} opts
* @param {string|null} opts.error - Error message (e.g. wrong password) to display
* @param {string} opts.username - Pre-fill the identifier field on re-render after error

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 253e730: JSDoc no longer mentions 'identifier (username or email)' — pass 2 dropped email from the form UI but the doc text still carried the old wording. Now consistently says 'username + password'.

Comment thread src/idp/views.js Outdated
<input type="checkbox" id="purgeData" name="purgeData" value="on">
<label for="purgeData">
<strong>Also delete all my pod data.</strong> Removes
<code>&lt;dataRoot&gt;/&lt;username&gt;/</code> from disk —

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real correctness issue. Fixed in 253e730: copy now says 'Removes the pod folder on disk' instead of promising `//` specifically. The actual purge path uses account.podName (per #391 pass 2 — username gets lowercased in createAccount, podName preserves the original case).

Three small fixes:

1. handleAccountDeleteForm JSDoc clarifies it's POST-only; GET is a
   separate route in index.js that just renders the form via
   accountDeletePage. Pass-1 wording was 'GET / POST' which implied
   both routes flow through this handler.

2. accountDeletePage's JSDoc still mentioned 'identifier (username or
   email)' from before the email simplification in pass 2. Now says
   'username + password' to match the actual UI.

3. The checkbox copy promised 'Removes <dataRoot>/<username>/' but
   the purge path is derived from account.podName (which can differ
   from the lowercased username on case-sensitive FS, per #391
   pass 2). Softened to 'Removes the pod folder on disk' — accurate
   without leaking the path shape.

17/17 tests pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread src/idp/credentials.js Outdated
Comment on lines +448 to +454
// Parse form-encoded body. Fastify with @fastify/formbody (registered
// for /idp/register etc.) puts fields directly on request.body.
let body = request.body;
if (Buffer.isBuffer(body)) body = body.toString('utf-8');
if (typeof body === 'string') {
// Manual urlencoded parse fallback if formbody isn't registered for
// this content-type on this route.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real catch. Fixed in 63aae7f: comment now describes what actually happens — JSS's wildcard parseAs:'buffer' parser at server.js:190 means request.body arrives as a Buffer, and the URLSearchParams parse is the load-bearing logic, not a fallback. Same shape as handleChangePassword's JSON-side body parsing. No @fastify/formbody anywhere in the repo.

The comment claimed @fastify/formbody puts fields directly on
request.body, but no such plugin is installed. JSS's wildcard
parseAs:'buffer' content parser at server.js:190 means request.body
arrives as a Buffer; the URLSearchParams parse below is what's
actually load-bearing. Comment updated to describe what the code
genuinely does.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread src/idp/credentials.js Outdated
Comment on lines +486 to +494
// Authenticate. authenticate() looks up by username then by email,
// verifies bcrypt, and returns the account (sans password hash) or null.
const account = await authenticate(username, currentPassword);
if (!account) {
return reply.type('text/html').send(accountDeletePage({
error: 'Username or password is incorrect.',
username,
}));
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real catch. Fixed in 8f83cb9: now uses `findByUsername` + `verifyPassword` (no side effects) instead of `authenticate` (which writes lastLogin). Mirrors what handleChangePassword and the JSON handleDeleteAccount both do for the same reason — destructive proof-of-possession checks shouldn't trigger account-file writes.

Comment thread src/idp/views.js Outdated
Comment on lines +673 to +675
removed; future sign-ins with this username will fail. Anyone holding your
WebID URI will see it become a tombstone — federated references (ActivityPub,
Nostr, type indexes) cannot be retracted from this server.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real correctness. Fixed in 8f83cb9. Also surfacing here that the maintainer decided to flip the form's purge default to ON — leaving-user UX. So the warning copy now says 'pod data … will be removed' as the default expectation, with the inverse 'Keep my pod data' checkbox as the explicit opt-out. JSON endpoint keeps purge-off default (matches CLI for operator scripts). Updated copy: future sign-ins fail, federated references become dangling links.

Comment thread src/idp/index.js
Comment on lines +299 to +318
// GET account-delete form (#392) - human-friendly UI for #352. Public
// unauthenticated page; auth happens at form submission via password.
fastify.get('/idp/account/delete', async (request, reply) => {
return reply.type('text/html').send(accountDeletePage({ singleUser }));
});

// POST account-delete form (#392) - processes the form submission.
// Same rate-limit as the JSON endpoint to keep the destructive-action
// surface consistent across both paths.
fastify.post('/idp/account/delete', {
config: {
rateLimit: {
max: 5,
timeWindow: '1 minute',
keyGenerator: (request) => request.ip
}
}
}, async (request, reply) => {
return handleAccountDeleteForm(request, reply, { singleUser });
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real consistency issue. Fixed in 8f83cb9: GET and POST both return 403 in single-user mode now (HTML body kept). Matches /idp/register's disabled-route policy and the JSON DELETE endpoint's 403. Tests updated to expect 403.

…e-user, flip purge default to ON

Five fixes (4 from Copilot, 1 from maintainer call to flip the default):

1. handleAccountDeleteForm now uses findByUsername + verifyPassword
   instead of authenticate(). authenticate() writes lastLogin as a
   side effect, which is wrong shape for a destructive proof-of-
   possession check (and would fail the deletion if the account file
   weren't writable). Mirrors handleChangePassword and the JSON
   handleDeleteAccount.

2. Warning copy corrected. Previous text said "any active sessions
   will be removed" + "WebID URI will become a tombstone" — the
   server does neither. Now: "Future sign-ins with this username will
   fail" + "external links to your WebID URI will become dangling"
   (since pod data is being wiped, the URI no longer resolves).

3. Single-user mode now returns 403 from both GET and POST routes
   (matched HTML body kept). Consistency: /idp/register's
   disabled-route policy is 403, the JSON DELETE endpoint is 403,
   and now the form path is too.

4. Body-parsing comment corrected: was claiming @fastify/formbody is
   registered (it isn't); JSS uses parseAs:'buffer' for `*` so body
   arrives as a Buffer and the URLSearchParams parse is the
   load-bearing logic.

5. Form's purge default flipped from off → ON. The checkbox is now
   `keepData` (inverse of the JSON endpoint's `purgeData`), default
   unchecked = purge-on. A user filling out the form is leaving the
   server; wiping their pod data with the account is the expected
   shape, and "Keep my pod data on this server" is the explicit
   opt-out. The JSON DELETE endpoint keeps `purgeData: false` as the
   default (matches the CLI's --purge being opt-in for operator
   scripts; programmatic callers shouldn't accidentally wipe data).
   Form and JSON endpoint diverge deliberately because the use cases
   differ.

Tests:
  - happy path now expects pod data wiped by default + login fails
  - new test for keepData=on (opts out of purge; account still
    deleted, pod data preserved)
  - single-user GET / POST now expect 403
  - form-renders test now checks for name="keepData"

17/17 delete-account tests pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread src/idp/views.js
Comment on lines +686 to +706
<form method="POST" action="/idp/account/delete">
<label for="username">Username</label>
<input type="text" id="username" name="username" required autofocus
value="${escapeHtml(username || '')}"
placeholder="alice">

<label for="currentPassword">Current password</label>
<input type="password" id="currentPassword" name="currentPassword" required
placeholder="Re-enter your password">

<label for="confirmUsername">Type your username again to confirm</label>
<input type="text" id="confirmUsername" name="confirmUsername" required
placeholder="Must match the username above">

<div class="checkbox-row">
<input type="checkbox" id="keepData" name="keepData" value="on">
<label for="keepData">
<strong>Keep my pod data on this server.</strong> Check only if you want
to delete just your account record and leave your files in place. Default
(unchecked) wipes the pod folder along with the account.
</label>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair catch — PR description was stale after pass 2's email simplification and pass 5's purge-default flip. Updated to match: username-only form, `keepData` opt-out checkbox, purge-on default for the form path, with a comparison table showing why the form / JSON / CLI surfaces deliberately have different defaults (form = leaving-user UX; JSON + CLI = programmatic / operator-script parity).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread src/idp/views.js Outdated
Comment on lines +672 to +676
<strong>This is permanent.</strong> Your account record, credentials, and pod data
(every file you've stored — including your WebID profile document) will be removed.
Future sign-ins with this username will fail. Federated references (ActivityPub
follows, Nostr relays, type indexes) cannot be retracted from this server, so
external links to your WebID URI will become dangling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real correctness gap. Fixed in 93e8f40: warning now says 'By default, your pod data is also wiped — check the box below if you want to keep it.' Honest description of the conditional behavior; user reads the warning before deciding on the checkbox state.

Comment thread src/idp/credentials.js
// Don't surface the raw error to the user (file paths,
// permission detail leak); response.purged signals the
// outcome.
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 93e8f40: added a log.warn with path/dataRoot/podName/username context when isProperChild is false. Operators can now correlate any 'purge did not complete' message back to the actual reason without paths leaking to the client.

Comment thread src/idp/credentials.js Outdated
Comment on lines +487 to +505
// Destructive-action UX guard: typed username must match. Compare
// case-sensitively against the *typed* form value, not the resolved
// account — the user shouldn't be able to typo their way to a delete
// that hits a different (lowercased) record.
if (username !== confirmUsername) {
return reply.type('text/html').send(accountDeletePage({
error: 'Confirmation does not match the username you entered.',
username,
}));
}

// Look up + verify password without side effects. authenticate() is
// tempting (looks up + verifies in one call) but writes lastLogin on
// success — wrong shape for a destructive proof-of-possession check
// (and would fail the deletion if the account file weren't writable).
// Mirrors handleChangePassword / handleDeleteAccount which both use
// verifyPassword for the same reason.
const account = await findByUsername(username);
if (!account || !(await verifyPassword(account, currentPassword))) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real misleading comment. Fixed in 93e8f40: the comment used to claim case-sensitivity prevents 'typo hitting a different lowercased record', but findByUsername() lowercases internally so 'Alice' + 'Alice' both resolve to 'alice'. The case-sensitive string compare is purely the typing-it-twice confirmation pattern (typo / accidental-submit guard) — doesn't actually gate which record gets deleted. Comment now describes the real shape.

…tion, fix confirm comment

Three fixes:

1. Warning copy now describes the conditional behavior accurately:
   'By default your pod data is also wiped — check the box below if
   you want to keep it.' Previous wording said pod data 'will be
   removed' unconditionally, which becomes wrong if the user checks
   the keep-data opt-out.

2. Path-safety rejection (when isProperChild is false) now logs a
   warn with path/dataRoot/podName context so operators can diagnose
   any 'purge did not complete' message that wasn't a fs.remove
   throw. No path leaks to the client; signal stays in server logs.

3. Comment on the destructive-action UX guard was misleading. It
   claimed the case-sensitive compare prevents typos hitting a
   different (lowercased) record — but findByUsername lowercases
   internally, so 'Alice' + 'Alice' both resolve to the 'alice'
   record. The compare is purely the typing-it-twice confirmation
   pattern (typo catch / accidental-submit guard); doesn't gate which
   record gets deleted. Comment now reflects what the code actually
   does.

17/17 tests pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread src/idp/index.js
Comment on lines +299 to +310
// GET account-delete form (#392) - human-friendly UI for #352. Public
// unauthenticated page; auth happens at form submission via password.
// Single-user mode returns 403 to stay consistent with /idp/register's
// disabled-route policy and the JSON DELETE /idp/account endpoint
// (which also 403s in single-user mode). Body is still HTML so a
// browser visitor sees the explanation.
fastify.get('/idp/account/delete', async (request, reply) => {
if (singleUser) {
return reply.code(403).type('text/html').send(accountDeletePage({ singleUser: true }));
}
return reply.type('text/html').send(accountDeletePage({ singleUser: false }));
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real concern. Fixed in 91275c2: GET handler now calls `setNoCacheClickjackHeaders(reply)` (new exported helper in credentials.js) before sending. All four headers go out: `Cache-Control: no-store`, `Pragma: no-cache`, `X-Frame-Options: DENY`, `Content-Security-Policy: frame-ancestors 'none'`. Test asserts all four on the GET response.

Comment thread src/idp/credentials.js
Comment on lines +453 to +459
export async function handleAccountDeleteForm(request, reply, options = {}) {
if (options.singleUser) {
// 403 matches the GET route, /idp/register's disabled-route policy,
// and the JSON DELETE endpoint's 403 — consistent status across
// every disabled-in-single-user surface.
return reply.code(403).type('text/html').send(accountDeletePage({ singleUser: true }));
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fix applied to the POST handler in 91275c2. The helper call sits at the top of `handleAccountDeleteForm`, so every return path (success, error re-render, single-user 403, missing fields, mismatched confirm, wrong password) carries the headers. Test asserts them on both the missing-fields error response and the full-delete success response.

…elete-form response

Real security findings. The delete-account form takes a current-password
re-entry and the success/error pages echo state — neither should be
cacheable or embeddable in an iframe. Both threats (cache replay,
clickjacking) are real on a destructive endpoint.

Added a small exported helper:

  setNoCacheClickjackHeaders(reply):
    Cache-Control: no-store
    Pragma: no-cache
    X-Frame-Options: DENY
    Content-Security-Policy: frame-ancestors 'none'

Wired into:
  - GET /idp/account/delete (form + 403 disabled-message variant)
  - POST handler entry point — covers every return path (success,
    error re-render, single-user 403, missing-field, mismatched
    confirm, wrong password)

Tests added (2):
  - GET response carries all four headers
  - POST response carries them on both error (missing fields) and
    success (full delete) paths

19/19 delete-account tests pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@melvincarvalho
melvincarvalho merged commit 51152a4 into gh-pages May 9, 2026
4 checks passed
@melvincarvalho
melvincarvalho deleted the issue-392-delete-account-form branch May 9, 2026 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

idp: HTML form for self-service account deletion at /idp/account/delete

2 participants