Skip to content

DATACMNS-1757 PagedResourcesAssembler generated navigation links ignore non pageable request parameters #452

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

Closed
wants to merge 1 commit into from

Conversation

reda-alaoui
Copy link
Contributor

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@reda-alaoui
Copy link
Contributor Author

Hello?

@reda-alaoui
Copy link
Contributor Author

Ping :)

@reda-alaoui
Copy link
Contributor Author

Is there something I can do to ease the merge of this fix?

@mp911de
Copy link
Member

mp911de commented Jan 11, 2021

I'm not sure we want to include this change because it echos all incoming parameters which open up potential attack vectors. Instead, the page links are intended to remain neutral in terms of the original URL that produced the response.

@reda-alaoui
Copy link
Contributor Author

reda-alaoui commented Jan 11, 2021

Hi @mp911de ,

What kind of attack are you referring to?

the page links are intended to remain neutral in terms of the original URL that produced the response

This is already not the case. prev and next relations are already built based on the original URL that produced the response.

IMO, this is part of the core philosophy of HATEOAS. Ideally, the client should not ever have to build an uri itself, it should only follow relations returned by the server.

Now suppose the server sends a relation referring to /users?page_number=0&page_size=10&fuzzy_name=john to the client. The client does not know how the url has been built. The client does not know that fuzzy_name parameter has been set by the server. The client can only perform a GET on the url.
Without a response containing next relation referring to /users?page_number=1&page_size=10&fuzzy_name=john, the client has no way to fetch the next page.

@mp911de
Copy link
Member

mp911de commented Jan 11, 2021

I was referring to injection and XSS vulnerabilities. Looking at the outcome of a Spring Data REST repository, we indeed report all query parameters in the self and pagination _links.

Looking at the implementation, parameters seem to be skipped if a baseUri or a base link are provided.

@reda-alaoui
Copy link
Contributor Author

If you cannot accept it as is, here are alternatives I can think of from most preferred to the least:

  • condition the feature on a global option
  • condition the feature on a local option
  • make the Resolver extensible enough to allow its customization

@mp911de
Copy link
Member

mp911de commented Jan 13, 2021

Actually, we need to investigate why PagedResourcesAssembler behaves differently between using it with Repositories and the test case you've supplied. Right now, the behavior isn't consistent and after reiterating a bit on this ticket, we should follow your suggestion to include the additional query string options.

@schauder schauder removed the status: waiting-for-triage An issue we've not yet triaged label Jan 18, 2021
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 18, 2021
@odrotbohm odrotbohm added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 26, 2021
@odrotbohm odrotbohm closed this Jan 26, 2021
@odrotbohm
Copy link
Member

See #2173 for details on how we decided to solve this.

odrotbohm added a commit that referenced this pull request Jan 26, 2021
We now avoid the pre-computation of the base URI in PagedResourceAssemblerArgumentResolver as the actual assembler will fall back to using the URI of the current request by default anyway. The latter makes sure that request parameters, that are contained in the original requests appear in links created. If a controller wants to deviate from that behavior, they can create a dedicated link themselves and hand that to the assembler explicitly.

Fixes GH-2173, GH-452.
odrotbohm added a commit that referenced this pull request Jan 26, 2021
We now avoid the pre-computation of the base URI in PagedResourceAssemblerArgumentResolver as the actual assembler will fall back to using the URI of the current request by default anyway. The latter makes sure that request parameters, that are contained in the original requests appear in links created. If a controller wants to deviate from that behavior, they can create a dedicated link themselves and hand that to the assembler explicitly.

Fixes GH-2173, GH-452.
odrotbohm added a commit that referenced this pull request Jan 26, 2021
We now avoid the pre-computation of the base URI in PagedResourceAssemblerArgumentResolver as the actual assembler will fall back to using the URI of the current request by default anyway. The latter makes sure that request parameters, that are contained in the original requests appear in links created. If a controller wants to deviate from that behavior, they can create a dedicated link themselves and hand that to the assembler explicitly.

Fixes GH-2173, GH-452.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants