Friendly /idp landing + /idp/auth no-client_id guard (#286, PR-A) by melvincarvalho · Pull Request #287 · JavaScriptSolidServer/JavaScriptSolidServer

melvincarvalho · 2026-04-22T02:17:12Z

First half of #286 — the cheap, no-OIDC-risk parts. Standalone `/idp/login` and auto-login on register are deferred to PR-B (they share the `oidc-provider` session-minting unknown).

What's new

`GET /idp`: friendly landing card with gradient header (matches the register page from IDP register UX polish — WebID preview, relaxed username, auto-login #284), single "Create Account" CTA, and a sign-in note explaining that login goes through the Solid app you want to use. Issuer URL displayed in the footer.
`/idp/auth` no-`client_id` guard: a human opening `/idp/auth` directly used to get a raw `invalid_request` OIDC error. GET requests with no `client_id` now `302 → /idp`. Real OIDC requests (with `client_id`) pass through to `oidc-provider` unchanged.
Register page "Sign In" link: previously pointed at `/idp/auth` (the broken-without-client_id endpoint), now points at `/idp`. The link text reads "Back to home" outside an interaction; "Sign In" inside one (where the link actually leads somewhere useful).
Auth bypass: `server.js` already let `/idp/*` through unauthenticated — extended to cover the trailing-slash-less `/idp` and `/idp?…` so the landing is reachable without auth.

Files

`src/idp/views.js` — new `landingPage()` view, register Sign-In link repointed.
`src/idp/index.js` — `GET /idp` route, `/idp/auth` pulled out of catch-all into a guarded handler.
`src/server.js` — auth-bypass list extended for `/idp`.
`test/idp.test.js` — three new cases covering the landing render, the redirect, and the pass-through.

Test plan

Full suite: 394/394 pass (was 391).
Smoke against a live `--idp` server:
- `GET /idp` → 200 `text/html` containing "Solid Pod Server" + "Create Account" + `href="/idp/register"`.
- `GET /idp/auth` (no client_id) → `302 Location: /idp`.
- `GET /idp/auth?client_id=test&…` → 400 from oidc-provider, not redirected.

What's still open under #286

PR-B will add:

`GET /idp/login` (standalone form, no client_id required)
`POST /idp/login` (authenticate + set OIDC session cookie)
Auto-login on register (folds in once session-minting is solved)
Re-link this landing page's sign-in note to a real "Sign In" button

…Server#286, PR-A) A human visitor opening /idp, /idp/auth, or pasting an old register-page "Sign In" link used to land on a raw OIDC error (invalid_request / missing client_id). This adds the cheap, no-OIDC-risk half of JavaScriptSolidServer#286: - New landingPage() view in src/idp/views.js: gradient header matching the register page (JavaScriptSolidServer#284), Create Account button, sign-in note, issuer footer. Single primary CTA — standalone /idp/login is intentionally PR-B (needs oidc-provider session-minting). - GET /idp registered in src/idp/index.js. Auth bypass extended in server.js to cover /idp (and /idp?…) so the landing page is publicly accessible. - /idp/auth pulled out of the oidc-provider catch-all loop into its own guarded route: GET requests with no client_id now 302 to /idp; OIDC requests pass through unchanged. - Register page's "Already have an account?" link now points at /idp rather than /idp/auth (broken without client_id) — the link reads "Back to home" without an active interaction, "Sign In" with one. - Tests cover landing render, redirect-on-bare-/idp/auth, and that real OIDC requests (with client_id) still reach the provider. Closes the dead-link wart from JavaScriptSolidServer#285. Standalone /idp/login + auto-login remain tracked under JavaScriptSolidServer#286 as PR-B.

Copilot

Pull request overview

Adds a human-friendly entry point for the built-in IdP and prevents raw OIDC errors when a user manually visits /idp/auth without an OIDC client context.

Changes:

Introduces GET /idp landing page HTML and updates the register page “Sign In” link to point to /idp when not in an interaction.
Adds a /idp/auth guard to redirect GET requests missing client_id to /idp, while forwarding real OIDC requests unchanged.
Extends the server auth-bypass list to allow unauthenticated access to /idp (including querystring variants) and adds regression tests.

Reviewed changes

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

File	Description
`src/idp/views.js`	Adds `landingPage()` HTML and repoints the register page link to `/idp` outside interactions.
`src/idp/index.js`	Registers `GET /idp` and replaces the `/idp/auth` catch-all with a guarded handler.
`src/server.js`	Allows unauthenticated access to `/idp` and `/idp?...` in the auth bypass hook.
`test/idp.test.js`	Adds regression tests for landing render and `/idp/auth` redirect/pass-through behavior.

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

Copilot · 2026-04-22T02:19:19Z

 * Registration page HTML
 */
+/**


There’s now a misleading JSDoc header: the /** Registration page HTML */ block sits immediately above landingPage() instead of above registerPage(). This makes the docs inaccurate and harder to navigate; move/update the comment so it describes the function that follows (or relocate it to just before registerPage).

Suggested change

* Registration page HTML

*/

/**

Copilot · 2026-04-22T02:19:20Z

+    method: ['GET', 'POST', 'DELETE', 'OPTIONS'],
+    url: '/idp/auth',
+    handler: async (request, reply) => {
+      if (request.method === 'GET' && !request.query?.client_id) {


The /idp/auth guard currently triggers when client_id is present but falsy (e.g. ?client_id=), which changes behavior for malformed-but-real OIDC requests by redirecting to /idp instead of letting oidc-provider return the spec error. If the intent is strictly “missing client_id”, check specifically for request.query.client_id === undefined (or use Object.prototype.hasOwnProperty.call(request.query, 'client_id')) rather than a falsy check.

Suggested change

if (request.method === 'GET' && !request.query?.client_id) {

if (request.method === 'GET' && request.query?.client_id === undefined) {

1. Move the orphaned /** Registration page HTML */ docblock back above registerPage where it belongs — it had been pushed off its function when the new landingPage was inserted ahead of it. 2. /idp/auth no-client_id guard now triggers only when the parameter is truly absent (=== undefined), not when it's present-but-empty. An explicit ?client_id= is a malformed OIDC request and oidc-provider should be the one to surface the spec error, not us silently redirecting to the friendly landing.

Copilot

Pull request overview

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

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

Copilot · 2026-04-22T02:27:49Z


    <p style="text-align: center; margin-top: 24px; color: #666; font-size: 14px;">
-      Already have an account? <a href="${uid ? `/idp/interaction/${uid}` : '/idp/auth'}" style="color: #0066cc;">Sign In</a>
+      Already have an account? <a href="${uid ? `/idp/interaction/${uid}` : '/idp'}" style="color: #0066cc;">${uid ? 'Sign In' : 'Back to home'}</a>


In the non-interaction register footer (uid is null), the text becomes "Already have an account? Back to home", which is self-contradictory. Consider rendering different surrounding copy when uid is null (e.g., just a "Back to home" link without the "Already have an account?" prefix), or keep the prefix and use link text that reflects the destination (/idp landing).

Suggested change

Already have an account? <a href="${uid ? `/idp/interaction/${uid}` : '/idp'}" style="color: #0066cc;">${uid ? 'Sign In' : 'Back to home'}</a>

${uid

? `Already have an account? <a href="/idp/interaction/${uid}" style="color: #0066cc;">Sign In</a>`

: `<a href="/idp" style="color: #0066cc;">Back to home</a>`}

Copilot · 2026-04-22T02:27:49Z

+      // surface its own error) — only that we did not 302 to /idp.
+      const res = await fetch(
+        `${baseUrl}/idp/auth?client_id=test&redirect_uri=http://localhost&response_type=code&scope=openid`,
+        { redirect: 'manual' }
+      );
+      assert.notStrictEqual(res.headers.get('location'), '/idp');


This assertion can pass even if the guard mistakenly redirects to the landing with additional query params (e.g., /idp?…) or if the response is an unrelated failure that omits Location. To make this regression test meaningful, assert on the status (e.g., not a redirect / specifically 400) and/or assert that Location is absent rather than only !== '/idp'.

Suggested change

// surface its own error) — only that we did not 302 to /idp.

const res = await fetch(

`${baseUrl}/idp/auth?client_id=test&redirect_uri=http://localhost&response_type=code&scope=openid`,

{ redirect: 'manual' }

);

assert.notStrictEqual(res.headers.get('location'), '/idp');

// surface its own error) — only that we were not redirected to the

// /idp landing page by the guard.

const res = await fetch(

`${baseUrl}/idp/auth?client_id=test&redirect_uri=http://localhost&response_type=code&scope=openid`,

{ redirect: 'manual' }

);

assert.ok(res.status < 300 || res.status >= 400, `expected non-redirect response, got ${res.status}`);

assert.strictEqual(res.headers.get('location'), null);

1. Register footer text is no longer self-contradictory. The page used to render "Already have an account? Back to home" outside an interaction; now it just renders "Back to home" (with the prefix only when uid is present and the link actually leads to Sign In). 2. Strengthen the /idp/auth-with-client_id test. The previous assertion only checked that Location !== '/idp'; that passed even when oidc-provider issued an unrelated redirect or no Location at all. Now we assert non-redirect status (< 300 || >= 400) AND absence of Location header — that's the precise contract. Also add a regression case for the tightened guard: `?client_id=` (empty string) must not be redirected to /idp.

Copilot

Pull request overview

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

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

Copilot · 2026-04-22T02:32:53Z

+      // Only catch the truly-bare case (no `client_id` param at all). An
+      // explicit empty string is a malformed OIDC request — let
+      // oidc-provider surface the spec error instead of redirecting.
+      if (request.method === 'GET' && request.query?.client_id === undefined) {


The /idp/auth guard only triggers for GET. If Fastify exposes a HEAD route for GET (default behavior), a HEAD /idp/auth request without client_id would still be forwarded to oidc-provider and return the raw invalid_request instead of the friendly redirect. Consider treating HEAD the same as GET for the bare-client_id check (or normalizing by checking the route method rather than request.method).

Suggested change

if (request.method === 'GET' && request.query?.client_id === undefined) {

if (

(request.method === 'GET' || request.method === 'HEAD') &&

request.query?.client_id === undefined

) {

Copilot · 2026-04-22T02:32:53Z

+      // what oidc-provider does with this fake client_id (it'll likely
+      // surface its own error) — only that the guard did not redirect us.
+      const res = await fetch(
+        `${baseUrl}/idp/auth?client_id=test&redirect_uri=http://localhost&response_type=code&scope=openid`,
+        { redirect: 'manual' }
+      );
+      // Must NOT be a redirect (the guard's only effect would be a 302).
+      assert.ok(res.status < 300 || res.status >= 400,
+        `expected non-redirect response, got ${res.status}`);
+      // And must not have a Location header pointing anywhere.
+      assert.strictEqual(res.headers.get('location'), null);


This test asserts the oidc-provider response is never a redirect and has no Location header. oidc-provider can legitimately return 3xx redirects (e.g., to /idp/interaction/:uid) for some client_id/parameter combinations, so this makes the test brittle and stronger than needed to validate the guard. Instead, assert specifically that the response is not the guard redirect (e.g., Location !== '/idp' and/or not a 302 to /idp), and allow other 3xx responses to pass.

Suggested change

// what oidc-provider does with this fake client_id (it'll likely

// surface its own error) — only that the guard did not redirect us.

const res = await fetch(

`${baseUrl}/idp/auth?client_id=test&redirect_uri=http://localhost&response_type=code&scope=openid`,

{ redirect: 'manual' }

);

// Must NOT be a redirect (the guard's only effect would be a 302).

assert.ok(res.status < 300 || res.status >= 400,

`expected non-redirect response, got ${res.status}`);

// And must not have a Location header pointing anywhere.

assert.strictEqual(res.headers.get('location'), null);

// what oidc-provider does with this fake client_id (it may error or

// legitimately redirect) — only that the guard did not redirect us

// back to /idp.

const res = await fetch(

`${baseUrl}/idp/auth?client_id=test&redirect_uri=http://localhost&response_type=code&scope=openid`,

{ redirect: 'manual' }

);

const location = res.headers.get('location');

assert.ok(

!(res.status === 302 && location === '/idp'),

`expected request to bypass the /idp guard, got ${res.status} redirect to ${location}`

);

assert.notStrictEqual(location, '/idp');

1. /idp/auth guard now also catches HEAD (Fastify auto-generates a HEAD handler from each GET route). Without this, HEAD /idp/auth without client_id would skip the guard and surface the raw OIDC error. New test covers the HEAD path. 2. Loosen the previous test's assertion. Last round I tightened it to "no redirect at all + no Location header", but oidc-provider can legitimately 302 to /idp/interaction/:uid for valid OIDC inputs. Test the precise contract instead — that the response is not specifically a 302→/idp from our guard — and let other 3xx flows pass through.

Copilot

Pull request overview

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

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

Copilot · 2026-04-22T02:38:41Z

+  fastify.route({
+    method: ['GET', 'POST', 'DELETE', 'OPTIONS'],
+    url: '/idp/auth',
+    handler: async (request, reply) => {
+      // Only catch the truly-bare case (no `client_id` param at all). An
+      // explicit empty string is a malformed OIDC request — let
+      // oidc-provider surface the spec error instead of redirecting.
+      // HEAD is included so the friendly redirect also covers tools (and
+      // Fastify's auto-HEAD-from-GET) that probe with HEAD.
+      if (
+        (request.method === 'GET' || request.method === 'HEAD') &&
+        request.query?.client_id === undefined


The /idp/auth route handler has logic and comments referencing HEAD requests, but HEAD is not explicitly included in the route's method list (it currently relies on Fastify's auto-HEAD-from-GET behavior). To make the contract explicit and avoid future regressions if Fastify config changes (e.g., exposeHeadRoutes), add 'HEAD' to the method array (or remove the HEAD-specific logic/comment if you intend to rely on auto-HEAD).

Add HEAD to the /idp/auth route's explicit method array, so the contract no longer depends on Fastify's auto-HEAD-from-GET behaviour (which exposeHeadRoutes can disable). The handler already treats HEAD the same as GET for the bare-client_id redirect; this just makes the route registration match the handler logic.

Copilot

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 requested a review from Copilot April 22, 2026 02:17

Copilot started reviewing on behalf of melvincarvalho April 22, 2026 02:17 View session