CORS Problems

[HoffmannP]

• Gitea version (or commit ref): 1.9

Description

Even after the 1.9-update I still have issues with CORS:

1. only one single domain is configurable in cors.ALLOW_DOMAIN, setting it to '*' does not respond with the Origin-Domain but with * (which is not allowed for auth-Requests).
2. the cors.SCHEMA option doesn't seem to do anything (also, probably only allows one value)
3. in preflight-requests the authorization-header is omitted but querying the API even with an OPTIONS request results in a 403 Forbidden
4. What is repository.ACCESS_CONTROL_ALLOW_ORIGIN doing?

[sapk]

Those are mainly limitation/bug in the module: https://github.com/go-macaron/cors

For repository.ACCESS_CONTROL_ALLOW_ORIGIN it is for git http(s) protocol. You can find the PR that introduce it : #5719

On side-note: We maybe should clean some configuration and replace hsts and cors specific case via configuration like suggested here: #7423 (comment)
It will allow more flexibility and we can suggest some configuration cases.

[HoffmannP]

1 . & 2 . I understand, that cors.ALLOW_DOMAIN is limited to a single value. Adding a matcher or a list is something I should discuss at Macaron. Same for cors.SCHEMA.

4 . Thanks for clearing that

3 . is somewhat confusing as go-macaron src code explicitly states HTTP-200 for OPTIONS-Requests but Gitea responds with 403 Forbidden (as in not-authorized) which is obvious as a preflight request does explicitly not have any auth-header.

[lunny]

@sapk we have forked almost all macaron repositories to https://gitea.com/macaron. Please send PR to there and I think we could replace macaron with our forks on v1.10

[HoffmannP]

1 . & 2. are taken care of in https://gitea.com/macaron/cors/pulls/3
3 . seems to be harder b/c the macaron CORS module should respond directly to OPTION-requests (with an empty body) as it does for errors instead of forwarding the request the next request handler
Can someone confirm that this is the right strategy?

[tamalsaha]

@HoffmannP , I am the original author of the macaron cors module. I thought that the module already responding to preflight requests here:
https://github.com/go-macaron/cors/blob/6fd6a9bfe14e9ed6ddf9265b6580f7638105b27b/cors.go#L124

Isn't that enough?

[tonivj5]

I think #7204 is related to 3.

[tamalsaha]

ah! So, currently CORS handler is only set on the api endpoints.
https://github.com/go-gitea/gitea/pull/6289/files#diff-2620fdbeeb83ce43692534e6c2c39452R505

I would argue that anything that is not an "api" should not have to deal with CORS since it is not meant to called via Ajax from external domains.

So, is this login_oauth an api? Is this use-case trying to login via Oauth2 implicit flow? Does Gitea ouath supports oauth2 implicit flow or just the authorization code flow? Because you are not supposed to use authorization code from JS/Ajax. If someone know how this type of things work with other websites, could possibly weigh in.

[tamalsaha]

I am referring to this sections of the Oauth2 RFC.

• Authorization Code Grant
• Implicit Grant

Implicit Grant can be used via Ajax. Authorization Code Grant should not be used from Ajax.

[HoffmannP]

@tonivj5

I think #7204 is related to 3.

Sorry, I don't see the relation

[HoffmannP]

@tamalsaha

I'm not talking about non-API-endpoints, I'm talking about the API. As I see it, the problem is 1. the CORS handler is executed after the reqToken handler and second it returns nothing so according to the handler workflow it would not finish the response and hence reqToken could set it's own status.
CORS handler needs to be a before handler that responds with something (I have no idea what "If response anything" in the diagram means exactly)

[tamalsaha]

Thanks @HoffmannP ! I am starting to see some of the issues but the fix is not clear to me.

1. (I have no idea what "If response anything" in the diagram means exactly)

Same here. Do I need to return true or something?

2. I think CORS handler should go before any other handler and only respond to preflight requests without checking for cookie or authz header. This will only check the CORS domain, verb etc settings.

This will need some rewiring. Right now the order is not correct. May be you can open a pr. I did not see this problem because in my case we were using cookie for auth and cookies are forwarded with cross site calls.

[tamalsaha]

https://github.com/go-macaron/macaron/blob/213788aac5bca04b4ef40b4b3ab821d417dbf396/macaron.go#L173

// BeforeHandler represents a handler executes at beginning of every request.
// Macaron stops future process when it returns true.
type BeforeHandler func(rw http.ResponseWriter, req *http.Request) bool

Macaron stops future process when it returns true.

https://github.com/go-macaron/macaron/blob/master/macaron.go#L215

[tamalsaha]

I looked into the macaron source code and this is what I found:

• m.Before() does not check for path so will not work for CORS.
  https://go-macaron.com/docs/intro/core_concepts#handler-workflow

• m.Use() are added a handlers in the Macaron instance
  https://github.com/go-macaron/macaron/blob/213788aac5bca04b4ef40b4b3ab821d417dbf396/macaron.go#L184

• for routed paths Use() handlers (middlewares) are run first and then the group handlers and then the actual handler
  https://github.com/go-macaron/macaron/blob/213788aac5bca04b4ef40b4b3ab821d417dbf396/router.go#L185

• context.run() is where the handlers are checked and if Written() and following handlers are not consulted.
  https://github.com/go-macaron/macaron/blob/master/context.go#L119

So, I see 2 fixes are needed;

1. CORS handler should use context.Resp.WriteHeader() instead of context.Status() (comes from Renderer). This will property set the the Written() to true. Renderer also gets the same ResponseWriter() so this should not be necessary. But I am doing this out of an abundance of caution.

PR: https://gitea.com/macaron/cors/pulls/5

2. The #3 issue by the OP.

in preflight-requests the authorization-header is omitted but querying the API even with an OPTIONS request results in a 403 Forbidden

For this I have opened #7967

@HoffmannP might want to try this branch and see if this fixes your issue.