Skip to content

spring-security-web is not consistence with % and ; #8705

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
patelneelj opened this issue Jun 17, 2020 · 9 comments
Closed

spring-security-web is not consistence with % and ; #8705

patelneelj opened this issue Jun 17, 2020 · 9 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug

Comments

@patelneelj
Copy link

patelneelj commented Jun 17, 2020

Describe the bug
In the spring version: 5+ (5.1.9 spring-security-web) for % and ;

  1. spring framework is throwing full stack trace.
  2. with Http response code 500 instead of 400

To Reproduce
If I send input filed like for GET /Users/{name} -- Name of User = "User1;" or "User2%" we can see the following response with full stack trace information.

With this //, \, or // without any customization, spring-security by default responding with 400 response code with out any stack trace. Based on my investigation I found that with for "%" and ";" has different workflows then other special chars like "//", "\" etc . It is going through StrictHttpFirewall.rejectedBlacklistedUrls() which is a private method of part of the Spring-Security-web framework.

Expected behavior
Same like "//" or'/'other special character, for % and ;

  1. It should not throw a full stack trace
  2. Response code should be 400 instead of 500 for

Example

  1. For "//"
    Screen Shot 2020-06-17 at 12 28 56 PM

  2. For % or ;
    Screen Shot 2020-06-17 at 12 29 44 PM

@patelneelj patelneelj added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jun 17, 2020
@rwinch
Copy link
Member

rwinch commented Jun 19, 2020

Thanks for the report. This appears to be a duplicate of gh-5007 which was resolved via gh-7052. Starting in Spring Security 5.4 you will be able to configure the RequestRejectedHandler by exposing it as a Bean with something like HttpStatusRequestRejectedHandler to change how rejected requests are processed.

Can you confirm this is a duplicate?

@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label Jun 19, 2020
@rwinch rwinch self-assigned this Jun 19, 2020
@rwinch rwinch added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 19, 2020
@patelneelj
Copy link
Author

patelneelj commented Jun 19, 2020

@rwinch Thank you for your response. I believe the one which you are talking about #5007 is only dealing with "//" or '\'
This is some special edge case, where for "%" and ";" spring security web is throwing 500 response code with the full stack trace.
I think, for % and ; the same expectation we have as in response wise without exposing any bean or handle it explicitly.
i.e 400 Response code without a stack trace. Please let me know if you have any concern to reproduce this issue or with my explanation. Thank you.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 19, 2020
@rwinch
Copy link
Member

rwinch commented Jun 22, 2020

Thanks for the response. What is the stacktrace you see when providing % or ;? The screenshot you provided states for that scenario you are getting RequestRejectedException. If it is a RequestRejectedException then you can customize how it is handled with the changes for gh-5007 You can also test by trying out Spring Security 5.4.0-M1

@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 22, 2020
@patelneelj
Copy link
Author

@rwinch Yes. I can customize the workflow and can do something to handle especially for requestRejectedException.
But I believe, how, for "//" things was an issue before in #5007 and it was resolved in #7052 and now, we do not need to customize the workflow for "//".
The same way, I believe, the spring security should handle for ";" and "%" and the user should not require to customize for it by extending httpfirewall or something like that. With the latest spring version, you can easily reproduce this scenario. Please let me know if you have any concerns.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 22, 2020
@rwinch
Copy link
Member

rwinch commented Jun 25, 2020

Even after gh-7052 you need to customize the RequestRejectedHandler. Spring Security will process any request that is rejected using RequestRejectedHandler. This includes // ; and %. We will not change the default behaviour until the next major release. Please let me know if tehre are any additional changes you think that need to be made.

@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 25, 2020
@patelneelj
Copy link
Author

patelneelj commented Jun 25, 2020

@rwinch That's the one I am explaining to you that we do not need to required to customize for// or /or \\
It is very well working as expected in spring security i.e no stack trace and 400 response code.

And I think, the same way, spring-security should behave for % and ;
However, currently it is not behaving the same way. For % and ; spring-security generated stack-trace and 500 response code which is totally different behavior then // or /or \\

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 25, 2020
@rwinch
Copy link
Member

rwinch commented Jun 29, 2020

Thanks for your response. Some containers attempt to perform normalization for certain characters. For example, it might convert //foo//bar to /foo/bar before Spring Security even sees the URL. This might be why it appears that Spring Security is handling // differently.

Without a sample, I really cannot help you. The code always throws a RequestRejectedException if an invalid character is found and that RequestRejectedException is consistently handled with the RequestRejectedHandler. The default is to rethrow the RequestRejectedHandler which causes the default error handling to process it (typically this would be an internal server error).

I'm going to mark this as waiting for feedback again. If you need any further help, then I need a minimal and complete sample (i.e. link to a github repository that reproduces the issue) so I can reproduce/understand your problem.

@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 29, 2020
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Jul 6, 2020
@spring-projects-issues
Copy link

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@spring-projects-issues spring-projects-issues removed the status: waiting-for-feedback We need additional information before we can continue label Jul 13, 2020
@spring-projects-issues spring-projects-issues removed the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants