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.

Sign up for GitHub

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

Jump to bottom

Allow for customised content-type validation #80906

Merged
merged 8 commits into from
Jan 25, 2022
Merged

Allow for customised content-type validation #80906

merged 8 commits into from
Jan 25, 2022

Conversation

pgomulka
Copy link
Contributor

In order to support additional media types in request body a custom
validation has to be supported.
This commit moves validation from RestController to RestHandler
interface (default) and allows new RestHandler implementations to
provide its custom implementation

closes #80482

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@pgomulka
Allow for customized content-type validation
b598fe3 
In order to support additional media types in request body a custom
validation has to be supported.
This commit moves validation from RestController to RestHandler
interface (default) and allows new RestHandler implementations to
provide its custom implementation

closes elastic#80482
@pgomulka pgomulka added the WIP label Nov 22, 2021
@pgomulka pgomulka self-assigned this Nov 22, 2021
@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@pgomulka pgomulka marked this pull request as ready for review January 17, 2022 10:41
@pgomulka pgomulka added :Core/Infra/REST API REST infrastructure and utilities and removed WIP labels Jan 17, 2022
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 17, 2022
@elasticmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @pgomulka, I've created a changelog YAML for you.

grcevski
grcevski reviewed Jan 17, 2022
View reviewed changes
server/src/main/java/org/elasticsearch/rest/RestHandler.java
@@ -76,8 +73,8 @@ default boolean allowSystemIndexAccessByDefault() {
return false;
}

default MediaTypeRegistry<? extends MediaType> validAcceptMediaTypes() {
return XContentType.MEDIA_TYPE_REGISTRY;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own learning, was nobody using this interface method before? Do we need to still report somewhere that the default supported media type is MEDIA_TYPE_REGISTRY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it was not used. I added it in #64406 (comment) (see the description) but the validation of Accept header was never implemented.
In fact, the mediaTypesValid added in this PR could be used for this.
The goal of this PR is to make it less strict though - to allow RestHandlers which intentionally want to implement their own Content-Type validation

grcevski
grcevski approved these changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM!

@pgomulka pgomulka merged commit c8d98cf into elastic:master Jan 25, 2022
@tvernum
Copy link
Contributor

tvernum commented Jan 27, 2022

@pgomulka On the original issue we discussed maintaining CSRF protection by continuing to reject any of the safelisted content types.

It doesn't look like this PR does that. Was that intentional?

@pgomulka
Copy link
Contributor Author

great find @tvernum . It wasn't intentional, it was a while when I read the discussion and assumed the PR was ready.
I missed that the explicit check for XContentType values on Content-Type header was preventing the safelisted to be sent.. I will follow up in a new PR to fix this.
thanks again!

@pgomulka pgomulka mentioned this pull request Feb 3, 2022
Merged
pgomulka added a commit that referenced this pull request Feb 4, 2022
@pgomulka
Do not allow safelisted media types on Content-Type (#83448)
a2949f5 
The commit to allow custom media types validation accidentally allowed for CSRF.
The rules for media types which should be rejected on content-type were mentioned here

PR that introduced a bug: #80906
the comment that describes safelisted media types to be rejected #80482 (comment)
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Feb 4, 2022
@pgomulka
Do not allow safelisted media types on Content-Type (elastic#83448)
8c48115 
The commit to allow custom media types validation accidentally allowed for CSRF.
The rules for media types which should be rejected on content-type were mentioned here

PR that introduced a bug: elastic#80906
the comment that describes safelisted media types to be rejected elastic#80482 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grcevski grcevski grcevski approved these changes

Assignees

@pgomulka pgomulka

Labels
:Core/Infra/REST API REST infrastructure and utilities >enhancement Team:Core/Infra Meta label for core/infra team v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow RestHandlers for Content-Type outside XContentType
5 participants
@pgomulka @elasticmachine @elasticsearchmachine @tvernum @grcevski