Repository navigation
Start GitHub's OAuth flow server-side, and let the wizard report its own failures #636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| } | ||
| } | ||
| } |
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 }}`)) | ||
| } |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.