Allow storage proxy to work with IPv4 #893
Conversation
|
@droguljic Please base this on release/5.0 😉 Thx! |
server/shared/storage/proxy/mw.js
Outdated
| function getDomain() { | ||
| if (IPV4_REGEX.test(config.hostname)) return null; | ||
| return psl.parse(config.hostname).domain; | ||
| } |
There was a problem hiding this comment.
Does this work only for the local proxy provider?
There was a problem hiding this comment.
Mostly, as domain is not specified and thus determined by viewer URL, which in turn won't expose signed cookies to CloudFront.
There is possibility for this to work with CloudFront in following circumstances, host name is set to IPv4, viewer URL is different from host name and CloudFront CNAME option is set to match viewer URL, thus signed cookies are set on correct domain, but I doubt this as a real use case.
This fix mostly targets Local proxy, as without it local proxy cannot work when host name is IPv4.
There was a problem hiding this comment.
@droguljic got it. Can we throw an error in case that the proxy is set to CloudFront and the hostname is IPV4 to warn developer that their configuration is invalid?
There was a problem hiding this comment.
@MiroDojkic done. Please see the latest commit.
There was a problem hiding this comment.
@droguljic sorry for nitpicking, but should we move config validation to storage providers where the rest of the validation is?
There was a problem hiding this comment.
@MiroDojkic inside this file https://github.com/ExtensionEngine/tailor/blob/release/5.0/config/server/index.js?
In order to put it here, logic for testing is something IPv4 needs to be extracted into separate function, probably into server/shared/util/isIpv4. Importing something from server into config seems a bit weird and copy pasting regular expression is not DRY, so I'm in dilemma again.
There was a problem hiding this comment.
@droguljic yep. If it's not being used anywhere else, we could put it at the bottom of config/server/index.js. If the file becomes too bloated with noise, would it make sense to extract it to something like config/server/validation.js?
There was a problem hiding this comment.
@MiroDojkic it needs to be used in two places, server/shared/storage/proxy/mw.js and config/server/index.js, already is used in former and will be added to latter. Maybe adding new dependency is-ip is a better option and it'll definitely resolve this dilemma, so going with new dependency.
Storage proxy signed cookies would get assigned to wrong domain if configured host name is IPv4 address, thus blocking display of assets. Problem is in `psl` package which parses IPv4 addresses, thus producing wrong domains. Fix detects IPv4 addresses using regular expression and just returns `null` for domain.
droguljic
force-pushed
the
fix/storage-proxy-cookie-domain
branch
from
610974a to
4159f62
Compare
Storage proxy middleware will throw an error for IPv4 host name and CloudFront proxy combination.
Storage proxy signed cookies would get assigned to wrong domain if
configured host name is IPv4 address, thus blocking display of assets.
Problem is in
pslpackage which parses IPv4 addresses, thus producingwrong domains.
Fix detects IPv4 addresses using regular expression and just returns
nullfor domain.