-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Scala Stream Collector: allow use of the originating scheme during cookie bounce #3512
Scala Stream Collector: allow use of the originating scheme during cookie bounce #3512
Conversation
@rbolkey has signed the Software Grant and Corporate Contributor License Agreement. Thanks so much |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall 👍
RawHeader("P3P", "policyref=\"%s\", CP=\"%s\"".format(config.p3p.policyRef, config.p3p.CP)), | ||
accessControlAllowOriginHeader(request), | ||
`Access-Control-Allow-Credentials`(true) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we leave this as it was?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops ... didn't realize I had changed that
queryParams: Map[String, String], | ||
uri: Uri, | ||
cookieBounceName: String, | ||
queryParams: Map[String,String], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing space [String, String]
enabled: Boolean, | ||
name: String, | ||
fallbackNetworkUserId: String, | ||
forwardedProtocolHeader: Option[String] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the indent seems off
) | ||
}.getOrElse(redirectUri) | ||
|
||
Some(`Location`(redirectUriWithForwardedScheme)) | ||
} else { | ||
None | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's refactor this as:
if (bounce) {
val forwardedScheme = for {
headerName <- bounceConfig.forwardedProtocolHeader
headerValue <- request.headers.find(_.lowercaseName == headerName.toLowerCase)
scheme <-
if (Set("http", "https").contains(headerValue)) {
Some(headerValue)
else {
logger.warn(s"Header $headerName contains invalid protocol value $headerValue.")
None
}
} yield scheme
val redirectUri = request.uri
.withQuery(Uri.Query(queryParams + (bounceConfig.name -> "true")))
.withScheme(forwardedScheme.getOrElse(request.uri.scheme))
Some(`Location`(redirectUri))
} else {
None
}
Thanks @BenFradet. I've applied the recommended changes. Let me know if I missed anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, could you rebase against master? 👍
# Optionally, specify the name of the header containing the originating protocol for use in the bounce redirect | ||
# location. Use this if behind a load balancer that performs SSL termination. The value of this header must | ||
# be http or https. Example, if behind an AWS Classic ELB. | ||
forwardedProtocolHeader: "X-Forwarded-Proto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use =
as above?
Commit adds a new configuration parameter (`forwardedProtocolHeader`) in the cookie bounce feature. This parameter specifies the name of an http header that contains the originating protocol if deployed behind a load balancer. This is useful if SSL termination happens at the load balancer in order to maintain the security model of you web page.
3f04abf
to
f6ae60d
Compare
@BenFradet fixed the example config, and rebased. |
…okie bounce (closes #3512) Commit adds a new configuration parameter (`forwardedProtocolHeader`) in the cookie bounce feature. This parameter specifies the name of an http header that contains the originating protocol if deployed behind a load balancer. This is useful if SSL termination happens at the load balancer in order to maintain the security model of you web page.
…okie bounce (closes #3512) Commit adds a new configuration parameter (`forwardedProtocolHeader`) in the cookie bounce feature. This parameter specifies the name of an http header that contains the originating protocol if deployed behind a load balancer. This is useful if SSL termination happens at the load balancer in order to maintain the security model of you web page.
…okie bounce (closes #3512) Commit adds a new configuration parameter (`forwardedProtocolHeader`) in the cookie bounce feature. This parameter specifies the name of an http header that contains the originating protocol if deployed behind a load balancer. This is useful if SSL termination happens at the load balancer in order to maintain the security model of you web page.
…plow/snowplow#3512) Commit adds a new configuration parameter (`forwardedProtocolHeader`) in the cookie bounce feature. This parameter specifies the name of an http header that contains the originating protocol if deployed behind a load balancer. This is useful if SSL termination happens at the load balancer in order to maintain the security model of you web page.
…plow/snowplow#3512) Commit adds a new configuration parameter (`forwardedProtocolHeader`) in the cookie bounce feature. This parameter specifies the name of an http header that contains the originating protocol if deployed behind a load balancer. This is useful if SSL termination happens at the load balancer in order to maintain the security model of you web page.
Commit adds a new configuration parameter (
forwardedProtocolHeader
) in thecookie bounce feature. This parameter specifies the name of an http header
that contains the originating protocol if deployed behind a load balancer.
This is useful if SSL termination happens at the load balancer in order to
maintain the security model of you web page.
Fixes #3505