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

add note on origins w/ default port to README.md #163

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nbr
Copy link

@nbr nbr commented Apr 4, 2018

When http://EXAMPLE:80 is an allowed origin, requests are not allowed from
http://EXAMPLE. Since port 80 is the default port for HTTP, browsers
will strip it and thus rack-cors never receives a request from http://EXAMPLE.

A similar problem is discussed here: request/request#515

When `http://EXAMPLE:80` is an allowed origin, requests are not allowed from
`http://EXAMPLE`. Since port 80 is the default port for HTTP, browsers
will strip it and thus rack-cors never receives a request from `http://EXAMPLE`.

A similar problem is discussed here: request/request#515
@nbr
Copy link
Author

nbr commented Jun 13, 2018

@cyu Is this still being considered? If not, I can close the PR.

@cyu
Copy link
Owner

cyu commented Jun 13, 2018

@nbr I'm hesitant because of potential unintended side effects of using URI.parse for this purpose. Perhaps use another method to normalize this?

@nbr
Copy link
Author

nbr commented Jun 13, 2018

@cyu That makes sense, since it is not an obvious requirement of parse.

Latest commit does not rely on URI#parse to remove the default port. Instead, we use URI#default_port to decide when to strip the port. We could avoid using URI altogether but it simplifies the code.

@nbr
Copy link
Author

nbr commented Jul 18, 2018

@cyu Any feedback on the latest commit?

@cyu
Copy link
Owner

cyu commented Jul 19, 2018

@nbr I left a comment after the last change (about parse errors in URI.parse)

Also, having thought about this more, I’m thinking we should do this on initialization or at least on first eval — we shouldn’t penalize every call with this evaluation. Thoughts?

@nbr nbr changed the title Allow specifying redundant default port in origin add note on origins w/ default port to README.md Aug 3, 2022
@nbr
Copy link
Author

nbr commented Aug 3, 2022

@cyu The root cause of the issue in requests was fixed (request/request#2904), so I suspect less Ruby apps will run into this. I pivoted this PR to just add a note to the README.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants