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

Refine default filtered headers for web data binding #34182

Closed
ioanclarke opened this issue Dec 31, 2024 · 11 comments
Closed

Refine default filtered headers for web data binding #34182

ioanclarke opened this issue Dec 31, 2024 · 11 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@ioanclarke
Copy link

ioanclarke commented Dec 31, 2024

Here's a minimal reproducible example of, what I believe is, a bug in Spring Boot 3.4.0.

I have a Spring Boot web app with a controller that looks like this:

@RestController
@RequestMapping("/sftp")
public class SftpController {
    @GetMapping
    public ResponseEntity<Void> find(SftpSearchRequest searchRequest) {
        System.out.println(searchRequest.getHost());
        return ResponseEntity.ok().build();
    }
}

where SftpSearchRequest looks like this:

public class SftpSearchRequest {
    private String id;
    private String host;

    public String getId() { return id; }

    public void setId(String id) { this.id = id; }

    public String getHost() { return host; }

    public void setHost(String host) { this.host = host; }
}

I have the app running on port 8088 and I am sending a request that looks like this: GET http://localhost:8088/sftp?id=123.

  • When using Spring Boot 3.3.5, "null" is printed to the console
  • When using Spring Boot 3.4.0, "localhost:8088" is printed to the console

I believe Spring Boot attempts to following SemVer - and this appears to be a breaking change - which is why I believe this may be a bug, especially since I haven't found this behaviour change mentioned in release notes.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 31, 2024
@snicoll snicoll transferred this issue from spring-projects/spring-boot Dec 31, 2024
@snicoll snicoll added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Dec 31, 2024
@ngocnhan-tran1996
Copy link
Contributor

@ioanclarke

This change mentioned in here

https://github.com/spring-projects/spring-framework/releases/tag/v6.2.0-M4

Support data binding from request headers #32676

@ioanclarke
Copy link
Author

ioanclarke commented Jan 1, 2025

@ioanclarke

This change mentioned in here

https://github.com/spring-projects/spring-framework/releases/tag/v6.2.0-M4

Support data binding from request headers #32676

Thank you for that - great find!
Any ideas for how to use the old behaviour here, except by changing the name of thehost field?

@quaff

This comment was marked as off-topic.

@bclozel
Copy link
Member

bclozel commented Jan 2, 2025

@ioanclarke you can try the following: #34125 (comment)

This issue is almost a duplicate of #34125, but we could consider it as a request to disable this feature by default. We'll discuss this as a team.

@bclozel bclozel changed the title Breaking change in binding behaviour between 3.3.5 and 3.4.0 Refine default filtered headers for web data binding Jan 7, 2025
@bclozel bclozel added this to the 6.2.2 milestone Jan 7, 2025
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 7, 2025
@bclozel
Copy link
Member

bclozel commented Jan 7, 2025

We have discussed this as a team and we came to the following conclusion:

  • we understand the disruption here and we will extend the list of header names filtered by default; "Priority" was added in Priority header causes binding exception after upgrade to Spring Framework 6.2.0 #34039 and we should extend that list with more well-known header names
  • in addition to this change, we think that the addHeaderPredicate and setHeaderPredicate should go a long way for uncommon issues. The general binding feature has been out there for a while and we're extending it by popular demand. Let's see if we get more feedback about this after the next release.

I'm repurposing this issue to extend the list of header names filtered by default, adding "Host" and more.

@bclozel
Copy link
Member

bclozel commented Jan 7, 2025

Right now I'm leaning towards selecting the following headers to be ignored by default because they are:

  • common in HTTP requests
  • not likely to be bound on purpose
  • simple words probably used as class attributes:

"Accept", "Authorization", "Connection", "Cookie", "From", "Host", "Origin", "Priority", "Range", "Referer", "Upgrade"

We can refine this list if we get further feedback from the community.

Applications can still opt back in by replacing the default predicate with your own:

import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.InitBinder;
import org.springframework.web.servlet.mvc.method.annotation.ExtendedServletRequestDataBinder;

@ControllerAdvice
public class MyControllerAdvice {

	@InitBinder
	public void initBinder(ExtendedServletRequestDataBinder binder) {
		binder.setHeaderPredicate(header -> ...);
	}
}

@bclozel bclozel closed this as completed in c971276 Jan 7, 2025
quaff pushed a commit to quaff/spring-framework that referenced this issue Jan 8, 2025
Prior to this commit, HTTP request data binding had been improved to
filter out by default the "Priority" header in spring-projects#34039.

This commit extends the set of filtered header names with:
"Accept", "Authorization", "Connection", "Cookie", "From", "Host",
"Origin", "Priority", "Range", "Referer", "Upgrade".

If an application wishes to let those header be bound, it will need to
configure the binder and replace the default header predicate by calling
`setHeaderPredicate`.

Closes spring-projectsgh-34182
bclozel added a commit that referenced this issue Jan 29, 2025
Prior to this commit, several common HTTP headers were ignored from the
data binding process when collecting property values, in gh-34039 and
gh-34182.

This commit completes the initial enhancement by ensuring that the
default header predicate is also considering cases where constructor
binding is applied and the Java type has a lowercase variant of the HTTP
header name to filter.

Fixes gh-34292
@nastharl
Copy link

Following up from a related thread: I suspect that 6.2.3 will fix the issue i know about (origin header vs origin param), but i dont know if there are other places in my company where someones custom header happens to collide with some model value, and will start having erroneous information without us knowing it. As a feature its perfectly fine for it to work this way, it just feels like it sprung (heh) up out of nowhere for us. Hopefully nothing breaks?

@bclozel
Copy link
Member

bclozel commented Jan 29, 2025

@nastharl please give a try to the latest 6.2.3 SNAPSHOTs and let us know if we need to refine things more.

@nastharl
Copy link

Will do :) Thanks again for the quick fixes.

@apoorvam
Copy link

@bclozel Credentials is another request header that would be good to exclude.

@bclozel
Copy link
Member

bclozel commented Mar 19, 2025

@apoorvam Unlikely. This doesn't look like a well-known header so you'll have to configure the filtering for your application.

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

No branches or pull requests

8 participants