feat(apps/mock-edupass): local OIDC provider with form_post and PKCE

[evtpano]

Close #27

🚀 Summary

Add a local OIDC provider (apps/mock-edupass) that stands in for Edupass during development and CI testing. This lets developers work on authentication flows without real credentials.

✏️ Changes

• Scaffolded apps/mock-edupass package with Express + oidc-provider
• Implemented Authorization Code flow with PKCE (S256), form_post response mode, and client_secret_post token auth
• Added 3 hard-coded test accounts (teacher-1, teacher-2, teacher-3) with auto-login (no UI)
• Account selection via ?account=<id> query param (defaults to teacher-1)
• Added test suite covering discovery, full OIDC flow, and error cases (8 tests)
• Added README documenting quick start, client configuration, and fake accounts

🧪 Test Plan

• pnpm --filter @teacher-workspace/mock-edupass test passes (8 tests)
• Verified discovery endpoint returns correct issuer, response modes, and code challenge methods
• Verified full authorization code flow with PKCE produces a valid signed JWT
• Verified error cases: missing PKCE rejected, wrong verifier rejected, wrong secret rejected, code reuse rejected

Notes

Auto-login (no UI): Provider is dev/CI only. Auto-login keeps tests deterministic.

Sample authorization URL:

CODE_VERIFIER=$(openssl rand -base64 32 | tr -d '=' | tr '+/' '-_')
CODE_CHALLENGE=$(printf '%s' "$CODE_VERIFIER" | openssl dgst -sha256 -binary | base64 | tr -d '=' | tr '+/' '-_')

curl -v -L -b /tmp/cookies -c /tmp/cookies "http://localhost:9000/authorize?client_id=teacher-workspace&redirect_uri=http://localhost:3000/auth/callback&response_type=code&scope=openid&response_mode=form_post&code_challenge=${CODE_CHALLENGE}&code_challenge_method=S256&&account=teacher-2"

Acceptance criteria coverage:

• Discovery & JWKS advertise required capabilities — .well-known/openid-configuration returns correct issuer, response modes, and code challenge methods; JWKS endpoint serves a valid signing key.

  • discovery document has required fields
  • JWKS serves a valid public key

• Authorization response reaches callback as form post — Full auth code flow with PKCE completes and delivers the code via HTML form POST to the redirect URI.

  • completes authorization code flow with PKCE and form_post

• ID token carries fake account's claims — Token exchange returns a signed JWT with correct sub, email, name; missing claims are omitted (not null).

  • completes authorization code flow with PKCE and form_post
  • absent claim is not present in ID token (teacher-3)

• Authorization without PKCE is rejected — Requests missing code_challenge are refused.

  • rejects authorization without PKCE

• Token exchange with invalid credential is rejected — Wrong verifier, wrong client secret, and reused authorization codes all fail.

  • rejects token exchange with wrong code_verifier
  • rejects token exchange with wrong client_secret
  • rejects token exchange with reused code

[nwsgerald]

Code Review — feat/fake-edupass (2026-08-12 08:41)

File: review/feat-fake-edupass/report-20260812084115.md
Based on: full branch diff since diverging from main (71f9f1c) — 12 files, +1061/−94
PR: #74 · Closes: #27

Summary

Severity	Count
🔴 Important	3
🟡 Nit	4
🟣 Pre-existing	0

The provider itself is sound: PKCE, form_post, and the confidential client are configured against a certified library rather than hand-rolled, the ID token omits unset claims exactly as #27 requires, and the error-case tests assert specific OIDC error codes instead of just non-200s. All 8 tests pass locally and tsc --noEmit is clean. The risk is not in the OIDC logic but around it — the README documents an authorization route that returns 404, and nothing in CI runs the test suite or the type checker, so both the code and its documentation are unguarded from the moment this merges. Recommended next step: fix the README route (one line), add a CI job that runs pnpm --filter @teacher-workspace/mock-edupass test and typecheck, then merge; the nits can follow.

---

Findings

🔴 Important

1. README's only account example points at a route that returns 404

File: apps/mock-edupass/README.md · Line: 49 · Triage: To be fixed

## How It Works

There is no login page or consent screen. Authentication and consent complete automatically:

- Defaults to `teacher-1` unless `?account=<id>` is passed to the authorize endpoint
- Example: `/auth?...&account=teacher-2` logs in as John Smith   <!-- ← /auth is not a route here -->

The provider overrides the authorization route in src/provider.ts:30-32:

routes: {
  authorization: '/authorize',
},

Problem: /auth is oidc-provider's default authorization route, but this provider overrides it to /authorize. Verified against a running instance: GET /auth?client_id=... returns 404, GET /authorize?client_id=... returns 303, and the discovery document advertises authorization_endpoint: http://localhost:9000/authorize. This is the README's only example of the account parameter — the feature a developer is most likely to copy — and it fails outright. The PR description's sample curl uses /authorize correctly, so the defect is confined to the README.

Suggestion:

- Example: `/authorize?...&account=teacher-2` logs in as John Smith

---

2. The new test suite never runs in CI

File: .github/workflows/ci.yml · Line: 15 · Triage: To be fixed

jobs:
  format:      # oxfmt
  lint:        # oxlint
  go-lint:     # golangci-lint
  go-test:     # go test -race ./...
  # ← no JS/TS test job

Problem: this PR adds the repository's first JS/TS test suite (8 tests), and the PR's test plan rests entirely on it, but no CI job executes it. lefthook.yml only runs oxfmt and oxlint on staged files at pre-commit. Nothing runs pnpm test on any pull request. The provider can break — a dependency bump, a config change, a route rename — and CI stays green. That suite is the sole evidence for five of the six acceptance criteria on #27, and it is unenforced the moment it merges.

Suggestion:

  test:
    name: Test
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
      - name: Setup PNPM
        uses: pnpm/action-setup@0ebf47130e4866e96fce0953f49152a61190b271 # v6.0.9
        with:
          version: 11.11.0
      - name: Setup Node.js
        uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
        with:
          node-version: 24
          cache: pnpm
      - name: Install dependencies
        run: pnpm install --frozen-lockfile
      - name: Test
        run: pnpm --filter @teacher-workspace/mock-edupass test

Add a root "test": "pnpm -r test" script if you'd rather the job stay generic as more packages gain tests.

---

3. Missing automated test for: provider starts from a documented command

File: apps/mock-edupass/test/provider.test.ts · Line: 96 · Triage: To be fixed

before(async () => {
  const { app } = createApp(TEST_PORT);   // ← in-process; index.ts and config.ts never run
  server = app.listen(TEST_PORT);
  await new Promise<void>((resolve) => server.once('listening', resolve));
});

Problem: issue #27 lists six acceptance criteria; the PR's "Acceptance criteria coverage" section maps five and omits "Provider starts from a documented command". Every test calls createApp() directly, so src/index.ts, src/config.ts, and the MOCK_EDUPASS_PORT environment variable are never exercised. Nothing verifies that the documented start command boots a server that serves discovery — and that untested seam is exactly where finding #1 lives: the docs and the config disagree, and no test could have caught it.

Verified manually during this review: MOCK_EDUPASS_PORT=9911 node --experimental-strip-types src/index.ts starts, GET /health returns 200, and discovery returns authorization_endpoint: http://localhost:9911/authorize. The criterion holds — it just isn't guarded.

Suggestion: spawn the documented entry point and assert on discovery, so the README's URLs are covered by the same test.

import { spawn } from 'node:child_process';

it('starts from the documented command and serves discovery', async () => {
  const proc = spawn('node', ['--experimental-strip-types', 'src/index.ts'], {
    env: { ...process.env, MOCK_EDUPASS_PORT: '9877' },
  });
  try {
    await waitForHealth('http://localhost:9877/health');
    const doc = await (await fetch('http://localhost:9877/.well-known/openid-configuration')).json();
    assert.equal(doc.authorization_endpoint, 'http://localhost:9877/authorize');
  } finally {
    proc.kill();
  }
});

---

🟡 Nit

4. An unknown account id silently authenticates as teacher-1

File: apps/mock-edupass/src/interaction.ts · Line: 14 · Triage: To be fixed

if (details.prompt.name === 'login') {
  const requestedAccount = details.params.account as string | undefined;
  const account = accounts.find((a) => a.sub === requestedAccount) ?? accounts[0];   // ←

  await provider.interactionFinished(
    req,
    res,
    { login: { accountId: account.sub } },
    { mergeWithLastSubmission: false },
  );
  return;
}

Problem: ?account=teacher-99 does not error — it authenticates as teacher-1. Verified end to end: the flow returns an authorization code and the ID token carries sub=teacher-1, email=jane.doe@example.com, name=Jane Doe. The ?? fallback is correct for an absent parameter but wrong for an invalid one. In a downstream relying-party test, a typo'd account id produces a passing flow with the wrong claims, surfacing later as a confusing assertion mismatch — if at all. This matters more than usual here, since deterministic account selection is the package's entire purpose.

Suggestion:

const requestedAccount = details.params.account as string | undefined;
const account = requestedAccount
  ? accounts.find((a) => a.sub === requestedAccount)
  : accounts[0];

if (!account) {
  throw new Error(
    `Unknown account '${requestedAccount}'. Available: ${accounts.map((a) => a.sub).join(', ')}`,
  );
}

Express 5 forwards a rejected async handler to the error handler, so this surfaces as a 500 carrying the message rather than a silent wrong login.

---

5. An unhandled prompt name sends no response and logs nothing

File: apps/mock-edupass/src/interaction.ts · Line: 38 · Triage: To be fixed

router.get('/interaction/:uid', async (req, res) => {
  const details = await provider.interactionDetails(req, res);

  if (details.prompt.name === 'login') { /* ... */ return; }

  if (details.prompt.name === 'consent') {
    /* ... */
    await provider.interactionFinished(req, res, { consent: { grantId } }, { mergeWithLastSubmission: true });
  }
  // ← falls through: no response written, nothing logged
});

Problem: if prompt.name is neither value, the handler returns without touching res, and the request hangs until the client or server.requestTimeout gives up — with nothing logged to explain it. Checked against oidc-provider@9.11.1: its default interaction policy defines exactly two prompts (lib/helpers/interaction_policy/prompts/{login,consent}.js), so this is unreachable today. It becomes reachable as soon as someone sets a custom interactions.policy or a major upgrade introduces a prompt.

Suggestion:

    await provider.interactionFinished(req, res, { consent: { grantId } }, { mergeWithLastSubmission: true });
    return;
  }

  throw new Error(`Unhandled interaction prompt: ${details.prompt.name}`);
});

---

6. Hardcoded test port turns a port collision into an infinite hang

File: apps/mock-edupass/test/provider.test.ts · Line: 13 · Triage: To be fixed

const TEST_PORT = 9876;                                                     // ←
// ...
before(async () => {
  const { app } = createApp(TEST_PORT);
  server = app.listen(TEST_PORT);
  await new Promise<void>((resolve) => server.once('listening', resolve));  // ← never resolves
});

Problem: verified by occupying port 9876 and running the suite — 'listening' never fires, and since node:test applies no default timeout, pnpm test hangs indefinitely. Forcing --test-timeout=20000 reports 'test did not finish before its parent and was cancelled', which never mentions EADDRINUSE. On a developer machine already running the provider, or on a shared CI runner, this is a long unexplained stall instead of a one-line error.

The root cause is in src/app.ts:6-7 — createApp(port) derives the issuer from a port it does not own, so the port must be known before listen() and an ephemeral port is impossible:

export function createApp(port: number) {
  const provider = createProvider(port);   // issuer = http://localhost:${port}

Suggestion — Option A (three lines, keeps the port hardcoded, only improves the error):

await new Promise<void>((resolve, reject) => {
  server.once('listening', resolve);
  server.once('error', reject);   // ← EADDRINUSE surfaces immediately
});

Suggestion — Option B (removes the coupling, immune to collisions):

export function createApp(issuer: string) { /* ... */ }

// index.ts
createApp(`http://localhost:${config.port}`);

// test
const server = app.listen(0);
const { port } = server.address() as AddressInfo;
const BASE_URL = `http://localhost:${port}`;

B costs a two-line signature change across the only two callers and makes the suite collision-proof; A only improves the message.

---

7. tsconfig.json and the typescript dependency are never executed

File: apps/mock-edupass/package.json · Line: 6 · Triage: To be fixed

"scripts": {
  "start": "node --experimental-strip-types src/index.ts",
  "test": "node --experimental-strip-types --test test/**/*.test.ts"
},                                          // ← no typecheck script
"devDependencies": {
  "typescript": "^6.0.3"                    // ← never invoked
}

Problem: the package ships a tsconfig.json with strict, noUnusedLocals, and noUnusedParameters, but both scripts run under --experimental-strip-types, which strips type annotations without checking them. No typecheck script exists and no CI job runs tsc. pnpm exec tsc --noEmit passes cleanly today, so there is nothing to fix in the code — the gap is that those strictness settings guard nothing going forward.

Suggestion:

"scripts": {
  "start": "node --experimental-strip-types src/index.ts",
  "test": "node --experimental-strip-types --test test/**/*.test.ts",
  "typecheck": "tsc --noEmit"
}

Wire it into the CI job from finding #2.

---

Reviewer To-Do

• Manually test: provider starts from the documented command and serves discovery on the configured port — performed during this review, passes
• Manually test: no authorization response parameter appears anywhere in a URL — the form_post test asserts the form action and body but never asserts the absence of code/state from a redirect URL

---

What Looks Good

• findAccount builds claims conditionally rather than emitting nulls (src/provider.ts:53-58) — satisfies feat(apps/mock-edupass): local OIDC provider with form_post and PKCE #27's "absent, not null or empty" criterion exactly, and the test asserts it with Object.hasOwn rather than a truthiness check that an empty string would pass
• No body parser mounted ahead of provider.callback() (src/app.ts:12-17) — the issue called this constraint out explicitly and it is honored; /health is registered with app.get so it cannot intercept the provider's POST routes
• Error cases assert specific OIDC error codes (test/provider.test.ts:245-278) — wrong verifier, wrong client secret, and code reuse each check for invalid_grant / invalid_client rather than a bare non-200, which is what makes the PKCE and confidential-client configuration trustworthy
• PKCE enforced through configuration, not hand-rolled validation (src/provider.ts:26-28) — required: () => true delegates to the certified library instead of re-implementing the check

[nwsgerald]

the above is slop produced by review skill take it with a handful of salt

[evtpano]

Addressed Review Comments

Review Point 1 — README's authorize route example (Important)
Fixed /auth?...&account=teacher-2 → /authorize?...&account=teacher-2 in README.md.

Review Point 4 — Unknown account silently authenticates as teacher-1 (Nit)
Invalid account ids now throw an error listing available accounts instead of falling back silently. Verified end-to-end: ?account=teacher-99 returns a 500 with Unknown account 'teacher-99'. Available: teacher-1, teacher-2, teacher-3.

Review Point 5 — Unhandled prompt name sends no response (Nit)
Added explicit return after consent handling and a throw for unrecognized prompt names, so the request fails loudly instead of hanging.

Review Point 6 — Hardcoded test port causes infinite hang (Nit)
Added server.once('error', reject) to the test setup so a port collision surfaces immediately as an EADDRINUSE error instead of hanging indefinitely.

Review Point 7 — typecheck script missing (Nit)
Added "typecheck": "tsc --noEmit" to package.json scripts.

Not addressed (for now)

Review Point 2 — CI test job
Deferred. Currently we only have a single ci.yml and need to discuss the monorepo CI strategy first (running jobs only for packages with changes). TBD

Not addressing

Review Point 3 — Test for documented start command
The OIDC logic is covered by the existing in-process tests. The doc/code mismatch this finding would guard against is fixed directly by Review Point 1

[nwsgerald]

lgtm

[evtpano]

Rebased and downgraded existing typescript from "^6.0.3" to "^5.9.3" with reference to #91

[evtpano]

With reference to Slack message, ok to merge since this will likely remain a small mock app. Merged accounts.ts into provider.ts and interaction.ts into app.ts.

Kept index.ts separate from app.ts because the test imports createApp from app.ts. Merging them would require a main-module guard to prevent the server auto-starting on import during test runs.