AWS S3: Handle Illegal Headers in Copy Part #1246

Luka-J9 · 2025-10-16T21:44:08Z

Currently this is a minimal change to demonstrate the issue so that the strategy can be discussed before addressed.

Luka-J9 · 2025-10-16T21:49:23Z

s3/src/test/scala/docs/scaladsl/S3SinkSpec.scala

+          .withCustomHeaders(
+            Map(
+              requestPayerHeader -> requestPayerHeaderValue,
+              storgeClassHeader -> storageClassHeaderValue,


This adds the header that is valid for creating the multipart copy request, but is not valid for the put requests

Docs list all valid headers here

I realize that there is also the storage class header value, but it also doesn't include all valid header values which is why I'm adding it this way. Additionally looking at the code it appears the behavior would be the same.

Luka-J9 · 2025-10-16T21:51:10Z

s3/src/test/scala/docs/scaladsl/S3SinkSpec.scala

        .withHeader(sseCSourceAlgorithmHeader, new EqualToPattern(sseCSourceAlgorithmHeaderValue))
        .withHeader(sseCSourceKeyHeader, new EqualToPattern(sseCSourceKeyHeaderValue))
-        .withHeader(requestPayerHeader, new EqualToPattern(requestPayerHeaderValue)))
+        .withHeader(requestPayerHeader, new EqualToPattern(requestPayerHeaderValue))


This is showing the correct outcome, but with the change in S3Stream.scala we remove the valid header of requestPayerHeaderValue. Without it we get illegal storageClassHeader

This test now passes with the proposed change

Luka-J9 · 2025-10-16T22:07:09Z

@mdedetrich / @pjfanning - I made this draft PR to demonstrate the problem in order to discuss the path forward for fixing this problem. I'm going to look to see if I can create an integration test tomorrow.

I'm thinking that we have an allow list of all correct headers and filter out all the headers that start with the x-amz- that do not belong to that list?

I say starts with x-amz- as there might exist use cases with S3 compliant stores that have custom headers. This is just pure speculation however.

The main con here is that if AWS introduces new headers in the future this would require an update. Alternatively we may wish to simply remove x-amz-storage-class and create a block list, which would still need to be maintained.

Open to alternative suggestions if there are any

mdedetrich · 2025-10-17T06:04:31Z

@mdedetrich / @pjfanning - I made this draft PR to demonstrate the problem in order to discuss the path forward for fixing this problem. I'm going to look to see if I can create an integration test tomorrow.

I'm thinking that we have an allow list of all correct headers and filter out all the headers that start with the x-amz- that do not belong to that list?

I say starts with x-amz- as there might exist use cases with S3 compliant stores that have custom headers. This is just pure speculation however.

The main con here is that if AWS introduces new headers in the future this would require an update. Alternatively we may wish to simply remove x-amz-storage-class and create a block list, which would still need to be maintained.

Open to alternative suggestions if there are any

I would say, if we do have an allowlist it should be provided in as configuration in reference.conf so people can add more values without needing us to do an additional release. The initial list can be hardcoded, but there can be additional pekko.connectors.s3.additional-upload-whitelist-headers configuration where people can also add additional headers in the case that s3 adds something new.

@pjfanning @raboof thoughts?

raboof · 2025-10-17T15:01:48Z

Sorry, I don't think I understand s3 well enough to have a meaningful opinion here

pjfanning · 2025-10-17T15:41:06Z

I have no strong opinion about being proactive about headers. It seems to introduce its own risks. If we do introduce a new config, avoid any name that uses colour based naming, eg white or black.

Luka-J9 · 2025-10-17T17:22:21Z

@mdedetrich / @pjfanning - Do I need to worry about source/binary compat? Not sure I can add the configuration handling without breaking compatibility on some level

Additionally I assume I should be handling the headers the same way for all header S3Request types and not just CopyPart?

pjfanning · 2025-10-17T18:32:37Z

@mdedetrich / @pjfanning - Do I need to worry about source/binary compat? Not sure I can add the configuration handling without breaking compatibility on some level

Additionally I assume I should be handling the headers the same way for all header S3Request types and not just CopyPart?

@Luka-J9 it's ok to break binary compatibility on main branch as it is targeted at a 2.0.0 release. Just add a mima-filters file. You'll find examples in the src code already (.excludes file extension).
We can decide based on whether it is safe to backport to a 1.3.0 release after that. We would accept some level of change for that but it would probably need careful thought as to what is acceptable.

mdedetrich · 2025-10-17T18:54:14Z

@mdedetrich / @pjfanning - Do I need to worry about source/binary compat? Not sure I can add the configuration handling without breaking compatibility on some level

Additionally I assume I should be handling the headers the same way for all header S3Request types and not just CopyPart?

So I am not sure how this change would be binary or even source breaking, it should be just internal behaviour without needing to change and function/method signatures. We already have a mima check that will verify if there is a binary breaking change.

Regarding adding the configuration setting, this is fine as long as its released in a version with a bumped minor version.

Luka-J9 · 2025-10-17T19:04:18Z

s3/src/main/scala/org/apache/pekko/stream/connectors/s3/impl/S3Request.scala

-@InternalApi private[s3] case object HeadObject extends S3Request
+@InternalApi private[s3] case object GetObject extends S3Request {
+
+  override def allowedHeaders: Set[String] = Set(


I'll add links to where in the docs I sourced this from

I'll also double check these when I move this out of draft

Luka-J9 · 2025-10-17T19:05:02Z

s3/src/main/scala/org/apache/pekko/stream/connectors/s3/impl/S3Stream.scala

      bucket: String,
      method: HttpMethod,
      httpRequest: (HttpMethod, S3Settings) => HttpRequest,
-      headers: Seq[HttpHeader],


Oddly it seems this was unused

Yes I noticed this as well, if you want to fix this I would prefer to put it in a separate PR so we don't mix this together (its easier to bisect/revert changes if they are in their own PR's/commits)

Luka-J9 · 2025-10-17T19:10:41Z

I've updated the PR with something more concrete in terms of implementation. I still need to add tests and docs, but wanted to get this out there for some early feedback about the strategy. If the approach is good I can add tests and move this out of draft for a more detailed and formal review

Luka-J9 · 2025-10-20T18:05:59Z

@mdedetrich / @pjfanning - turns out the only errors when running mimaReportBinaryIssues were with one of the case classes creation methods, which only required a couple of compatibility functions to be added 🥳

If the general structure is okay - I'll write tests in the next day or two, just would like a rough 👍 before investing more time into this approach

Minimal update to demonstrate problem

ad49f3a

Luka-J9 commented Oct 16, 2025

View reviewed changes

Luka-J9 marked this pull request as draft October 16, 2025 21:52

Luka-J9 changed the title ~~[DRAFT] AWS S3: Handle Illegal Headers in Copy Part~~ AWS S3: Handle Illegal Headers in Copy Part Oct 16, 2025

update to structure discussed in PR

272bc2a

Luka-J9 commented Oct 17, 2025

View reviewed changes

add functions to remove mima issues

37c46d6

fix broken test

e5f5c63

AWS S3: Handle Illegal Headers in Copy Part #1246

Are you sure you want to change the base?

AWS S3: Handle Illegal Headers in Copy Part #1246

Conversation

Luka-J9 commented Oct 16, 2025

Uh oh!

Luka-J9 Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Luka-J9 Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Luka-J9 Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Luka-J9 commented Oct 16, 2025

Uh oh!

mdedetrich commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raboof commented Oct 17, 2025

Uh oh!

pjfanning commented Oct 17, 2025

Uh oh!

Luka-J9 commented Oct 17, 2025

Uh oh!

pjfanning commented Oct 17, 2025

Uh oh!

mdedetrich commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luka-J9 Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Luka-J9 Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Luka-J9 Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

mdedetrich Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Luka-J9 commented Oct 17, 2025

Uh oh!

Luka-J9 commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mdedetrich commented Oct 17, 2025 •

edited

Loading

mdedetrich commented Oct 17, 2025 •

edited

Loading