Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes 433: convert latin1 to utf for filenames return by multer #908

Merged
merged 1 commit into from May 30, 2023

Conversation

sadiqkhoja
Copy link
Contributor

@sadiqkhoja sadiqkhoja commented May 30, 2023

Closes getodk/central#433

Root cause:

Multer is parsing filename with latin1 encoding instead of UTF-8. expressjs/multer#1104

What has been done to verify that this works as intended?

  • Tested locally + integration test

Why is this the best possible solution? Were any other approaches considered?

Created local middleware to convert latin1 encoding to utf8

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This is localized changed, if submission are tested from Enketo and Collect are tested then we should be good

Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.

None

Before submitting this PR, please make sure you have:

  • run make test-full and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

@sadiqkhoja sadiqkhoja added the needs testing Needs manual testing label May 30, 2023
@sadiqkhoja sadiqkhoja marked this pull request as ready for review May 30, 2023 21:55
@sadiqkhoja sadiqkhoja merged commit 3095cb0 into getodk:master May 30, 2023
1 check passed
@lognaturel
Copy link
Member

Thank you @sadiqkhoja

Not great that a seemingly routine dependency version bump led to data loss because of a subdependency change. A good reminder that we are at the mercy of our dependencies. Not sure there's anything we could have done differently to catch this one.

@matthew-white matthew-white removed the needs testing Needs manual testing label May 31, 2023
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.

Submissions with attachments with unicode filenames are not saved
3 participants