use exact matching of allowed domain entries, issue #489 #493
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## v3 #493 +/- ##
==========================================
- Coverage 70.07% 69.63% -0.44%
==========================================
Files 26 27 +1
Lines 1223 1581 +358
==========================================
+ Hits 857 1101 +244
- Misses 293 409 +116
+ Partials 73 71 -2 ☔ View full report in Codecov by Sentry. |
|
The docs should be updated to indicate that each allowed domain isn't a potential regexp pattern, it is a regexp pattern. So |
|
@JamieSlome is your hunt issue resolved by this PR? |
cors_filter.go
Outdated
| AllowedDomains []string // list of allowed values for Http Origin. An allowed value can be a regular expression to support subdomain matching. If empty all are allowed. | ||
| // AllowedDomains list of allowed values for Http Origin. | ||
| // An allowed value can be a regular expression to support subdomain matching. | ||
| // Non-regular expression values will be changed into an exact match: ^yourdomain.com$ |
There was a problem hiding this comment.
I'm not sure ^yourdomain.com$ is an intuitive "exact match". The . is still a wildcard. You'd have to do fmt.Sprintf("^%s$", regexp.QuoteMeta(each)) to get an "exact match".
There was a problem hiding this comment.
agreed. ^www.amazon.com$ still matches wwwqamazon.com and www.amazonqcom.
cors_filter.go
Outdated
| AllowedDomains []string // list of allowed values for Http Origin. An allowed value can be a regular expression to support subdomain matching. If empty all are allowed. | ||
| // AllowedDomains list of allowed values for Http Origin. | ||
| // An allowed value can be a regular expression to support subdomain matching. | ||
| // Non-regular expression values will be changed into an exact match: ^yourdomain.com$ |
There was a problem hiding this comment.
It's not clear to me from this what qualifies as "non-regular expression". example.com and .* and example\.com are all regular expressions, even though they don't start with ^ or end with $.
|
Just (cc) @sickcodes and @ktg9 to see if they think this addresses the reported issue 👍 |
|
@emicklei if I'm reading the code correctly, this just simply isn't going to work in practice in most cases. |
|
@steven-collins-omega thank you for commenting on this. I agree that it is not correct and confusing. I will give it second time to come up with a solution |
|
hi all, I am thinking about changing the behaviour of CrossOriginResourceSharing:
|
cors_filter.go
Outdated
| if domain == origin { | ||
| allowed = true | ||
| break | ||
| if domain == ".*" || domain == origin { |
There was a problem hiding this comment.
Should domain and origin be lowercased before comparison?
There was a problem hiding this comment.
yes, i think this should happen. RFC4343: Domain Name System (DNS) names are "case insensitive".
…cklei#493) * use exact matching of allowed domain entries, issue emicklei#489 * update doc, add testcases from PR conversation * introduce AllowedDomainFunc emicklei#489 * more tests, fix doc * lowercase origin before checking cors
No description provided.