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..c6bd783 100644
--- a/themes/default/templates/login.html
+++ b/themes/default/templates/login.html
@@ -64,16 +64,10 @@
$('#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/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)
-
+
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 }}`))
+}