From 9d073991cd55595a9377958cdc26c059b6a7f829 Mon Sep 17 00:00:00 2001 From: Jason Ernst Date: Fri, 25 Sep 2026 11:05:10 -0700 Subject: [PATCH 1/3] Start GitHub's OAuth flow server-side (#631), and let the wizard report its own failures (#632) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #631 — every theme assembled GitHub's authorize URL in an inline script, concatenating window.location straight into redirect_uri. A next containing & ended the redirect_uri value early and the rest reached GitHub as further authorize parameters. /login/github now does it in Go, and the themes point a plain anchor at it: no client_id in the page, no inline script, and the escaping is tested rather than duplicated four times. The issue proposed stripping the query from redirect_uri, which would have been a regression: next round-trips *through* redirect_uri, since GitHub returns the visitor to that exact URL with &code= appended and Login reads next from the query. So the fix escapes rather than strips, and both the redirect and the next inside it are escaped. SafeNext guards the new entry point too — next reaches redirect_uri, so an open redirect here would be handed to GitHub. The origin comes from the request rather than the site_url setting, on purpose: GitHub matches redirect_uri against the callback registered for the app, so it has to be the host the visitor is on — which is what window.location used to give. #632 — both wizard database handlers returned nothing when ParseForm failed, which gin sends as a bare 200 with an empty body. For /test_db that is worse than silence: the wizard's jQuery call has no dataType, so an empty 200 takes the success path and the Test Database button turns green on a request the server never read. /wizard_db rendered a blank page despite having a fail() helper for exactly this. On the client, the error handler read data.responseJSON.error, which is undefined whenever the body was not JSON — a proxy error, an HTML error page, a dropped connection. Reading .error threw, so the button went red and the reason never appeared. It now falls back through statusText, and renders with .text() rather than .html(). Verified against a local instance: the anchor carries an escaped next, /login/github escapes both layers, an offsite next is dropped, X-Forwarded-Proto is honoured, and replaying GitHub's return leg resolves next back to /search?q=a&b=c — the case that was broken. Co-Authored-By: Claude Opus 5 (1M context) --- blog/blog.go | 55 +++++++++++ blog/github_login_test.go | 120 ++++++++++++++++++++++++ goblog.go | 10 ++ themes/default/templates/login.html | 7 +- themes/default/templates/wizard_db.html | 11 ++- wizard_errors_test.go | 79 ++++++++++++++++ 6 files changed, 273 insertions(+), 9 deletions(-) create mode 100644 blog/github_login_test.go create mode 100644 wizard_errors_test.go diff --git a/blog/blog.go b/blog/blog.go index 94aa95a..c3d53cf 100644 --- a/blog/blog.go +++ b/blog/blog.go @@ -1453,6 +1453,61 @@ func SafeNext(raw string) string { return raw } +// RequestOrigin is the scheme and host the client actually used, without a +// trailing slash. Unlike SiteURL it ignores the site_url setting: GitHub +// matches an OAuth redirect_uri against the callback registered for the app, +// so the origin has to be the one the visitor is on — which is what the login +// page used to send when it built the URL from window.location. +func RequestOrigin(c *gin.Context) string { + if c == nil || c.Request == nil { + return "" + } + scheme := "http" + // Proxies may chain values ("https, http") and vary the case; the first + // is the one the client used. + forwarded, _, _ := strings.Cut(c.GetHeader("X-Forwarded-Proto"), ",") + if c.Request.TLS != nil || strings.EqualFold(strings.TrimSpace(forwarded), "https") { + scheme = "https" + } + return scheme + "://" + c.Request.Host +} + +// GithubAuthorizeURL builds the URL that starts GitHub's OAuth flow. +// +// redirect_uri keeps the ?next= the visitor arrived with, because that is how +// next survives the round trip: GitHub sends them back to this exact URL with +// &code= appended, and Login reads next from that query. Stripping the query +// would quietly land everyone on / after signing in. +// +// Both the redirect and the next inside it are escaped. Unescaped, a next +// containing & would end the redirect_uri value early and the rest would +// reach GitHub as further authorize parameters (#631). +func GithubAuthorizeURL(origin, clientID, next string) string { + redirect := origin + "/login" + if next != "" && next != "/" { + redirect += "?next=" + url.QueryEscape(next) + } + return "https://github.com/login/oauth/authorize?client_id=" + url.QueryEscape(clientID) + + "&redirect_uri=" + url.QueryEscape(redirect) +} + +// GithubLogin serves /login/github: it sends the visitor to GitHub rather than +// having each theme assemble the authorize URL in an inline script, where the +// escaping was wrong and had to be fixed in every theme separately (#631). +func (b *Blog) GithubLogin(c *gin.Context) { + if err := godotenv.Load(".env"); err != nil { + _ = godotenv.Load("local.env") + } + clientID := os.Getenv("client_id") + if clientID == "" { + // Nothing to send them to; the login page explains the situation. + c.Redirect(http.StatusFound, "/login") + return + } + next := SafeNext(c.Query("next")) + c.Redirect(http.StatusFound, GithubAuthorizeURL(RequestOrigin(c), clientID, next)) +} + func (b *Blog) Logout(c *gin.Context) { session := sessions.Default(c) session.Delete("token") diff --git a/blog/github_login_test.go b/blog/github_login_test.go new file mode 100644 index 0000000..f1a7d7b --- /dev/null +++ b/blog/github_login_test.go @@ -0,0 +1,120 @@ +package blog_test + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "goblog/blog" +) + +// The login page used to build GitHub's authorize URL in an inline script, +// concatenating window.location straight into redirect_uri. #631 moved it into +// Go so the escaping is done once and can be tested. + +// redirectURI pulls redirect_uri back out of an authorize URL, decoded. +func redirectURI(t *testing.T, authorize string) string { + t.Helper() + u, err := url.Parse(authorize) + if err != nil { + t.Fatalf("authorize URL does not parse: %v", err) + } + return u.Query().Get("redirect_uri") +} + +func TestGithubAuthorizeURL_KeepsNextThroughTheRoundTrip(t *testing.T) { + got := blog.GithubAuthorizeURL("https://example.com", "abc123", "/admin/settings") + if want := "https://example.com/login?next=%2Fadmin%2Fsettings"; redirectURI(t, got) != want { + t.Errorf("redirect_uri = %q, want %q", redirectURI(t, got), want) + } + // GitHub returns the visitor to redirect_uri with &code= appended, and + // Login reads next from that query. Dropping it would send everyone to /. + if !strings.Contains(redirectURI(t, got), "next=") { + t.Error("redirect_uri must keep ?next=, it is how next survives the round trip") + } +} + +// TestGithubAuthorizeURL_EscapesAmpersandInNext is the bug in #631: unescaped, +// everything after the & in next reaches GitHub as further authorize +// parameters and redirect_uri is truncated. +func TestGithubAuthorizeURL_EscapesAmpersandInNext(t *testing.T) { + got := blog.GithubAuthorizeURL("https://example.com", "abc123", "/search?q=a&b=c") + + u, err := url.Parse(got) + if err != nil { + t.Fatalf("authorize URL does not parse: %v", err) + } + q := u.Query() + if len(q) != 2 { + t.Errorf("authorize URL has %d parameters, want exactly client_id and redirect_uri: %v", len(q), q) + } + if want := "https://example.com/login?next=%2Fsearch%3Fq%3Da%26b%3Dc"; q.Get("redirect_uri") != want { + t.Errorf("redirect_uri = %q, want %q", q.Get("redirect_uri"), want) + } + // The raw string must not carry a bare & or ? from next. + raw := got[strings.Index(got, "redirect_uri="):] + if strings.Contains(raw, "&b=c") || strings.Contains(raw, "?q=") { + t.Errorf("next is not escaped inside redirect_uri: %s", raw) + } +} + +func TestGithubAuthorizeURL_OmitsEmptyNext(t *testing.T) { + for _, next := range []string{"", "/"} { + got := redirectURI(t, blog.GithubAuthorizeURL("https://example.com", "abc", next)) + if got != "https://example.com/login" { + t.Errorf("next %q: redirect_uri = %q, want no query string", next, got) + } + } +} + +// TestRequestOrigin: the origin must be the host the visitor is on, because +// GitHub matches redirect_uri against the app's registered callback. +func TestRequestOrigin(t *testing.T) { + for _, tc := range []struct { + name, host, forwarded, want string + }{ + {"plain http", "example.com", "", "http://example.com"}, + {"behind a TLS proxy", "example.com", "https", "https://example.com"}, + {"proxy chain takes the first", "example.com", "https, http", "https://example.com"}, + {"case insensitive", "example.com", "HTTPS", "https://example.com"}, + {"host with a port", "localhost:7000", "", "http://localhost:7000"}, + } { + t.Run(tc.name, func(t *testing.T) { + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Request = httptest.NewRequest(http.MethodGet, "/login/github", nil) + c.Request.Host = tc.host + if tc.forwarded != "" { + c.Request.Header.Set("X-Forwarded-Proto", tc.forwarded) + } + if got := blog.RequestOrigin(c); got != tc.want { + t.Errorf("RequestOrigin = %q, want %q", got, tc.want) + } + }) + } +} + +// TestGithubLogin_RejectsOffsiteNext: next reaches redirect_uri, so an open +// redirect here would be handed to GitHub. SafeNext already guards Login; it +// has to guard this entry point too. +func TestGithubLogin_RejectsOffsiteNext(t *testing.T) { + t.Setenv("client_id", "abc123") + b := blog.New(nil, nil, "test") + + for _, next := range []string{"https://evil.example", "//evil.example", "/\\evil.example"} { + router := gin.New() + router.GET("/login/github", b.GithubLogin) + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/login/github?next="+url.QueryEscape(next), nil)) + + if w.Code != http.StatusFound { + t.Fatalf("next %q: code = %d, want 302", next, w.Code) + } + got := redirectURI(t, w.Header().Get("Location")) + if strings.Contains(got, "evil.example") { + t.Errorf("next %q leaked into redirect_uri: %s", next, got) + } + } +} diff --git a/goblog.go b/goblog.go index c58d5e6..d578963 100644 --- a/goblog.go +++ b/goblog.go @@ -434,6 +434,9 @@ func main() { getAndHead(router, "/", goblog.rootHandler) getAndHead(router, "/login", goblog.loginHandler) + // Starting GitHub's OAuth flow belongs here rather than in each theme's + // inline script, where the redirect_uri escaping was wrong (#631). + router.GET("/login/github", goblog._blog.GithubLogin) router.GET("/wizard", goblog._wizard.SaveToken) router.POST("/wizard_db", updateDB) router.POST("/test_db", testDB) @@ -628,7 +631,10 @@ func updateDB(c *gin.Context) { }) } if err := c.Request.ParseForm(); err != nil { + // Returning nothing rendered a blank 200: the wizard appeared to do + // nothing at all. fail() puts the reason back on the page (#632). log.Println("Couldn't parse the form: " + err.Error()) + fail("Couldn't read the form: " + err.Error()) return } cfg := dbConfigFromForm(c) @@ -647,7 +653,11 @@ func updateDB(c *gin.Context) { func testDB(c *gin.Context) { if err := c.Request.ParseForm(); err != nil { + // Returning nothing meant gin sent a bare 200, which the wizard's + // jQuery call read as success: the Test Database button turned green + // on a request that was never looked at (#632). log.Println("Couldn't parse the form: " + err.Error()) + c.JSON(http.StatusBadRequest, gin.H{"error": "Couldn't read the form: " + err.Error()}) return } // Don't log the form: it carries the database password. diff --git a/themes/default/templates/login.html b/themes/default/templates/login.html index 04eff37..b99fed0 100644 --- a/themes/default/templates/login.html +++ b/themes/default/templates/login.html @@ -64,15 +64,10 @@

Site Login

- + Sign in with Github - {{ if .email_login_enabled }}
 
diff --git a/themes/default/templates/wizard_db.html b/themes/default/templates/wizard_db.html index 1b41a96..7a752b0 100644 --- a/themes/default/templates/wizard_db.html +++ b/themes/default/templates/wizard_db.html @@ -147,9 +147,14 @@
Database:
$('#database-test').removeClass('btn-secondary').addClass('btn-danger'); // change the icon within the button $('#database-test').html(' Database Test Failed'); - // get the error message and show it at top of page - var error = data.responseJSON.error; - $('#ajax-error').html(error).show(); + // responseJSON is only set when the body parsed as JSON. A 502 + // from a proxy, an HTML error page or a dropped connection left + // it undefined, and reading .error threw here — so the button + // went red and the reason never appeared, which is the one + // thing someone stuck on this step needs (#632). + var error = (data.responseJSON && data.responseJSON.error) || + data.statusText || 'Database test failed.'; + $('#ajax-error').text(error).show(); } }); }); diff --git a/wizard_errors_test.go b/wizard_errors_test.go new file mode 100644 index 0000000..3889842 --- /dev/null +++ b/wizard_errors_test.go @@ -0,0 +1,79 @@ +package main + +import ( + "encoding/json" + "html/template" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" +) + +// Both wizard database handlers used to return nothing when ParseForm failed, +// which gin turns into a bare 200 with an empty body (#632). For /test_db that +// is worse than useless: the wizard's jQuery call has no dataType, so an empty +// 200 takes the success path and the Test Database button goes green on a +// request the server never looked at. + +// malformedForm is a body ParseForm rejects: %zz is not valid percent-encoding. +func malformedForm(t *testing.T, path string) *http.Request { + t.Helper() + req := httptest.NewRequest(http.MethodPost, path, strings.NewReader("database=%zz")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + return req +} + +func TestTestDB_MalformedFormIsNotReportedAsSuccess(t *testing.T) { + gin.SetMode(gin.TestMode) + router := gin.New() + router.POST("/test_db", testDB) + + w := httptest.NewRecorder() + router.ServeHTTP(w, malformedForm(t, "/test_db")) + + if w.Code == http.StatusOK { + t.Fatalf("code = 200 for a form the server could not read; the wizard shows that as a passing database test") + } + if w.Code != http.StatusBadRequest { + t.Errorf("code = %d, want 400", w.Code) + } + // The wizard reads .error out of the body, so it has to be JSON with that key. + var body map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("body is not JSON (%v): %q", err, w.Body.String()) + } + if body["error"] == "" { + t.Errorf("body has no error to show the user: %q", w.Body.String()) + } +} + +// TestUpdateDB_MalformedFormRendersTheWizardAgain: /wizard_db answers with a +// page rather than JSON, and it already has a fail() helper that re-renders +// the wizard with the reason on it. The ParseForm branch skipped it and +// returned a blank 200, so the wizard appeared to do nothing at all. +func TestUpdateDB_MalformedFormRendersTheWizardAgain(t *testing.T) { + gin.SetMode(gin.TestMode) + router := gin.New() + router.SetHTMLTemplate(templateWithErrors(t)) + router.POST("/wizard_db", updateDB) + + w := httptest.NewRecorder() + router.ServeHTTP(w, malformedForm(t, "/wizard_db")) + + if w.Body.Len() == 0 { + t.Fatal("blank response: the wizard gives the user nothing to act on") + } + // html/template escapes the apostrophe, so match on a plain substring. + if !strings.Contains(w.Body.String(), "read the form") { + t.Errorf("page does not say why it failed: %q", w.Body.String()) + } +} + +// templateWithErrors is a stand-in for wizard_db.html that prints just the +// errors, so this test covers the handler rather than the theme's markup. +func templateWithErrors(t *testing.T) *template.Template { + t.Helper() + return template.Must(template.New("wizard_db.html").Parse(`{{ .errors }}`)) +} From f68daf4f644e4b1db5de484da64c954ba4eb6914 Mon Sep 17 00:00:00 2001 From: Jason Ernst Date: Fri, 25 Sep 2026 11:06:03 -0700 Subject: [PATCH 2/3] Tidy the blank line left by the removed login script Co-Authored-By: Claude Opus 5 (1M context) --- themes/default/templates/login.html | 1 - 1 file changed, 1 deletion(-) diff --git a/themes/default/templates/login.html b/themes/default/templates/login.html index b99fed0..0bf21fa 100644 --- a/themes/default/templates/login.html +++ b/themes/default/templates/login.html @@ -68,7 +68,6 @@

Site Login

Sign in with Github - {{ if .email_login_enabled }}
 

or sign in with email

From 4d19dfde08b61afe868b2f49f83c585443fb07f8 Mon Sep 17 00:00:00 2001 From: Jason Ernst Date: Fri, 25 Sep 2026 11:12:27 -0700 Subject: [PATCH 3/3] Capitalise GitHub in the prose the login and wizard pages show Review caught it on the login button. Fixed everywhere it appears as user-facing text, leaving identifiers (github_url, fa-github, /login/github) alone. Co-Authored-By: Claude Opus 5 (1M context) --- themes/default/templates/login.html | 2 +- themes/default/templates/wizard_auth.html | 6 +++--- themes/default/templates/wizard_settings.html | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/themes/default/templates/login.html b/themes/default/templates/login.html index 0bf21fa..c6bd783 100644 --- a/themes/default/templates/login.html +++ b/themes/default/templates/login.html @@ -65,7 +65,7 @@

Site Login

- Sign in with Github + Sign in with GitHub {{ if .email_login_enabled }} diff --git a/themes/default/templates/wizard_auth.html b/themes/default/templates/wizard_auth.html index c0887ae..7ee53be 100644 --- a/themes/default/templates/wizard_auth.html +++ b/themes/default/templates/wizard_auth.html @@ -13,7 +13,7 @@

Goblog Install Wizard

-
Github OAuth
+
GitHub OAuth
{{ if .errors }}
Create Admin Account
-

Once you setup the OAuth app, login with Github to test it out. The user you login with will be the +

Once you setup the OAuth app, login with GitHub to test it out. The user you login with will be the admin user for this site.

Powered by goblog {{ .version }}
diff --git a/themes/default/templates/wizard_settings.html b/themes/default/templates/wizard_settings.html index 5bab3b3..0d1edbe 100644 --- a/themes/default/templates/wizard_settings.html +++ b/themes/default/templates/wizard_settings.html @@ -58,7 +58,7 @@
Socials: (leave blank if you don't want to display)
- +