Skip to content

WPB-24669 [fix] upload of files with umlaut when audit log enabled - #5359

Merged
battermann merged 6 commits into
developfrom
WPB-24669-backend-upload-of-files-with-umlaut-not-possible-on-most-operating-systems
Jul 22, 2026
Merged

WPB-24669 [fix] upload of files with umlaut when audit log enabled#5359
battermann merged 6 commits into
developfrom
WPB-24669-backend-upload-of-files-with-umlaut-not-possible-on-most-operating-systems

Conversation

@battermann

@battermann battermann commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

https://wearezeta.atlassian.net/browse/WPB-24669

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jul 22, 2026
@battermann
battermann marked this pull request as ready for review July 22, 2026 11:35
@battermann
battermann requested review from a team as code owners July 22, 2026 11:35
@battermann
battermann requested a review from Copilot July 22, 2026 11:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes asset uploads failing for non-ASCII filenames (e.g. umlauts) when audit logging is enabled by percent-encoding the audit-log JSON metadata before putting it into S3 user-metadata headers, and decoding it when reading metadata back.

Changes:

  • Percent-encode audit-log metadata for S3 headers and URL-decode it on read-back.
  • Add a unit (QuickCheck) regression test ensuring produced S3 metadata header values are ASCII even with umlaut filenames.
  • Wire the new test module into the cargohold unit test suite and add a changelog entry.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
services/cargohold/src/CargoHold/S3.hs Encode audit-log metadata as percent-encoded ASCII for S3 headers; decode when reading metadata back.
services/cargohold/test/unit/Test/CargoHold/S3Test.hs Adds a QuickCheck property asserting the metadata header value remains ASCII for umlaut filenames.
services/cargohold/test/unit/Main.hs Registers the new S3 unit tests in the test runner.
services/cargohold/cargohold.cabal Adds the new test module to the test-suite’s other-modules.
changelog.d/3-bug-fixes/WPB-24669 Documents the bug fix and the encoding/decoding approach.

Comment thread services/cargohold/src/CargoHold/S3.hs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

services/cargohold/src/CargoHold/S3.hs:431

  • decodePercentEncoded uses decodeLatin1 on the percent-decoded bytes and then parseJSON re-encodes with encodeUtf8. For non-ASCII filenames this corrupts the original UTF-8 JSON bytes (bytes >= 0x80 become multi-byte UTF-8), so percent-encoded audit-log metadata cannot be decoded correctly.

A simpler and correct approach is to parse JSON directly from the strict ByteString (either the raw header bytes, or the urlDecoded bytes) and avoid the Text roundtrip.

    parseJSON :: Text -> Maybe AssetAuditLogMetadata
    parseJSON = A.decode . fromStrict . encodeUtf8

    decodePercentEncoded :: Text -> Text
    decodePercentEncoded = decodeLatin1 . HTTPURI.urlDecode False . encodeUtf8

Comment thread services/cargohold/test/unit/Test/CargoHold/S3Test.hs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread services/cargohold/src/CargoHold/S3.hs Outdated
Comment thread services/cargohold/test/unit/Test/CargoHold/S3Test.hs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@battermann
battermann merged commit 0da4884 into develop Jul 22, 2026
10 checks passed
@battermann
battermann deleted the WPB-24669-backend-upload-of-files-with-umlaut-not-possible-on-most-operating-systems branch July 22, 2026 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants