New generic mode that always returns * (less secure, better caching)#32
New generic mode that always returns * (less secure, better caching)#32rprieto wants to merge 1 commit into
Conversation
rprieto
changed the title
| exports.allowed = function (list, requestOrigin) { | ||
| function match (origin) { | ||
| if (origin.indexOf('*') !== -1) { | ||
| var regex = '^' + origin.replace('.', '\\.').replace('*', '.*') + '$' |
There was a problem hiding this comment.
small performance comment, could we generate a list of these regex when the middleware is created rather then doing it on each request?
There was a problem hiding this comment.
I'll implement that on #33 (of top of which this PR should be rebased)
retrohacker
left a comment
retrohacker
left a comment
There was a problem hiding this comment.
Interesting idea! Some code changes requested. On the fence about special casing *, especially if we special case * only when it is the only element in the array.
| // If either no origin was set, or the origin isn't supported, continue | ||
| // without setting any headers | ||
| if (!originHeader || !matcher(originHeader)) { | ||
| if (originMatcher.generic(options.origins)) { |
There was a problem hiding this comment.
Is there a way to handle this without treating the whole block as a special case? A nested if/else of this size increases the complexity of the code base quite a lot and fragments related changes across multiple lines in the code base.
| @@ -1,4 +1,8 @@ | |||
|
|
|||
| exports.generic = function (list) { | |||
| return list.length === 1 && list[0] === '*' | |||
There was a problem hiding this comment.
Is there a case where we would want to explicitly match some domains, and wildcard the rest? Would it be surprising that ['*'] would have one behaviour for a domain matched against the wildcard and an entirely different behaviour for a domain matched against the wildcard in this list: ['foo.bar', '*']
There was a problem hiding this comment.
I agree that's quite strange, not 100% happy with that design. Do you think it would make more sense to have some sort of option flag like
{
// specify one or more origins, wildcard supported
// will only return the matched origin, with a Vary header
origins: ['https://myapp.com', 'https://*.myapp.com']
// or use the generic mode
// which always returns "Accept", and can be cached across the board
// but has security implications
allowAllOrigins: true
}There was a problem hiding this comment.
I like the allowAllOrigins override. It seems like it would both simplify the implementation and make the user's intent explicit.
| // 6.2.1 and 6.2.2 | ||
| var originHeader = req.headers['origin'] | ||
| if (!matcher(originHeader)) return next() | ||
| if (originMatcher.generic(options.origins)) { |
There was a problem hiding this comment.
Same comment as above, but to a much greater extent. Is there a way we can do this that doesn't treat the whole block as a special case? We are adding an incredibly amount of complexity here to support this.
|
Is this dead in the water? It would be greatly appreciated if this was something that could be added. |
|
Yeah, @kokarn, it looks like this has lost momentum. If you want to pick it up and implement the feedback I would be happy to do a code review on another PR |
|
I'm not opposed at all but pretty sure I won't have the time in the near future. Will keep it in the back of my head tho, so who knows :) |
4 participants
New "generic" mode which always returns
access-control-allow-origin: *, similar to what thefullResponseRestify plugin used to do.This is the less secure option, but increases performance since responses can now be cached 100%, instead of having
Vary: Origin.