idp: add PUT /idp/credentials for self-service password change - #355
Conversation
Logged-in users can rotate their own password over HTTP without shell
access to the server.
PUT /idp/credentials
Authorization: Bearer/DPoP/Nostr
Body: { currentPassword, newPassword }
200 — rotated; returns { ok, webid, passwordChangedAt }
400 — missing fields
401 — unauthenticated, or currentPassword wrong (hash unchanged)
403 — caller's WebID has no account
Caller's WebID is extracted via getWebIdFromRequestAsync, the account is
resolved via findByWebId, and currentPassword is re-verified with
authenticate() before updatePassword() rotates the bcrypt hash. This
binds the rotation to the authenticated owner and forecloses
cross-account writes — A authenticated with B's password as
currentPassword still 401s because the lookup goes through A's WebID.
Session/refresh-token invalidation for *other* live sessions is the
follow-up PR — issue #351 acceptance item, but a security-critical
change to token verification deserves its own diff for review focus.
The data layer already stamps passwordChangedAt on rotation, so the
follow-up just adds an iat-vs-passwordChangedAt check in verifyToken.
Tests cover all five cases: unauth 401, missing fields 400, wrong
current password 401 (with old password still working after), happy
path (old fails / new succeeds), cross-account 401 (both passwords
unchanged).
There was a problem hiding this comment.
Pull request overview
Adds a self-service password rotation capability to the built-in IdP so authenticated users can change their own password over HTTP (issue #351, partial—session invalidation deferred).
Changes:
- Add
PUT /idp/credentialsroute in the IdP plugin with rate limiting. - Implement
handleChangePassword()to re-auth via current password and rotate the bcrypt hash (stampingpasswordChangedAt). - Add acceptance-style tests covering unauthenticated, missing fields, wrong current password, success, and cross-account attempts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/idp/index.js |
Registers the new PUT /idp/credentials endpoint with rate limiting. |
src/idp/credentials.js |
Implements the password-change handler using token-derived WebID → account lookup → re-auth → password update. |
test/idp-change-password.test.js |
Adds end-to-end tests for the new password-change behavior and failure modes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 2. Parse body | ||
| let body = request.body; | ||
| if (Buffer.isBuffer(body)) body = body.toString('utf-8'); | ||
| if (typeof body === 'string') { | ||
| try { body = JSON.parse(body); } catch { body = {}; } | ||
| } | ||
| const currentPassword = body?.currentPassword; | ||
| const newPassword = body?.newPassword; | ||
|
|
||
| if (!currentPassword || !newPassword) { | ||
| return reply.code(400).send({ | ||
| error: 'invalid_request', | ||
| error_description: 'currentPassword and newPassword are required', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Declining: this endpoint's contract is JSON-only — the spec in #351 lists only application/json, and any caller already sends JSON for the auth header (Bearer/DPoP/Nostr). Adding a 415 path or form-encoded support is bytes for a hypothetical client that doesn't exist. The current behavior — non-JSON body → 400 "missing fields" — is acceptable for a contract-violating request.
The other three comments (Pragma header, JSDoc 403 wording, DATA_ROOT save/restore) are all addressed in f0556c4.
| // Re-read to surface passwordChangedAt | ||
| const updated = await findByWebId(webId); | ||
|
|
||
| reply.header('Cache-Control', 'no-store'); |
| before(async () => { | ||
| await fs.remove(DATA_DIR); | ||
| await fs.ensureDir(DATA_DIR); | ||
| const port = await getAvailablePort(); | ||
| baseUrl = `http://${TEST_HOST}:${port}`; | ||
| server = createServer({ | ||
| logger: false, | ||
| root: DATA_DIR, | ||
| idp: true, | ||
| idpIssuer: baseUrl, | ||
| forceCloseConnections: true, | ||
| }); | ||
| await server.listen({ port, host: TEST_HOST }); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| await server.close(); | ||
| await fs.remove(DATA_DIR); | ||
| }); |
| * 401 unauthenticated, or currentPassword wrong | ||
| * 403 caller's WebID does not match any account / cross-account write |
- credentials.js: drop "/ cross-account write" from 403 doc — that path returns 401 via wrong-password (intentionally; tested in PR description) - credentials.js: add Pragma: no-cache for parity with handleCredentials - test: save/restore process.env.DATA_ROOT around the suite, matching idp.test.js convention to avoid global leak under same-process runners Declined: Content-Type-aware body parsing — this endpoint's contract is JSON-only; adding 415/form-encoded handling is bytes for hypothetical clients.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const currentPassword = body?.currentPassword; | ||
| const newPassword = body?.newPassword; | ||
|
|
||
| if (!currentPassword || !newPassword) { | ||
| return reply.code(400).send({ | ||
| error: 'invalid_request', | ||
| error_description: 'currentPassword and newPassword are required', | ||
| }); |
There was a problem hiding this comment.
Addressed in 6ea89c7 — both fields now require typeof === 'string' and non-empty. Non-string values return a clean 400 invalid_request instead of reaching bcrypt.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 4. Verify currentPassword (re-auth proof) | ||
| const reauth = await authenticate(account.email, currentPassword); | ||
| if (!reauth || reauth.id !== account.id) { |
There was a problem hiding this comment.
Addressed in cf05d1c — added verifyPassword(account, password) to accounts.js (pure bcrypt compare, no fs writes), and handleChangePassword now uses it instead of authenticate(). Password rotation no longer stamps lastLogin.
| fastify.put('/idp/credentials', { | ||
| config: { | ||
| rateLimit: { | ||
| max: 10, | ||
| timeWindow: '1 minute', | ||
| keyGenerator: (request) => request.ip | ||
| } |
There was a problem hiding this comment.
Deferred to a separate issue: #356. The same keyGenerator: (request) => request.ip is on existing routes (POST /idp/credentials, POST /idp/interaction/:uid); fixing only PUT here would be inconsistent and the right fix is server-wide — audit trustProxy (default to 'loopback'), document deployment requirements. Tracking in #356.
…on rotation authenticate() updates lastLogin + writes the account file as a side effect of a successful login. Calling it for re-auth during password rotation falsified the audit trail (lastLogin updated despite no login) and added a redundant fs write before the bcrypt rotation's own write. verifyPassword(account, password) is a pure bcrypt compare — no fs writes, no metadata updates. handleChangePassword now uses it for the rotation proof. Caught by Copilot on PR #355.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* account-pane: phase 1 — change-password form (#12) New pane that drives JSS's PUT /idp/credentials endpoint (shipped in JavaScriptSolidServer/JavaScriptSolidServer#355, JSS 0.0.165). - canHandle: only on the authenticated user's own WebID profile (subject doc URI matches window.xlogin.id's doc URI) - IDP endpoint resolved from solid:oidcIssuer in the profile JSON-LD (the very resource we render on always carries it) - Form: current / new / confirm-new with client-side validation (non-empty, new !== current, new === confirm) - Submit via window.xlogin.authFetch (Bearer/DPoP added by xlogin) - Status mapping: 200 → ok toast, 401 → wrong-password inline error, 400/403/other → contextual error - Anonymous users never see this pane (canHandle returns false when no auth) Phase 2+ (delete account, export, portability) waits on JSS endpoints in #352/#353/#354. * account-pane: preflight before sending password (#12) Critical: prior commit could leak the password to disk on JSS <0.0.165 because the wildcard LDP PUT handler caught the request and wrote the JSON body as a file at /idp/credentials. Verified happened on solid.social during testing — see PR #13 thread. Now the pane sends an UNAUTHENTICATED probe PUT first. The dedicated handler in JSS 0.0.165+ returns a specific 401 with body {error: "invalid_token", error_description: "Authentication required"}. Wildcard fallthrough on older JSS produces a different error shape (or no JSON body), so we only proceed with the real PUT after seeing the sentinel. The probe is unauthenticated and has no body, so even if it falls through to the wildcard on a misconfigured server, no credentials are leaked — at worst an empty file is created. Also: bcrypt rotation fix (e.g. solid:oidcIssuer key shape) from prior commit retained. * account-pane: address Copilot security + a11y feedback (PR #13) Security (CRITICAL): - validateIssuer: refuse to send credentials when solid:oidcIssuer's hostname doesn't match the current page's hostname (or a parent domain — e.g. melvin.solid.social on solid.social IDP). Tampered profile cannot redirect the password to an attacker origin. Same-protocol enforcement prevents downgrade attacks. Bug fixes: - readOidcIssuer: try a wider set of common subject fragments (#me/#this/#i/#card and the doc URI itself) so profiles with non-#me fragments resolve correctly - clearInputs via refs: html.js patches attributes via setAttribute() which doesn't sync input.value. Use ref()/ref.el.value = '' to imperatively clear after success Accessibility: - label for=/input id= association on all three password fields (click-to-focus + screen reader labelling) - role="status" aria-live="polite" wrapper around success/error messages so assistive tech announces the state change Declined: - Preflight removal (Copilot: brittle gate). Leak risk strictly worse than false-negative gate; preflight stays. - Doubled-PUT cost. Same tradeoff. * account-pane: also compare port in validateIssuer (PR #13 round 3) Tampered profile could swap port (e.g. solid.social:8443) and pass the hostname check while pointing at an attacker's listener. URL.port comparison closes that. External-IDP setups (WebID points to shared IDP on unrelated host) remain rejected — known limitation; proper fix waits on melvincarvalho/xlogin#15 to expose the authenticated issuer. * account-pane: render tampered issuer via html template, not raw innerHTML Tampered solid:oidcIssuer like <img onerror=...> would execute when interpolated into innerHTML. Switch the validateIssuer-fail branch to losos/html.js's template tag, which treats ${} as text nodes. * account-pane: tighten canHandle + document residual probe risk (PR #13 round 4) canHandle: exact-match on subject.value vs window.xlogin.id. Same-doc match was too loose — profile docs with both #me and #this would show the Account tab on whichever node was rendered, breaking the contract. Preflight comment: documents the residual risk (misconfigured JSS <0.0.165 with world-writable ACL on /idp/credentials could see an empty file created during probe). WAC normally rejects unauth PUT before the wildcard runs, so this is theoretical. Proper zero-mutation fix tracked at #14 (GET-based discovery via JSS's handleCredentialsInfo, ships in JSS 0.0.166). * account-pane: normalize default ports + handle JSON-LD array issuer (PR #13 round 5) Caught by Copilot: - URL.port is "" for default ports and "443"/"80" when explicit. The raw string comparison rejected same-origin pairs that just serialize the port differently. Normalize empty -> default-for-protocol before comparing. - pickIssuer didn't unwrap JSON-LD array values. solid:oidcIssuer can be [{"@id": "..."}] in conformant JSON-LD; previous code returned the array and typeof check fell through to "no issuer found". * account-pane: relax canHandle, target WebID directly in render (PR #13 round 6) Previous exact-match canHandle hid the Account tab on profiles where LOSOS's findSubject() prefers #this over #me (multi-node docs). The underlying concern (wrong-subject issuer lookup) is better solved by making render() target window.xlogin.id directly, regardless of which subject the shell picked. - canHandle: same-doc check (the pane is "this is my profile doc") - render: readOidcIssuer(store, window.xlogin.id) — always the WebID * account-pane: re-check auth at render-time for logout-after-boot (PR #13 round 7) Tab list is built once during boot when canHandle ran. If the user logs out between boot and clicking the tab, render() crashes on window.xlogin.id deref. Guard at the top of render shows a friendly 'Log in to manage your account' empty-state instead.
Closes #351 (partial — endpoint + bcrypt rotation; session invalidation in follow-up).
Summary
Logged-in users can rotate their own password over HTTP without shell access to the server.
Why this shape
Caller's WebID is extracted via
getWebIdFromRequestAsync, the account is resolved viafindByWebId, andcurrentPasswordis re-verified withauthenticate()beforeupdatePassword()rotates the bcrypt hash.Re-auth-by-current-password serves two purposes:
currentPasswordstill 401s because the lookup goes through A's WebID, not the body. Tested.What's deferred to PR2
Issue #351 acceptance includes "all other active sessions are invalidated." That's a security-critical change to token verification (
iat-vs-passwordChangedAtcheck inverifyToken) and deserves its own diff for review focus. The data layer already stampspasswordChangedAton rotation, so PR2 is small.Test plan
5 acceptance cases, all passing:
newPassword→ 400 (and missingcurrentPassword)currentPassword→ 401, original password still logs in