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

Build: Fix an XSS in the test server HTML serving logic #2309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgol
Copy link
Member

@mgol mgol commented Oct 25, 2024

The test server has a rule for /tests/unit/*/*.html paths that serves a proper local file. However, the parameters after /unit/ were so far not escaped, leading to possibly reading a file from outside of the Git repository. Fix that by replacing non-alphanumeric characters that are also not - or _.

This should resolve one CodeQL alert.

@mgol mgol added this to the 1.14.1 milestone Oct 25, 2024
@mgol mgol requested a review from timmywil October 25, 2024 22:36
@mgol mgol self-assigned this Oct 25, 2024
The test server has a rule for `/tests/unit/*/*.html` paths that serves
a proper local file. However, the parameters after `/unit/` were so far not
escaped, leading to possibly reading a file from outside of the Git repository.
Fix that by replacing non-alphanumeric characters that are also not `-` or `_`.

This should resolve one CodeQL alert.
const html = await readFile(
`tests/unit/${ req.params[ 0 ] }/${ req.params[ 0 ] }.html`,
`tests/unit/${ moduleEscaped }/${ moduleEscaped }.html`,

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High test

This path depends on a
user-provided value
.
Copy link
Member Author

Choose a reason for hiding this comment

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

Funny, considering this is exactly the issue I'm fixing in this PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

1 participant