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

[media] Fix SQL injection #8908

Merged
merged 1 commit into from
Oct 3, 2023
Merged

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Oct 2, 2023

This fixes 2 problems with the SQL in the media FileUpload?action=getData endpoint

  1. There is an obvious SQL injection attack where user input from the request is directly concatenated into a string that's passed to the database.
  2. There was an unnecessary sub-select that could have been a join

This whole section of the code is a mess that should to be re-written, but this PR just tackles the urgent string concatenation.

@driusan driusan added Category: Security PR or issue that aims to improve security Priority: High PR or issue should be prioritised over others for review and testing labels Oct 2, 2023
This fixes 2 problems with the SQL in the media FileUpload?action=getData
endpoint
1. There is an obvious SQL injection attack where user input from the
   request is directly concatenated into a string that's passed to the
   database.
2. There was an unnecessary sub-select that could have been a join

This whole section of the code is a mess that should to be re-written,
but this PR just tackles the urgent string concatenation.
@driusan driusan force-pushed the MediaSQLInjection branch from a7ac54b to 0bf6cd0 Compare October 2, 2023 18:01
@driusan driusan changed the base branch from 25.0-release to 24.1-release October 2, 2023 18:02
@driusan driusan closed this Oct 2, 2023
@driusan driusan reopened this Oct 2, 2023
@CamilleBeau CamilleBeau added the Passed manual tests PR has been successfully tested by at least one peer label Oct 3, 2023
@CamilleBeau
Copy link
Contributor

Tested and reviewed, working well and looks good.

@xlecours
Copy link
Contributor

xlecours commented Oct 3, 2023

@driusan , can we get this merge today? I would like to have it on HBCD and tomorrow would be ideal because it's maintenance day.
Thank you

@driusan driusan merged commit 9b08f46 into aces:24.1-release Oct 3, 2023
9 checks passed
@driusan
Copy link
Collaborator Author

driusan commented Oct 3, 2023

@xlecours @CamilleBeau reviewed and approved it so yes.

Do you need the release tagged today or just the PR merged?

@xlecours
Copy link
Contributor

xlecours commented Oct 3, 2023

@driusan it can wait tomorrow

@ridz1208 ridz1208 added this to the 24.1.5 milestone Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Security PR or issue that aims to improve security Passed manual tests PR has been successfully tested by at least one peer Priority: High PR or issue should be prioritised over others for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants