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

signup controller handles CORS by itself #2739

Merged
merged 4 commits into from
Nov 22, 2021

Conversation

akostadinov
Copy link
Contributor

rack-cors gem handles all pre-flight requests but signup controller has its
own way of handling these. So we make rack-cors skip handling of requests
with the signup controller path.

Otherwise 3scale.net signup form gets broken.

To test you need cors enabled in config/cors.yml. The this is with the fix:

$ curl -v 'http://master-account.3scale.localhost:3000/p/signup?signup_origin=website-signup' -X OPTIONS -H 'User-Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:94.0) Gecko/20100101 Firefox/94.0' -H 'Accept: */*' -H 'Accept-Language: en-US,en;q=0.7,bg;q=0.3' --compressed -H 'Access-Control-Request-Method: GET' -H 'Access-Control-Request-Headers: 3scale-origin,x-requested-with' -H 'Referer: https://www.3scale.net/' -H 'Origin: https://www.3scale.net' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Sec-Fetch-Dest: empty' -H 'Sec-Fetch-Mode: cors' -H 'Sec-Fetch-Site: same-site' -H 'Pragma: no-cache' -H 'Cache-Control: no-cache'
*   Trying ::1:3000...
* connect to ::1 port 3000 failed: Connection refused
*   Trying 127.0.0.1:3000...
* Connected to master-account.3scale.localhost (127.0.0.1) port 3000 (#0)
> OPTIONS /p/signup?signup_origin=website-signup HTTP/1.1
> Host: master-account.3scale.localhost:3000
> Accept-Encoding: deflate, gzip, br
> User-Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:94.0) Gecko/20100101 Firefox/94.0
> Accept: */*
> Accept-Language: en-US,en;q=0.7,bg;q=0.3
> Access-Control-Request-Method: GET
> Access-Control-Request-Headers: 3scale-origin,x-requested-with
> Referer: https://www.3scale.net/
> Origin: https://www.3scale.net
> DNT: 1
> Connection: keep-alive
> Sec-Fetch-Dest: empty
> Sec-Fetch-Mode: cors
> Sec-Fetch-Site: same-site
> Pragma: no-cache
> Cache-Control: no-cache
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Mon, 15 Nov 2021 21:21:34 GMT
< Connection: close
< Access-Control-Allow-Origin: https://www.3scale.net
< Access-Control-Allow-Methods: GET, POST
< Access-Control-Allow-Headers: Origin, Accept, Cookie, Content-Type, X-Requested-With, X-CSRF-Token, 3scale-Origin
< Access-Control-Allow-Credentials: true
< Content-Type: text/html
< Cache-Control: no-cache
< Set-Cookie: _system_session=TzxcFDSlkjxkvFDAzhjv3ZnUDM5b3F4eDBjWHc9PS0tVXZpU1VtaFhiQkZUU3hwTVRTTGQ5dz09--a4bb8c1f611b5d2e19ace44da48c9ce3920a9dbb; path=/; HttpOnly; SameSite=Lax
< X-Request-Id: 17d731b0-8e68-449b-a1fa-641ca5d56e56
< X-Runtime: 0.542218
< X-Served-By: Horak
< Vary: Accept-Encoding
< Content-Encoding: gzip
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 1; mode=block
< 
* Closing connection 0

Without the fix:

curl -v 'http://master-account.3scale.localhost:3000/p/signup?signup_origin=website-signup' -X OPTIONS -H 'User-Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:94.0) Gecko/20100101 Firefox/94.0' -H 'Accept: */*' -H 'Accept-Language: en-US,en;q=0.7,bg;q=0.3' --compressed -H 'Access-Control-Request-Method: GET' -H 'Access-Control-Request-Headers: 3scale-origin,x-requested-with' -H 'Referer: https://www.3scale.net/' -H 'Origin: https://www.3scale.net' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Sec-Fetch-Dest: empty' -H 'Sec-Fetch-Mode: cors' -H 'Sec-Fetch-Site: same-site' -H 'Pragma: no-cache' -H 'Cache-Control: no-cache'
*   Trying ::1:3000...
* connect to ::1 port 3000 failed: Connection refused
*   Trying 127.0.0.1:3000...
* Connected to master-account.3scale.localhost (127.0.0.1) port 3000 (#0)
> OPTIONS /p/signup?signup_origin=website-signup HTTP/1.1
> Host: master-account.3scale.localhost:3000
> Accept-Encoding: deflate, gzip, br
> User-Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:94.0) Gecko/20100101 Firefox/94.0
> Accept: */*
> Accept-Language: en-US,en;q=0.7,bg;q=0.3
> Access-Control-Request-Method: GET
> Access-Control-Request-Headers: 3scale-origin,x-requested-with
> Referer: https://www.3scale.net/
> Origin: https://www.3scale.net
> DNT: 1
> Connection: keep-alive
> Sec-Fetch-Dest: empty
> Sec-Fetch-Mode: cors
> Sec-Fetch-Site: same-site
> Pragma: no-cache
> Cache-Control: no-cache
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Mon, 15 Nov 2021 21:22:21 GMT
< Connection: close
< X-Frame-Options: DENY
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 1; mode=block
< 
* Closing connection 0

@akostadinov akostadinov self-assigned this Nov 15, 2021
@akostadinov
Copy link
Contributor Author

Failures seem to be because of shoulda instability.

@hallelujah
Copy link
Contributor

I wonder if we could just ignore some paths in our CORS lib instead, like adding a global setting in the config:

ignore:
  - path: /p/signup

@akostadinov
Copy link
Contributor Author

akostadinov commented Nov 16, 2021

It is possible to make it configuration. But this particular exception is kind of code induced. And I find it more reasonable to have it handled in code as well.

Please ignore failing tests and merge if you agree to keep in code. Or let me know if you want me to make it configuration.

@hallelujah
Copy link
Contributor

hallelujah commented Nov 17, 2021

Well I prefer it is a configuration, later if we want to change for another forgotten controller we should be able to do that as well without redeploying another code.

Another valid reason I do not think the CORS class should know anything about the controller

@akostadinov
Copy link
Contributor Author

Ok, I'm checking how to do it. I don't like configuration that should by no use case be changed though. I don't mean if we find or implement something that needs it. I mean users should never change it.

@akostadinov
Copy link
Contributor Author

How about testing, should we test the configuration file that it ignores signup controller or leave it to some other system?

@hallelujah
Copy link
Contributor

About testing, this basically can be a gem on its own, adding more functionality to Rack::Cors
So think about it this way.

Copy link
Contributor

@hallelujah hallelujah left a comment

Choose a reason for hiding this comment

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

I would do it with the configuration, In suggestions this is how I would do it. Not completely implemented but that is the idea

lib/three_scale/middleware/cors.rb Outdated Show resolved Hide resolved
lib/three_scale/middleware/cors.rb Outdated Show resolved Hide resolved
lib/three_scale/middleware/cors.rb Outdated Show resolved Hide resolved
@akostadinov
Copy link
Contributor Author

Updated, please check again. I didn't notice your suggested changes so I implemented in a little bit different way.

One thing is that I wanted to use start_with? because it is much faster than regexp matching. And it will be performed for all requests so I thought that it is worth going for it. I can add regexp matcher as well if you want.

I also extended tests. I added a test for the example configuration files. I'm not sure if this was the proper way.

Another thing I changed is to completely avoid inserting the middleware when it is disabled. This bugged me from the beginning, that we always inserted it.

So let me know any suggestions that you have.

lib/three_scale/middleware/cors.rb Outdated Show resolved Hide resolved
lib/three_scale/middleware/cors.rb Outdated Show resolved Hide resolved
lib/three_scale/middleware/cors.rb Outdated Show resolved Hide resolved
lib/three_scale/middleware/cors.rb Outdated Show resolved Hide resolved
@akostadinov akostadinov merged commit bc82ebe into 3scale:master Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants