Process ACCESS_CONTROL_ALLOW_ORIGIN parameter #24225

aralex · 2023-04-20T07:07:54Z

read ACCESS_CONTROL_ALLOW_ORIGIN parameter from app.ini;
send Access-Control-Allow-Origin header for request with the same domain in ORIGIN header.

silverwind · 2023-04-20T17:54:35Z

This looks insecure as it will echo the Origin header back, basically allowing all origins. We have a setting repo.ACCESS_CONTROL_ALLOW_ORIGIN which allows specifying a single origin header, which seems a slightly more secure approach.

Edit: I think I've misread. Retracting.

Generally I would recommend avoiding CORS where possible via reverse proxies.

silverwind · 2023-04-20T18:00:52Z

Maybe describe a bit what problem you are trying to solve to help us understand why this is needed.

I'm no expert on CORS but in the few situations I've dealt with it, I found it easier to just set up reverse proxies to reduce to a single origin and not have CORS in first place, which has many negative effects, like pre-flight requests and such.

aralex · 2023-04-20T18:39:26Z

Generally I would recommend avoiding CORS where possible via reverse proxies.

I'm agree that CORS is more flexible and safe. But I didn's succeeded in activating CORS. I explored the sources. It seemed to Me that both CORS and repo.ACCESS_CONTROL_ALLOW_ORIGIN are not realized completly.

There's no reading of ACCESS_CONTROL_ALLOW_ORIGIN from app.ini. There's no place of setting.Repository.AccessControlAllowOrigin assignment. It can not work at all.
The only place of header Access-Control-Allow-Origin creation is here. But that function doesn't work while sending file via HTTP. Besides setting.Repository.AccessControlAllowOrigin didn't assigned a real domain. It will not work too. I see that CORS can not create header Access-Control-Allow-Origin too.

I decided to go the simple way: to add setting.Repository.AccessControlAllowOrigin reading from app.ini. And I added creation of the header to the code which creates other similar headers. This feature is not deprecated and it's useful. Admins may choose whether to use it or not.

I need this feature to read data files from Gitea in some web page. I'd like to load files from Gitea in JavaScript by HTTP request. It's the single domain (My web server). I can use repo.ACCESS_CONTROL_ALLOW_ORIGIN to specify it. In My case is simple and safe.

As you obviously understood, I realized sending of Access-Control-Allow-Origin header to the specified domain only. In case of simple CORS it has no negative effects. In more heavy cases admins should use CORS section of app.ini.

I think My PR is useful.

wxiaoguang · 2023-04-21T01:25:56Z

I read the code briefly. At the moment, my thoughts are:

The sec.MapTo(&Repository) below should read the setting.Repository.AccessControlAllowOrigin, if it doesn't, then there seems to be a bug for INI handling.
There is similar logic in repo/http.go: httpBase(), so I would assume this PR is right.

Correct me if I am wrong.

aralex · 2023-04-21T05:37:38Z

Thank You, wxiaoguang!

I'd like to add some details...

There is similar logic in repo/http.go: httpBase()...

Yes. But repo/http.go: httpBase() doesn't take part in responsing to My requests. I'm trying to load files from Gitea in raw mode by URLs like this: https://raw.githubusercontent.com/go-gitea/gitea/main/LICENSE. So I had to dub the code.
repo/http.go: httpBase() sends a header Access-Control-Allow-Origin: <my_domain> on requests from any domains (as I see). It isn't critical, but it's no good. It should be corrected, I think.

So what will be further? I'm novice in Gitea development and in development on Github too... :)

wxiaoguang · 2023-04-21T06:34:06Z

Maybe let's keep this PR open? If you don't mind, I could try this PR on my side and maybe make some changes on it.

aralex · 2023-04-21T06:56:21Z

I'm agree! I really need this feature! Thank You!

I tested it by requests like this:

curl -s -I -H "Origin: https://mydomain.org" -u <user>:<password>  http://<repo_url>/raw/branch/master/<data_file>

I think specifying a value of ACCESS_CONTROL_ALLOW_ORIGIN without a protocol is better. Like this:

ACCESS_CONTROL_ALLOW_ORIGIN: mydomain.org

It allows both HTTP and HTTPS. But I prepose to add an example in app.example.ini because this detail is not obvious.

aralex · 2023-04-21T08:48:42Z

Maybe let's keep this PR open?..

Should I reopen this PR?

modules/setting/repository.go

wxiaoguang · 2023-04-24T04:00:06Z

modules/context/context.go

+		if len(setting.Repository.AccessControlAllowOrigin) > 0 {
+			origin := strings.ToLower(ctx.Req.Header.Get("Origin"))
+			matcher := regexp.MustCompile(`http(s?)://`)
+			origin = matcher.ReplaceAllString(origin, "")
+			if origin == setting.Repository.AccessControlAllowOrigin {
+				header.Set("Access-Control-Allow-Origin", setting.Repository.AccessControlAllowOrigin)
+			}
+		}


TBH, it doesn't look good to me.

Although I think we can respect the Repository.AccessControlAllowOrigin config, but this logic seems strange, it's different from how the AccessControlAllowOrigin is used in httpBase.

Although I think we can respect the Repository.AccessControlAllowOrigin config, but this logic seems strange, it's different from how the AccessControlAllowOrigin is used in httpBase.

You mean removing of a protocol prefix, don't You? I'm agree that logics should be the same. Do You think that this removing is excess?

You mean removing of a protocol prefix

No, I mean this logic is different from how the AccessControlAllowOrigin is used in httpBase.

gitea/routers/web/repo/http.go

Line 38 in 04347eb

func httpBase(ctx *context.Context) (h *serviceHandler) {

I believe every single line should have its clear purpose and meaning. If httpBase is correct, then new code should follow it (extract the common part to a new function, to avoid duplicate code). If your new code is correct, then is old httpBase incorrect and should be fixed? If httpBase and your code are both correct, then what's the difference, why they should be written differently (which causes inconsistent behavior for the single config option) ?

Quote:

repo/http.go: httpBase() sends a header Access-Control-Allow-Origin: <my_domain> on requests from any domains (as I see).

I haven't looked into details, but there is a standard: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin

I believe every single line should have its clear purpose and meaning. If httpBase is correct, then new code should follow it (extract the common part to a new function, to avoid duplicate code). If your new code is correct, then is old httpBase incorrect and should be fixed? If httpBase and your code are both correct, then what's the difference, why they should be written differently (which causes inconsistent behavior for the single config option)?

You're quite right! But I see the following facts:

http.go implements Git Smart HTTP protocol. This module doesn't take part in responsing to usual requests from web browser or other simple clients.

httpBase sends Access-Control-Allow-Origin to all clients. It's wrong. Someone should correct it by another PR. I'm better just to add TODO remark in http.go, I think.

http.go and context.go really have some similar code. This fragment could be located in a separate function. But I think it is not useful right now. It should be a part of further refactoring. The changes could be much more wide. I'd like to minimize My changes now. But I can add one more TODO remark about this idea.

What do You think?

At least, according to MDN: Access-Control-Allow-Origin: <origin> , https://developer.mozilla.org/en-US/docs/Glossary/Origin , the code origin = matcher.ReplaceAllString(origin, "") doesn't look right.

I would suggest to use a strict (and correct) CORS implementation.

To fix the httpBase problem, I proposed #24303 , using a standard CORS package could be better.

after it gets merged, I think you can just add repo.CorsHandler() to m.Group("/raw", func() {, then the CORS should work.

OK, I'll wait for #24303

ps: usually the PR doesn't need to sync with main frequently (unless it is out-dated for long time, or there are conflicts). Just leave it there, as long as the PR is ready and approved, maintainers could help to sync & update & merge.

#24303 has been merged

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 20, 2023

aralex closed this Apr 20, 2023

aralex reopened this Apr 21, 2023

pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 21, 2023

wxiaoguang reviewed Apr 24, 2023

View reviewed changes

modules/setting/repository.go Outdated Show resolved Hide resolved

wxiaoguang reviewed Apr 24, 2023

View reviewed changes

aralex closed this May 12, 2023

pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 12, 2023

go-gitea locked as resolved and limited conversation to collaborators Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process ACCESS_CONTROL_ALLOW_ORIGIN parameter #24225

Process ACCESS_CONTROL_ALLOW_ORIGIN parameter #24225

aralex commented Apr 20, 2023 •

edited

Loading

silverwind commented Apr 20, 2023 •

edited

Loading

silverwind commented Apr 20, 2023

aralex commented Apr 20, 2023 •

edited

Loading

wxiaoguang commented Apr 21, 2023

aralex commented Apr 21, 2023

wxiaoguang commented Apr 21, 2023

aralex commented Apr 21, 2023 •

edited

Loading

aralex commented Apr 21, 2023

wxiaoguang Apr 24, 2023

aralex Apr 24, 2023

wxiaoguang Apr 24, 2023 •

edited

Loading

aralex Apr 24, 2023 •

edited

Loading

wxiaoguang Apr 24, 2023 •

edited

Loading

wxiaoguang Apr 24, 2023 •

edited

Loading

aralex Apr 24, 2023

wxiaoguang Apr 24, 2023

wxiaoguang Apr 26, 2023

Process ACCESS_CONTROL_ALLOW_ORIGIN parameter #24225

Process ACCESS_CONTROL_ALLOW_ORIGIN parameter #24225

Conversation

aralex commented Apr 20, 2023 • edited Loading

silverwind commented Apr 20, 2023 • edited Loading

silverwind commented Apr 20, 2023

aralex commented Apr 20, 2023 • edited Loading

wxiaoguang commented Apr 21, 2023

aralex commented Apr 21, 2023

wxiaoguang commented Apr 21, 2023

aralex commented Apr 21, 2023 • edited Loading

aralex commented Apr 21, 2023

wxiaoguang Apr 24, 2023

Choose a reason for hiding this comment

aralex Apr 24, 2023

Choose a reason for hiding this comment

wxiaoguang Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

aralex Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

aralex Apr 24, 2023

Choose a reason for hiding this comment

wxiaoguang Apr 24, 2023

Choose a reason for hiding this comment

wxiaoguang Apr 26, 2023

Choose a reason for hiding this comment

aralex commented Apr 20, 2023 •

edited

Loading

silverwind commented Apr 20, 2023 •

edited

Loading

aralex commented Apr 20, 2023 •

edited

Loading

aralex commented Apr 21, 2023 •

edited

Loading

wxiaoguang Apr 24, 2023 •

edited

Loading

aralex Apr 24, 2023 •

edited

Loading

wxiaoguang Apr 24, 2023 •

edited

Loading

wxiaoguang Apr 24, 2023 •

edited

Loading