Skip to content
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

Update HTTP status codes #18063

Merged
merged 20 commits into from
Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/web_letsencrypt.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,5 +95,5 @@ func runLetsEncryptFallbackHandler(w http.ResponseWriter, r *http.Request) {
// URI always contains a leading slash, which would result in a double
// slash
target := strings.TrimSuffix(setting.AppURL, "/") + r.URL.RequestURI()
http.Redirect(w, r, target, http.StatusFound)
http.Redirect(w, r, target, http.StatusTemporaryRedirect)
}
2 changes: 1 addition & 1 deletion modules/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ func (ctx *Context) JSON(status int, content interface{}) {

// Redirect redirects the request
func (ctx *Context) Redirect(location string, status ...int) {
code := http.StatusFound
code := http.StatusTemporaryRedirect
if len(status) == 1 {
code = status[0]
}
Expand Down
2 changes: 1 addition & 1 deletion modules/lfs/http_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func lfsTestRoundtripHandler(req *http.Request) *http.Response {
Objects: []*ObjectResponse{
{
Error: &ObjectError{
Code: 404,
Code: http.StatusNotFound,
Message: "Object not found",
},
},
Expand Down
2 changes: 1 addition & 1 deletion modules/private/restore_repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func RestoreRepo(ctx context.Context, repoDir, ownerName, repoName string, units
}
defer resp.Body.Close()

if resp.StatusCode != 200 {
if resp.StatusCode != http.StatusOK {
var ret = struct {
Err string `json:"err"`
}{}
Expand Down
2 changes: 1 addition & 1 deletion modules/web/route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func TestRoute2(t *testing.T) {
route = 1
})
}, func(resp http.ResponseWriter, req *http.Request) {
resp.WriteHeader(200)
resp.WriteHeader(http.StatusOK)
})

r.Group("/issues/{index}", func() {
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/org/member.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func IsMember(ctx *context.APIContext) {
}

redirectURL := setting.AppSubURL + "/api/v1/orgs/" + url.PathEscape(ctx.Org.Organization.Name) + "/public_members/" + url.PathEscape(userToCheck.Name)
ctx.Redirect(redirectURL, 302)
ctx.Redirect(redirectURL, http.StatusTemporaryRedirect)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a sense, this is breaking as this will return a different status code now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it might break some old HTTP clients, IMO these old HTTP clients should upgrade themselves to support modern HTTP status codes. 😀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a practical tool really check "if I send request x it should always be redirected"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it might break some old HTTP clients, IMO these old HTTP clients should upgrade themselves to support modern HTTP status codes. grinning

Well not only that, certain tools built for gitea could have a check that expects a specific statusCode, e.g. 302

Does a practical tool really check "if I send request x it should always be redirected"?

In a certain sense they work the same, but from experience, tools can differ in the implementation and the behavior as the specs aren't clear on it.

}

// IsPublicMember check if a user is a public member of an organization
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/issue_tracked_time.go
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ func ResetIssueTime(ctx *context.APIContext) {
}
return
}
ctx.Status(204)
ctx.Status(http.StatusNoContent)
}

// DeleteTime delete a specific time by id
Expand Down
4 changes: 2 additions & 2 deletions routers/common/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ func Middlewares() []func(http.Handler) http.Handler {
combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, string(log.Stack(2)))
log.Error("%v", combinedErr)
if setting.IsProd {
http.Error(resp, http.StatusText(500), 500)
http.Error(resp, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
} else {
http.Error(resp, combinedErr, 500)
http.Error(resp, combinedErr, http.StatusInternalServerError)
}
}
}()
Expand Down
2 changes: 1 addition & 1 deletion routers/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func Init(next http.Handler) http.Handler {
return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
if setting.InstallLock {
resp.Header().Add("Refresh", "1; url="+setting.AppURL+"user/login")
_ = rnd.HTML(resp, 200, string(tplPostInstall), nil)
_ = rnd.HTML(resp, http.StatusOK, string(tplPostInstall), nil)
return
}
var locale = middleware.Locale(resp, req)
Expand Down
8 changes: 4 additions & 4 deletions routers/install/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ func installRecovery() func(next http.Handler) http.Handler {
combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, string(log.Stack(2)))
log.Error(combinedErr)
if setting.IsProd {
http.Error(w, http.StatusText(500), 500)
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
} else {
http.Error(w, combinedErr, 500)
http.Error(w, combinedErr, http.StatusInternalServerError)
}
}
}()
Expand All @@ -66,7 +66,7 @@ func installRecovery() func(next http.Handler) http.Handler {
if !setting.IsProd {
store["ErrorMsg"] = combinedErr
}
err = rnd.HTML(w, 500, "status/500", templates.BaseVars().Merge(store))
err = rnd.HTML(w, http.StatusInternalServerError, "status/500", templates.BaseVars().Merge(store))
if err != nil {
log.Error("%v", err)
}
Expand Down Expand Up @@ -107,7 +107,7 @@ func Routes() *web.Route {
r.Get("/", Install)
r.Post("/", web.Bind(forms.InstallForm{}), SubmitInstall)
r.NotFound(func(w http.ResponseWriter, req *http.Request) {
http.Redirect(w, req, setting.AppURL, http.StatusFound)
http.Redirect(w, req, setting.AppURL, http.StatusTemporaryRedirect)
})
return r
}
10 changes: 5 additions & 5 deletions routers/web/admin/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ func Queue(ctx *context.Context) {
qid := ctx.ParamsInt64("qid")
mq := queue.GetManager().GetManagedQueue(qid)
if mq == nil {
ctx.Status(404)
ctx.Status(http.StatusNotFound)
return
}
ctx.Data["Title"] = ctx.Tr("admin.monitor.queue", mq.Name)
Expand All @@ -361,7 +361,7 @@ func WorkerCancel(ctx *context.Context) {
qid := ctx.ParamsInt64("qid")
mq := queue.GetManager().GetManagedQueue(qid)
if mq == nil {
ctx.Status(404)
ctx.Status(http.StatusNotFound)
return
}
pid := ctx.ParamsInt64("pid")
Expand All @@ -377,7 +377,7 @@ func Flush(ctx *context.Context) {
qid := ctx.ParamsInt64("qid")
mq := queue.GetManager().GetManagedQueue(qid)
if mq == nil {
ctx.Status(404)
ctx.Status(http.StatusNotFound)
return
}
timeout, err := time.ParseDuration(ctx.FormString("timeout"))
Expand All @@ -399,7 +399,7 @@ func AddWorkers(ctx *context.Context) {
qid := ctx.ParamsInt64("qid")
mq := queue.GetManager().GetManagedQueue(qid)
if mq == nil {
ctx.Status(404)
ctx.Status(http.StatusNotFound)
return
}
number := ctx.FormInt("number")
Expand Down Expand Up @@ -429,7 +429,7 @@ func SetQueueSettings(ctx *context.Context) {
qid := ctx.ParamsInt64("qid")
mq := queue.GetManager().GetManagedQueue(qid)
if mq == nil {
ctx.Status(404)
ctx.Status(http.StatusNotFound)
return
}
if _, ok := mq.Managed.(queue.ManagedPool); !ok {
Expand Down
4 changes: 2 additions & 2 deletions routers/web/admin/notice.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ func DeleteNotices(ctx *context.Context) {

if err := admin_model.DeleteNoticesByIDs(ids); err != nil {
ctx.Flash.Error("DeleteNoticesByIDs: " + err.Error())
ctx.Status(500)
ctx.Status(http.StatusInternalServerError)
} else {
ctx.Flash.Success(ctx.Tr("admin.notices.delete_success"))
ctx.Status(200)
ctx.Status(http.StatusOK)
}
}

Expand Down
16 changes: 8 additions & 8 deletions routers/web/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,18 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
if err != nil {
if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) {
log.Warn("Unable to find %s %s", prefix, rPath)
http.Error(w, "file not found", 404)
http.Error(w, "file not found", http.StatusNotFound)
return
}
log.Error("Error whilst getting URL for %s %s. Error: %v", prefix, rPath, err)
http.Error(w, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath), 500)
http.Error(w, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath), http.StatusInternalServerError)
return
}
http.Redirect(
w,
req,
u.String(),
301,
http.StatusPermanentRedirect,
)
})
}
Expand All @@ -77,7 +77,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
rPath := strings.TrimPrefix(req.URL.EscapedPath(), "/"+prefix+"/")
rPath = strings.TrimPrefix(rPath, "/")
if rPath == "" {
http.Error(w, "file not found", 404)
http.Error(w, "file not found", http.StatusNotFound)
return
}
rPath = path.Clean("/" + filepath.ToSlash(rPath))
Expand All @@ -93,19 +93,19 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
if err != nil {
if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) {
log.Warn("Unable to find %s %s", prefix, rPath)
http.Error(w, "file not found", 404)
http.Error(w, "file not found", http.StatusNotFound)
return
}
log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err)
http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), 500)
http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError)
return
}
defer fr.Close()

_, err = io.Copy(w, fr)
if err != nil {
log.Error("Error whilst rendering %s %s. Error: %v", prefix, rPath, err)
http.Error(w, fmt.Sprintf("Error whilst rendering %s %s", prefix, rPath), 500)
http.Error(w, fmt.Sprintf("Error whilst rendering %s %s", prefix, rPath), http.StatusInternalServerError)
return
}
})
Expand Down Expand Up @@ -159,7 +159,7 @@ func Recovery() func(next http.Handler) http.Handler {
if !setting.IsProd {
store["ErrorMsg"] = combinedErr
}
err = rnd.HTML(w, 500, "status/500", templates.BaseVars().Merge(store))
err = rnd.HTML(w, http.StatusInternalServerError, "status/500", templates.BaseVars().Merge(store))
if err != nil {
log.Error("%v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion routers/web/explore/code.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const (
// Code render explore code page
func Code(ctx *context.Context) {
if !setting.Indexer.RepoIndexerEnabled {
ctx.Redirect(setting.AppSubURL+"/explore", 302)
ctx.Redirect(setting.AppSubURL+"/explore", http.StatusTemporaryRedirect)
return
}

Expand Down
2 changes: 1 addition & 1 deletion routers/web/goget.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func goGet(ctx *context.Context) {
</body>
</html>
`))
ctx.Status(400)
ctx.Status(http.StatusBadRequest)
return
}
branchName := setting.Repository.DefaultBranch
Expand Down
4 changes: 2 additions & 2 deletions routers/web/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ func Metrics(resp http.ResponseWriter, req *http.Request) {
}
header := req.Header.Get("Authorization")
if header == "" {
http.Error(resp, "", 401)
http.Error(resp, "", http.StatusUnauthorized)
return
}
got := []byte(header)
want := []byte("Bearer " + setting.Metrics.Token)
if subtle.ConstantTimeCompare(got, want) != 1 {
http.Error(resp, "", 401)
http.Error(resp, "", http.StatusUnauthorized)
return
}
promhttp.Handler().ServeHTTP(resp, req)
Expand Down
4 changes: 2 additions & 2 deletions routers/web/repo/editor.go
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ func UploadFileToServer(ctx *context.Context) {
func RemoveUploadFileFromServer(ctx *context.Context) {
form := web.GetForm(ctx).(*forms.RemoveUploadFileForm)
if len(form.File) == 0 {
ctx.Status(204)
ctx.Status(http.StatusNoContent)
return
}

Expand All @@ -795,7 +795,7 @@ func RemoveUploadFileFromServer(ctx *context.Context) {
}

log.Trace("Upload file removed: %s", form.File)
ctx.Status(204)
ctx.Status(http.StatusNoContent)
}

// GetUniquePatchBranchName Gets a unique branch name for a new patch branch
Expand Down
16 changes: 8 additions & 8 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1886,7 +1886,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {

// TODO: Not support 'clear' now
if action != "attach" && action != "detach" {
ctx.Status(403)
ctx.Status(http.StatusForbidden)
return
}

Expand All @@ -1901,7 +1901,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
"UpdatePullReviewRequest: refusing to add review request for non-PR issue %-v#%d",
issue.Repo, issue.Index,
)
ctx.Status(403)
ctx.Status(http.StatusForbidden)
return
}
if reviewID < 0 {
Expand All @@ -1916,7 +1916,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
"UpdatePullReviewRequest: refusing to add team review request for %s#%d owned by non organization UID[%d]",
issue.Repo.FullName(), issue.Index, issue.Repo.ID,
)
ctx.Status(403)
ctx.Status(http.StatusForbidden)
return
}

Expand All @@ -1930,7 +1930,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
log.Warn(
"UpdatePullReviewRequest: refusing to add team review request for UID[%d] team %s to %s#%d owned by UID[%d]",
team.OrgID, team.Name, issue.Repo.FullName(), issue.Index, issue.Repo.ID)
ctx.Status(403)
ctx.Status(http.StatusForbidden)
return
}

Expand All @@ -1942,7 +1942,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
team.OrgID, team.Name, issue.Repo.FullName(), issue.Index, issue.Repo.ID,
err,
)
ctx.Status(403)
ctx.Status(http.StatusForbidden)
return
}
ctx.ServerError("IsValidTeamReviewRequest", err)
Expand All @@ -1965,7 +1965,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
reviewID, issue.Repo, issue.Index,
err,
)
ctx.Status(403)
ctx.Status(http.StatusForbidden)
return
}
ctx.ServerError("GetUserByID", err)
Expand All @@ -1980,7 +1980,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
reviewer, issue.Repo, issue.Index,
err,
)
ctx.Status(403)
ctx.Status(http.StatusForbidden)
return
}
ctx.ServerError("isValidReviewRequest", err)
Expand Down Expand Up @@ -2261,7 +2261,7 @@ func DeleteComment(ctx *context.Context) {
return
}

ctx.Status(200)
ctx.Status(http.StatusOK)
}

// ChangeIssueReaction create a reaction for issue
Expand Down
6 changes: 3 additions & 3 deletions routers/web/repo/issue_label_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestInitializeLabels(t *testing.T) {
test.LoadRepo(t, ctx, 2)
web.SetForm(ctx, &forms.InitializeLabelsForm{TemplateName: "Default"})
InitializeLabels(ctx)
assert.EqualValues(t, http.StatusFound, ctx.Resp.Status())
assert.EqualValues(t, http.StatusTemporaryRedirect, ctx.Resp.Status())
unittest.AssertExistsAndLoadBean(t, &models.Label{
RepoID: 2,
Name: "enhancement",
Expand Down Expand Up @@ -82,7 +82,7 @@ func TestNewLabel(t *testing.T) {
Color: "#abcdef",
})
NewLabel(ctx)
assert.EqualValues(t, http.StatusFound, ctx.Resp.Status())
assert.EqualValues(t, http.StatusTemporaryRedirect, ctx.Resp.Status())
unittest.AssertExistsAndLoadBean(t, &models.Label{
Name: "newlabel",
Color: "#abcdef",
Expand All @@ -101,7 +101,7 @@ func TestUpdateLabel(t *testing.T) {
Color: "#abcdef",
})
UpdateLabel(ctx)
assert.EqualValues(t, http.StatusFound, ctx.Resp.Status())
assert.EqualValues(t, http.StatusTemporaryRedirect, ctx.Resp.Status())
unittest.AssertExistsAndLoadBean(t, &models.Label{
ID: 2,
Name: "newnameforlabel",
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,7 @@ func TriggerTask(ctx *context.Context) {
log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name)

go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, "", "")
ctx.Status(202)
ctx.Status(http.StatusAccepted)
}

// CleanUpPullRequest responses for delete merged branch when PR has been merged
Expand Down
Loading