Update HTTP status codes #18063

KN4CK3R · 2021-12-21T22:22:27Z

xxx -> http.Status...
http.StatusFound -> http.StatusTemporaryRedirect
http.StatusMovedPermanently -> http.StatusPermanentRedirect

From MDN:

Even if the specification requires the method (and the body) not to be altered when the redirection is performed, not all user-agents conform here - you can still find this type of bugged software out there. It is therefore recommended to set the 302 code only as a response for GET or HEAD methods and to use 307 Temporary Redirect instead, as the method change is explicitly prohibited in that case.

The only difference between 307 and 302 is that 307 guarantees that the method and the body will not be changed when the redirected request is made. With 302, some old clients were incorrectly changing the method to GET: the behavior with non-GET methods and 302 is then unpredictable on the Web, whereas the behavior with 307 is predictable. For GET requests, their behavior is identical.

Gusted

Hmm, changing status code could break some existing tools out for Gitea which could expect a specific status code.

Gusted · 2021-12-21T23:15:47Z

routers/api/v1/org/member.go

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


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

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

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

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.

routers/install/routes.go

…r-status-code

zeripath · 2021-12-22T16:18:58Z

@silverwind are we ok with turning these StatusFounds into StatusTemporaryRedirect ?

silverwind · 2021-12-22T16:43:59Z

We need to check that our form submissions that end up being redirected still load the page correctly after this change because with 307/308 browsers will not change method to GET like before with 302/303 in response to non-GET requests.

KN4CK3R · 2021-12-22T17:26:58Z

Just a note: Browsers are not supposed to change the method to GET with 302.

wxiaoguang · 2021-12-22T17:35:25Z

We need to check that our form submissions that end up being redirected still load the page correctly after this change because with 307/308 browsers will not change method to GET like before with 302/303 in response to non-GET requests.

Yep, we might keep 302 for login forms / logout request, etc, since they are fine now.

Maybe we can keep most 301/302, and only use 307/308 if there is really a necessary case.

For most cases, 301/302 are still correct and won't bring problems.

KN4CK3R · 2021-12-22T17:44:31Z

Here is a quick check per file (tests excluded):

file	note
routers/api/v1/org/member.go	GET only
routers/install/routes.go	NotFoundHandler, may be every method and should be a 303
routers/web/base.go	GET
routers/web/explore/code.go	GET
routers/web/repo/search.go	GET
routers/web/user/oauth.go	POST and the redirects should be GET. in that case 303 should be used and not 302/307
routers/web/web.go	GET

I think most of the time we used 302 it should more likely be a 303 to enforce the use of GET.

silverwind · 2021-12-22T17:46:41Z

Browsers are not supposed to change the method to GET with 302

True, the spec forbids a method change during 302 redirection, but I'm note sure what the current status of this behavior is in modern browsers. MDN refers to them as "bugged software" while Wikipedia says "popular browsers", so it might just be that this behaviour was already corrected and we don't need to worry about it.

303 redirection on the other hand seems to be specced to change method to GET, so if we have cases where we want this method change, we should use 303.

KN4CK3R · 2021-12-22T17:55:03Z

Yes, our OAuth handling is wrong as per spec

If an HTTP redirection (and not, for example, JavaScript) is used for such a request, AS SHOULD use HTTP status code 303 "See Other".

We use 302 at the moment. The NotFound handler should become 303 too because it redirects all requests to the root url.

silverwind · 2021-12-23T16:34:08Z

Agree, we should remove all usage of 302 and replace it with either

303 See Other (method change to GET required, caching allowed)
307 Temporary Redirect (method change forbidden, caching forbidden)
308 Permanent Redirect (method change forbidden, caching allowed)

API should probably exclusively use 307 and 308, while form submissions may need 303 to work.

…r-status-code

KN4CK3R · 2021-12-29T21:38:13Z

I created a table with all redirect calls which contains a reason for every change. I have added a comment if I was not sure or there is something else. Please have a look at it.

redirect.ods

…r-status-code

silverwind · 2022-01-26T17:31:50Z

Is this ready?

KN4CK3R · 2022-01-26T22:11:16Z

Yes.

singuliere

Since this is a breaking change, this PR should have a detailed explanation in the release notes. If an existing tool breaks as a result, it will help the person impacted by the problem to figure out why it happened. Otherwise they will be left with a breakage that would be difficult to diagnose.

There should be a solid rationale to justify a breaking change in Gitea. Something like a list of pros and cons that anyone could read to understand why the breaking change was necessary. In other words, the associated problems caused by the breaking change for Gitea users and software depending on Gitea have to be justified so that people that will have to do extra work because of it can be convinced this is for the greater good.

KN4CK3R · 2022-02-04T08:32:38Z

The API surface does not change (except from IsMember, see above). Every tool that scrapes the normal webpages does so on it's own risk. I don't think the structure of our pages should be considered stable.

singuliere · 2022-02-04T08:48:30Z

The API surface does not change (except from IsMember, see above). Every tool that scrapes the normal webpages does so on it's own risk. I don't think the structure of our pages should be considered stable.

I'm confused then. In what respect is this PR introducing a breaking change? My understanding is that it is not related to the structure of the pages but to the http status changing:

http.StatusFound -> http.StatusTemporaryRedirect
http.StatusMovedPermanently -> http.StatusPermanentRedirect

Which may impact known tools depending on Gitea and strict conformance to the specs cannot be expected at all times.

Are you referring to something else?

KN4CK3R · 2022-02-04T08:58:20Z

The tools you mentioned use the API which does not change with this PR.
The only change they may notice, is the login which uses the correct status codes now.

singuliere · 2022-02-04T10:22:24Z

The tools you mentioned use the API which does not change with this PR. The only change they may notice, is the login which uses the correct status codes now.

This PR is marked as kind/breaking. What breaking change does this refer to?

KN4CK3R · 2022-02-04T10:26:16Z

@6543 added that label. From my understanding it should not be breaking with the API in mind.
The only "breaking" changes are that OAuth follows the spec now and this.

Gusted · 2022-02-04T10:28:15Z

Since this is a breaking change, this PR should have a detailed explanation in the release notes. If an existing tool breaks as a result, it will help the person impacted by the problem to figure out why it happened. Otherwise they will be left with a breakage that would be difficult to diagnose.

There should be a solid rationale to justify a breaking change in Gitea. Something like a list of pros and cons that anyone could read to understand why the breaking change was necessary. In other words, the associated problems caused by the breaking change for Gitea users and software depending on Gitea have to be justified so that people that will have to do extra work because of it can be convinced this is for the greater good.

I'd just like to say that there is a specified format currently for breaking PR's: see https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md#code-review

wxiaoguang · 2022-02-11T14:50:43Z

To me, I think this PR is not that breaking, maybe we do not need to worry about it. I am fine to remove the breaking label.

If most people agree to move on, we had better get this merged ASAP, otherwise conflicts would come ....

…r-status-code

wxiaoguang · 2022-03-22T05:07:15Z

No objection since one month ago. If CI passes, this PR can be merged.

wxiaoguang · 2022-03-23T04:54:34Z

@go-gitea/maintainers merged. please help to double check.

* giteaofficial/main: Bump minimist from 1.2.5 to 1.2.6 (go-gitea#19194) Changelog for 1.16.5 (go-gitea#19189) (go-gitea#19192) Fix showing issues in your repositories (go-gitea#18916) Update issue_no_dependencies description (go-gitea#19112) Prevent redirect to Host (2) (go-gitea#19175) Prevent start panic due to missing DotEscape function Fix compare link in active feeds for new branch (go-gitea#19149) Redirect .wiki/* ui link to /wiki (go-gitea#18831) Try to prevent autolinking of displaynames by email readers (go-gitea#19169) Update HTTP status codes to modern codes (go-gitea#18063)

* 2xx/3xx/4xx/5xx -> http.Status... * http.StatusFound -> http.StatusTemporaryRedirect * http.StatusMovedPermanently -> http.StatusPermanentRedirect

Updated status codes.

2f3ba18

KN4CK3R added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 21, 2021

Gusted reviewed Dec 21, 2021

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 21, 2021

wxiaoguang reviewed Dec 22, 2021

View reviewed changes

routers/install/routes.go Outdated Show resolved Hide resolved

Fix status code.

d043ac9

wxiaoguang approved these changes Dec 22, 2021

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 22, 2021

KN4CK3R added 4 commits December 22, 2021 15:42

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

48b4a50

…r-status-code

Replaced missed numbers with constants.

081ef79

Replaced constants in integration tests.

9b6d725

Make swagger.

7627e3b

Use SeeOther as per spec.

b8c7fc1

6543 added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Dec 23, 2021

KN4CK3R added 3 commits December 28, 2021 21:07

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

fb5c706

…r-status-code

Updated http status codes.

d77129e

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

07cb4fe

…r-status-code

KN4CK3R added 3 commits December 29, 2021 23:02

lint

853c1ce

Adjusted tests.

1ea8684

swagger

cdf0611

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

5a2678d

…r-status-code

POPSuL approved these changes Jan 26, 2022

View reviewed changes

silverwind added this to the 1.17.0 milestone Jan 27, 2022

silverwind approved these changes Jan 27, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 27, 2022

singuliere suggested changes Feb 4, 2022

View reviewed changes

KN4CK3R and others added 2 commits February 17, 2022 20:27

Merge branch 'main' of https://github.com/go-gitea/gitea into refacto…

d74809a

…r-status-code

Merge branch 'main' into refactor-status-code

f261adc

wxiaoguang and others added 4 commits March 22, 2022 13:51

Merge branch 'main' into refactor-status-code

f7df5cb

Merge branch 'main' into refactor-status-code

607d88c

Merge branch 'main' into refactor-status-code

df7c6e6

Merge branch 'main' into refactor-status-code

aceda60

wxiaoguang approved these changes Mar 23, 2022

View reviewed changes

wxiaoguang merged commit 3f280f8 into go-gitea:main Mar 23, 2022

KN4CK3R deleted the refactor-status-code branch March 23, 2022 07:53

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update HTTP status codes #18063

Update HTTP status codes #18063

KN4CK3R commented Dec 21, 2021 •

edited

Loading

Gusted left a comment

Gusted Dec 21, 2021

wxiaoguang Dec 22, 2021

KN4CK3R Dec 22, 2021

Gusted Dec 22, 2021

zeripath commented Dec 22, 2021

silverwind commented Dec 22, 2021

KN4CK3R commented Dec 22, 2021

wxiaoguang commented Dec 22, 2021 •

edited

Loading

KN4CK3R commented Dec 22, 2021

silverwind commented Dec 22, 2021 •

edited

Loading

KN4CK3R commented Dec 22, 2021

silverwind commented Dec 23, 2021 •

edited

Loading

KN4CK3R commented Dec 29, 2021

silverwind commented Jan 26, 2022

KN4CK3R commented Jan 26, 2022

singuliere left a comment •

edited

Loading

KN4CK3R commented Feb 4, 2022

singuliere commented Feb 4, 2022

KN4CK3R commented Feb 4, 2022

singuliere commented Feb 4, 2022

KN4CK3R commented Feb 4, 2022

Gusted commented Feb 4, 2022

wxiaoguang commented Feb 11, 2022 •

edited

Loading

wxiaoguang commented Mar 22, 2022

wxiaoguang commented Mar 23, 2022

Update HTTP status codes #18063

Update HTTP status codes #18063

Conversation

KN4CK3R commented Dec 21, 2021 • edited Loading

Gusted left a comment

Choose a reason for hiding this comment

Gusted Dec 21, 2021

Choose a reason for hiding this comment

wxiaoguang Dec 22, 2021

Choose a reason for hiding this comment

KN4CK3R Dec 22, 2021

Choose a reason for hiding this comment

Gusted Dec 22, 2021

Choose a reason for hiding this comment

zeripath commented Dec 22, 2021

silverwind commented Dec 22, 2021

KN4CK3R commented Dec 22, 2021

wxiaoguang commented Dec 22, 2021 • edited Loading

KN4CK3R commented Dec 22, 2021

silverwind commented Dec 22, 2021 • edited Loading

KN4CK3R commented Dec 22, 2021

silverwind commented Dec 23, 2021 • edited Loading

KN4CK3R commented Dec 29, 2021

silverwind commented Jan 26, 2022

KN4CK3R commented Jan 26, 2022

singuliere left a comment • edited Loading

Choose a reason for hiding this comment

KN4CK3R commented Feb 4, 2022

singuliere commented Feb 4, 2022

KN4CK3R commented Feb 4, 2022

singuliere commented Feb 4, 2022

KN4CK3R commented Feb 4, 2022

Gusted commented Feb 4, 2022

wxiaoguang commented Feb 11, 2022 • edited Loading

wxiaoguang commented Mar 22, 2022

wxiaoguang commented Mar 23, 2022

KN4CK3R commented Dec 21, 2021 •

edited

Loading

wxiaoguang commented Dec 22, 2021 •

edited

Loading

silverwind commented Dec 22, 2021 •

edited

Loading

silverwind commented Dec 23, 2021 •

edited

Loading

singuliere left a comment •

edited

Loading

wxiaoguang commented Feb 11, 2022 •

edited

Loading