Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,28 @@ constructor(
private fun getHeader(key: String, headers: List<HttpHeader>): String? =
headers.firstOrNull { it.name.equals(key, true) }?.value

private fun getRequestCookies(headers: List<HttpHeader>): String? {
val cookies = getHeader("Cookie", headers)
return if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
HttpUtils.filterCookies(cookies, scopes.options.dataCollectionResolver.cookies, null)
} else if (scopes.options.isSendDefaultPii) {
cookies
} else {
null
}
}

private fun getResponseCookies(headers: List<HttpHeader>): String? {
val cookies = getHeader("Set-Cookie", headers)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There could be multiple Cookie and Set-Cookie headers. here we are currently only ever processing one of them and send them to sentry. Is that what we want or should we go through all of these?

return if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
HttpUtils.filterSetCookie(cookies, scopes.options.dataCollectionResolver.cookies)
} else if (scopes.options.isSendDefaultPii) {
cookies
} else {
null
}
}

private fun getRequestHeaders(headers: List<HttpHeader>): MutableMap<String, String>? {
if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
val requestHeaders = mutableMapOf<String, String>()
Expand Down Expand Up @@ -391,9 +413,7 @@ constructor(
val sentryRequest =
Request().apply {
urlDetails.applyToRequest(this)
// Cookie is only sent if isSendDefaultPii is enabled
cookies =
if (scopes.options.isSendDefaultPii) getHeader("Cookie", request.headers) else null
cookies = getRequestCookies(request.headers)
method = request.method.name
headers = getRequestHeaders(request.headers)
apiTarget = "graphql"
Expand All @@ -419,13 +439,7 @@ constructor(

val sentryResponse =
Response().apply {
// Set-Cookie is only sent if isSendDefaultPii is enabled due to PII
cookies =
if (scopes.options.isSendDefaultPii) {
getHeader("Set-Cookie", response.headers)
} else {
null
}
cookies = getResponseCookies(response.headers)
headers = getResponseHeaders(response.headers)
statusCode = response.statusCode

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class SentryApollo3InterceptorClientErrors {
httpStatusCode: Int = 200,
responseBody: String = responseBodyOk,
sendDefaultPii: Boolean = false,
includeCookies: Boolean = sendDefaultPii,
socketPolicy: SocketPolicy = SocketPolicy.KEEP_OPEN,
configureOptions: SentryOptions.() -> Unit = {},
): ApolloClient {
Expand All @@ -98,8 +99,8 @@ class SentryApollo3InterceptorClientErrors {
.setSocketPolicy(socketPolicy)
.setResponseCode(httpStatusCode)

if (sendDefaultPii) {
response.addHeader("Set-Cookie", "Test")
if (includeCookies) {
response.addHeader("Set-Cookie", "theme=dark; Path=/")
}

server.enqueue(response)
Expand All @@ -112,8 +113,8 @@ class SentryApollo3InterceptorClientErrors {
captureFailedRequests = captureFailedRequests,
failedRequestTargets = failedRequestTargets,
)
if (sendDefaultPii) {
builder.addHttpHeader("Cookie", "Test")
if (includeCookies) {
builder.addHttpHeader("Cookie", "theme=dark; sessionId=secret")
}

return builder.build()
Expand Down Expand Up @@ -362,6 +363,46 @@ class SentryApollo3InterceptorClientErrors {
)
}

@Test
fun `data collection filters cookies`() {
val sut =
fixture.getSut(responseBody = fixture.responseBodyNotOk, includeCookies = true) {
dataCollection.cookies = KeyValueCollectionBehavior.denyList("theme")
}
executeQuery(sut)

verify(fixture.scopes)
.captureEvent(
check {
assertEquals("theme=[Filtered]; sessionId=[Filtered]", it.request!!.cookies)
assertEquals("theme=[Filtered]; Path=/", it.contexts.response!!.cookies)
},
any<Hint>(),
)
}

@Test
fun `data collection can disable cookies`() {
val sut =
fixture.getSut(
responseBody = fixture.responseBodyNotOk,
sendDefaultPii = true,
includeCookies = true,
) {
dataCollection.cookies = KeyValueCollectionBehavior.off()
}
executeQuery(sut)

verify(fixture.scopes)
.captureEvent(
check {
assertNull(it.request!!.cookies)
assertNull(it.contexts.response!!.cookies)
},
any<Hint>(),
)
}

@Test
fun `data collection can disable request headers`() {
val sut =
Expand All @@ -387,7 +428,7 @@ class SentryApollo3InterceptorClientErrors {
check {
val request = it.request!!

assertEquals("Test", request.cookies)
assertEquals("theme=dark; sessionId=secret", request.cookies)
assertNotNull(request.headers)
assertEquals("LaunchDetails", request.headers?.get("X-APOLLO-OPERATION-NAME"))
},
Expand Down Expand Up @@ -477,7 +518,7 @@ class SentryApollo3InterceptorClientErrors {
check {
val response = it.contexts.response!!

assertEquals("Test", response.cookies)
assertEquals("theme=dark; Path=/", response.cookies)
assertNotNull(response.headers)
assertEquals(200, response.headers?.get("Content-Length")?.toInt())
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,28 @@ constructor(
private fun getHeader(key: String, headers: List<HttpHeader>): String? =
headers.firstOrNull { it.name.equals(key, true) }?.value

private fun getRequestCookies(headers: List<HttpHeader>): String? {
val cookies = getHeader("Cookie", headers)
return if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
HttpUtils.filterCookies(cookies, scopes.options.dataCollectionResolver.cookies, null)
} else if (scopes.options.isSendDefaultPii) {
cookies
} else {
null
}
}

private fun getResponseCookies(headers: List<HttpHeader>): String? {
val cookies = getHeader("Set-Cookie", headers)
return if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
HttpUtils.filterSetCookie(cookies, scopes.options.dataCollectionResolver.cookies)
} else if (scopes.options.isSendDefaultPii) {
cookies
} else {
null
}
}

private fun getRequestHeaders(headers: List<HttpHeader>): MutableMap<String, String>? {
if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
val requestHeaders = mutableMapOf<String, String>()
Expand Down Expand Up @@ -390,9 +412,7 @@ constructor(
val sentryRequest =
Request().apply {
urlDetails.applyToRequest(this)
// Cookie is only sent if isSendDefaultPii is enabled
cookies =
if (scopes.options.isSendDefaultPii) getHeader("Cookie", request.headers) else null
cookies = getRequestCookies(request.headers)
method = request.method.name
headers = getRequestHeaders(request.headers)
apiTarget = "graphql"
Expand All @@ -418,13 +438,7 @@ constructor(

val sentryResponse =
Response().apply {
// Set-Cookie is only sent if isSendDefaultPii is enabled due to PII
cookies =
if (scopes.options.isSendDefaultPii) {
getHeader("Set-Cookie", response.headers)
} else {
null
}
cookies = getResponseCookies(response.headers)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There could be multiple Cookie and Set-Cookie headers. here we are currently only ever processing one of them and send them to sentry. Is that what we want or should we go through all of these?

headers = getResponseHeaders(response.headers)
statusCode = response.statusCode

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ abstract class SentryApollo4BuilderExtensionsClientErrorsTest(
httpStatusCode: Int = 200,
responseBody: String = responseBodyOk,
sendDefaultPii: Boolean = false,
includeCookies: Boolean = sendDefaultPii,
socketPolicy: SocketPolicy = SocketPolicy.KEEP_OPEN,
configureOptions: SentryOptions.() -> Unit = {},
): ApolloClient {
Expand All @@ -112,8 +113,8 @@ abstract class SentryApollo4BuilderExtensionsClientErrorsTest(
.setSocketPolicy(socketPolicy)
.setResponseCode(httpStatusCode)

if (sendDefaultPii) {
response.addHeader("Set-Cookie", "Test")
if (includeCookies) {
response.addHeader("Set-Cookie", "theme=dark; Path=/")
}

server.enqueue(response)
Expand All @@ -126,8 +127,8 @@ abstract class SentryApollo4BuilderExtensionsClientErrorsTest(
captureFailedRequests = captureFailedRequests,
failedRequestTargets = failedRequestTargets,
)
if (sendDefaultPii) {
builder.addHttpHeader("Cookie", "Test")
if (includeCookies) {
builder.addHttpHeader("Cookie", "theme=dark; sessionId=secret")
}

return builder.build()
Expand Down Expand Up @@ -356,6 +357,46 @@ abstract class SentryApollo4BuilderExtensionsClientErrorsTest(
)
}

@Test
fun `data collection filters cookies`() {
val sut =
fixture.getSut(responseBody = fixture.responseBodyNotOk, includeCookies = true) {
dataCollection.cookies = KeyValueCollectionBehavior.denyList("theme")
}
executeQuery(sut)

verify(fixture.scopes)
.captureEvent(
check {
assertEquals("theme=[Filtered]; sessionId=[Filtered]", it.request!!.cookies)
assertEquals("theme=[Filtered]; Path=/", it.contexts.response!!.cookies)
},
any<Hint>(),
)
}

@Test
fun `data collection can disable cookies`() {
val sut =
fixture.getSut(
responseBody = fixture.responseBodyNotOk,
sendDefaultPii = true,
includeCookies = true,
) {
dataCollection.cookies = KeyValueCollectionBehavior.off()
}
executeQuery(sut)

verify(fixture.scopes)
.captureEvent(
check {
assertNull(it.request!!.cookies)
assertNull(it.contexts.response!!.cookies)
},
any<Hint>(),
)
}

@Test
fun `data collection filters request headers`() {
val sut =
Expand Down Expand Up @@ -398,7 +439,7 @@ abstract class SentryApollo4BuilderExtensionsClientErrorsTest(
check {
val request = it.request!!

assertEquals("Test", request.cookies)
assertEquals("theme=dark; sessionId=secret", request.cookies)
assertNotNull(request.headers)
},
any<Hint>(),
Expand Down Expand Up @@ -487,7 +528,7 @@ abstract class SentryApollo4BuilderExtensionsClientErrorsTest(
check {
val response = it.contexts.response!!

assertEquals("Test", response.cookies)
assertEquals("theme=dark; Path=/", response.cookies)
assertNotNull(response.headers)
assertEquals(200, response.headers?.get("Content-Length")?.toInt())
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,16 @@ internal object SentryKtorClientUtils {

val sentryRequest =
io.sentry.protocol.Request().apply {
// Cookie is only sent if isSendDefaultPii is enabled
urlDetails.applyToRequest(this)
cookies = if (scopes.options.isSendDefaultPii) request.headers["Cookie"] else null
cookies = getRequestCookies(scopes, request.headers["Cookie"])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There could be multiple Cookie and Set-Cookie headers. we are currently only ever processing one of them and send them to sentry. Is that what we want or should we go through all of these?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've created #5982 so we can change this (likely) in the next major.

method = request.method.value
headers = getRequestHeaders(scopes, request.headers)
bodySize = request.content.contentLength
}

val sentryResponse =
io.sentry.protocol.Response().apply {
// Set-Cookie is only sent if isSendDefaultPii is enabled due to PII
cookies = if (scopes.options.isSendDefaultPii) response.headers["Set-Cookie"] else null
cookies = getResponseCookies(scopes, response.headers["Set-Cookie"])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There could be multiple Cookie and Set-Cookie headers. we are currently only ever processing one of them and send them to sentry. Is that what we want or should we go through all of these?

headers = getResponseHeaders(scopes, response.headers)
statusCode = response.status.value
try {
Expand All @@ -67,6 +65,28 @@ internal object SentryKtorClientUtils {
scopes.captureEvent(event, hint)
}

private fun getRequestCookies(scopes: IScopes, cookies: String?): String? =
if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
HttpUtils.filterCookies(
cookies,
scopes.options.dataCollectionResolver.cookies,
null,
)
} else if (scopes.options.isSendDefaultPii) {
cookies
} else {
null
}

private fun getResponseCookies(scopes: IScopes, cookies: String?): String? =
if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
HttpUtils.filterSetCookie(cookies, scopes.options.dataCollectionResolver.cookies)
} else if (scopes.options.isSendDefaultPii) {
cookies
} else {
null
}

private fun getRequestHeaders(scopes: IScopes, headers: Headers): MutableMap<String, String>? {
if (scopes.options.dataCollectionResolver.isDataCollectionConfigured) {
val requestHeaders =
Expand Down
Loading
Loading