Skip to content

Conversation

lauzadis
Copy link
Contributor

@lauzadis lauzadis commented Sep 15, 2025

We were appending to Content-Length rather than overwriting it. This means if content length was already set by the user, we send a list of content lengths, causing a signature mismatch.

Issue #

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lauzadis lauzadis requested a review from a team as a code owner September 15, 2025 16:35

This comment has been minimized.

This comment has been minimized.

1 similar comment

This comment has been minimized.

@lauzadis lauzadis changed the title fix: override Content-Length rather than appending fix: overwrite Content-Length rather than appending Sep 15, 2025

This comment has been minimized.

1 similar comment

This comment has been minimized.

Comment on lines 70 to 73
val contentLength = body.contentLength?.takeIf { it >= 0 }?.toString() ?: headers[CONTENT_LENGTH_HEADER]
contentLength?.let { crtHeaders.append(CONTENT_LENGTH_HEADER, it) }
contentLength?.let { crtHeaders[CONTENT_LENGTH_HEADER] = it }

return aws.sdk.kotlin.crt.http.HttpRequest(method.name, url.requestRelativePath, crtHeaders.build(), bodyStream)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a new test for this? And also one that verifies the (presumably already correct?) behavior in OkHttp engine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some tests

This comment has been minimized.

This comment has been minimized.

1 similar comment
Copy link

Affected Artifacts

No artifacts changed size

@lauzadis lauzadis merged commit 94b2959 into main Sep 23, 2025
21 checks passed
@lauzadis lauzadis deleted the fix-crt-content-length branch September 23, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants