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

[Security] Fix Admin Translation Export SQL Injection #14968

Merged
merged 5 commits into from Apr 20, 2023

Conversation

mattamon
Copy link
Contributor

@mattamon mattamon commented Apr 19, 2023

Additional info

WHAT

🤖 Generated by Copilot at 87dcc28

This pull request enhances the translation controller by using safer and more flexible SQL queries, and by handling errors gracefully. It also allows more advanced filtering options for the translation grid.

🤖 Generated by Copilot at 87dcc28

To make the translation controller secure
We use placeholders and catch errors for sure
We also change getGridFilterCondition
To return an array with more precision
And support filters that are complex and obscure

HOW

🤖 Generated by Copilot at 87dcc28

  • Import SyntaxErrorException class to handle syntax errors in SQL queries (link)
  • Modify exportAction and translationsAction methods to use array format of getGridFilterCondition method and catch SyntaxErrorException (link, link, link, link)
  • Refactor getGridFilterCondition method to return array with condition and parameters, use placeholders, and support admin filter (link, link, link, link, link, link)

@mattamon mattamon added this to the 10.5.21 milestone Apr 19, 2023
@github-actions
Copy link

github-actions bot commented Apr 19, 2023

Review Checklist

  • Target branch (10.5 for bug fixes, others 11.x)
  • Bug fix: check if files are affected that were moved to a bundle - create a PR there if applicable
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

Copy link
Contributor

@martineiber martineiber left a comment

Choose a reason for hiding this comment

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

LGTM!

@martineiber martineiber merged commit c6c8090 into 10.5 Apr 20, 2023
11 checks passed
@martineiber martineiber deleted the fix-admin-translation-export-sql-injection branch April 20, 2023 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants