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

CORS Problems #7840

Closed
HoffmannP opened this issue Aug 13, 2019 · 13 comments · Fixed by #7967
Closed

CORS Problems #7840

HoffmannP opened this issue Aug 13, 2019 · 13 comments · Fixed by #7967
Labels
Milestone

Comments

@HoffmannP
Copy link
Contributor

HoffmannP commented Aug 13, 2019

  • 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
Copy link
Member

sapk commented Aug 13, 2019

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
Copy link
Contributor Author

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
Copy link
Member

lunny commented Aug 14, 2019

@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
Copy link
Contributor Author

HoffmannP commented Aug 15, 2019

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
Copy link
Contributor

@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
Copy link
Contributor

tonivj5 commented Aug 24, 2019

I think #7204 is related to 3.

@tamalsaha
Copy link
Contributor

tamalsaha commented Aug 24, 2019

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
Copy link
Contributor

I am referring to this sections of the Oauth2 RFC.

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

@HoffmannP
Copy link
Contributor Author

@tonivj5

I think #7204 is related to 3.

Sorry, I don't see the relation

@HoffmannP
Copy link
Contributor Author

@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
Copy link
Contributor

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?

  1. 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
Copy link
Contributor

tamalsaha commented Aug 24, 2019

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
Copy link
Contributor

tamalsaha commented Aug 25, 2019

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

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

  1. 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.

@lafriks lafriks added this to the 1.9.3 milestone Aug 26, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants