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.

Sign up for GitHub

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

Allow custom headers to be returned as part of pre-flight requests #423

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Allow custom headers to be returned as part of pre-flight requests #423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/main/resources/reference.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@

cors {
accessControlMaxAge = 60 minutes
allowedCorsHeaders = ["X-Example"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This file provides defaults for the app. So by adding "X-Example" here, it means all pipelines everywhere will respond with "X-Example" in the list of allowed headers.

Guessing that's probably not what you want?

}

streams {
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/scala/com.snowplowanalytics.snowplow.collector.core/Config.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ object Config {
)

case class CORS(
accessControlMaxAge: FiniteDuration
accessControlMaxAge: FiniteDuration,
allowedCorsHeaders: List[String]
)

case class Streams[+SinkConfig](
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/scala/com.snowplowanalytics.snowplow.collector.core/Service.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import org.apache.commons.codec.binary.Base64
import scala.concurrent.duration._
import scala.jdk.CollectionConverters._

import cats.data.NonEmptyList
import cats.effect.{Clock, Sync}
import cats.implicits._

Expand Down Expand Up @@ -139,11 +140,13 @@ class Service[F[_]: Sync](
}

override def preflightResponse(req: Request[F]): F[Response[F]] = Sync[F].pure {
val allowedHeaders = (List("Content-Type", "SP-Anonymous") ++ config.cors.allowedCorsHeaders).map(CIString(_))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about just making Content-Type and SP-Anonymous a part of the allowedCorsHeaders setting? That would avoid rebuilding this list on every request. And actually allowedCorsHeaders is a misnomer if it does not include all allowed headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean as part of the config? My only concern would be if people remove this from a default config.

val corsHeaders = NonEmptyList.fromListUnsafe(allowedHeaders)
Comment on lines +143 to +144
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we can remove the "unsafe" method. Which is harmless in this case, but stands out as worrisome when you see it in collector code.

val corsHeaders = NonEmptyList("Content-Type", "SP-Anonymous" :: config.cors.allowedCorsHeaders).map(CIString(_))

Response[F](
headers = Headers(
accessControlAllowOriginHeader(req),
`Access-Control-Allow-Credentials`(),
`Access-Control-Allow-Headers`(ci"Content-Type", ci"SP-Anonymous"),
`Access-Control-Allow-Headers`(corsHeaders),
`Access-Control-Max-Age`.Cache(config.cors.accessControlMaxAge.toSeconds).asInstanceOf[`Access-Control-Max-Age`]
)
)
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/scala/com.snowplowanalytics.snowplow.collector.core/ServiceSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ class ServiceSpec extends Specification {
val expected = Headers(
Header.Raw(ci"Access-Control-Allow-Origin", "*"),
`Access-Control-Allow-Credentials`(),
`Access-Control-Allow-Headers`(ci"Content-Type", ci"SP-Anonymous"),
`Access-Control-Allow-Headers`(ci"Content-Type", ci"SP-Anonymous", ci"X-Howdy"),
`Access-Control-Max-Age`.Cache(3600).asInstanceOf[`Access-Control-Max-Age`]
)
service.preflightResponse(Request[IO]()).unsafeRunSync().headers shouldEqual expected
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/scala/com.snowplowanalytics.snowplow.collector.core/TestUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ object TestUtils {
Map.empty[String, String],
""
),
cors = CORS(60.minutes),
cors = CORS(60.minutes, List("X-Howdy")),
streams = Streams(
good = SinkConfig(
name = "raw",
Expand Down
3 changes: 3 additions & 0 deletions examples/config.kafka.extended.hocon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ collector {
# The Access-Control-Max-Age response header indicates how long the results of a preflight
# request can be cached. -1 seconds disables the cache. Chromium max is 10m, Firefox is 24h.
accessControlMaxAge = 60 minutes
# The allowedCorsHeaders response header allows non-safelisted CORS headers to be returned
# as part of OPTIONS (preflight) requests
allowedCorsHeaders = ["X-Example"]
}

streams {
Expand Down
3 changes: 3 additions & 0 deletions examples/config.kinesis.extended.hocon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ collector {
# The Access-Control-Max-Age response header indicates how long the results of a preflight
# request can be cached. -1 seconds disables the cache. Chromium max is 10m, Firefox is 24h.
accessControlMaxAge = 60 minutes
# The allowedCorsHeaders response header allows non-safelisted CORS headers to be returned
# as part of OPTIONS (preflight) requests
allowedCorsHeaders = ["X-Example"]
}

streams {
Expand Down
3 changes: 3 additions & 0 deletions examples/config.nsq.extended.hocon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ collector {
# The Access-Control-Max-Age response header indicates how long the results of a preflight
# request can be cached. -1 seconds disables the cache. Chromium max is 10m, Firefox is 24h.
accessControlMaxAge = 60 minutes
# The allowedCorsHeaders response header allows non-safelisted CORS headers to be returned
# as part of OPTIONS (preflight) requests
allowedCorsHeaders = ["X-Example"]
}

streams {
Expand Down
3 changes: 3 additions & 0 deletions examples/config.pubsub.extended.hocon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ collector {
# The Access-Control-Max-Age response header indicates how long the results of a preflight
# request can be cached. -1 seconds disables the cache. Chromium max is 10m, Firefox is 24h.
accessControlMaxAge = 60 minutes
# The allowedCorsHeaders response header allows non-safelisted CORS headers to be returned
# as part of OPTIONS (preflight) requests
allowedCorsHeaders = ["X-Example"]
}

streams {
Expand Down
3 changes: 3 additions & 0 deletions examples/config.sqs.extended.hocon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ collector {
# The Access-Control-Max-Age response header indicates how long the results of a preflight
# request can be cached. -1 seconds disables the cache. Chromium max is 10m, Firefox is 24h.
accessControlMaxAge = 60 minutes
# The allowedCorsHeaders response header allows non-safelisted CORS headers to be returned
# as part of OPTIONS (preflight) requests
allowedCorsHeaders = ["X-Example"]
}

streams {
Expand Down
3 changes: 3 additions & 0 deletions examples/config.stdout.extended.hocon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ collector {
# The Access-Control-Max-Age response header indicates how long the results of a preflight
# request can be cached. -1 seconds disables the cache. Chromium max is 10m, Firefox is 24h.
accessControlMaxAge = 60 minutes
# The allowedCorsHeaders response header allows non-safelisted CORS headers to be returned
# as part of OPTIONS (preflight) requests
allowedCorsHeaders = ["X-Example"]
}

streams {
Expand Down
2 changes: 1 addition & 1 deletion ...rc/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/KafkaConfigSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ object KafkaConfigSpec {
headers = Map.empty[String, String],
body = ""
),
cors = Config.CORS(1.hour),
cors = Config.CORS(1.hour, List("X-Example")),
monitoring = Config.Monitoring(
Config.Metrics(
Config.Statsd(false, "localhost", 8125, 10.seconds, "snowplow.collector", Map.empty)
Expand Down
2 changes: 1 addition & 1 deletion ...scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisConfigSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ object KinesisConfigSpec {
headers = Map.empty[String, String],
body = ""
),
cors = Config.CORS(1.hour),
cors = Config.CORS(1.hour, List("X-Example")),
monitoring = Config.Monitoring(
Config.Metrics(
Config.Statsd(false, "localhost", 8125, 10.seconds, "snowplow.collector", Map.empty)
Expand Down
2 changes: 1 addition & 1 deletion nsq/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/NsqConfigSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ object NsqConfigSpec {
headers = Map.empty[String, String],
body = ""
),
cors = Config.CORS(1.hour),
cors = Config.CORS(1.hour, List("X-Example")),
monitoring = Config.Monitoring(
Config.Metrics(
Config.Statsd(false, "localhost", 8125, 10.seconds, "snowplow.collector", Map.empty)
Expand Down
2 changes: 1 addition & 1 deletion pubsub/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/ConfigSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ object ConfigSpec {
headers = Map.empty[String, String],
body = ""
),
cors = Config.CORS(1.hour),
cors = Config.CORS(1.hour, List("X-Example")),
monitoring = Config.Monitoring(
Config.Metrics(
Config.Statsd(false, "localhost", 8125, 10.seconds, "snowplow.collector", Map.empty)
Expand Down
2 changes: 1 addition & 1 deletion sqs/src/test/scala/com.snowplowanalytics.snowplow.collectors.scalastream/SqsConfigSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ object SqsConfigSpec {
headers = Map.empty[String, String],
body = ""
),
cors = Config.CORS(1.hour),
cors = Config.CORS(1.hour, List("X-Example")),
monitoring = Config.Monitoring(
Config.Metrics(
Config.Statsd(false, "localhost", 8125, 10.seconds, "snowplow.collector", Map.empty)
Expand Down
Loading