Skip to content

idp: add PUT /idp/credentials for self-service password change - #355

Merged
melvincarvalho merged 4 commits into
gh-pagesfrom
issue-351-change-password
May 3, 2026
Merged

idp: add PUT /idp/credentials for self-service password change#355
melvincarvalho merged 4 commits into
gh-pagesfrom
issue-351-change-password

Conversation

@melvincarvalho

Copy link
Copy Markdown
Contributor

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.

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

Why this shape

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.

Re-auth-by-current-password serves two purposes:

  1. Rotation proof — protects against stolen-token-but-don't-know-password attackers locking the legitimate owner out.
  2. Cross-account write defense — A authenticated with B's password as currentPassword still 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-passwordChangedAt check in verifyToken) and deserves its own diff for review focus. The data layer already stamps passwordChangedAt on rotation, so PR2 is small.

Test plan

5 acceptance cases, all passing:

  • Unauthenticated PUT → 401
  • Missing newPassword → 400 (and missing currentPassword)
  • Wrong currentPassword → 401, original password still logs in
  • Happy path → 200, old password rejected, new password accepted
  • Cross-account write (A authenticated, sends B's password) → 401, both passwords unchanged
  • All 48 existing IDP tests still pass — no regression

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).

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 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/credentials route in the IdP plugin with rate limiting.
  • Implement handleChangePassword() to re-auth via current password and rotate the bcrypt hash (stamping passwordChangedAt).
  • 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.

Comment thread src/idp/credentials.js
Comment on lines +225 to +239
// 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',
});
}

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.

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.

Comment thread src/idp/credentials.js
// Re-read to surface passwordChangedAt
const updated = await findByWebId(webId);

reply.header('Cache-Control', 'no-store');
Comment on lines +51 to +69
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);
});
Comment thread src/idp/credentials.js Outdated
Comment on lines +212 to +213
* 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.

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 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.

Comment thread src/idp/credentials.js
Comment on lines +231 to +238
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',
});

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.

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.

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 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.

Comment thread src/idp/credentials.js Outdated
Comment on lines +251 to +253
// 4. Verify currentPassword (re-auth proof)
const reauth = await authenticate(account.email, currentPassword);
if (!reauth || reauth.id !== account.id) {

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.

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.

Comment thread src/idp/index.js
Comment on lines +269 to +275
fastify.put('/idp/credentials', {
config: {
rateLimit: {
max: 10,
timeWindow: '1 minute',
keyGenerator: (request) => request.ip
}

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.

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.

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.


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

@melvincarvalho
melvincarvalho merged commit 254f485 into gh-pages May 3, 2026
4 checks passed
@melvincarvalho
melvincarvalho deleted the issue-351-change-password branch May 3, 2026 14:12
melvincarvalho added a commit to nosdav/browser that referenced this pull request May 3, 2026
* 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.
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.

Add HTTP endpoint for end users to change their own password

2 participants