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 @@
-<form class="issue-content ui comment form" id="new-issue" action="{{.Link}}" method="post">
+<form class="issue-content ui comment form form-fetch-action" id="new-issue" action="{{.Link}}" method="post">
{{.CsrfTokenHtml}}
{{if .Flash}}
<div class="sixteen wide column">
@@ -35,7 +35,7 @@
{{template "repo/issue/comment_tab" .}}
{{end}}
<div class="text right">
- <button class="ui green button loading-button" tabindex="6">
+ <button class="ui green button" tabindex="6">
{{if .PageIsComparePull}}
{{.locale.Tr "repo.pulls.create"}}
{{else}}
diff --git a/templates/repo/issue/view_content.tmpl b/templates/repo/issue/view_content.tmpl
index e451db16f0..dc2754d5c6 100644
--- a/templates/repo/issue/view_content.tmpl
+++ b/templates/repo/issue/view_content.tmpl
@@ -96,15 +96,14 @@
{{avatar $.Context .SignedUser 40}}
</a>
<div class="content">
- <form class="ui segment form" id="comment-form" action="{{$.RepoLink}}/issues/{{.Issue.Index}}/comments" method="post">
+ <form class="ui segment form form-fetch-action" id="comment-form" action="{{$.RepoLink}}/issues/{{.Issue.Index}}/comments" method="post">
{{template "repo/issue/comment_tab" .}}
{{.CsrfTokenHtml}}
- <input id="status" name="status" type="hidden">
<div class="field footer">
<div class="text right">
{{if and (or .HasIssuesOrPullsWritePermission .IsIssuePoster) (not .DisableStatusChange)}}
{{if .Issue.IsClosed}}
- <button id="status-button" class="ui green basic button" tabindex="6" data-status="{{.locale.Tr "repo.issues.reopen_issue"}}" data-status-and-comment="{{.locale.Tr "repo.issues.reopen_comment_issue"}}" data-status-val="reopen">
+ <button id="status-button" class="ui green basic button" tabindex="6" data-status="{{.locale.Tr "repo.issues.reopen_issue"}}" data-status-and-comment="{{.locale.Tr "repo.issues.reopen_comment_issue"}}" name="status" value="reopen">
{{.locale.Tr "repo.issues.reopen_issue"}}
</button>
{{else}}
@@ -112,12 +111,12 @@
{{if .Issue.IsPull}}
{{$closeTranslationKey = "repo.pulls.close"}}
{{end}}
- <button id="status-button" class="ui red basic button" tabindex="6" data-status="{{.locale.Tr $closeTranslationKey}}" data-status-and-comment="{{.locale.Tr "repo.issues.close_comment_issue"}}" data-status-val="close">
+ <button id="status-button" class="ui red basic button" tabindex="6" data-status="{{.locale.Tr $closeTranslationKey}}" data-status-and-comment="{{.locale.Tr "repo.issues.close_comment_issue"}}" name="status" value="close">
{{.locale.Tr $closeTranslationKey}}
</button>
{{end}}
{{end}}
- <button class="ui green button loading-button" tabindex="5">
+ <button class="ui green button" tabindex="5">
{{.locale.Tr "repo.issues.create_comment"}}
</button>
</div>
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'));
};