Skip to content

Add logging to CsrfTokenRequestHandler implementations#16994

Merged
jzheaux merged 1 commit intospring-projects:mainfrom
yybmion:gh-13626
May 13, 2025
Merged

Add logging to CsrfTokenRequestHandler implementations#16994
jzheaux merged 1 commit intospring-projects:mainfrom
yybmion:gh-13626

Conversation

@yybmion
Copy link
Copy Markdown
Contributor

@yybmion yybmion commented Apr 24, 2025

Issue

This PR adds trace-level logging to CSRF token handlers to improve debugging capabilities.

Changes

  • Add logging to show token source (header or parameter) in resolveCsrfTokenValue
  • Add logging to show request attribute names used in handle methods
  • Add logging in XorCsrfTokenRequestAttributeHandler when token processing fails (as specifically requested in the issue)
  • Apply similar logging improvements to XorServerCsrfTokenRequestAttributeHandler for consistency

Fixes #13626

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 24, 2025
jzheaux
jzheaux requested changes Apr 29, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jzheaux jzheaux left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi, @yybmion, thanks for the PR! I've left some feedback inline for the servlet code. Will you please apply the same to the reactive code?

Also, I don't see changes to ServerCsrfTokenRequestHandler. Can you add logging changes equivalent to CsrfTokenRequestHandler?

Comment thread web/src/main/java/org/springframework/security/web/csrf/CsrfTokenRequestAttributeHandler.java Outdated
Comment thread web/src/main/java/org/springframework/security/web/csrf/CsrfTokenRequestHandler.java Outdated
Comment thread web/src/main/java/org/springframework/security/web/csrf/CsrfTokenRequestHandler.java Outdated
Comment thread ...src/main/java/org/springframework/security/web/csrf/XorCsrfTokenRequestAttributeHandler.java Outdated
Comment thread ...src/main/java/org/springframework/security/web/csrf/XorCsrfTokenRequestAttributeHandler.java Outdated
Comment thread ...src/main/java/org/springframework/security/web/csrf/XorCsrfTokenRequestAttributeHandler.java Outdated
Comment thread ...src/main/java/org/springframework/security/web/csrf/XorCsrfTokenRequestAttributeHandler.java Outdated
Comment thread web/src/main/java/org/springframework/security/web/csrf/CsrfTokenRequestAttributeHandler.java Outdated
Comment thread ...src/main/java/org/springframework/security/web/csrf/XorCsrfTokenRequestAttributeHandler.java Outdated
Comment thread ...src/main/java/org/springframework/security/web/csrf/XorCsrfTokenRequestAttributeHandler.java Outdated
@yybmion
Copy link
Copy Markdown
Contributor Author

yybmion commented Apr 30, 2025

Thank you @jzheaux for thorough review and detailed guidance!

jzheaux
jzheaux requested changes May 1, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @yybmion! I've left some additional feedback based on your changes.

Comment thread ...src/main/java/org/springframework/security/web/csrf/CsrfTokenRequestHandlerLoggerHolder.java Outdated
Comment thread ...src/main/java/org/springframework/security/web/csrf/XorCsrfTokenRequestAttributeHandler.java Outdated
Comment thread .../org/springframework/security/web/server/csrf/XorServerCsrfTokenRequestAttributeHandler.java Outdated
Comment thread .../org/springframework/security/web/server/csrf/XorServerCsrfTokenRequestAttributeHandler.java Outdated
Comment thread ...ava/org/springframework/security/web/server/csrf/ServerCsrfTokenRequestAttributeHandler.java Outdated
Comment thread .../org/springframework/security/web/server/csrf/ServerCsrfTokenRequestHandlerLoggerHolder.java Outdated
@yybmion
Copy link
Copy Markdown
Contributor Author

yybmion commented May 2, 2025

Thanks for additional feedback @jzheaux! I updated feedback.

@jzheaux jzheaux self-assigned this May 6, 2025
@jzheaux jzheaux added in: cas An issue in spring-security-cas in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged in: cas An issue in spring-security-cas labels May 6, 2025
@jzheaux jzheaux added this to the 6.5.0 milestone May 6, 2025
@jzheaux
Copy link
Copy Markdown
Contributor

jzheaux commented May 6, 2025

Thanks, @yybmion, this is taking shape nicely.

When I run the build (./gradlew :spring-security-web:check) it appears that a unit test is failing and also there are some checkstyle errors. Will you please address those? Note that the unit test shouldn't be changed.

@yybmion
Copy link
Copy Markdown
Contributor Author

yybmion commented May 7, 2025

@jzheaux I ran "./gradlew :spring-security-web:check" and the build completed successfully.

Sorry for the small issues that extended the review process. I've learned a lot from this contribution opportunity and appreciate your patience!"

@yybmion @jzheaux
Add logging to CsrfTokenRequestHandler implementations
c5fc7c7 
Add trace-level logging to show the logical path of CSRF token processing
- Log token source (header or parameter) in resolveCsrfTokenValue
- Log request attribute names in handle methods
- Log failures in XorCsrfTokenRequestAttributeHandler (especially Base64 decoding)
- Add similar logging to XorServerCsrfTokenRequestAttributeHandler

Improves debugging capabilities without changing functionality.

Closes spring-projectsgh-13626

Signed-off-by: yybmion <yunyubin54@gmail.com>
jzheaux
jzheaux approved these changes May 12, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jzheaux jzheaux left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work, @yybmion! I've squashed your commits and will merge this once the build finishes.

And don't worry at all, it's part of the learning process and you did great. I hope you continue to contribute and we get to learn more together!

@jzheaux jzheaux merged commit a90ce51 into spring-projects:main May 13, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jzheaux jzheaux jzheaux approved these changes

Assignees

@jzheaux jzheaux

Labels

in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement

Projects

None yet

Milestone

6.5.0

Development

Successfully merging this pull request may close these issues.

Add logging to CsrfTokenRequestHandler implementations

3 participants

@yybmion @jzheaux @spring-projects-issues