Add level parameter validation in rest layer. by howardhuanghua · Pull Request #94136 · elastic/elasticsearch

howardhuanghua · 2023-02-26T11:37:33Z

This PR is going to fix #93981.
Validate ?level= up-front in rest layer, also keeps validation in transport layer.

elasticsearchmachine · 2023-02-26T11:37:56Z

@howardhuanghua please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation.

elasticsearchmachine · 2023-02-27T10:37:48Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

thecoop · 2023-02-27T10:43:24Z

Thanks for the PR @howardhuanghua. This could really do with tests being added. All the Action classes have got associated Test classes; could you add tests for valid/invalid parameters to those classes please.

howardhuanghua · 2023-02-27T10:54:03Z

Thanks for the PR @howardhuanghua. This could really do with tests being added. All the Action classes have got associated Test classes; could you add tests for valid/invalid parameters to those classes please.

Hi @thecoop, thanks. I am going to add test case, just to make sure should be in org.elasticsearch.client.RestClientTests or org.elasticsearch.rest.action.RestActionsTests ? Could you please give me some suggestions?

thecoop · 2023-02-27T10:59:20Z

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterHealthAction.java

        final ClusterHealthRequest clusterHealthRequest = fromRequest(request);
-        return channel -> client.admin().cluster().health(clusterHealthRequest, new RestStatusToXContentListener<>(channel));
+        return channel -> {
+            ActionListener<ClusterHealthResponse> listener = new RestStatusToXContentListener<>(channel);


Rather than checking here, ActionRequest has a validate method that checks if the request is valid, the checking should be done there. eg for RestClusterHealthAction, its in the ClusterHealthRequest.validate() method.

thecoop · 2023-02-27T11:00:10Z

server/src/main/java/org/elasticsearch/action/ActionResponse.java

        }
    }
+
+    public static void validateClusterResponseLevel(final String level) {


These methods are too specific to go into ActionResponse, ideally in some subclass of ActionRequest common to all the request classes using the level parameter.

Actually I'm not sure there is a good place for the common code, as some Request classes have their own separate way of representing level. We may just need to keep each implementation separate for now, and not have a common method.

thecoop · 2023-02-27T11:05:28Z

The tests should be in the corresponding classes for the request - eg ClusterHealthRequestTests. See IndexRequestTests calling the validate method for a good example.

howardhuanghua · 2023-02-27T13:18:25Z

OK. Thanks, I would update PR later.

DaveCTurner

I reckon we should have an enum for the level parameter rather than having all these string constants around.

Also I think we should require response params to be validated while reading the request, i.e. that we should drop the filter line here:

elasticsearch/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java

Lines 82 to 87 in 74c68d3

    
           // validate unconsumed params, but we must exclude params used to format the response 
        
           // use a sorted set so the unconsumed parameters appear in a reliable sorted order 
        
           final SortedSet<String> unconsumedParams = request.unconsumedParams() 
        
               .stream() 
        
               .filter(p -> responseParams(request.getRestApiVersion()).contains(p) == false) 
        
               .collect(Collectors.toCollection(TreeSet::new));

howardhuanghua · 2023-03-01T10:04:34Z

Also I think we should require response params to be validated while reading the request, i.e. that we should drop the filter line here:

Thanks @DaveCTurner , just want to double check that in prepareRequest:

RestRequest request;
request.param("level", "node")

Above request.param would also contains response parameters? So we validate level in prepareRequest is enough?
If we drop the above filter line, that user input response parameter like level would be treated as unrecognized parameters?

DaveCTurner · 2023-03-01T10:20:20Z

If we drop the above filter line, that user input response parameter like level would be treated as unrecognized parameters?

Only if we don't validate them in prepareRequest. Which IMO we should do: there's no point in handling the request if we're just going to throw an exception when we get around to sending back the response.

howardhuanghua · 2023-03-01T16:27:37Z

Update PR based on suggestions. Please help to review again, thank you.

DaveCTurner

I would prefer to keep the change you proposed to BaseRestHandler, and adjust the other REST handlers accordingly.

DaveCTurner · 2023-03-20T10:23:56Z

I will submit another PR to remove base rest handler response paramter filter logic and related cases.

Ah ok I didn't see this comment until after my previous one. A follow-up PR would be fine too. Let me take another look.

howardhuanghua · 2023-03-20T10:26:54Z

Ah ok I didn't see this comment until after my previous one. A follow-up PR would be fine too. Let me take another look.

Hmm, I think remove the response filter would cause lots of case failed. So I will do it in a follow-up PR.

DaveCTurner

Ok I like it, LGTM (assuming CI is happy)

server/src/main/java/org/elasticsearch/action/ClusterStatsLevel.java

server/src/main/java/org/elasticsearch/action/NodeStatsLevel.java

DaveCTurner

Ah sorry, LGTM except those two small nits. I'll enable CI anyway.

DaveCTurner · 2023-03-20T10:29:30Z

@elasticmachine ok to test

…l.java Co-authored-by: David Turner <david.turner@elastic.co>

Co-authored-by: David Turner <david.turner@elastic.co>

DaveCTurner · 2023-03-20T10:37:07Z

I don't have permission to fix the changelog, but here's the patch I'd like you to apply:

diff --git a/docs/changelog/94136.yaml b/docs/changelog/94136.yaml
index 94b55c7f4b4..6b63723b039 100644
--- a/docs/changelog/94136.yaml
+++ b/docs/changelog/94136.yaml
@@ -1,5 +1,5 @@
 pr: 94136
-summary: Add level parameter validation in rest layer.
-area: REST API
+summary: Add level parameter validation in REST layer
+area: Infra/REST API
 type: bug
 issues: [93981]

…icsearch into valid_level

howardhuanghua · 2023-03-21T03:09:28Z

It seems all the tests passed finally. ^^

DaveCTurner

Nice work, I like it. I left a few tiny suggestions.

server/src/main/java/org/elasticsearch/action/ClusterStatsLevel.java

server/src/main/java/org/elasticsearch/action/NodeStatsLevel.java

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterHealthAction.java

...src/test/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponsesTests.java

...ava/org/elasticsearch/xpack/searchablesnapshots/rest/RestSearchableSnapshotsStatsAction.java

DaveCTurner · 2023-03-21T10:53:55Z

server/src/main/java/org/elasticsearch/cluster/health/ClusterIndexHealth.java

        PARSER.declareString(constructorArg(), new ParseField(STATUS));
        // Can be absent if LEVEL == 'indices' or 'cluster'
-        PARSER.declareNamedObjects(optionalConstructorArg(), SHARD_PARSER, new ParseField(SHARDS));
+        PARSER.declareNamedObjects(optionalConstructorArg(), SHARD_PARSER, new ParseField(ClusterStatsLevel.SHARDS.getLevel()));


Hmm it is true that it's the same literal string, but that's something of a coincidence. I don't think we should link the field names in the response to the query parameters in the request like this.

DaveCTurner · 2023-03-21T10:54:08Z

server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponse.java

        PARSER.declareString(constructorArg(), new ParseField(STATUS));
        // Can be absent if LEVEL == 'cluster'
-        PARSER.declareNamedObjects(optionalConstructorArg(), INDEX_PARSER, new ParseField(INDICES));
+        PARSER.declareNamedObjects(optionalConstructorArg(), INDEX_PARSER, new ParseField(ClusterStatsLevel.INDICES.getLevel()));


Likewise here - it is true that it's the same literal string, but that's something of a coincidence. I don't think we should link the field names in the response to the query parameters in the request like this.

server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponse.java

DaveCTurner

Great, LGTM

howardhuanghua · 2023-03-21T11:27:26Z

Great, LGTM

Thanks for the patient help.

howardhuanghua · 2023-03-24T05:12:06Z

I have added a independent PR to remove base rest handler parameter filter logic.
#94711

Validate level parameter in rest layer.

355cd5c

elasticsearchmachine added v8.8.0 external-contributor Pull request authored by a developer outside the Elasticsearch team needs:triage Requires assignment of a team area label labels Feb 26, 2023

Add change log

cad20ba

howardhuanghua mentioned this pull request Feb 26, 2023

Validate response parameters up-front (especially for chunked APIs) #93981

Closed

thecoop added >bug :Core/Infra/REST API REST infrastructure and utilities and removed needs:triage Requires assignment of a team area label labels Feb 27, 2023

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 27, 2023

thecoop reviewed Feb 27, 2023

View reviewed changes

thecoop requested a review from DaveCTurner February 27, 2023 11:02

DaveCTurner reviewed Mar 1, 2023

View reviewed changes

howardhuanghua added 3 commits March 2, 2023 00:18

update level response parameter validation logic.

778f4e2

Merge remote-tracking branch 'origin' into valid_level

cc492bd

replace enum value

355a4b1

howardhuanghua requested review from DaveCTurner and thecoop and removed request for DaveCTurner and thecoop March 1, 2023 16:27

remove extra validation

b909f67

howardhuanghua requested a review from DaveCTurner March 20, 2023 10:19

DaveCTurner reviewed Mar 20, 2023

View reviewed changes

DaveCTurner approved these changes Mar 20, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/action/ClusterStatsLevel.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/action/NodeStatsLevel.java Outdated Show resolved Hide resolved

DaveCTurner requested changes Mar 20, 2023

View reviewed changes

DaveCTurner self-assigned this Mar 20, 2023

howardhuanghua and others added 2 commits March 20, 2023 18:30

Update server/src/main/java/org/elasticsearch/action/ClusterStatsLeve…

fe1777c

…l.java Co-authored-by: David Turner <david.turner@elastic.co>

Update server/src/main/java/org/elasticsearch/action/NodeStatsLevel.java

ac0dcdc

Co-authored-by: David Turner <david.turner@elastic.co>

howardhuanghua added 6 commits March 20, 2023 18:40

fix changelog

cf5e9ab

Merge branch 'valid_level' of https://github.com/TencentCloudES/elast…

fc3d655

…icsearch into valid_level

Fix spotless

ceb208e

Fix searchable snapshot spotless

c7fe57a

Fix failure case

9d89abf

Merge remote-tracking branch 'origin' into valid_level

610fbc4

DaveCTurner reviewed Mar 21, 2023

View reviewed changes

howardhuanghua added 2 commits March 21, 2023 17:04

Merge branch 'elastic:main' into valid_level

7c830ef

fix comment related

a242d2d

DaveCTurner reviewed Mar 21, 2023

View reviewed changes

revert parser and enhance of func

0c0d3d5

DaveCTurner approved these changes Mar 21, 2023

View reviewed changes

DaveCTurner merged commit fc9c0ce into elastic:main Mar 21, 2023

howardhuanghua mentioned this pull request Mar 24, 2023

Remove base rest handler unconsumed parameter filter to force response parameter validation. #94711

Open

	// validate unconsumed params, but we must exclude params used to format the response
	// use a sorted set so the unconsumed parameters appear in a reliable sorted order
	final SortedSet<String> unconsumedParams = request.unconsumedParams()
	.stream()
	.filter(p -> responseParams(request.getRestApiVersion()).contains(p) == false)
	.collect(Collectors.toCollection(TreeSet::new));

Conversation

howardhuanghua commented Feb 26, 2023

Uh oh!

elasticsearchmachine commented Feb 26, 2023

Uh oh!

elasticsearchmachine commented Feb 27, 2023

Uh oh!

thecoop commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

howardhuanghua commented Feb 27, 2023

Uh oh!

thecoop Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

thecoop Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

thecoop Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

thecoop commented Feb 27, 2023

Uh oh!

howardhuanghua commented Feb 27, 2023

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

howardhuanghua commented Mar 1, 2023

Uh oh!

DaveCTurner commented Mar 1, 2023

Uh oh!

howardhuanghua commented Mar 1, 2023

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Mar 20, 2023

Uh oh!

howardhuanghua commented Mar 20, 2023

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Mar 20, 2023

Uh oh!

DaveCTurner commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

howardhuanghua commented Mar 21, 2023

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

howardhuanghua commented Mar 21, 2023

Uh oh!

howardhuanghua commented Mar 24, 2023

Uh oh!

Reviewers

thecoop commented Feb 27, 2023 •

edited

Loading

DaveCTurner commented Mar 20, 2023 •

edited

Loading