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

Panache - Sort properties injected in the query should be escaped somehow #1120

Closed
gsmet opened this issue Mar 1, 2019 · 9 comments · Fixed by #37282 or #38645
Closed

Panache - Sort properties injected in the query should be escaped somehow #1120

gsmet opened this issue Mar 1, 2019 · 9 comments · Fixed by #37282 or #38645
Assignees
Labels
area/panache good first issue Good for newcomers pinned Issue will never be marked as stale
Milestone

Comments

@gsmet
Copy link
Member

gsmet commented Mar 1, 2019

Currently, they are not and we could imagine a user passing the sort property from the UI to the backend leading to a potential SQL injection.

@gsmet
Copy link
Member Author

gsmet commented Nov 8, 2019

@FroMage I think this one should be taken care of sooner rather than later.

@gsmet gsmet added the pinned Issue will never be marked as stale label Nov 13, 2019
@glefloch
Copy link
Member

@gsmet, I would to start contributing to Quarkus. Is a fix for this issue still needed ?

@FroMage
Copy link
Member

FroMage commented Mar 19, 2020

yes, that'd be great.

@ifeelgood
Copy link

@FroMage, I saw the 'good first issue' tag and the issue looks interesting enough to be my first one in open source. Can you assign it to me?
@gsmet, as a solution I'm going to deprecate io.quarkus.panache.hibernate.common.runtime.PanacheJpaUtil#toOrderBy(Sort sort) and introduce io.quarkus.panache.hibernate.common.runtime.PanacheJpaUtil#toOrderBy(Sort sort, Dialect dialect).
Is it the right direction or should I consider other options?

@FroMage
Copy link
Member

FroMage commented Nov 13, 2023

Yeah, that could work. But why do you need a Dialect for that?

@FroMage FroMage assigned ifeelgood and unassigned FroMage Nov 13, 2023
@ifeelgood
Copy link

ifeelgood commented Nov 15, 2023

Yeah, that could work. But why do you need a Dialect for that?

I want to escape the column names but, unfortunately, not all databases use just double quotes for this (like MySql as an exception). And hibernate Dialect has a class that implemented the logic that I want to reuse - https://docs.jboss.org/hibernate/orm/6.2/javadocs/org/hibernate/dialect/Dialect.html#toQuotedIdentifier(java.lang.String)

@yrodiere
Copy link
Member

yrodiere commented Feb 1, 2024

Reopening: we had to revert the fix in #38527 because it caused problems with attribute paths (containing dots, see #38521).

We should try again with the same code, but maybe splitting on dots before quoting; see #38521 (comment):

Split strings passed to panache on dots before escaping individual components of the path. This will prevent the use of dots in attribute names, but I guess that's acceptable? They are probably forbidden anyway; for Java fields/methods they are for sure, but I suspect there are ways to set an attribute name to something else than the field it's extracted from.

@yrodiere yrodiere reopened this Feb 1, 2024
Quarkus persistence automation moved this from Done to In progress Feb 1, 2024
@gsmet gsmet removed this from the 3.7.0.CR1 milestone Feb 1, 2024
@gsmet
Copy link
Member Author

gsmet commented Feb 1, 2024

Also, maybe we should provide a way to pass a raw string that is not quoted at all so that people can work around possible issues.

@ifeelgood
Copy link

Looks like I missed all the fun due to the timezone difference. I will re-work the solution next week.

Quarkus persistence automation moved this from In progress to Done Feb 9, 2024
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment