Skip to content

web.ignoring().mvcMatchers is confuse in someway about the debug output in the console #9334

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
manueljordan opened this issue Jan 9, 2021 · 11 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: feedback-provided Feedback has been provided type: bug A general bug
Milestone

Comments

@manueljordan
Copy link

manueljordan commented Jan 9, 2021

Hello

Not sure if the following is a possible bug.

Having

  • Spring Boot 2.4.1
  • Spring dev tools 2.4.1
  • Spring Security 5.4.2

I understand the following:

@Override
public void configure(WebSecurity web) throws Exception {
	web.ignoring().mvcMatchers("/resources/static/css/**", "/static/css/**");
}

Ignore the second url pattern (it would be the same than the first).
I know that it instructs to Spring Security to ignore that URLs to avoid any kind of security.

When I use logging.level.org.springframework.security=debug

I can see in the console

Alpha

[  restartedMain] o.s.s.web.DefaultSecurityFilterChain     : Will secure Mvc [pattern='/resources/static/css/**'] with []
[  restartedMain] o.s.s.web.DefaultSecurityFilterChain     : Will secure Mvc [pattern='/static/css/**'] with []

If those URLs are supposed to be ignored, why appears the Will secure Mvc term? it gives the impression they are being secured yet

Furthermore when the app is used appears for a simple GET to render the home page:

Beta

[nio-8080-exec-3] o.s.security.web.FilterChainProxy        : Securing GET /demo/browser/home
[nio-8080-exec-3] w.c.HttpSessionSecurityContextRepository : Retrieved SecurityContextImpl [...]
[nio-8080-exec-3] s.s.w.c.SecurityContextPersistenceFilter : Set SecurityContextHolder to SecurityContextImpl [...]
[nio-8080-exec-3] o.s.s.w.a.i.FilterSecurityInterceptor    : Authorized filter invocation [GET /demo/browser/home] with attributes [authenticated]
[nio-8080-exec-3] o.s.security.web.FilterChainProxy        : Secured GET /demo/browser/home
[nio-8080-exec-3] s.s.w.c.SecurityContextPersistenceFilter : Cleared SecurityContextHolder to complete request
[nio-8080-exec-4] o.s.security.web.FilterChainProxy        : Securing GET /demo/css/general.css
[nio-8080-exec-4] w.c.HttpSessionSecurityContextRepository : Retrieved SecurityContextImpl [...]
[nio-8080-exec-4] s.s.w.c.SecurityContextPersistenceFilter : Set SecurityContextHolder to SecurityContextImpl [...]
[nio-8080-exec-4] o.s.s.w.a.i.FilterSecurityInterceptor    : Authorized public object filter invocation [GET /demo/css/general.css]
[nio-8080-exec-4] o.s.security.web.FilterChainProxy        : Secured GET /demo/css/general.css
[nio-8080-exec-4] s.s.w.c.SecurityContextPersistenceFilter : Cleared SecurityContextHolder to complete request

Observe the repeat data of these two lines:

[nio-8080-exec-4] o.s.s.w.a.i.FilterSecurityInterceptor    : Authorized public object filter invocation [GET /demo/css/general.css]
[nio-8080-exec-4] o.s.security.web.FilterChainProxy        : Secured GET /demo/css/general.css

Not sure if the configure(WebSecurity web) configuration is wrong and that's why I am see that lines or the bug

I have this behaviour for:

  • even with the "/css/**" pattern in web.ignoring().mvcMatchers() (happens alpha and beta)
  • ignoring or not @Override the configure(WebSecurity web) method (happens only beta)
@manueljordan manueljordan added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jan 9, 2021
@jzheaux jzheaux self-assigned this Jan 11, 2021
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 11, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Jan 11, 2021

@manueljordan, thanks for the report.

I think the logging could be adjusted in these circumstances to something like:

Will not secure Mvc [pattern='/resources/static/css/*']

Would you be able to submit a PR adjusting the logging in DefaultSecurityFilterChain?

As for the requests you point out, they do not match the pattern in your configuration. /demo/css/general.css doesn't match /resources/static/css/**, /static/css/**, nor /css/**. For that reason, it isn't ignored.

@jzheaux jzheaux added the status: waiting-for-feedback We need additional information before we can continue label Jan 11, 2021
@manueljordan
Copy link
Author

Hi @jzheaux

As for the requests you point out, they do not match the pattern in your configuration. /demo/css/general.css doesn't match /resources/static/css/, /static/css/, nor /css/**. For that reason, it isn't ignored.

Not sure what happened, but with /css/** works. Not sure if Spring Dev Tools was involved here, therefore with the following works:

@Override
public void configure(WebSecurity web) throws Exception {
	web.ignoring().mvcMatchers("/css/**");
}

/demo comes from: spring.mvc.servlet.path=/demo and in the rendered page about the source code shows:

<link rel="stylesheet" href="https://github.com/spring-boot-draft/demo/css/general.css" />

So with mvcMatchers("/css/**"); disappears the Secured GET /demo/css/general.css message.

About the DefaultSecurityFilterChain class, the constructor is the key

public DefaultSecurityFilterChain(RequestMatcher requestMatcher, List<Filter> filters) {
	logger.info(LogMessage.format("Will secure %s with %s", requestMatcher, filters));
	this.requestMatcher = requestMatcher;
	this.filters = new ArrayList<>(filters);
}

That we have the logger and the situation mentioned, seems the goal is work through the RequestMatcher interface.

Let me do a research to find what implementation represents the type according from web.ignoring().mvcMatchers() and see if is enough change the logger line with a if/else statement to add the not part. Apart let me find what Test classes would use indirectly or directly the DefaultSecurityFilterChain to test the output.

@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 Jan 13, 2021
@manueljordan
Copy link
Author

manueljordan commented Feb 3, 2021

I found the solution of this, please now give some time to let me pass/accomplish all the requirements requested on the contributors rules.

BTW, for the new methods added, about the javadoc, about the @since tag should be added with @since 5.4 right?
And any class that I edited and/or created, mostly for the former, I should update to the following Copyright 2002-2021 right?

Thanks for your understanding

@jzheaux
Copy link
Contributor

jzheaux commented Feb 9, 2021

Correct, @manueljordan. The @since value changes depending on what is the next minor version. At the time I'm posting this, the next minor version is 5.5.

@manueljordan
Copy link
Author

Ok I am going to use that value (5.5), once I am able to build with any problem on Mac and Windows. I reported some issues

Thanks for your understanding

@manueljordan
Copy link
Author

BTW @jzheaux it is in progress, I think I am on 85% to complete. I found more places to update code ...

@manueljordan
Copy link
Author

Hello @jzheaux

I have completed the scenarios (with many @Tets methods) for:

  • web.ignoring().mvcMatchers() (all their variations)
  • web.ignoring().antMatchers() (all their variations)

The two methods mentioned above are defined in the configure(WebSecurity web) method

Now I am working with CSRF where exists available the settings about .csrf().ignoringAntMatchers(""), but working in the configure(HttpSecurity http) method (not in the configure(WebSecurity web) method how would be expected - it about to apply the ignoring settings.

Apart - If I have any consult to do about the code. Should I use this channel? or Gitter?

@jzheaux
Copy link
Contributor

jzheaux commented Mar 22, 2021

Glad to hear you are making progress.

To consult with CsrfFilter's configs before printing out this log would be the same as consulting with BasicAuthenticationFilter's configs before printing out the log. You would end up running all the filters to see which filters are going to participate.

IMO, subsequent logs will clarify if CsrfFilter chooses not to participate. The fact is that Spring Security is still securing with CsrfFilter, but it's that filter that's stating that it has no opinion on the security of that request.

Regarding consulting, you're welcome to continue posting here since that creates a durable record. I am on Gitter about once a week, but I'm not very consistent.

@manueljordan
Copy link
Author

Thanks for your time - Something I did realize, with DEBUG mode is possible see what is not going to be protected (ignored), but does not appear what would be protected. Of course it in DEBUG mode and only showing the paths (not the roles). Even when the output would be verbose, the info is valuable. I would consider it for a new feature later.

I want resolve and apply the respective @Test about the CRSF for the ignore case. Just being curious, I know mvcMatchers is better than antMatchers, so why CRSF (about ignore) only works with through antMatchers?

@jzheaux
Copy link
Contributor

jzheaux commented Mar 29, 2021

DEBUG mode is possible see what is not going to be protected (ignored), but does not appear what would be protected. Of course it in DEBUG mode and only showing the paths (not the roles).

I'm not sure I'm following. Would you mind creating a separate ticket about this and showing what you would expect to see in the logs at the DEBUG level?

so why CRSF (about ignore) only works with through antMatchers?

I think you are referring to csrf().ignoringAntMatchers(). The reason is that it's nice to keep the DSL small, waiting for the expressed need to add something to it. Since an application can do csrf().ignoringRequestMatchers(new MvcRequestMatcher(introspector, "/path/**")), there hasn't been a compelling need to add ignoringMvcMatchers.

@manueljordan
Copy link
Author

Let me send a pull request for the former scenario, it for web.ignoring() (that is tightly related with DefaultSecurityFilterChain) for mvcMatchers() and antMatchers()

Later on in other issue we can handle the CSRF scenario, it because is not related with web.ignoring()

manueljordan pushed a commit to manueljordan/spring-security that referenced this issue Mar 29, 2021
When either `web.ignoring().mvcMatchers(...)` or
`web.ignoring().antMatchers(...)` methods are used, for all their
variations, the DefaultSecurityFilterChain class now indicates
correctly through its ouput what paths are ignored according the
`ignoring()` settings.

Closes spring-projectsgh-9334
jzheaux pushed a commit that referenced this issue Feb 7, 2022
When either `web.ignoring().mvcMatchers(...)` or
`web.ignoring().antMatchers(...)` methods are used, for all their
variations, the DefaultSecurityFilterChain class now indicates
correctly through its ouput what paths are ignored according the
`ignoring()` settings.

Closes gh-9334
jzheaux added a commit that referenced this issue Feb 7, 2022
- Public API remains unchanged

Issue gh-9334
@jzheaux jzheaux closed this as completed in 6ae651b Feb 7, 2022
jzheaux added a commit that referenced this issue Feb 7, 2022
- Public API remains unchanged

Issue gh-9334
@jzheaux jzheaux added this to the 5.7.0-M2 milestone Feb 7, 2022
jzheaux pushed a commit that referenced this issue Feb 7, 2022
When either `web.ignoring().mvcMatchers(...)` or
`web.ignoring().antMatchers(...)` methods are used, for all their
variations, the DefaultSecurityFilterChain class now indicates
correctly through its ouput what paths are ignored according the
`ignoring()` settings.

Closes gh-9334
jzheaux added a commit that referenced this issue Feb 7, 2022
- Public API remains unchanged

Issue gh-9334
jzheaux pushed a commit that referenced this issue Feb 7, 2022
When either `web.ignoring().mvcMatchers(...)` or
`web.ignoring().antMatchers(...)` methods are used, for all their
variations, the DefaultSecurityFilterChain class now indicates
correctly through its ouput what paths are ignored according the
`ignoring()` settings.

Closes gh-9334
jzheaux added a commit that referenced this issue Feb 7, 2022
- Public API remains unchanged

Issue gh-9334
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) status: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants