Skip to content

Commit 15a6400

Browse files
wxiaoguangwxiaoguang
committed
fix
1 parent 3afc3e4 commit 15a6400

File tree

11 files changed

+140
-50
lines changed

11 files changed

+140
-50
lines changed

‎modules/context/base.go

+4
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,10 @@ func (b *Base) JSONRedirect(redirect string) {
136136
b.JSON(http.StatusOK, map[string]any{"redirect": redirect})
137137
}
138138

139+
func (b *Base) JSONError(msg string) {
140+
b.JSON(http.StatusBadRequest, map[string]any{"errorMessage": msg})
141+
}
142+
139143
// RemoteAddr returns the client machine ip address
140144
func (b *Base) RemoteAddr() string {
141145
return b.Req.RemoteAddr

‎modules/context/context_response.go

+2-8
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616

1717
user_model "code.gitea.io/gitea/models/user"
1818
"code.gitea.io/gitea/modules/base"
19+
"code.gitea.io/gitea/modules/httplib"
1920
"code.gitea.io/gitea/modules/log"
2021
"code.gitea.io/gitea/modules/setting"
2122
"code.gitea.io/gitea/modules/templates"
@@ -49,14 +50,7 @@ func (ctx *Context) RedirectToFirst(location ...string) {
4950
continue
5051
}
5152

52-
// Unfortunately browsers consider a redirect Location with preceding "//", "\\" and "/\" as meaning redirect to "http(s)://REST_OF_PATH"
53-
// Therefore we should ignore these redirect locations to prevent open redirects
54-
if len(loc) > 1 && (loc[0] == '/' || loc[0] == '\\') && (loc[1] == '/' || loc[1] == '\\') {
55-
continue
56-
}
57-
58-
u, err := url.Parse(loc)
59-
if err != nil || ((u.Scheme != "" || u.Host != "") && !strings.HasPrefix(strings.ToLower(loc), strings.ToLower(setting.AppURL))) {
53+
if httplib.IsRiskyRedirectURL(loc) {
6054
continue
6155
}
6256

‎modules/httplib/url.go

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// Copyright 2023 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package httplib
5+
6+
import (
7+
"net/url"
8+
"strings"
9+
10+
"code.gitea.io/gitea/modules/setting"
11+
)
12+
13+
// IsRiskyRedirectURL returns true if the URL is considered risky for redirects
14+
func IsRiskyRedirectURL(s string) bool {
15+
// Unfortunately browsers consider a redirect Location with preceding "//", "\\", "/\" and "\/" as meaning redirect to "http(s)://REST_OF_PATH"
16+
// Therefore we should ignore these redirect locations to prevent open redirects
17+
if len(s) > 1 && (s[0] == '/' || s[0] == '\\') && (s[1] == '/' || s[1] == '\\') {
18+
return true
19+
}
20+
21+
u, err := url.Parse(s)
22+
if err != nil || ((u.Scheme != "" || u.Host != "") && !strings.HasPrefix(strings.ToLower(s), strings.ToLower(setting.AppURL))) {
23+
return true
24+
}
25+
26+
return false
27+
}

‎modules/httplib/url_test.go

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// Copyright 2023 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package httplib
5+
6+
import (
7+
"testing"
8+
9+
"code.gitea.io/gitea/modules/setting"
10+
11+
"github.com/stretchr/testify/assert"
12+
)
13+
14+
func TestIsRiskyRedirectURL(t *testing.T) {
15+
setting.AppURL = "http://localhost:3000/"
16+
tests := []struct {
17+
input string
18+
want bool
19+
}{
20+
{"empty", false},
21+
{"/", false},
22+
23+
{"//", true},
24+
{"\\\\", true},
25+
{"/\\", true},
26+
{"\\/", true},
27+
{"mail:[email protected]", true},
28+
{"https://test.com", true},
29+
{setting.AppURL + "/foo", false},
30+
}
31+
for _, tt := range tests {
32+
t.Run(tt.input, func(t *testing.T) {
33+
assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input))
34+
})
35+
}
36+
}

‎routers/common/redirect.go

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// Copyright 2023 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package common
5+
6+
import (
7+
"net/http"
8+
9+
"code.gitea.io/gitea/modules/httplib"
10+
)
11+
12+
// FetchRedirectDelegate helps the "fetch" requests to redirect to the correct location
13+
func FetchRedirectDelegate(resp http.ResponseWriter, req *http.Request) {
14+
// When use "fetch" to post requests and the response is a redirect, browser's "location.href = uri" has limitations.
15+
// 1. change "location" from old "/foo" to new "/foo#hash", the browser will not reload the page.
16+
// 2. when use "window.reload()", the hash is not respected, the newly loaded page won't scroll to the hash target.
17+
// The typical page is "issue comment" page. The backend responds "/owner/repo/issues/1#comment-2",
18+
// then frontend needs this delegate to redirect to the new location with hash correctly.
19+
redirect := req.PostFormValue("redirect")
20+
if httplib.IsRiskyRedirectURL(redirect) {
21+
resp.WriteHeader(http.StatusBadRequest)
22+
return
23+
}
24+
resp.Header().Add("Location", redirect)
25+
resp.WriteHeader(http.StatusSeeOther)
26+
}

‎routers/init.go

+2
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,8 @@ func NormalRoutes(ctx context.Context) *web.Route {
183183
r.Mount("/api/v1", apiv1.Routes(ctx))
184184
r.Mount("/api/internal", private.Routes())
185185

186+
r.Post("/-/fetch-redirect", common.FetchRedirectDelegate)
187+
186188
if setting.Packages.Enabled {
187189
// This implements package support for most package managers
188190
r.Mount("/api/packages", packages_router.CommonRoutes(ctx))

‎routers/web/repo/issue.go

+12-19
Original file line numberDiff line numberDiff line change
@@ -1134,12 +1134,12 @@ func NewIssuePost(ctx *context.Context) {
11341134
}
11351135

11361136
if ctx.HasError() {
1137-
ctx.HTML(http.StatusOK, tplIssueNew)
1137+
ctx.JSONError(ctx.GetErrMsg())
11381138
return
11391139
}
11401140

11411141
if util.IsEmptyString(form.Title) {
1142-
ctx.RenderWithErr(ctx.Tr("repo.issues.new.title_empty"), tplIssueNew, form)
1142+
ctx.JSONError(ctx.Tr("repo.issues.new.title_empty"))
11431143
return
11441144
}
11451145

@@ -1184,9 +1184,9 @@ func NewIssuePost(ctx *context.Context) {
11841184

11851185
log.Trace("Issue created: %d/%d", repo.ID, issue.ID)
11861186
if ctx.FormString("redirect_after_creation") == "project" && projectID > 0 {
1187-
ctx.Redirect(ctx.Repo.RepoLink + "/projects/" + strconv.FormatInt(projectID, 10))
1187+
ctx.JSONRedirect(ctx.Repo.RepoLink + "/projects/" + strconv.FormatInt(projectID, 10))
11881188
} else {
1189-
ctx.Redirect(issue.Link())
1189+
ctx.JSONRedirect(issue.Link())
11901190
}
11911191
}
11921192

@@ -2777,8 +2777,7 @@ func NewComment(ctx *context.Context) {
27772777
}
27782778

27792779
if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin {
2780-
ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked"))
2781-
ctx.Redirect(issue.Link())
2780+
ctx.JSONError(ctx.Tr("repo.issues.comment_on_locked"))
27822781
return
27832782
}
27842783

@@ -2788,8 +2787,7 @@ func NewComment(ctx *context.Context) {
27882787
}
27892788

27902789
if ctx.HasError() {
2791-
ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
2792-
ctx.Redirect(issue.Link())
2790+
ctx.JSONError(ctx.Data["ErrorMsg"].(string))
27932791
return
27942792
}
27952793

@@ -2809,8 +2807,7 @@ func NewComment(ctx *context.Context) {
28092807
pr, err = issues_model.GetUnmergedPullRequest(ctx, pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch, pull.Flow)
28102808
if err != nil {
28112809
if !issues_model.IsErrPullRequestNotExist(err) {
2812-
ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
2813-
ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pull.Index))
2810+
ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
28142811
return
28152812
}
28162813
}
@@ -2841,8 +2838,7 @@ func NewComment(ctx *context.Context) {
28412838
}
28422839
if ok := git.IsBranchExist(ctx, pull.HeadRepo.RepoPath(), pull.BaseBranch); !ok {
28432840
// todo localize
2844-
ctx.Flash.Error("The origin branch is delete, cannot reopen.")
2845-
ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pull.Index))
2841+
ctx.JSONError("The origin branch is delete, cannot reopen.")
28462842
return
28472843
}
28482844
headBranchRef := pull.GetGitHeadBranchRefName()
@@ -2882,11 +2878,9 @@ func NewComment(ctx *context.Context) {
28822878

28832879
if issues_model.IsErrDependenciesLeft(err) {
28842880
if issue.IsPull {
2885-
ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
2886-
ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index))
2881+
ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
28872882
} else {
2888-
ctx.Flash.Error(ctx.Tr("repo.issues.dependency.issue_close_blocked"))
2889-
ctx.Redirect(fmt.Sprintf("%s/issues/%d", ctx.Repo.RepoLink, issue.Index))
2883+
ctx.JSONError(ctx.Tr("repo.issues.dependency.issue_close_blocked"))
28902884
}
28912885
return
28922886
}
@@ -2899,7 +2893,6 @@ func NewComment(ctx *context.Context) {
28992893
log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
29002894
}
29012895
}
2902-
29032896
}
29042897

29052898
// Redirect to comment hashtag if there is any actual content.
@@ -2908,9 +2901,9 @@ func NewComment(ctx *context.Context) {
29082901
typeName = "pulls"
29092902
}
29102903
if comment != nil {
2911-
ctx.Redirect(fmt.Sprintf("%s/%s/%d#%s", ctx.Repo.RepoLink, typeName, issue.Index, comment.HashTag()))
2904+
ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d#%s", ctx.Repo.RepoLink, typeName, issue.Index, comment.HashTag()))
29122905
} else {
2913-
ctx.Redirect(fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, typeName, issue.Index))
2906+
ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, typeName, issue.Index))
29142907
}
29152908
}()
29162909

‎templates/repo/issue/new_form.tmpl

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<form class="issue-content ui comment form" id="new-issue" action="{{.Link}}" method="post">
1+
<form class="issue-content ui comment form form-fetch-action" id="new-issue" action="{{.Link}}" method="post">
22
{{.CsrfTokenHtml}}
33
{{if .Flash}}
44
<div class="sixteen wide column">
@@ -35,7 +35,7 @@
3535
{{template "repo/issue/comment_tab" .}}
3636
{{end}}
3737
<div class="text right">
38-
<button class="ui green button loading-button" tabindex="6">
38+
<button class="ui green button" tabindex="6">
3939
{{if .PageIsComparePull}}
4040
{{.locale.Tr "repo.pulls.create"}}
4141
{{else}}

‎templates/repo/issue/view_content.tmpl

+4-5
Original file line numberDiff line numberDiff line change
@@ -96,28 +96,27 @@
9696
{{avatar $.Context .SignedUser 40}}
9797
</a>
9898
<div class="content">
99-
<form class="ui segment form" id="comment-form" action="{{$.RepoLink}}/issues/{{.Issue.Index}}/comments" method="post">
99+
<form class="ui segment form form-fetch-action" id="comment-form" action="{{$.RepoLink}}/issues/{{.Issue.Index}}/comments" method="post">
100100
{{template "repo/issue/comment_tab" .}}
101101
{{.CsrfTokenHtml}}
102-
<input id="status" name="status" type="hidden">
103102
<div class="field footer">
104103
<div class="text right">
105104
{{if and (or .HasIssuesOrPullsWritePermission .IsIssuePoster) (not .DisableStatusChange)}}
106105
{{if .Issue.IsClosed}}
107-
<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">
106+
<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">
108107
{{.locale.Tr "repo.issues.reopen_issue"}}
109108
</button>
110109
{{else}}
111110
{{$closeTranslationKey := "repo.issues.close"}}
112111
{{if .Issue.IsPull}}
113112
{{$closeTranslationKey = "repo.pulls.close"}}
114113
{{end}}
115-
<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">
114+
<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">
116115
{{.locale.Tr $closeTranslationKey}}
117116
</button>
118117
{{end}}
119118
{{end}}
120-
<button class="ui green button loading-button" tabindex="5">
119+
<button class="ui green button" tabindex="5">
121120
{{.locale.Tr "repo.issues.create_comment"}}
122121
</button>
123122
</div>

‎web_src/js/features/common-global.js

+25-11
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {hideElem, showElem, toggleElem} from '../utils/dom.js';
99
import {htmlEscape} from 'escape-goat';
1010
import {createTippy} from '../modules/tippy.js';
1111

12-
const {appUrl, csrfToken, i18n} = window.config;
12+
const {appUrl, appSubUrl, csrfToken, i18n} = window.config;
1313

1414
export function initGlobalFormDirtyLeaveConfirm() {
1515
// Warn users that try to leave a page after entering data into a form.
@@ -61,6 +61,21 @@ export function initGlobalButtonClickOnEnter() {
6161
});
6262
}
6363

64+
// doRedirect does real redirection to bypass the browser's limitations of "location"
65+
// more details are in the backend's fetch-redirect handler
66+
function doRedirect(redirect) {
67+
const form = document.createElement('form');
68+
const input = document.createElement('input');
69+
form.method = 'post';
70+
form.action = `${appSubUrl}/-/fetch-redirect`;
71+
input.type = 'hidden';
72+
input.name = 'redirect';
73+
input.value = redirect;
74+
form.append(input);
75+
document.body.append(form);
76+
form.submit();
77+
}
78+
6479
async function formFetchAction(e) {
6580
if (!e.target.classList.contains('form-fetch-action')) return;
6681

@@ -101,6 +116,7 @@ async function formFetchAction(e) {
101116
const onError = (msg) => {
102117
formEl.classList.remove('is-loading', 'small-loading-icon');
103118
if (errorTippy) errorTippy.destroy();
119+
// TODO: use a better toast UI instead of the tippy. If the form height is large, the tippy position is not good
104120
errorTippy = createTippy(formEl, {
105121
content: msg,
106122
interactive: true,
@@ -120,15 +136,21 @@ async function formFetchAction(e) {
120136
const {redirect} = await resp.json();
121137
formEl.classList.remove('dirty'); // remove the areYouSure check before reloading
122138
if (redirect) {
123-
window.location.href = redirect;
139+
doRedirect(redirect);
124140
} else {
125141
window.location.reload();
126142
}
143+
} else if (resp.status >= 400 && resp.status < 500) {
144+
const data = await resp.json();
145+
// the code was quite messy, sometimes the backend uses "err", sometimes it uses "error", and even "user_error"
146+
// but at the moment, as a new approach, we only use "errorMessage" here, backend can use JSONError() to respond.
147+
onError(data.errorMessage || `server error: ${resp.status}`);
127148
} else {
128149
onError(`server error: ${resp.status}`);
129150
}
130151
} catch (e) {
131-
onError(e.error);
152+
console.error('error when doRequest', e);
153+
onError(i18n.network_error);
132154
}
133155
};
134156

@@ -183,14 +205,6 @@ export function initGlobalCommon() {
183205

184206
$('.tabular.menu .item').tab();
185207

186-
// prevent multiple form submissions on forms containing .loading-button
187-
document.addEventListener('submit', (e) => {
188-
const btn = e.target.querySelector('.loading-button');
189-
if (!btn) return;
190-
if (btn.classList.contains('loading')) return e.preventDefault();
191-
btn.classList.add('loading');
192-
});
193-
194208
document.addEventListener('submit', formFetchAction);
195209
}
196210

‎web_src/js/features/repo-issue.js

-5
Original file line numberDiff line numberDiff line change
@@ -636,11 +636,6 @@ export function initSingleCommentEditor($commentForm) {
636636
const opts = {};
637637
const $statusButton = $('#status-button');
638638
if ($statusButton.length) {
639-
$statusButton.on('click', (e) => {
640-
e.preventDefault();
641-
$('#status').val($statusButton.data('status-val'));
642-
$('#comment-form').trigger('submit');
643-
});
644639
opts.onContentChanged = (editor) => {
645640
$statusButton.text($statusButton.attr(editor.value().trim() ? 'data-status-and-comment' : 'data-status'));
646641
};

0 commit comments

Comments
 (0)