-
Notifications
You must be signed in to change notification settings - Fork 5
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
Secure cookie fix #160
Secure cookie fix #160
Conversation
newm-server/build.gradle.kts
Outdated
@@ -57,6 +57,7 @@ dependencies { | |||
implementation(Dependencies.Ktor.SERIALIZATION_JSON) | |||
implementation(Dependencies.Ktor.SERVER_STATUS_PAGES) | |||
implementation(Dependencies.Ktor.SERVER_CORS) | |||
implementation(Dependencies.Ktor.SERVER_FORWARDED_HEADER) |
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.
per ktorio/ktor#1112 - ktor verifies that you are using https
protcol when you set the Secure option on a cookie. Because CIO does not support https
we need to add support for forwarding X-Forwarded-SSL
headers in order to use Secure
cookies
@@ -4,10 +4,9 @@ import kotlinx.serialization.Serializable | |||
|
|||
@Serializable | |||
data class AudioStreamResponse( | |||
val url: String, | |||
val cookies: Map<String, String> | |||
val url: 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.
Removed cookie argument because it is not required in the response (cookies are passed in the header)
@@ -789,6 +789,9 @@ class SongRoutesTests : BaseApplicationTests() { | |||
val response = client.get("v1/songs/$songId/stream") { | |||
bearerAuth(testUserToken) | |||
contentType(ContentType.Application.Json) | |||
url { |
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.
Have to explicitly set the test protocol to HTTPS in order to satisfy the Secure cookie check.
@@ -797,11 +800,6 @@ class SongRoutesTests : BaseApplicationTests() { | |||
val environment: ApplicationEnvironment by inject() | |||
val updatedHost = environment.getConfigString("aws.cloudFront.audioStream.hostUrl") | |||
assertThat(resp.url).startsWith(updatedHost) | |||
|
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.
No longer required
response.cookies.append( | ||
name = cookie.key, | ||
value = cookie.value, | ||
domain = "newm.io", |
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.
domain can be referenced from application.conf with a default value of "newm.io".
No description provided.