Skip to content

fix(idp): allow CORS for all clients on token endpoint#11

Open
melvincarvalho wants to merge 3 commits into
gh-pagesfrom
fix/oidc-cors-all-clients
Open

fix(idp): allow CORS for all clients on token endpoint#11
melvincarvalho wants to merge 3 commits into
gh-pagesfrom
fix/oidc-cors-all-clients

Conversation

@melvincarvalho
Copy link
Copy Markdown
Contributor

Summary

  • Simplify clientBasedCORS to return true for all clients
  • Fixes OIDC login for web apps loaded from CDN (like Mashlib from unpkg.com)

Problem

When Mashlib is loaded from CDN, the OIDC token exchange fails with:

400 Bad Request - origin not allowed for client

The previous code only allowed CORS for clients with tokenEndpointAuthMethod === 'none', but dynamically registered clients default to client_secret_basic.

Solution

Solid servers are public and should accept token requests from any web app origin, so clientBasedCORS now returns true for all clients.

Test plan

  • Clear browser cache/cookies for test server
  • Navigate to a resource with Mashlib enabled
  • Click "Sign In" and complete OIDC login
  • Verify token exchange succeeds (no 400 error)
  • Verify authenticated user can access protected resources

Fixes #10

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

OIDC token endpoint returns 400 'origin not allowed for client' for CDN-loaded apps

1 participant