Finish GitHub's OAuth flow server-side, against a state

[compscidr]

Fixes #637.

The gap

GitHub's OAuth flow finished in the browser: the login page read ?code= from the query and posted it to /api/login, which exchanged it and set the session. Nothing tied the code to the visitor who started the flow, so a code obtained for one account could be replayed into another visitor's browser, leaving them signed in as somebody else.

The fix

/login/github mints an unguessable state, keeps it and the intended destination in the session, and sends the state to GitHub. GitHub's redirect_uri is already /login, so its return lands in loginHandler, which hands it to GithubCallback instead of rendering the page. The code is exchanged only if the state comes back unchanged, and the state is spent either way.

/api/login is gone — it was the hole, not the page:

// csrf_test.go, before:
{"login form untouched", "POST", "/api/login", "application/x-www-form-urlencoded", "a=b", 200},

It took a bare code as a form post and set the session from it, and requireJSON deliberately exempted it. SameSite=Lax doesn't help — it governs sending a cookie cross-site, not setting one — so a cross-site form could plant a session without the login page being involved at all. Nothing accepts a raw code now.

No registered-callback change after all

#637 proposed a new /login/github/callback route, which would have changed redirect_uri and required the callback registered in each site's GitHub OAuth app to be updated — an operational step on every deployment. Handling the return at /login, where redirect_uri already points, gives the same security property with no such step. My write-up in the issue assumed the new route was necessary; it isn't.

Along the way

• redirect_uri is constant now. GitHub matches it against the registered callback, so a value that doesn't vary is likelier to match. next rides in the session instead, which also keeps it out of a URL handed to a third party and written to logs.
• requestAccessToken no longer fails outright when .env is missing. A deployment may set client_id/client_secret in the real environment; what matters is having both, which it now checks.
• The themes drop the ?code= block and post the email-login endpoints relatively rather than rebuilding an origin.

The tests needed a second attempt

My first version asserted only that no session was created on a bad state. Those passed with the state check removed — with no GitHub to talk to, the exchange fails anyway, so they proved nothing.

They're internal tests now, pointing the token endpoint at a stand-in, so they can assert on what actually distinguishes a rejected callback from a failed one: whether the token endpoint was reached at all. With the check bypassed they fail properly:

--- FAIL: TestGithubCallback_MismatchedStateNeverReachesGithub
    the code was exchanged despite a mismatched state (1 calls)
    a mismatched state logged someone in (token "tok-from-github")
--- FAIL: TestGithubCallback_NoLoginStartedNeverReachesGithub
--- FAIL: TestGithubCallback_EmptyStateBothSidesIsNotAMatch
--- FAIL: TestGithubCallback_StateIsSpentAfterOneUse

TestGithubCallback_HappyPath pins the other direction — a matching state does exchange and lands on the stored next — so the rejection tests mean something. TestNoRawOAuthCodeEndpoint guards the removed route.

Verified over HTTP against a local instance

	result
wrong state	302 /login?error=github, no exchange
code handed to a browser that never started a login	302 /login?error=github, no exchange
matching state	reaches the exchange (error requesting token access — no real GitHub)
replaying that state	state did not match the session; ignoring the code
POST /api/login	404

Merge order

Same as the last round: this merges and releases first, then the theme PRs, which now carry this change too — goblogplatform/goblog-site-theme#12, goblogplatform/goblog-theme-forest#6, goblogplatform/goblog-theme-minimal#4.

Un-updated themes keep working either way: the server handles /login?code= before the page renders, so their ?code= script never fires and never reaches the removed endpoint.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

A newly added test (TestNoRawOAuthCodeEndpoint) will fail due to an over-broad substring check that matches existing /api/login/email* routes.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Test checks too broad a login route substring · New

What changed in this PR

This PR hardens the GitHub OAuth login flow by moving the authorization-code exchange fully server-side and binding it to an unguessable state stored in the session, closing a login CSRF/session-swapping gap described in #637.

Changes:

• Add server-minted OAuth state stored in the session on /login/github, and validate/spend it on GitHub’s return to /login before exchanging the code.
• Remove the raw OAuth-code ingestion endpoint (POST /api/login) and update the default theme to stop posting ?code= from the browser.
• Add targeted tests (including internal tests with a fake token endpoint) to prove mismatched/missing state never reaches the token exchange.

File	Description
themes/​default/​templates/​login.html	Removes browser-side GitHub callback posting; makes email-login posts relative.
goblog.go	Routes GitHub callback handling through /login server-side; removes /api/login route.
csrf_test.go	Drops /api/login from requireJSON tests; adds a guard test intended to ensure the endpoint stays removed.
blog/​github_login_test.go	Updates authorize URL tests and adds session-backed login initiation/state assertions.
blog/​blog.go	Makes redirect_uri constant, adds state to authorize URL, and stores state/next in session on /login/github.
auth/​github_oauth.go	Introduces GithubCallback that validates/spends state and completes login server-side.
auth/​github_oauth_internal_test.go	Adds internal tests with a fake GitHub server to assert state mismatch prevents token exchange.
auth/​auth.go	Refactors login exchange into completeGithubLogin, hardens env loading, and makes GitHub endpoints overridable for tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.