[TC-007] - Prevent response body leakage in RestRetryInterceptor#3679
[TC-007] - Prevent response body leakage in RestRetryInterceptor#3679kdeakinstructure wants to merge 1 commit intomasterfrom
Conversation
…print it internally and throw only statuscode and content-type.
🧪 Unit Test Results📊 Summary
Last updated: Mon, 04 May 2026 14:46:56 GMT |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR improves diagnostics in RestRetryInterceptor by capturing and logging the response Content-Type alongside the body when all retries are exhausted — a useful improvement for debugging flaky test infrastructure.
Positive notes:
- Adding
contentTypeto the log is a good idea; it helps distinguish HTML error pages from JSON API errors at a glance. - Explicitly calling
response.close()afterbody?.string()is a safe defensive practice (it's a no-op when the body was read, but correctly closes an unconsumed body whenresponse.bodyis null).
Issues found:
- Potential sensitive data exposure in stdout (
line 45) — The body is printed unconditionally. If the Canvas API returns error responses that include tokens, credentials, or PII, those will appear in CI log artifacts. A length cap/truncation would also prevent large HTML error pages from flooding test output. - Body silently removed from exception message (
line 47) — The response body was previously part of theRuntimeExceptionmessage. Removing it is a behavior change: any caller or test framework that only captures the exception trace (not stdout) will lose that diagnostic info. Consider including a truncated body in the exception message to keep the information portable.
| val contentType = response.body?.contentType() | ||
| val body = response.body?.string() | ||
| throw RuntimeException("status code: $code\nbody: $body") | ||
| println("RestRetryInterceptor: all retries exhausted. status=$code, content-type=$contentType, body=$body") |
There was a problem hiding this comment.
The response body may contain sensitive data (auth tokens, user credentials, session info) returned from the Canvas API. Printing it unconditionally to stdout could expose it in CI log artifacts.
Consider either redacting the body or capping its length:
val truncatedBody = body?.take(500)?.let { if (body.length > 500) "$it…[truncated]" else it }
println("RestRetryInterceptor: all retries exhausted. status=$code, content-type=$contentType, body=$truncatedBody")This also guards against very large error bodies (HTML pages, stack traces) flooding test output.
| throw RuntimeException("status code: $code\nbody: $body") | ||
| println("RestRetryInterceptor: all retries exhausted. status=$code, content-type=$contentType, body=$body") | ||
| response.close() | ||
| throw RuntimeException("status code: $code, content-type: $contentType") |
There was a problem hiding this comment.
The body was deliberately removed from the exception message (previously it was included). This is a silent behavior change: callers that catch and log RuntimeException — or test frameworks that only capture the exception trace — will no longer see the response body. The println on line 45 helps when stdout is captured, but that isn't guaranteed in all CI environments.
Consider including at least the (possibly truncated) body in the message so the information travels with the exception:
throw RuntimeException("status code: $code, content-type: $contentType, body: ${body?.take(500)}")If the motivation for removing it was to avoid giant messages, a truncation strategy like this gives the best of both worlds.
| val contentType = response.body?.contentType() | ||
| val body = response.body?.string() | ||
| throw RuntimeException("status code: $code\nbody: $body") | ||
| println("RestRetryInterceptor: all retries exhausted. status=$code, content-type=$contentType, body=$body") |
There was a problem hiding this comment.
Don't have this println at all.
2 participants
Prevent response body leakage in RestRetryInterceptor
Instead, only print it internally and throw only statuscode and content-type.