Spring Data REST #13373

gytis · 2020-11-19T10:36:29Z

cc @geoand

gytis · 2020-11-19T10:43:35Z

The quickstart PR quarkusio/quarkus-quickstarts#714

geoand

Great job!!!

I've added some minor comments

...ain/java/io/quarkus/rest/data/panache/deployment/properties/ResourcePropertiesBuildItem.java

docs/src/main/asciidoc/spring-data-rest.adoc

integration-tests/spring-data-rest/pom.xml

geoand · 2020-11-19T15:31:50Z

...deployment/src/main/java/io/quarkus/spring/data/rest/deployment/SpringDataRestProcessor.java

+                getRepositoriesToImplement(index, PAGING_AND_SORTING_REPOSITORY_INTERFACE, JPA_REPOSITORY_INTERFACE));
+    }
+
+    private void implementResources(BuildProducer<GeneratedBeanBuildItem> implementationsProducer,


I'd like to have a comment on this method describing what steps it performs and why they are needed

geoand

This is awesome, thanks!

geoand · 2020-11-20T13:02:44Z

Mind rebasing onto master to pick up some CI fixes?

gytis · 2020-11-20T13:24:16Z

Done

geoand · 2020-11-20T21:20:58Z

Seems like there is a compilation failure now

geoand · 2020-11-23T07:00:22Z

Also, it would be best to rebase onto master again, as there were more changes that should make CI happier.

Thanks

gsmet

Added a bunch of comments, mostly on documentation.

docs/src/main/asciidoc/spring-data-rest.adoc

extensions/spring-data-rest/deployment/pom.xml

extensions/spring-data-rest/runtime/pom.xml

gsmet · 2020-11-23T11:08:41Z

extensions/spring-data-rest/runtime/pom.xml

+    <dependency><!-- TODO create Quarkus Spring Data REST API jar -->
+      <groupId>org.springframework.data</groupId>
+      <artifactId>spring-data-rest-core</artifactId>
+      <version>3.3.0.RELEASE</version>


This should be in the BOM once you have created the artifact.

extensions/spring-data-rest/runtime/src/main/resources/META-INF/quarkus-extension.yaml

gsmet · 2020-11-23T11:10:24Z

...n/java/io/quarkus/spring/data/rest/deployment/paging/PagingAndSortingMethodsImplementor.java

+
+        ResultHandle page = methodCreator.getMethodParam(0);
+        ResultHandle sort = methodCreator.getMethodParam(1);
+        ResultHandle pageable = toPageable(methodCreator, page, sort);


Given we are taking the sort parameter from the REST URL, can we check it's not sensitive to SQL injections?

Here page and sort are already io.quarkus.panache.common.Page and io.quarkus.panache.common.Sort. They're instantiated from the query parameters by rest-data-panache extension.
This extension (same as Hibernate and MongoDB Data REST alternatives) handle only data access logic. JAX-RS resource is handled in the same way for all of them by the rest-data-panache.

I disagree with that.

Your extension makes it visible and applies absolutely no checks on the content coming from the URL. It's different when it's just Panache API, people can do their own checks.

So we need to check it's not sensitive to SQL injections otherwise you have a big security risk.

I'm not saying that it shouldn't be checked. I'm saying that this is not the place to do it. This extension does not handle the JAX-RS logic. It only implements user defined interface that extends https://github.com/quarkusio/quarkus/blob/master/extensions/panache/rest-data-panache/runtime/src/main/java/io/quarkus/rest/data/panache/RestDataResource.java.

rest-data-panache generates the JAX-RS resource and then calls spring-data-rest generated method for data access. So these two page and sort parameters are already instances of io.quarkus.panache.common.Page and io.quarkus.panache.common.Sort.

If we need to add query parameter validation we need to add it here https://github.com/quarkusio/quarkus/blob/master/extensions/panache/rest-data-panache/deployment/src/main/java/io/quarkus/rest/data/panache/deployment/methods/ListMethodImplementor.java#L104. However, wouldn't Panache Query builder escape all the query parameters automatically?

I'm not saying that it shouldn't be checked. I'm saying that this is not the place to do it.

Well, I haven't really reviewed the other extension so I see it now for this one. I still think this should carefully be addressed.

However, wouldn't Panache Query builder escape all the query parameters automatically?

It didn't a while ago. That's why I think we should check if it's safe or not.

@FroMage maybe you know how unsafe it is to pass arbitrary parameters to the Panache Sort object? Are they somehow parsed down the line by Panache or Hibernate/Mongo? If this is unsafe, do you have a recommendation on what type of validation is needed?

In any case, I don't think that this issue should holdup this PR. If the validation has to be added, it will be added to the rest-data-panache extension.

It's indeed unsafe because we end up generating an HQL query in PanacheJpaUtil.toOrderBy (not sure about Mongo but probably it is too).

Indeed the check really belongs in the REST layer as you say, especially wrt error handling. As to what you can check, here's what HQL allows:

orderByClause // todo (6.0) : null precedence : ORDER BY sortSpecification (COMMA sortSpecification)* ; sortSpecification : expression collationSpecification? orderingSpecification? ; collationSpecification : COLLATE collateName ; collateName : dotIdentifierSequence ; orderingSpecification : ASC | DESC ;

expression is particularly nasty because it's pretty much a free for all.

OTOH if your REST API specifies that the supported set of sort options is limited, then you can probably limit it to:

IDENTIFIER : ('a'..'z'|'A'..'Z'|'_'|'$'|'\u0080'..'\ufffe')('a'..'z'|'A'..'Z'|'_'|'$'|'0'..'9'|'\u0080'..'\ufffe')*

HTH :)

BTW, all this is from the panacheql extension, you have the lexer and parser files there with all the info you need ;)

Thanks @FroMage, this is very helpful. I'll add a query parameter validator to the rest-data-extension and will raise a new PR with it.

I've created a separate pull request to add a sort parameter validation #13514

geoand · 2020-11-23T16:03:52Z

CI is reporting some NPEs

gytis · 2020-11-24T08:00:36Z

CI is reporting some NPEs

Seems that the problem is with java.lang.NoSuchMethodError: java.lang.Math.floorDiv(JI)J here https://github.com/quarkusio/quarkus/pull/13373/files#diff-d42992bf5a468b46cc7a587c9a189baa7b54fdf2c005159a4f38de943c5be6c7R68. I'll look into it.

gytis · 2020-11-24T08:05:51Z

Seems that the method is not available in Java 8. I'll replace it.

gytis · 2020-11-24T08:44:50Z

Minimised API JAR PR quarkusio/quarkus-spring-data-api#5

gsmet · 2020-11-24T11:43:38Z

@gytis you haven't taken into account all my review comments (probably because some are hidden by GH by default). Could you adjust?

gytis · 2020-11-24T11:46:35Z

@gsmet you're right, I didn't see them. Thanks!

gsmet · 2021-01-04T17:26:57Z

Merged, thanks!

ghost added area/core area/dependencies Pull requests that update a dependency file area/documentation area/panache area/spring Issues relating to the Spring integration labels Nov 19, 2020

gytis mentioned this pull request Nov 19, 2020

Spring Data REST quickstart quarkusio/quarkus-quickstarts#714

Merged

geoand requested changes Nov 19, 2020

View reviewed changes

ghost added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Nov 20, 2020

gytis force-pushed the spring-data-rest branch 2 times, most recently from 6b8eed6 to fe0fc4d Compare November 20, 2020 09:13

geoand approved these changes Nov 20, 2020

View reviewed changes

gytis force-pushed the spring-data-rest branch from fe0fc4d to 889f9d3 Compare November 20, 2020 13:24

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 20, 2020

geoand added this to the 1.11 - master milestone Nov 20, 2020

geoand added the release/noteworthy-feature label Nov 20, 2020

gsmet requested changes Nov 23, 2020

View reviewed changes

gytis force-pushed the spring-data-rest branch 3 times, most recently from 1e017ce to 16a005d Compare November 23, 2020 11:48

gytis force-pushed the spring-data-rest branch from 16a005d to 7579a10 Compare November 24, 2020 08:28

gytis force-pushed the spring-data-rest branch from da7e56b to ed784d1 Compare November 25, 2020 11:15

gytis force-pushed the spring-data-rest branch from ed784d1 to e29b917 Compare December 9, 2020 11:06

Spring Data REST

d6d5200

gytis force-pushed the spring-data-rest branch from e29b917 to d6d5200 Compare December 10, 2020 12:36

gsmet modified the milestones: 1.11.0.Beta1, 1.11 - master Dec 17, 2020

gsmet modified the milestones: 1.11.0.Beta2, 1.11 - master Dec 24, 2020

gsmet approved these changes Jan 4, 2021

View reviewed changes

gsmet merged commit 75510b1 into quarkusio:master Jan 4, 2021

gytis deleted the spring-data-rest branch February 17, 2021 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spring Data REST #13373

Spring Data REST #13373

gytis commented Nov 19, 2020

gytis commented Nov 19, 2020

geoand left a comment

geoand Nov 19, 2020

geoand left a comment

geoand commented Nov 20, 2020

gytis commented Nov 20, 2020

geoand commented Nov 20, 2020

geoand commented Nov 23, 2020

gsmet left a comment

gsmet Nov 23, 2020 •

edited

Loading

gsmet Nov 23, 2020

gytis Nov 23, 2020

gsmet Nov 23, 2020

gytis Nov 23, 2020

gsmet Nov 24, 2020

gytis Nov 26, 2020

FroMage Nov 26, 2020

FroMage Nov 26, 2020

gytis Nov 26, 2020

gytis Nov 27, 2020

geoand commented Nov 23, 2020

gytis commented Nov 24, 2020

gytis commented Nov 24, 2020

gytis commented Nov 24, 2020

gsmet commented Nov 24, 2020

gytis commented Nov 24, 2020

gsmet commented Jan 4, 2021

Spring Data REST #13373

Spring Data REST #13373

Conversation

gytis commented Nov 19, 2020

gytis commented Nov 19, 2020

geoand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand left a comment

Choose a reason for hiding this comment

geoand commented Nov 20, 2020

gytis commented Nov 20, 2020

geoand commented Nov 20, 2020

geoand commented Nov 23, 2020

gsmet left a comment

Choose a reason for hiding this comment

gsmet Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand commented Nov 23, 2020

gytis commented Nov 24, 2020

gytis commented Nov 24, 2020

gytis commented Nov 24, 2020

gsmet commented Nov 24, 2020

gytis commented Nov 24, 2020

gsmet commented Jan 4, 2021

gsmet Nov 23, 2020 •

edited

Loading