fix(idp): allow CORS for all clients on token endpoint#11
Open
melvincarvalho wants to merge 3 commits into
Open
fix(idp): allow CORS for all clients on token endpoint#11melvincarvalho wants to merge 3 commits into
melvincarvalho wants to merge 3 commits into
Conversation
Simplify clientBasedCORS to return true for all clients, not just public clients with tokenEndpointAuthMethod 'none'. This fixes OIDC login for web apps loaded from CDN (like Mashlib from unpkg.com) where the origin doesn't match the client's redirect URIs. Solid servers are public and should accept token requests from any web app origin. Fixes #10
Solid-OIDC requires DPoP (Demonstrating Proof-of-Possession) tokens which use a DPoP HTTP header. Without this header in the CORS allowed headers list, browsers block the token exchange request.
oidc-provider v9+ always includes the iss parameter (RFC 9207) in authorization responses. Mashlib's solid-ui auth library doesn't handle this parameter, causing the OAuth callback to fail. This intercepts the Location header and strips the iss parameter before sending the redirect to the client.
melvincarvalho
added a commit
that referenced
this pull request
May 2, 2026
The user feedback on round-3 was clear: the original ask was a simple default change, and "people will figure it out" was explicit license to skip migration cleverness. Round 3's auto-fallback (and round 4's tweaks to it) were scope creep. Revert the auto-fallback. Pre-#348 installs that upgrade in place without flag changes will get a fresh empty root pod alongside their existing /me/ data — operators who hit that pick one of two explicit paths on restart, both documented: 1. Add `--single-user-name me` → legacy layout, no further work. 2. Move `<root>/me/*` to `<root>/`, delete the legacy IDP account for "me", restart → new root pod, new account seeded. At v0.0.x with a small operator base, that one-time intervention is acceptable and keeps the codebase clean. Changes from round 3 / unpushed round 4: - src/server.js: drop the `existsSync(/me/...)` auto-detect block, the `migratedFromMeDefault` flag, and the corresponding warning in onReady. Drop the `existsSync` import. - test/idp.test.js: drop the `Single-user upgrade fallback` suite (no fallback to test). - test/idp.test.js: tighten the `seeds the profile at root` assertion — also fs.pathExists() check that no /me/ files were written, so a regression that left /me/ behind under a 401 wouldn't slip through (round-4 review #7). - docs/configuration.md: add an explicit "Upgrading from a pre-#348 install" callout under Single-User Mode listing both migration paths (round-4 review #11). 593/593 tests pass.
melvincarvalho
added a commit
that referenced
this pull request
May 2, 2026
* Default --single-user to root pod (closes #348) Change the default `singleUserName` from `'me'` to `null`. Without the flag, `jss start --single-user` now serves the pod at the server origin (`/profile/card#me`) instead of `/me/profile/card#me`. Why: in single-user mode there is by definition exactly one pod, so the `/me/` prefix has no namespace-disambiguation purpose. It only adds friction — most Solid clients and tutorials assume the pod coincides with the origin. Operators who actually want a named pod still pass `--single-user-name X` explicitly; that path is unchanged. Behaviour summary: - `jss start --single-user` → root pod (new default) - `jss start --single-user --single-user-name me` → `/me/` (legacy) - `jss start --single-user --single-user-name alice` → `/alice/` Migration note: anyone upgrading a fresh-default install in place needs to either move `data/me/*` → `data/*` or pass `--single-user-name me` to keep the old pod. With the project still at 0.0.x and the userbase small this is an acceptable break; deployed servers in our orbit (solid.social, melvin.me, melvincarvalho.com) all pass `--single-user-name` explicitly and are unaffected. Implementation: - `src/config.js`: defaultConfig.singleUserName: 'me' → null; printConfig now formats the root-pod case cleanly. - `src/server.js`: drop the `?? 'me'` fallback; null = root pod. - `bin/jss.js`: --single-user-name help text updated. - `test/idp.test.js`: new "Single-user default — root pod (#348)" describe asserting the seeded profile lands at /profile/card.jsonld with WebID at the server origin when no name flag is passed. All 592 tests pass. * PR #349 round-1 review (Copilot): seed IDP account for root pod Round-1 review caught a real gap: my first commit moved single-user mode's default to root pod but left `seedSingleUserIdpAccount()` gated on `!isRootPod`, so a fresh `jss start --single-user --idp` produced a pod no operator could log in to (registration is disabled in single-user mode, no fallback). Fix: also seed the IDP account for root-pod mode, defaulting the login username to "me". The pod URL now lives at the origin while the login flow stays the same as a named pod — operator types "me" + password into the login form, gets a token for the WebID at `${origin}/profile/card.jsonld#me`. - src/server.js: drop `&& !isRootPod` from the IDP-seed guard; derive `username = isRootPod ? 'me' : singleUserName` and `podName = isRootPod ? null : singleUserName`. - src/config.js: print-config now reads `Single-user: / (root pod, login as "me") (password: ...)` — removes the misleading "password not seeded" wording. - docs/configuration.md: rewrite the Single-User section to document the new default (root pod, WebID at origin, login as "me") and show `--single-user-name me` as the explicit-legacy knob. - test/idp.test.js: extend the #348 describe to assert that `POST /idp/credentials` with `{username: "me", password}` returns a 200 + access_token — black-box check that the seed actually produces a loggable account. All 593 tests pass. * PR #349 round-2 review (Copilot) Addresses 5 genuinely new points (the other 4 inline comments re-flag round-1 items already in HEAD). 1. Normalize singleUserName at the top of createServer(). '/' / '' / null all collapse to null, so downstream code (remoteStoragePlugin at line 292: `singleUserName || 'me'`) doesn't get a literal '/' string and end up registering /storage/%2F/ instead of /storage/me/. 2. Migration warning. When seeding a fresh root pod, check whether `/me/profile/card.jsonld` (or legacy extensionless card) already exists on disk. If it does, log a clear warning that the default changed in #348 and suggest restarting with --single-user-name me. This catches the silent-upgrade case where an operator on the pre-#348 default ends up with a fresh empty root pod alongside their stranded /me/ data. 3. printConfig wording. The "(login as me)" / "(password: ...)" bits only fire under --idp now, so a `--single-user --no-idp` deployment doesn't claim a built-in login that doesn't exist. 4. WebID examples corrected. Fresh JSS pods seed `/profile/card.jsonld#me`, not `/profile/card#me` — the docs and config.js comment now reflect the canonical URI. Legacy extensionless pods (created before the .jsonld convention) continue to work via the existing fallback in onReady. 5. Now that singleUserName is normalized at the top, the inline `singleUserName === '/'` check in onReady is redundant; replace with a plain truthiness test. All 593 tests pass. * PR #349 round-3 review: in-place upgrade fallback Round-3 surfaced a concrete bad outcome from the prior approach: when an operator upgrades a pre-#348 install without moving data, the wildcard LDP routes still serve /me/* directly from disk, the old IDP account "me" still authenticates against /me/profile/card, and the new code seeds an *empty* root pod alongside it. That split-brain leaves clients reading/writing the legacy pod while the operator believes they're on the new default. Switch from "warn loudly and seed anyway" to "auto-fall-back to --single-user-name me when /me/ data exists". This is strictly better: - Fresh install (no /me/ data) → root pod, the new default. - Pre-#348 install (default 'me' was used) → keeps working exactly as before, no surprise pod, no IDP account collision. - Operator who actively wants to migrate to root → moves data/me/* → data/* themselves, then restart picks up root. Implementation: - src/server.js: detect pre-existing /me/ pod via existsSync before plugin registration (remoteStoragePlugin captures the username at registration time, so the check has to be sync). When detected, set effective singleUserName = 'me' and stash a flag for the onReady warning. - onReady logs a one-liner explaining the fallback so operators know why their pod is at /me/. - The previous round-2 "split-brain" warning becomes redundant (the auto-fallback prevents the split-brain from happening at all) and is removed. Test: - New describe `Single-user upgrade fallback — pre-existing /me/ pod (#348)`. Phase 1 seeds the legacy layout (explicit --single-user-name me + password). Phase 2 restarts on the same data dir with no name flag. Asserts: - GET /me/profile/card.jsonld is still 200 (no fresh root pod overwrites or hides it) - POST /idp/credentials with {username: me, password} still returns an access_token (existing IDP account intact) - Both phases reuse the same port so ACLs (which carry absolute URIs) keep matching. 595/595 tests pass. * PR #349: simplify — drop the upgrade auto-fallback The user feedback on round-3 was clear: the original ask was a simple default change, and "people will figure it out" was explicit license to skip migration cleverness. Round 3's auto-fallback (and round 4's tweaks to it) were scope creep. Revert the auto-fallback. Pre-#348 installs that upgrade in place without flag changes will get a fresh empty root pod alongside their existing /me/ data — operators who hit that pick one of two explicit paths on restart, both documented: 1. Add `--single-user-name me` → legacy layout, no further work. 2. Move `<root>/me/*` to `<root>/`, delete the legacy IDP account for "me", restart → new root pod, new account seeded. At v0.0.x with a small operator base, that one-time intervention is acceptable and keeps the codebase clean. Changes from round 3 / unpushed round 4: - src/server.js: drop the `existsSync(/me/...)` auto-detect block, the `migratedFromMeDefault` flag, and the corresponding warning in onReady. Drop the `existsSync` import. - test/idp.test.js: drop the `Single-user upgrade fallback` suite (no fallback to test). - test/idp.test.js: tighten the `seeds the profile at root` assertion — also fs.pathExists() check that no /me/ files were written, so a regression that left /me/ behind under a 401 wouldn't slip through (round-4 review #7). - docs/configuration.md: add an explicit "Upgrading from a pre-#348 install" callout under Single-User Mode listing both migration paths (round-4 review #11). 593/593 tests pass. * PR #349 round-5 review: fix two real points Of the 11 inline comments, 5 re-flag round-1/2/3 work already on the branch, 3 re-raise the pre-#348 upgrade incompatibility we deliberately decline (documented in the migration callout), 1 is a pre-existing printConfig accuracy issue out of scope here. The remaining two are new and worth fixing: 1. Root-pod podName was null, but src/idp/accounts.js:438-440 surfaces account.podName as the `name` claim under the OIDC `profile` scope. A null there propagates as missing/null profile.name on every login. Use 'me' for the root-pod case so the claim matches the username. 2. Add a getPodName regression test for `singleUserName: null`. The existing url.test.js coverage tests `''` and `'/'` but not the normalized null shape that most root-pod requests now reach it with after the createServer-level normalization. 594/594 tests pass. * PR #349 round-6 review: two real points Of the 10 inline comments, 8 re-flag earlier-round work or re-raise the deliberately-declined upgrade trade-off. Two new points are worth fixing: 1. test/config.test.js: pin the singleUserName: null default at the config layer. createServer() has its own root-pod tests, but a future refactor of loadConfig() could silently restore the old 'me' default and only behavioural tests would catch it. Add three focused assertions: default is null, explicit CLI arg is preserved, and JSS_SINGLE_USER_NAME env is honoured (operator's escape hatch back to /me/). 2. docs/configuration.md: the previous migration command `mv <root>/me/* <root>/` silently skips dotfiles (`.acl`, `.meta`, `.quota.json`), which would leave the migrated root pod without ACL or quota state. Replace with two explicit options that handle dotfiles correctly: rsync (default) or bash+dotglob. 597/597 tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
clientBasedCORSto returntruefor all clientsProblem
When Mashlib is loaded from CDN, the OIDC token exchange fails with:
The previous code only allowed CORS for clients with
tokenEndpointAuthMethod === 'none', but dynamically registered clients default toclient_secret_basic.Solution
Solid servers are public and should accept token requests from any web app origin, so
clientBasedCORSnow returnstruefor all clients.Test plan
Fixes #10