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

Set subdomains to AllowOrigins with wildcard #1301

Merged
merged 4 commits into from Mar 9, 2019

Conversation

atsushi-ishibashi
Copy link
Contributor

@atsushi-ishibashi atsushi-ishibashi commented Mar 4, 2019

close: #1300

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #1301 into master will increase coverage by 0.05%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1301      +/-   ##
==========================================
+ Coverage   84.21%   84.26%   +0.05%     
==========================================
  Files          26       27       +1     
  Lines        1951     1989      +38     
==========================================
+ Hits         1643     1676      +33     
- Misses        202      205       +3     
- Partials      106      108       +2
Impacted Files Coverage Δ
middleware/cors.go 76.36% <100%> (+1.36%) ⬆️
middleware/util.go 84.37% <84.37%> (ø)
echo.go 88.19% <0%> (+0.04%) ⬆️
middleware/secure.go 93.33% <0%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 775b2ee...cf1aac1. Read the comment docs.

@vishr
Copy link
Member

vishr commented Mar 8, 2019

@atsushi-ishibashi I like the idea of supporting subdomains; however, I would like to see a better algorithm which can support nesting as well. Something like below:

Split origin & allowed origin by . and compare all tokens but *. You need to take care of length in the loop. Perhaps we can pull out this function in a util.go

func IsSubDomain(domain, pattern string) bool {
}

//cc @im-kulikov @alexaandru

@@ -102,6 +102,10 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc {
allowOrigin = o
break
}
if o != "*" && equalScheme(origin, o) && isSubDomain(origin, o) {
Copy link
Member

Choose a reason for hiding this comment

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

  • I don't think you need o != "*"
  • Make equalScheme() part of isSubdomain()`

"strings"
)

func equalScheme(domain, pattern string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I think matchScheme() is a better name

}

// isSubDomain compares authority with wildcard
func isSubDomain(domain, pattern string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above matchSubdomain() is a better name

Copy link
Member

@vishr vishr left a comment

Choose a reason for hiding this comment

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

👍

@vishr vishr merged commit 1f6cc36 into labstack:master Mar 9, 2019
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.

Set subdomains to AllowOrigins with wildcard
2 participants