Sanitize download stream filenames and update unit test infrastructure #2629
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Filenames come from the backend and don't contain dangerous characters but to play it safe we strip all sketchy characters and ensure there's no attempts at directory traversal. See https://github.local/HMDA-Operations/hmda-devops/issues/4932
Also update unit dependencies so that they run with the current version of React we use.
Our old unit tests were broken and using Jest. Vitest can be used for future ones. Also adds a test file for the download stream util file.
billhimmelsbach
approved these changes
Oct 23, 2025
Contributor
billhimmelsbach
left a comment
billhimmelsbach
left a comment
There was a problem hiding this comment.
This is really great! Approved 🚀
| import { describe, expect, it } from 'vitest' | ||
| import { sanitizeFileName } from '../downloadStream.js' | ||
|
|
||
| describe('sanitizeFileName', () => { |
Contributor
There was a problem hiding this comment.
You are a champion for getting the tests working / adding new tests for this. 🏆
| environment: 'jsdom', | ||
| globals: true, | ||
| include: ['src/**/__tests__/**/*.{test,spec}.{js,jsx}'], | ||
| exclude: ['oldTests/**', 'cypress/**', '**/node_modules/**'] |
Contributor
There was a problem hiding this comment.
I'm down for this approach of removing the old tests, then having a separate script to run them while excluding the broken tag ones. 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The filename passed to the frontend streaming API comes directly from the server and never contains anything nefarious but to play it safe, irregular characters and directory traversal attempts are now stripped out of the filename.
I also took the opportunity to get our legacy unit tests back online. About 80% fail due to outdated code references and/or other mysterious errors. I tagged all the broken ones with a
// @brokencomment at the top of the file and told the unit test npm script to ignore all files with that tag. I then installed Vitest for future unit tests and wrote a test for the new sanitization functionality.See #4932
Changes
// @brokencomment to the top of outdated legacy tests.Testing
npm unit-testsandnpm unit-tests-legacyshould pass.test.ymlworkflow.Notes
test:e2e,test:unit, etc. format but we can discuss that another day