Link to /login/github instead of building the OAuth URL here - #6
Merged
Merged
Conversation
The GitHub button used to assemble GitHub's authorize URL in an inline script, concatenating window.location straight into redirect_uri; a next containing & ended the value early and the rest reached GitHub as further authorize parameters (goblog #631). goblog builds the URL now, so this is a plain anchor at /login/github carrying an escaped next — no client_id in the page and no inline script. Needs goblog with #631 for the /login/github route. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is small, removes inline OAuth URL construction, and correctly URL-escapes the optional next parameter in the new server-route link.
Review effort: Lite
Findings: None
What changed in this PR
Updates the theme login page to link directly to the server-provided GitHub OAuth entrypoint (/login/github) instead of assembling the GitHub authorize URL in-browser, aligning with the upstream goblog change to centralize OAuth URL construction and escaping.
Changes:
- Replace the previously empty GitHub button + inline JS URL construction with a direct anchor to
/login/github. - Add optional
nextpropagation via?next={{ .next | urlquery }}(omitted when.nextis/).
| File | Description |
|---|---|
| templates/login.html | Switches GitHub login from client-side OAuth URL construction to a server route (/login/github) with optional next parameter forwarding. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
goblog exchanges the code at /login itself, against a state it minted, so this block can never fire (goblog #637). It was also the client half of the login CSRF: it posted whatever code appeared in the query to /api/login, which no longer exists. The email-login endpoints are posted to relatively rather than rebuilding an origin, since the variable holding it went with the removed block. Needs goblog with #637. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands with goblogplatform/goblog#636.
Why
The GitHub button assembled GitHub's authorize URL here, with
window.locationconcatenated straight intoredirect_uri. Anextcontaining&ended the value early and everything after it reached GitHub as further authorize parameters (goblogplatform/goblog#631).goblog builds the URL now, so this becomes a plain anchor:
No
client_idin the page, no inline script, and the escaping is tested once in Go instead of duplicated across four theme repos.goblogplatform/goblog#636 must merge and release first.
/login/githubdoes not exist until it ships; on an older goblog this link 404s and GitHub login is dead.This is the reverse of the #624 round, where the themes went first — there they had to tolerate a goblog change, here they depend on a new route.
Also now: the ?code= block goes
goblog exchanges the code at
/loginitself, against a state it minted, so that block can never fire (goblogplatform/goblog#637). It was also the client half of a login CSRF: it posted whatever code appeared in the query to/api/login, which no longer exists.Updated requirement: needs goblog with #638 (which includes #636).
🤖 Generated with Claude Code