Strip "authorization" header before proxying to s3

[donaldgray]

What does this change?

Fixes #1219

Main change is stripping "Authorization" header before proxying to S3. Inclusion of this was breaking streaming when using presignedUrls as s3 detected 2 authmechanisms.

This strips the header from the outgoing proxy request - yarp will copy all headers then we strip out "authorization", this will still handle incoming auth header for restricted content.

Made a small tweak to Yarp PathRewriteTransformer as part of change, no behaviour change. Initially this was to fix warnings but then noticed that a possible change in how yarp works could alter behaviour. The change was

• await TransformResponseAsync and store the response, rather than always returning true.
• Use TransformResponseAsync overload that accepts cancellation token, rather than current overload as that's obsolete.

The "Optionally handle this to not expose the full s3 error." step in issue was not tackled here, we can look to do this in future if required.

[donaldgray]

Note

As discussed internally with @tomcrane and @JackLewis-digirati , will merge this without review due to team annual leave.

Confirming I used Claude Opus 4.8 for review, which didn't note any issues. Will drop review below as it confirms changes made to Yarp

Reviewed the three commits on bugfix/strip_auth_for_s3 against issue #1219. I ran the unit tests (26 passed) and the FileHandlingTests integration suite (14 passed) to confirm the change behaves as claimed.

The fix is correct

Stripping Authorization in PathRewriteTransformer.TransformRequestAsync when proxyAction.Target == ProxyDestination.S3 is the right place — all four S3-capable route handlers (Files, Adjuncts, TimeBased, Images) construct this same transformer, so there's no second path to S3 that bypasses the strip. HttpHeaders.Remove matches case-insensitively, so a lowercase authorization from an HTTP/2 client is handled too.

The TransformResponseAsync change is a real bug fix, not just tidying

PathRewriteTransformer.cs:52 previously overrode YARP's obsolete two-argument overload and called base.TransformResponseAsync(...) without awaiting it, discarding the returned ValueTask. It happened to work because the base implementation completes synchronously, but it also meant the method always returned true rather than honouring the base's decision on whether to proxy the body. Moving to the three-argument override and returning the base's result is the correct shape.

I checked that the base still copies downstream response headers under the new override — TransformResponseAsync_DoesNotChangeCORSHeader_IfAlreadyInResponseFromDownstream only passes if the downstream Access-Control-Allow-Origin reaches context.Response, and it passes. So no regression there.

Two things worth considering

The issue's optional second half — "handle this to not expose the full s3 error" — isn't addressed. Given the fix removes the cause of that particular 400, leaving it is defensible, but S3 error bodies (with RequestId/HostId) can still reach clients through other failure modes. Worth deciding explicitly rather than by omission.

The integration test's NotContainKey("Authorization") assertion could pass vacuously if the header key ever serialised with different casing, since Dictionary<string, string[]> from System.Text.Json is case-sensitive on lookup. The unit test guards against exactly this with an explicit ContainKey pre-assertion; the integration test has no equivalent. Adding proxyResponse.Headers.Should().ContainKey("Host") — or building the dictionary with StringComparer.OrdinalIgnoreCase in ProxyResponse — would close that gap.

Minor: the Headers.Add → Headers.Append change at line 95 is an unrelated drive-by that silences an ASP0019 analyser warning. It's harmless and correct, just not part of the fix — the same warning still fires at Orchestrator/Infrastructure/ControllerX.cs:73 if you want to sweep them together.