From b71cb7acdc8840c9fc16b496c90a048051d15823 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 16 Jun 2023 14:32:43 +0800 Subject: [PATCH] Use fetch to send requests to create issues/comments (#25258) Follow #23290 Network error won't make content lost. And this is a much better approach than "loading-button". The UI is not perfect and there are still some TODOs, they can be done in following PRs, not a must in this PR's scope.
![image](https://github.com/go-gitea/gitea/assets/2114189/c94ba958-aa46-4747-8ddf-6584deeed25c)
--- modules/context/base.go | 4 +++ modules/context/context_response.go | 10 ++----- modules/httplib/url.go | 27 ++++++++++++++++++ modules/httplib/url_test.go | 38 ++++++++++++++++++++++++++ modules/test/utils.go | 19 ++++++++++++- routers/common/redirect.go | 26 ++++++++++++++++++ routers/init.go | 2 ++ routers/web/repo/issue.go | 31 ++++++++------------- templates/repo/issue/new_form.tmpl | 4 +-- templates/repo/issue/view_content.tmpl | 9 +++--- tests/integration/attachment_test.go | 2 +- tests/integration/issue_test.go | 4 +-- web_src/js/features/common-global.js | 36 ++++++++++++++++-------- web_src/js/features/repo-issue.js | 5 ---- 14 files changed, 163 insertions(+), 54 deletions(-) create mode 100644 modules/httplib/url.go create mode 100644 modules/httplib/url_test.go create mode 100644 routers/common/redirect.go diff --git a/modules/context/base.go b/modules/context/base.go index 5ae5e65d3e..c8238050f9 100644 --- a/modules/context/base.go +++ b/modules/context/base.go @@ -136,6 +136,10 @@ func (b *Base) JSONRedirect(redirect string) { b.JSON(http.StatusOK, map[string]any{"redirect": redirect}) } +func (b *Base) JSONError(msg string) { + b.JSON(http.StatusBadRequest, map[string]any{"errorMessage": msg}) +} + // RemoteAddr returns the client machine ip address func (b *Base) RemoteAddr() string { return b.Req.RemoteAddr diff --git a/modules/context/context_response.go b/modules/context/context_response.go index 1f215eb8ad..88e375986c 100644 --- a/modules/context/context_response.go +++ b/modules/context/context_response.go @@ -16,6 +16,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" + "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/templates" @@ -49,14 +50,7 @@ func (ctx *Context) RedirectToFirst(location ...string) { continue } - // Unfortunately browsers consider a redirect Location with preceding "//", "\\" and "/\" as meaning redirect to "http(s)://REST_OF_PATH" - // Therefore we should ignore these redirect locations to prevent open redirects - if len(loc) > 1 && (loc[0] == '/' || loc[0] == '\\') && (loc[1] == '/' || loc[1] == '\\') { - continue - } - - u, err := url.Parse(loc) - if err != nil || ((u.Scheme != "" || u.Host != "") && !strings.HasPrefix(strings.ToLower(loc), strings.ToLower(setting.AppURL))) { + if httplib.IsRiskyRedirectURL(loc) { continue } diff --git a/modules/httplib/url.go b/modules/httplib/url.go new file mode 100644 index 0000000000..14b95898f5 --- /dev/null +++ b/modules/httplib/url.go @@ -0,0 +1,27 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package httplib + +import ( + "net/url" + "strings" + + "code.gitea.io/gitea/modules/setting" +) + +// IsRiskyRedirectURL returns true if the URL is considered risky for redirects +func IsRiskyRedirectURL(s string) bool { + // Unfortunately browsers consider a redirect Location with preceding "//", "\\", "/\" and "\/" as meaning redirect to "http(s)://REST_OF_PATH" + // Therefore we should ignore these redirect locations to prevent open redirects + if len(s) > 1 && (s[0] == '/' || s[0] == '\\') && (s[1] == '/' || s[1] == '\\') { + return true + } + + u, err := url.Parse(s) + if err != nil || ((u.Scheme != "" || u.Host != "") && !strings.HasPrefix(strings.ToLower(s), strings.ToLower(setting.AppURL))) { + return true + } + + return false +} diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go new file mode 100644 index 0000000000..72033b1208 --- /dev/null +++ b/modules/httplib/url_test.go @@ -0,0 +1,38 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package httplib + +import ( + "testing" + + "code.gitea.io/gitea/modules/setting" + + "github.com/stretchr/testify/assert" +) + +func TestIsRiskyRedirectURL(t *testing.T) { + setting.AppURL = "http://localhost:3000/" + tests := []struct { + input string + want bool + }{ + {"", false}, + {"foo", false}, + {"/", false}, + {"/foo?k=%20#abc", false}, + + {"//", true}, + {"\\\\", true}, + {"/\\", true}, + {"\\/", true}, + {"mail:a@b.com", true}, + {"https://test.com", true}, + {setting.AppURL + "/foo", false}, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input)) + }) + } +} diff --git a/modules/test/utils.go b/modules/test/utils.go index 282895eaa9..2917741c45 100644 --- a/modules/test/utils.go +++ b/modules/test/utils.go @@ -5,12 +5,29 @@ package test import ( "net/http" + "net/http/httptest" "strings" + + "code.gitea.io/gitea/modules/json" ) // RedirectURL returns the redirect URL of a http response. +// It also works for JSONRedirect: `{"redirect": "..."}` func RedirectURL(resp http.ResponseWriter) string { - return resp.Header().Get("Location") + loc := resp.Header().Get("Location") + if loc != "" { + return loc + } + if r, ok := resp.(*httptest.ResponseRecorder); ok { + m := map[string]any{} + err := json.Unmarshal(r.Body.Bytes(), &m) + if err == nil { + if loc, ok := m["redirect"].(string); ok { + return loc + } + } + } + return "" } func IsNormalPageCompleted(s string) bool { diff --git a/routers/common/redirect.go b/routers/common/redirect.go new file mode 100644 index 0000000000..9bf2025e19 --- /dev/null +++ b/routers/common/redirect.go @@ -0,0 +1,26 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + "net/http" + + "code.gitea.io/gitea/modules/httplib" +) + +// FetchRedirectDelegate helps the "fetch" requests to redirect to the correct location +func FetchRedirectDelegate(resp http.ResponseWriter, req *http.Request) { + // When use "fetch" to post requests and the response is a redirect, browser's "location.href = uri" has limitations. + // 1. change "location" from old "/foo" to new "/foo#hash", the browser will not reload the page. + // 2. when use "window.reload()", the hash is not respected, the newly loaded page won't scroll to the hash target. + // The typical page is "issue comment" page. The backend responds "/owner/repo/issues/1#comment-2", + // then frontend needs this delegate to redirect to the new location with hash correctly. + redirect := req.PostFormValue("redirect") + if httplib.IsRiskyRedirectURL(redirect) { + resp.WriteHeader(http.StatusBadRequest) + return + } + resp.Header().Add("Location", redirect) + resp.WriteHeader(http.StatusSeeOther) +} diff --git a/routers/init.go b/routers/init.go index 5737ef3dc0..725e5c52ba 100644 --- a/routers/init.go +++ b/routers/init.go @@ -183,6 +183,8 @@ func NormalRoutes(ctx context.Context) *web.Route { r.Mount("/api/v1", apiv1.Routes(ctx)) r.Mount("/api/internal", private.Routes()) + r.Post("/-/fetch-redirect", common.FetchRedirectDelegate) + if setting.Packages.Enabled { // This implements package support for most package managers r.Mount("/api/packages", packages_router.CommonRoutes(ctx)) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 5ab8db2e05..9f087edc72 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1134,12 +1134,12 @@ func NewIssuePost(ctx *context.Context) { } if ctx.HasError() { - ctx.HTML(http.StatusOK, tplIssueNew) + ctx.JSONError(ctx.GetErrMsg()) return } if util.IsEmptyString(form.Title) { - ctx.RenderWithErr(ctx.Tr("repo.issues.new.title_empty"), tplIssueNew, form) + ctx.JSONError(ctx.Tr("repo.issues.new.title_empty")) return } @@ -1184,9 +1184,9 @@ func NewIssuePost(ctx *context.Context) { log.Trace("Issue created: %d/%d", repo.ID, issue.ID) if ctx.FormString("redirect_after_creation") == "project" && projectID > 0 { - ctx.Redirect(ctx.Repo.RepoLink + "/projects/" + strconv.FormatInt(projectID, 10)) + ctx.JSONRedirect(ctx.Repo.RepoLink + "/projects/" + strconv.FormatInt(projectID, 10)) } else { - ctx.Redirect(issue.Link()) + ctx.JSONRedirect(issue.Link()) } } @@ -2777,8 +2777,7 @@ func NewComment(ctx *context.Context) { } if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin { - ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked")) - ctx.Redirect(issue.Link()) + ctx.JSONError(ctx.Tr("repo.issues.comment_on_locked")) return } @@ -2788,8 +2787,7 @@ func NewComment(ctx *context.Context) { } if ctx.HasError() { - ctx.Flash.Error(ctx.Data["ErrorMsg"].(string)) - ctx.Redirect(issue.Link()) + ctx.JSONError(ctx.GetErrMsg()) return } @@ -2809,8 +2807,7 @@ func NewComment(ctx *context.Context) { pr, err = issues_model.GetUnmergedPullRequest(ctx, pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch, pull.Flow) if err != nil { if !issues_model.IsErrPullRequestNotExist(err) { - ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked")) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pull.Index)) + ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked")) return } } @@ -2841,8 +2838,7 @@ func NewComment(ctx *context.Context) { } if ok := git.IsBranchExist(ctx, pull.HeadRepo.RepoPath(), pull.BaseBranch); !ok { // todo localize - ctx.Flash.Error("The origin branch is delete, cannot reopen.") - ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pull.Index)) + ctx.JSONError("The origin branch is delete, cannot reopen.") return } headBranchRef := pull.GetGitHeadBranchRefName() @@ -2882,11 +2878,9 @@ func NewComment(ctx *context.Context) { if issues_model.IsErrDependenciesLeft(err) { if issue.IsPull { - ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked")) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) + ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked")) } else { - ctx.Flash.Error(ctx.Tr("repo.issues.dependency.issue_close_blocked")) - ctx.Redirect(fmt.Sprintf("%s/issues/%d", ctx.Repo.RepoLink, issue.Index)) + ctx.JSONError(ctx.Tr("repo.issues.dependency.issue_close_blocked")) } return } @@ -2899,7 +2893,6 @@ func NewComment(ctx *context.Context) { log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed) } } - } // Redirect to comment hashtag if there is any actual content. @@ -2908,9 +2901,9 @@ func NewComment(ctx *context.Context) { typeName = "pulls" } if comment != nil { - ctx.Redirect(fmt.Sprintf("%s/%s/%d#%s", ctx.Repo.RepoLink, typeName, issue.Index, comment.HashTag())) + ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d#%s", ctx.Repo.RepoLink, typeName, issue.Index, comment.HashTag())) } else { - ctx.Redirect(fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, typeName, issue.Index)) + ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, typeName, issue.Index)) } }() diff --git a/templates/repo/issue/new_form.tmpl b/templates/repo/issue/new_form.tmpl index 00da68eb06..70c45b37c8 100644 --- a/templates/repo/issue/new_form.tmpl +++ b/templates/repo/issue/new_form.tmpl @@ -1,4 +1,4 @@ -
+ {{.CsrfTokenHtml}} {{if .Flash}}
@@ -35,7 +35,7 @@ {{template "repo/issue/comment_tab" .}} {{end}}
- {{else}} @@ -112,12 +111,12 @@ {{if .Issue.IsPull}} {{$closeTranslationKey = "repo.pulls.close"}} {{end}} - {{end}} {{end}} -
diff --git a/tests/integration/attachment_test.go b/tests/integration/attachment_test.go index ff62726487..8206d8f4dc 100644 --- a/tests/integration/attachment_test.go +++ b/tests/integration/attachment_test.go @@ -83,7 +83,7 @@ func TestCreateIssueAttachment(t *testing.T) { } req = NewRequestWithValues(t, "POST", link, postData) - resp = session.MakeRequest(t, req, http.StatusSeeOther) + resp = session.MakeRequest(t, req, http.StatusOK) test.RedirectURL(resp) // check that redirect URL exists // Validate that attachment is available diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 7ea7fefb64..ab2986906b 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -135,7 +135,7 @@ func testNewIssue(t *testing.T, session *TestSession, user, repo, title, content "title": title, "content": content, }) - resp = session.MakeRequest(t, req, http.StatusSeeOther) + resp = session.MakeRequest(t, req, http.StatusOK) issueURL := test.RedirectURL(resp) req = NewRequest(t, "GET", issueURL) @@ -165,7 +165,7 @@ func testIssueAddComment(t *testing.T, session *TestSession, issueURL, content, "content": content, "status": status, }) - resp = session.MakeRequest(t, req, http.StatusSeeOther) + resp = session.MakeRequest(t, req, http.StatusOK) req = NewRequest(t, "GET", test.RedirectURL(resp)) resp = session.MakeRequest(t, req, http.StatusOK) diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index c0e66be51c..c5f973f31c 100644 --- a/web_src/js/features/common-global.js +++ b/web_src/js/features/common-global.js @@ -9,7 +9,7 @@ import {hideElem, showElem, toggleElem} from '../utils/dom.js'; import {htmlEscape} from 'escape-goat'; import {createTippy} from '../modules/tippy.js'; -const {appUrl, csrfToken, i18n} = window.config; +const {appUrl, appSubUrl, csrfToken, i18n} = window.config; export function initGlobalFormDirtyLeaveConfirm() { // Warn users that try to leave a page after entering data into a form. @@ -61,6 +61,21 @@ export function initGlobalButtonClickOnEnter() { }); } +// doRedirect does real redirection to bypass the browser's limitations of "location" +// more details are in the backend's fetch-redirect handler +function doRedirect(redirect) { + const form = document.createElement('form'); + const input = document.createElement('input'); + form.method = 'post'; + form.action = `${appSubUrl}/-/fetch-redirect`; + input.type = 'hidden'; + input.name = 'redirect'; + input.value = redirect; + form.append(input); + document.body.append(form); + form.submit(); +} + async function formFetchAction(e) { if (!e.target.classList.contains('form-fetch-action')) return; @@ -101,6 +116,7 @@ async function formFetchAction(e) { const onError = (msg) => { formEl.classList.remove('is-loading', 'small-loading-icon'); if (errorTippy) errorTippy.destroy(); + // TODO: use a better toast UI instead of the tippy. If the form height is large, the tippy position is not good errorTippy = createTippy(formEl, { content: msg, interactive: true, @@ -120,15 +136,21 @@ async function formFetchAction(e) { const {redirect} = await resp.json(); formEl.classList.remove('dirty'); // remove the areYouSure check before reloading if (redirect) { - window.location.href = redirect; + doRedirect(redirect); } else { window.location.reload(); } + } else if (resp.status >= 400 && resp.status < 500) { + const data = await resp.json(); + // the code was quite messy, sometimes the backend uses "err", sometimes it uses "error", and even "user_error" + // but at the moment, as a new approach, we only use "errorMessage" here, backend can use JSONError() to respond. + onError(data.errorMessage || `server error: ${resp.status}`); } else { onError(`server error: ${resp.status}`); } } catch (e) { - onError(e.error); + console.error('error when doRequest', e); + onError(i18n.network_error); } }; @@ -183,14 +205,6 @@ export function initGlobalCommon() { $('.tabular.menu .item').tab(); - // prevent multiple form submissions on forms containing .loading-button - document.addEventListener('submit', (e) => { - const btn = e.target.querySelector('.loading-button'); - if (!btn) return; - if (btn.classList.contains('loading')) return e.preventDefault(); - btn.classList.add('loading'); - }); - document.addEventListener('submit', formFetchAction); } diff --git a/web_src/js/features/repo-issue.js b/web_src/js/features/repo-issue.js index 0dc5728f58..d271d2b84e 100644 --- a/web_src/js/features/repo-issue.js +++ b/web_src/js/features/repo-issue.js @@ -636,11 +636,6 @@ export function initSingleCommentEditor($commentForm) { const opts = {}; const $statusButton = $('#status-button'); if ($statusButton.length) { - $statusButton.on('click', (e) => { - e.preventDefault(); - $('#status').val($statusButton.data('status-val')); - $('#comment-form').trigger('submit'); - }); opts.onContentChanged = (editor) => { $statusButton.text($statusButton.attr(editor.value().trim() ? 'data-status-and-comment' : 'data-status')); };