spring-projects / spring-projects/spring-security

Mark matchingRequestParameterName as @Nullable in HttpSessionRequestCache and WebSessionServerRequestCache

Open Beginner friendly
#19,153 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

status: waiting-for-triage type: enhancement
Dominant language
Java
Stars
9.6k
Forks
6.3k
Avg merge
2d 11h
Merged PRs (30d)
52

Description

Current Behavior

The package org.springframework.security.web.savedrequest is @NullMarked, so by contract setMatchingRequestParameterName(String) does not accept null.

https://github.com/spring-projects/spring-security/blob/8c4c5fe91f/web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java#L177-L179

However, the field is already null-checked at runtime. getMatchingRequest skips the query-parameter requirement entirely when the field is null.

https://github.com/spring-projects/spring-security/blob/8c4c5fe91f/web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java#L100-L111

So the implementation supports null, but the JSpecify type contract advertises non-null. The two disagree.

The reactive counterpart WebSessionServerRequestCache has a partial mismatch in the same direction. The field is already declared @Nullable String, but the setter parameter is not.

https://github.com/spring-projects/spring-security/blob/8c4c5fe91f/web/src/main/java/org/springframework/security/web/server/savedrequest/WebSessionServerRequestCache.java#L63

https://github.com/spring-projects/spring-security/blob/8c4c5fe91f/web/src/main/java/org/springframework/security/web/server/savedrequest/WebSessionServerRequestCache.java#L124-L126

Expected Behavior

Annotate the field and the setter parameter as @Nullable so the type contract matches the implementation. The change applies to HttpSessionRequestCache#matchingRequestParameterName (field), HttpSessionRequestCache#setMatchingRequestParameterName(String) (parameter), and WebSessionServerRequestCache#setMatchingRequestParameterName(String) (parameter; the field is already @Nullable).

The Javadoc should also state that passing null disables the query-parameter requirement, following the convention used by BasicAuthenticationEntryPoint#setCharset ("Set to null to ...").

Context

Aside from fixing the contract/implementation mismatch, this also legitimizes the only existing way to disable the ?continue optimization (introduced in #11454). The optimization is enabled by default (the field default is "continue") and reduces unnecessary HttpSession reads. However, appending ?continue to post-login redirect URLs can break client-side routing in SPAs. Applications hitting this currently have to call setMatchingRequestParameterName(null), which is supported at runtime but warns under JSpecify/NullAway.

Happy to send a PR.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with HttpSessionRequestCache#matchingRequestParameterName and setMatchingRequestParameterName in web/src/main/java/org/springframework/security/web/savedrequest/HttpSessionRequestCache.java, then compare the setter in WebSessionServerRequestCache. Mark the specified field and parameters as @Nullable and update the Javadoc to explain that null disables the query-parameter requirement.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
85/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.