Implement authentication with Okta SSO

[cass-clearly]

What changed

Implemented enterprise authentication support with Okta authorization-code SSO, session cookies, configurable timeout, trusted-auth-header mode, and operator docs.

Why

Closes #275. Block Security requires no anonymous access, Okta SSO, and expiring sessions before internal integration.

New/Changed Endpoints

Method	Endpoint	Description
GET	/login	Starts Okta SSO when auth is enabled.
GET	/auth/callback	Completes Okta callback and issues remarq_session.

How to verify

• npm run check
• Configure REMARQ_AUTH_REQUIRED=true plus OKTA_* vars and verify anonymous API requests return 401.

Manual testing checklist

• Server starts without errors (npm run start)
• Existing tests pass (npm test)
• Tested in browser (annotations, sidebar, highlights work)
• Tested API changes with curl (include example commands above)
• No console errors in browser DevTools

[cass-clearly]

The Marketing Guru — Round 1 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_marketing-guru_r1 npm test.

I do not recommend a totally different product path. Enterprise SSO is the right path for this requirement. The implementation is not ready because the public-facing docs are incomplete and the PR skips required release communication.

Required fixes:

1. Add a CHANGELOG.md entry under [Unreleased] for Okta SSO/auth-required mode. This is a user-visible enterprise security feature. Shipping it without the changelog violates the Council docs requirement.

2. Update /openapi.json for the new public endpoints or stop claiming complete spec coverage. The README says the OpenAPI spec is the machine-readable source and that every endpoint appears from the spec. /login and /auth/callback are now public endpoints, but server/openapi.js does not document them. That makes the docs lie to developers and agents.

3. Make docs/api.md complete for authentication. The new entries only say what the endpoints do. This file promises request/response schemas, error codes, and curl examples. Add status codes for 204, 302, 400, and relevant auth failures; document query params for /auth/callback; show copy-pasteable curl examples; mention the remarq_session cookie behavior.

4. Fix the operator guide to cover the actual happy path. docs/okta-auth.md needs the Okta app setup assumptions a deployer needs: redirect URI must match Okta exactly, authorization-code flow must be enabled, required scopes are openid profile email, and REMARQ_POST_LOGIN_REDIRECT exists if operators need a non-root redirect. Right now an operator has to infer too much.

5. Remove Closes #275 from docs/okta-auth.md. Issue-closing metadata belongs in the PR, not in operator documentation read by customers.

[cass-clearly]

The Architect — Round 1 Review

Verdict: REQUEST_CHANGES

Passed locally with exact command: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_architect_r1 npm test.

I do not recommend throwing this away, but the skipped plan review shows. The implementation treats Express HTTP auth as whole-service auth. It is not. Fix the coherence holes before shipping.

Required changes:

1. Authenticate WebSocket upgrades. server.on("upgrade") sends /ws straight to wss.handleUpgrade() and never runs the auth middleware. With REMARQ_AUTH_REQUIRED=true, an anonymous client can still connect to /ws and subscribe to document events. That violates the core requirement: no anonymous access. Extract reusable auth/session checking so HTTP middleware and WebSocket upgrade use the same decision, and add tests for rejected anonymous /ws plus accepted authenticated/trusted-header /ws.

2. Add integration tests for the actual auth behavior. The current tests only cover helper functions. They do not prove anonymous API/UI requests are denied, /login sets state and redirects, invalid callback state is rejected, a callback-created session authorizes a protected endpoint, or sessions expire through the middleware. For a P0 security change, helper-only tests are not enough.

3. Make auth configuration fail closed and obvious. REMARQ_AUTH_REQUIRED=true without a complete Okta config or REMARQ_TRUSTED_AUTH_HEADER currently produces an invalid Okta authorize URL instead of a clear startup/config failure. That is a deployment footgun. Validate config once and fail with an actionable error, or return a deliberate server error from /login with tests.

4. Keep the API surface coherent. README/docs add /login and /auth/callback, but server/openapi.js omits them while the README says /openapi.json is the machine-readable API surface. Add the auth endpoints and relevant 401/auth semantics to OpenAPI, or remove the claim that every endpoint is represented there.

No totally different path is required. A reverse-proxy-only Okta approach would have been the simpler plan for internal deployment, but this PR can ship if it actually gates every access path and documents/tests the contract it introduces.

[cass-clearly]

The Craftsperson — Round 1 Review

Verdict: REQUEST_CHANGES

Tests: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_craftsperson_r1 npm test passed.

I do recommend a different path for the Okta half of this implementation: do not ship a mostly untested, hand-rolled OIDC flow. Either keep this PR to the reverse-proxy/trusted-header integration, or put the Okta code behind a small auth-client port implemented with a maintained OIDC library and behavior-tested at the HTTP boundary.

Required fixes:

1. The tests are helper tests, not feature tests. server/test.mjs:107-150 never proves the acceptance criteria: anonymous API/UI access is denied, /login sets state and redirects, /auth/callback rejects bad state, callback success sets a session cookie, that cookie authenticates a protected API request, trusted-header mode works, and expired sessions are rejected through the middleware. Add integration tests around the Express app with REMARQ_AUTH_REQUIRED=true. Current overall coverage hides that server/auth.js is only ~62% covered.

2. Refactor server/auth.js for testable seams before adding those tests. registerAuthRoutes hard-codes crypto, time, and exchangeOktaCode (server/auth.js:113-140), which pushes tests toward brittle global-fetch/magic-cookie setups. Inject the code exchanger and clock/session generation, or isolate the Okta client behind a small function that can be substituted in route tests. This is the missing refactor step.

3. Authentication is not applied consistently to the server boundary. The Express middleware is installed at server/index.js:31-33, but WebSocket upgrades at server/index.js:662-667 bypass Express entirely. If auth is required, /ws must reject unauthenticated upgrades or validate the same session/trusted-header path. Add a failing test for this first.

4. Clean up the auth surface while you are there: parseCookies (server/auth.js:17-27) mishandles malformed cookie pairs and can throw on bad percent encoding; req.path.startsWith("/serve/") (server/auth.js:94) is an undocumented bypass that does not match the current static mount. Use a battle-tested cookie parser or make this parser total and tested, and remove or document/test every public-path exception.

This has not completed red-green-refactor. The shape is still "make it work" code with tests added around the easiest pure helpers, not tests that drove the design of the auth behavior.

[cass-clearly]

The Minimalist — Round 1 Review

Verdict: REQUEST_CHANGES

Test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_minimalist_r1 npm test

I recommend a different path.

This skipped plan review and went straight to 150 lines of hand-rolled OAuth/session code. For an internal enterprise deployment, the simpler design is: let Okta/SSO live at the edge (Okta Access Gateway, oauth2-proxy, corporate reverse proxy, etc.) and make Remarq enforce one trusted identity header. That deletes /login, /auth/callback, token exchange, state cookies, access-token handling, and in-process session storage from this app.

Blocking issues:

1. server/auth.js:53-139 is unjustified complexity for the current requirement. Remarq does not need to be an OAuth client if the deployment already has enterprise SSO infrastructure. Keep the app boundary simple: auth required + trusted identity header + docs for the proxy. If direct Okta OAuth is truly non-negotiable, that needed plan review before implementation.

2. server/index.js:662-667 leaves WebSocket upgrade unauthenticated. With REMARQ_AUTH_REQUIRED=true, an anonymous client can still open /ws and reach app logic. I verified this manually: the socket was accepted and returned the app-level Document not found response, not an auth rejection. The requirement says no anonymous access entirely; this misses a live access path.

3. server/auth.js:4 leaves /openapi.json public while the issue says anonymous API access must be rejected by default. If this exception is required, justify it in the spec. Otherwise delete it.

4. server/test.mjs only unit-tests helpers. It does not prove the actual server denies anonymous HTTP requests when auth is enabled, allows authenticated/trusted requests, rejects anonymous WebSockets, or expires real sessions through the mounted middleware. That is a smell caused by building too much behind helpers instead of testing the simplest behavior at the boundary.

Fix: delete the direct Okta flow unless there is a concrete reviewed requirement for Remarq to own OAuth. Implement the smaller edge-auth path and apply it consistently to HTTP and WebSocket upgrades. If direct OAuth stays, come back with a plan-reviewed design and integration tests covering the real mounted routes and /ws.

[cass-clearly]

The Steward — Round 1 Review

Verdict: REQUEST_CHANGES

Test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_steward_r1 npm test

I fetched/read the diff with gh pr diff 284 and git diff origin/main...HEAD; they match.

I do not recommend a totally different path. App-level Okta plus trusted-header mode is a viable path. This implementation is not production-ready.

Required fixes:

1. WebSocket auth is bypassed. server/index.js authenticates Express routes, but /ws is handled in the raw upgrade listener and never runs createAuthMiddleware. With REMARQ_AUTH_REQUIRED=true, an anonymous client can still connect to /ws, subscribe to any known document id, and receive comment events. That violates the stated “disallow anonymous access” requirement and leaks data. Enforce the same session/trusted-header checks before wss.handleUpgrade, reject unauthenticated upgrades with 401/close, and add tests for unauthenticated and authenticated/trusted-header WebSocket connections.

2. Protected requests are parsed before auth. express.json() runs before auth middleware, so unauthenticated POSTs still hit body parsing and malformed JSON can return parser errors instead of 401 Authentication required. Put auth in front of body parsing for protected routes, or otherwise ensure unauthenticated API/UI requests are rejected before request-body work. Add a regression test.

3. Okta network calls have no timeout. exchangeOktaCode() calls Okta token and userinfo endpoints with plain fetch. A slow or wedged Okta call can hang /auth/callback indefinitely. Add bounded timeouts/abort handling for both outbound calls and return/log enough context for operators to identify token vs userinfo failure.

4. The machine-readable API contract is stale. New public endpoints /login and /auth/callback are in README/docs but not in server/openapi.js; auth-required responses/security are also absent from the spec. This violates API stability/consumer expectations because the CLI and tooling are spec-driven. Update OpenAPI or explicitly decide these endpoints are not part of the public contract and remove the contradictory docs.

[cass-clearly]

The Marketing Guru — Round 2 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_marketing-guru_r2 npm test.

I fetched and read the current PR diff. I do not recommend a totally different path from the Marketing Guru lens; enterprise auth is still the right story. But the public docs still make promises the implementation does not keep.

Required fixes:

1. Reconcile /openapi.json public-access documentation with reality. docs/okta-auth.md says /openapi.json remains public for operations and discovery, but server/auth.js only exempts /health, /login, and /auth/callback. Under REMARQ_AUTH_REQUIRED=true, the OpenAPI spec is protected. Either make /openapi.json public or remove that claim. Do not make operators discover this by getting a 401 from the documented discovery endpoint.

2. Fix the README's OpenAPI/CLI promise. The README says every endpoint in the spec appears as a CLI command automatically, but this PR adds /login and /auth/callback to server/openapi.js without x-cli metadata. If auth endpoints should not become CLI commands, the README needs to say CLI-supported operations use x-cli, not that every endpoint becomes a command.

3. Document trusted-header mode accurately. GET /login returns 404 when auth is required via REMARQ_TRUSTED_AUTH_HEADER without Okta config, but docs/api.md, docs/okta-auth.md, and server/openapi.js only describe disabled/Okta/config-error behavior. The guide also says /login redirects to Okta whenever authentication is enabled, which is false in reverse-proxy mode. Add the 404/status behavior and state that /login is only for direct Okta mode.

These are documentation contract issues, not copy polish. The right thing must be easy for operators; right now the guide sends them to endpoints whose behavior differs from the docs.

[cass-clearly]

The Craftsperson — Round 2 Review

Verdict: REQUEST_CHANGES

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_craftsperson_r2 npm test.

I fetched/read the current PR diff and changed files (CHANGELOG.md, README.md, docs/api.md, docs/okta-auth.md, server/auth.js, server/index.js, server/openapi.js, server/package.json, server/test.mjs).

Round 2 fixed the obvious boundary gaps: auth now runs before JSON parsing and /ws uses the same auth decision. I do not think a wholesale proxy-only rewrite is the only acceptable path anymore. But if Remarq is going to own the direct Okta/session flow, the tests still need to prove that flow at the boundary.

Blocking issues:

1. The Okta callback success path is still untested. server/test.mjs tests redirect-to-Okta and bad state, but never proves that /auth/callback with a valid state and stubbed exchanger sets remarq_session, clears state, redirects, and that the issued cookie authenticates a protected API request. That is the core behavior introduced by server/auth.js:178-195. Add a boundary test around registerAuthRoutes + createAuthMiddleware (or an app factory) with an injected codeExchanger; avoid global network/fetch mocking.

2. Session expiry is only tested as a helper with a 2ms sleep (server/test.mjs:143-148). That does not prove expired cookies are rejected through the mounted middleware, and the sleep is a brittle test smell. Refactor createSessionStore to accept a now function (and ideally an id generator) so expiry can be tested deterministically, then add the middleware-level regression: valid session succeeds, same cookie after time advances returns 401.

3. The hand-rolled Okta client remains largely uncovered. exchangeOktaCode (server/auth.js:100-132) now has an injectable fetchFn, which is the right seam, but there are no tests for token request shape, userinfo request shape, token failure, userinfo failure, or timeout propagation. If the direct Okta path stays in this PR, put these behavior tests around the auth-client seam. Otherwise, delete the direct Okta code and ship the smaller trusted-header path.

This is still a red-green-refactor issue, not a request for more coverage percentage. The tests cover the easiest helpers and two boundary failures; they still do not document the successful session design that future refactors must preserve.

[cass-clearly]

The Minimalist — Round 2 Review

Verdict: REQUEST_CHANGES

Exact test command passed on retry: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_minimalist_r2 npm test.

I fetched/read the current PR diff and changed files with gh pr diff https://github.com/cass-clearly/remarq/pull/284 --name-only, gh pr diff https://github.com/cass-clearly/remarq/pull/284 --patch, and the changed files in this worktree.

I still think the simpler long-term shape is edge Okta + one trusted identity header. But given the issue explicitly asks for a functional Okta SSO flow, I am not blocking on a total rewrite in Round 2. If Remarq owns OAuth/session state, the code and tests have to stay boring and reliable.

Blocking issues:

1. The new session expiry test is flaky. My first run of the exact full-suite command failed in server/test.mjs at expires sessions: the 1ms session expired before the immediate positive assertion. The second run passed, which proves the test is timing-dependent, not trustworthy. Do the smallest fix: stop using a 1ms TTL for the positive assertion. Use a sane margin or otherwise make this deterministic without adding a clock framework.

2. The app-owned Okta/session path is still not proven at the boundary. Round 1 asked for direct-flow coverage if direct OAuth stayed. The added integration tests cover login redirect, bad callback state, trusted-header HTTP, and trusted-header WebSocket, but not the actual successful /auth/callback path setting remarq_session, that cookie authorizing a protected HTTP request, or expiry rejecting through the mounted middleware. That leaves the most complex code in the PR mostly trusted by helper tests. Either delete the direct Okta/session flow and keep trusted-header mode, or add the missing boundary tests for the session cookie path.

3. docs/okta-auth.md still says /openapi.json remains public, but Round 2 removed /openapi.json from PUBLIC_PATHS. Delete that claim or make the code match it. For the stated requirement, deleting the public exception is the simpler/right default.

[cass-clearly]

The Architect — Round 2 Review

Verdict: APPROVE

Exact test command passed: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_architect_r2 npm test.

I fetched and read the current PR diff and changed files. CI is green. I also manually verified the Okta callback happy path against a fake Okta server: /login set state, /auth/callback issued remarq_session, and that session authorized a protected API request.

No totally different path is warranted now. Reverse-proxy-only auth is still the smaller architecture, but issue #275 explicitly asks for an Okta SSO flow. The PR now holds together: auth is mounted before JSON parsing, WebSocket upgrades use the same auth decision, config failures fail closed with actionable errors, sessions expire, and the public API/docs surface includes the new auth endpoints.

Round 1's material coherence holes are resolved. Remaining cleanup is not worth another review loop: docs/okta-auth.md says /openapi.json remains public, while the current middleware protects it when auth is required. The code is right for “disallow anonymous access entirely”; fix that wording opportunistically or let Marketing call it if they want the doc exact before merge.

Ship it.

[cass-clearly]

The Steward — Round 2 Review

Verdict: REQUEST_CHANGES

Exact test command passed on rerun: DATABASE_URL=postgresql://remarq:remarq@localhost:5433/remarq_pr284_steward_r2 npm test.

I fetched and read the current PR diff and changed files. I still do not recommend throwing this away: app-level Okta plus trusted-header mode can be a viable path. Round 2 fixed the largest runtime holes from my Round 1 review: auth now runs before JSON parsing, WebSocket upgrades use the same auth decision, and Okta calls are bounded by a timeout.

I am still blocking on production/API-contract issues:

1. The auth/API discovery contract is contradictory and will surprise consumers. docs/okta-auth.md says /openapi.json remains public, but server/auth.js only exempts /health, /login, and /auth/callback, so /openapi.json returns 401 when auth is required. The README also says the CLI fetches /openapi.json and that every spec endpoint appears as a CLI command, but the new auth endpoints have no x-cli metadata and the CLI has no way to send a session cookie/trusted header. Pick a deliberate contract and make code/docs/spec match it: either keep discovery public and document/auth-model the protected operations, or protect discovery and update the CLI/docs so authenticated deployments are usable and predictable.

2. The OpenAPI contract still does not describe auth-required behavior for protected resources. Adding /login and /auth/callback was necessary, but the rest of the API still advertises only the old unauthenticated response shapes. In auth-required mode, these endpoints can return 401 Authentication required (and config-error 500s), but the machine-readable contract does not say so. API stability means consumers and generated tooling should not learn about auth failures only at runtime.

3. Okta timeout/network failures still lack operator context. fetchWithTimeout() adds the bound, but an abort or network error from the token request vs. userinfo request still bubbles as a generic fetch/Abort error to the global 500 handler. At 3am, the logs should say which Okta step failed and include the configured timeout/status context without leaking secrets. Non-2xx responses have that context; timeout/network failures should too.

One non-blocking observation: the first run of the exact test command failed in auth > expires sessions because the test uses a 1ms session lifetime and immediately asserts it has not expired; the rerun passed. Please consider stabilizing that test while touching auth, because flaky security tests erode confidence quickly.