Raised by review on #636. Pre-existing — the inline scripts that used to build the authorize URL never sent state either — but #636 moved that construction into Go, which is where the fix belongs, so recording it properly.
Problem
blog.GithubAuthorizeURL builds GitHub's authorize URL with client_id and redirect_uri and nothing else. RFC 6749 §10.12 asks for an unguessable state, echoed back by the provider and checked on return.
Without it the flow is open to login CSRF / session swapping: an authorization code obtained for one account can be replayed into another visitor's browser, leaving them signed in as somebody else. On goblog the practical harm is a visitor unknowingly acting inside an account that is not theirs — writing a comment, or entering something into a form, believing the session is their own.
Why it is not simply added
The callback is handled in the browser. The login page reads ?code= from the query and POSTs it to /api/login, where auth.LoginPostHandler reads c.PostForm("code"). There is nowhere server-side that currently sees the return leg, so there is nothing to compare a stored state against. Three ways out, and they are not equivalent:
-
Send state alongside code from the page. /login/github stores a random value in the session and adds it to the authorize URL; the login page posts it back; LoginPostHandler compares and clears it. Smallest server change, but the login templates in all four themes have to change again, and the check still depends on the theme doing its part.
-
Validate when /login renders. The server sees ?code= and ?state= on the return request and can compare there. But the page's JS posts the code regardless, so /api/login would still accept an unvalidated code unless the binding is carried through the session as well — fiddly, and the security property ends up spread across two requests.
-
Move the callback server-side — a /login/github/callback route that validates state, exchanges the code, sets the session and redirects to next. This is the correct OAuth shape: the code never touches the page, /api/login can stop accepting a raw code, and the themes lose their remaining login script entirely.
It changes redirect_uri, so the callback URL registered in each site's GitHub OAuth app has to be updated to match. That is an operational step for every deployment, and it cannot be rolled out silently.
Recommendation
(3), accepting the registered-callback change as a documented upgrade step, since it also removes the last inline script from the login page and lets /api/login stop taking a code from anywhere. (1) is the cheap version if the callback URL must stay as it is.
Either way it wants a deliberate decision rather than riding along with an escaping fix, which is why #636 left it alone.
Related
Raised by review on #636. Pre-existing — the inline scripts that used to build the authorize URL never sent
stateeither — but #636 moved that construction into Go, which is where the fix belongs, so recording it properly.Problem
blog.GithubAuthorizeURLbuilds GitHub's authorize URL withclient_idandredirect_uriand nothing else. RFC 6749 §10.12 asks for an unguessablestate, echoed back by the provider and checked on return.Without it the flow is open to login CSRF / session swapping: an authorization code obtained for one account can be replayed into another visitor's browser, leaving them signed in as somebody else. On goblog the practical harm is a visitor unknowingly acting inside an account that is not theirs — writing a comment, or entering something into a form, believing the session is their own.
Why it is not simply added
The callback is handled in the browser. The login page reads
?code=from the query and POSTs it to/api/login, whereauth.LoginPostHandlerreadsc.PostForm("code"). There is nowhere server-side that currently sees the return leg, so there is nothing to compare a storedstateagainst. Three ways out, and they are not equivalent:Send
statealongsidecodefrom the page./login/githubstores a random value in the session and adds it to the authorize URL; the login page posts it back;LoginPostHandlercompares and clears it. Smallest server change, but the login templates in all four themes have to change again, and the check still depends on the theme doing its part.Validate when
/loginrenders. The server sees?code=and?state=on the return request and can compare there. But the page's JS posts the code regardless, so/api/loginwould still accept an unvalidated code unless the binding is carried through the session as well — fiddly, and the security property ends up spread across two requests.Move the callback server-side — a
/login/github/callbackroute that validatesstate, exchanges the code, sets the session and redirects tonext. This is the correct OAuth shape: the code never touches the page,/api/logincan stop accepting a raw code, and the themes lose their remaining login script entirely.It changes
redirect_uri, so the callback URL registered in each site's GitHub OAuth app has to be updated to match. That is an operational step for every deployment, and it cannot be rolled out silently.Recommendation
(3), accepting the registered-callback change as a documented upgrade step, since it also removes the last inline script from the login page and lets
/api/loginstop taking a code from anywhere. (1) is the cheap version if the callback URL must stay as it is.Either way it wants a deliberate decision rather than riding along with an escaping fix, which is why #636 left it alone.
Related