validate column names of sort properties #8022
Conversation
| @@ -41,6 +42,8 @@ | |||
| } | |||
|
|
|||
| public class Column { | |||
|
|
|||
| private final Pattern COLUMN_PATTERN = Pattern.compile("^[a-zA-Z_][a-zA-Z0-9_]*$"); | |||
There was a problem hiding this comment.
| private final Pattern COLUMN_PATTERN = Pattern.compile("^[a-zA-Z_][a-zA-Z0-9_]*$"); | |
| private static final Pattern COLUMN_PATTERN = Pattern.compile("^[a-zA-Z_][a-zA-Z0-9_]*$"); |
There was a problem hiding this comment.
Are we sure this is OK and covering all the possibilities? Where does this pattern come from?
There was a problem hiding this comment.
At the beginning, I was more thinking about escaping the column names. And the hard part is probably to get the dialect at some point to get it to properly escape things.
There was a problem hiding this comment.
@gsmet, as you said, this pattern may not be valid for all databases.
Can we do a simple quote escape here ? Getting the dialect here seems complicated.
There was a problem hiding this comment.
While I agree it's probably be a bit difficult, this can cause all sorts of security issues so I would prefer we get this right.
@Sanne any advice on how we could get Hibernate ORM to properly escape this?
There was a problem hiding this comment.
This looks suspicious indeed. Why does it need to be validated, couldn't we pass the sort intruction to ORM directly?
Some background: in ORM 5.x there have been various attempts in the Dialets to automagically escape / veto column names, they had all to be rolled back as they would break in various ways - I don't remember the details but yes it's dangeour to impose such a rule without a thoutough analysis.
The proper solution is included in the design of ORM6, so I would suggest to just let the user be responsible for using the right names.
There was a problem hiding this comment.
The issue is that the sort column is directly appended to the generated HQL so you basically end up with an HQL injection if you're not cautious about it.
It somehow needs to be properly quoted/escaped/whatever makes sense.
There was a problem hiding this comment.
Ok, but is "HQL injection" actually a dangeous thing? We're not sending it to the DB unparsed.
There was a problem hiding this comment.
Hmmm, unparsed no, sure. But can't you completely change the meaning of the query just with carefully forged sort column?
There was a problem hiding this comment.
I suppose this could be explored - never seen it done. Also I just realized that these columns names are appended into HQL, didn't expect that
| @@ -49,6 +52,9 @@ public Column(String name) { | |||
| } | |||
|
|
|||
| public Column(String name, Direction direction) { | |||
| if (COLUMN_PATTERN.asPredicate().negate().test(name)) { | |||
There was a problem hiding this comment.
| if (COLUMN_PATTERN.asPredicate().negate().test(name)) { | |
| if (!COLUMN_PATTERN.matcher(name).matches()) { |
|
I think the original issue also asks to escape (quote) the properties used in the |
glefloch
force-pushed
the
fix/1120
branch
2 times, most recently
from
6d7fabb to
1dc5588
Compare
|
I'm closing this one as out of date but the issue remains. |
close #1120
Throws an
IllegalArgumenExceptionif column names used inSortexpression are not valid.