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.

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

Space in integer request parameter neither triggers the default value nor generates a 400 #29550

Closed
oscarfh opened this issue Nov 22, 2022 · 4 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@oscarfh
Copy link

oscarfh commented Nov 22, 2022

Affects: spring-web 5.3.22

Expectation

Given an endpoint with an Integer parameter named limit with a default value of 10, when a user makes a request with limit=%20 then the system should either return a 400 - Bad Request or have the default value applied.

Actual

Given an endpoint with an Integer parameter named limit with a default value of 10, when a user makes a request with limit=%20 then the system accepts the request and the limit variable has a null value.

This happens because when checking for the default value, Spring will not trim the string, so " " will be a value and will not trigger the default value.

But when binding the value to the variable, this line will resolve to true (because allowEmpty is set to true and !StringUtils.hasText(" ") resolves to true), so null will be set (instead of generating an exception).

I have no idea how to fix it, because this does not seem to be a bug in the code, but a mismatch of expectations.

Steps to reproduce

  1. Clone the reproducer https://github.com/oscarfh/spring-reproducer
  2. Start the application
  3. Call in your terminal: curl "http://localhost:8080/endpoint?limit=
    • "50" will be returned, this is the default value set for the limit endpoint, meaning that the default value was applied.
  4. Call curl "http://localhost:8080/endpoint?limit=a"
    • Note that a 400 is returned, meaning that limit does not support string values.
  5. Call curl "http://localhost:8080/endpoint?limit=%20"
    • Notice how "null" is returned, meaning that the limit variable was assigned the null value.

This is unexpected because, due to the fact that this variable is an integer with a default value. You either expect it to be the integer supplied by the user or the default value. You do not expect it to be null. This null will then very likely cause a null pointer exception in your code.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 22, 2022
@sbrannen sbrannen changed the title Whitespace (%20) in integer parameter neither triggers the default value not generates a 404 (with reproducer) Whitespace (%20) in integer parameter neither triggers the default value nor generates a 404 Nov 22, 2022
@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) in: core Issues in core modules (aop, beans, core, context, expression) status: waiting-for-triage An issue we've not yet triaged or decided on and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 22, 2022
@oscarfh oscarfh changed the title Whitespace (%20) in integer parameter neither triggers the default value nor generates a 404 Whitespace (%20) in integer parameter neither triggers the default value nor generates a 400 Nov 22, 2022
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 20, 2023

Thanks for the analysis and sample.

There was a similar case in #23939 where the request parameter was required and becomes null after conversion. We added handling in 5.3 to recognize it as a missing value. I think we have a variation of the same case here. The initial decisions around default/missing value are confounded due to the presence of an empty string, but conversion results in null, and a default value could be applied still, instead of leaving it as null. What do you think @jhoeller?

Backwards compatibility concerns are also similar. We cannot apply this in a maintenance release, so this would have to be a potential 6.1 target. In the meantime you can customize the behavior of CustomNumberEditor with an @InitBinder method either in the controller or in an @ControllerAdvice:

@InitBinder
void initBinder(WebDataBinder binder) {
	binder.registerCustomEditor(Number.class, new CustomNumberEditor(Number.class, false));
}

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 21, 2023
@rstoyanchev rstoyanchev added this to the 6.1.0-M1 milestone Feb 21, 2023
@RemusRD
Copy link
Contributor

RemusRD commented May 26, 2023

Hey can I work on this one? :)

@rstoyanchev
Copy link
Contributor

@RemusRD this one is not labelled with "status: ideal-for-contribution".

@rstoyanchev rstoyanchev changed the title Whitespace (%20) in integer parameter neither triggers the default value nor generates a 400 Space in integer request parameter neither triggers the default value nor generates a 400 Jun 20, 2023
@Nheyll
Copy link
Contributor

Nheyll commented Jun 21, 2023

@rstoyanchev

this one is not labelled with "status: ideal-for-contribution".

None of the issues are labelled with "status: ideal-for-contribution".
There were 3 issues with this label since 2020 :
status: ideal-for-contribution An issue that a contributor can help us with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants