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.

<details>


![image](https://github.com/go-gitea/gitea/assets/2114189/c94ba958-aa46-4747-8ddf-6584deeed25c)

</details>

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

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"
+		if httplib.IsRiskyRedirectURL(loc) {
-		// 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))) {
 			continue
 		}
 

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
+}

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))
+		})
+	}
+}

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 {

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)
+}

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))

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.JSONError(ctx.Tr("repo.issues.comment_on_locked"))
-		ctx.Redirect(issue.Link())
 		return
 	}
 
@@ -2788,8 +2787,7 @@ func NewComment(ctx *context.Context) {
 	}
 
 	if ctx.HasError() {
-		ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
+		ctx.JSONError(ctx.GetErrMsg())
-		ctx.Redirect(issue.Link())
 		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.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
-						ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pull.Index))
 						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.JSONError("The origin branch is delete, cannot reopen.")
-					ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pull.Index))
 					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.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
-							ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index))
 						} else {
-							ctx.Flash.Error(ctx.Tr("repo.issues.dependency.issue_close_blocked"))
+							ctx.JSONError(ctx.Tr("repo.issues.dependency.issue_close_blocked"))
-							ctx.Redirect(fmt.Sprintf("%s/issues/%d", ctx.Repo.RepoLink, issue.Index))
 						}
 						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))
 		}
 	}()
 

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}}

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>

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

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)

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);
 }
 

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'));
     };