Skip to content

Sanitize filenames prior to DSpace publication - #1534

Closed
jazairi wants to merge 2 commits into
mainfrom
etd-703
Closed

Sanitize filenames prior to DSpace publication#1534
jazairi wants to merge 2 commits into
mainfrom
etd-703

Conversation

@jazairi

@jazairi jazairi commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Why these changes are being introduced:

An end user reported that they could not download
a file from DSpace. We learned that this was
due to a decomposed unicode character, and found
an additional 9 theses that were broken due to
filenames with zero-width whitespace and en-dash
characters.

We've fixed the 10 published theses in ETD, but
we need a way to proactively address this in the
future.

Relevant ticket(s):

How this addresses that need:

This adds to the SQS Message model a method that
sanitizes filenames by removing the characters
mentioned above, plus a few other potential
culprits that we've not yet seen. (See side
effects.)

It avoids the more aggressive approach of changing
filenames via a lifecycle hook. That option was
considered, but a lower-intervention method
seemed preferable.

Side effects of this change:

This makes a few assumptions about potential
problematic characters that we've not yet seen
in the wild (e.g., hidden directional characters
and dash variants other than en-dash). It felt
useful to be proactive about this.

Developer

Accessibility
  • ANDI or WAVE has been run in accordance to our guide.
  • This PR contains no changes to the view layer.
  • New issues flagged by ANDI or WAVE have been resolved.
  • New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
  • No new accessibility issues have been flagged.
New ENV
  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.
Approval beyond code review
  • UXWS/stakeholder approval has been confirmed.
  • UXWS/stakeholder review will be completed retroactively.
  • UXWS/stakeholder review is not needed.
Additional context needed to review

I ran rubocop in its own commit because it updated a lot of unrelated lines of code. It might be useful to review the commits separately.

Code Reviewer

Code
  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.
Documentation
  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.
Testing
  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

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 adds filename sanitization to the DSpace Submission Service (DSS) SQS message generation to prevent DSpace publications that later fail to download due to problematic Unicode characters in bitstream names.

Changes:

  • Sanitize BitstreamName values in SqsMessage#map_files by normalizing Unicode (NFC) and removing/replacing known problematic invisible/dash characters.
  • Add unit tests covering normalization/removal/replacement behavior and an end-to-end assertion that sanitized filenames appear in map_files output.
  • Minor rubocop-driven refactors and small readability tweaks (e.g., numeric literals and degree-type predicate).

Reviewed changes

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

File Description
app/models/sqs_message.rb Sanitizes filenames used as BitstreamName before sending DSS payloads; minor readability tweaks.
test/models/sqs_message_test.rb Adds tests for new sanitization behavior and reformats existing assertions.
Suppressed comments (1)

test/models/sqs_message_test.rb:212

  • The filename used to simulate a decomposed accent is constructed as "garcía\u0301_thesis.pdf", which applies the combining acute accent to the preceding a (producing á) and leaves the precomposed í unchanged. After NFC normalization this would become garcíá_thesis.pdf, so the assertion expecting garcía_thesis.pdf is inconsistent. Build the decomposed input deterministically from the intended NFC string (e.g., .unicode_normalize(:nfd)).
    # Attach file with decomposed unicode
    @thesis.files.attach(io: File.open(f), filename: "garcía\u0301_thesis.pdf")
    @thesis.files.last.purpose = 'thesis_pdf'

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/models/sqs_message_test.rb
@jazairi
jazairi temporarily deployed to thesis-submit-pr-1534 August 14, 2026 22:50 Inactive
Why these changes are being introduced:

An end user reported that they could not download
a file from DSpace. We learned that this was
due to a decomposed unicode character, and found
an additional 9 theses that were broken due to
filenames with zero-width whitespace and en-dash
characters.

We've fixed the 10 published theses in ETD, but
we need a way to proactively address this in the
future.

Relevant ticket(s):

- [ETD-703](https://mitlibraries.atlassian.net/browse/ETD-703)
- [ETD-702](https://mitlibraries.atlassian.net/browse/ETD-702)

How this addresses that need:

This adds to the SQS Message model a method that
sanitizes filenames by removing the characters
mentioned above, plus a few other potential
culprits that we've not yet seen. (See side
effects.)

It avoids the more aggressive approach of changing
filenames via a lifecycle hook. That option was
considered, but a lower-intervention method
seemed preferable.

Side effects of this change:

This makes a few assumptions about potential
problematic characters that we've not yet seen
in the wild (e.g., hidden directional characters
and dash variants other than en-dash). It felt
useful to be proactive about this.
Running this as a separate commit because it touches several lines of code unrelated to the previous commit
@jazairi
jazairi temporarily deployed to thesis-submit-pr-1534 August 14, 2026 22:58 Inactive
@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 98.29% (+0.004%) from 98.286% — etd-703 into main

@jazairi jazairi closed this Aug 14, 2026
@jazairi
jazairi deleted the etd-703 branch August 14, 2026 23:02
@jazairi
jazairi restored the etd-703 branch August 14, 2026 23:02
@jazairi
jazairi deleted the etd-703 branch August 14, 2026 23:03
@jazairi
jazairi restored the etd-703 branch August 14, 2026 23:03
@jazairi
jazairi deleted the etd-703 branch August 14, 2026 23:04
@jazairi
jazairi restored the etd-703 branch August 14, 2026 23:05
@jazairi
jazairi deleted the etd-703 branch August 14, 2026 23:12
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.

4 participants