Tynamo

Security module does not protect urls correctly

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: security-0.3.0
  • Fix Version/s: security-0.2.2, security-0.3.1
  • Component/s: security
  • Labels:
    None
  • Environment:
    NA
  • Number of attachments :
    0

Description

If you want to protect /admin then your ini file contains something like

[urls]
/admin/** = authc, roles[administrator]

Unfortunately Tapestry will also allow /Admin, /ADmin etc. and the filter does not run for those urls. I think what needs to happen is create a TapestryPathMatchingFilterChainResolver

like this

public class TapestryPathMatchingFilterChainResolver extends PathMatchingFilterChainResolver {
@Override
protected boolean pathMatches(String pattern, String path)

{ return super.pathMatches(pattern.toLowerCase(), path.toLowerCase()); }

}

and use it instead of PathMatchingFilterChainResolver in the SecurityRequestFilter. Unfortunately I think that only solves half the problem because the authc filter will not match correctly either so you need a TapestryFormAuthenticationFilter like

public class TapestryFormAuthenticationFilter extends FormAuthenticationFilter {
@Override
protected boolean pathsMatch(String pattern, String path)

{ return super.pathsMatch(pattern.toLowerCase(), path.toLowerCase()); }

}

and then override the authc filter with that implementation. I suspect this needs to be done for all the default filters.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Barry Books added a comment - - edited

I worked on the SecurtyRequestFilter and added the following:

PathMatchingFilterChainResolver chainResolver = (PathMatchingFilterChainResolver) shiroFilter.getFilterChainResolver();
FilterChainManager m = chainResolver.getFilterChainManager();
chainResolver = null;
if (chainResolver == null)

{ //FilterChainManager manager = new DefaultFilterChainManager(); //Expose the constructed FilterChainManager by first wrapping it in a // FilterChainResolver implementation. The ShiroFilter implementations // do not know about FilterChainManagers - only resolvers: chainResolver = new TapestryPathMatchingFilterChainResolver(); chainResolver.setFilterChainManager(m); shiroFilter.setFilterChainResolver(chainResolver); }

It's ugly but it works. The problem is you really need to call new TapestryPathMatchingFilterChainResolver(filterConfig) but you don't have the filterConfig. You can't use the IniFilterChainResolverFactory because it's got the PathMatchingFilterChainResolver hardcoded. So you really need a TapestryIniFilterChainResolverFactory and override createDefaultInstance.

Show
Barry Books added a comment - - edited I worked on the SecurtyRequestFilter and added the following: PathMatchingFilterChainResolver chainResolver = (PathMatchingFilterChainResolver) shiroFilter.getFilterChainResolver(); FilterChainManager m = chainResolver.getFilterChainManager(); chainResolver = null; if (chainResolver == null) { //FilterChainManager manager = new DefaultFilterChainManager(); //Expose the constructed FilterChainManager by first wrapping it in a // FilterChainResolver implementation. The ShiroFilter implementations // do not know about FilterChainManagers - only resolvers: chainResolver = new TapestryPathMatchingFilterChainResolver(); chainResolver.setFilterChainManager(m); shiroFilter.setFilterChainResolver(chainResolver); } It's ugly but it works. The problem is you really need to call new TapestryPathMatchingFilterChainResolver(filterConfig) but you don't have the filterConfig. You can't use the IniFilterChainResolverFactory because it's got the PathMatchingFilterChainResolver hardcoded. So you really need a TapestryIniFilterChainResolverFactory and override createDefaultInstance.
Hide
Permalink
Kalle Korhonen added a comment -

Thanks Barry for digging into it! Three things:

  • pattern.toLowerCase() probably not needed
  • need to make this configurable
  • should be able to set a new PatternMatcher rather than overriding patchsMatch()

otherwise, the approach is right and I definitely think this should be the default. Should be able crank out a new release pretty quick.

Show
Kalle Korhonen added a comment - Thanks Barry for digging into it! Three things: pattern.toLowerCase() probably not needed need to make this configurable should be able to set a new PatternMatcher rather than overriding patchsMatch() otherwise, the approach is right and I definitely think this should be the default. Should be able crank out a new release pretty quick.
Hide
Permalink
Barry Books added a comment -

I put toLowerCase() on both so a config like

/Admin = authc

would work. I looked at replacing PatternMatcher. It's a protected field with no get/set but I guess you could add some. It might be nice to have a regex pattern matcher.

Show
Barry Books added a comment - I put toLowerCase() on both so a config like /Admin = authc would work. I looked at replacing PatternMatcher. It's a protected field with no get/set but I guess you could add some. It might be nice to have a regex pattern matcher.
Hide
Permalink
Kalle Korhonen added a comment -

Sure, understand but in incurs an unnecessary repeated cost. It's not the right place, if we want to do it, we need to lowercase when reading the configuration in. For PatternMatcher, something like:
chainResolver.setPathMatcher(new AntPathMatcher() {
@Override
public boolean match(String pattern, String string)

{ return super.match(pattern, string.toLowerCase()); }

});

Just looking at this - Shiro filters is still an issue as the resolver doesn't delegate the matcher to the filter. Likely will open issue against Shiro but will fix in tapestry-security immediately. Btw, there's a RegExPatternMatter built-in to Shiro.

Show
Kalle Korhonen added a comment - Sure, understand but in incurs an unnecessary repeated cost. It's not the right place, if we want to do it, we need to lowercase when reading the configuration in. For PatternMatcher, something like: chainResolver.setPathMatcher(new AntPathMatcher() { @Override public boolean match(String pattern, String string) { return super.match(pattern, string.toLowerCase()); } }); Just looking at this - Shiro filters is still an issue as the resolver doesn't delegate the matcher to the filter. Likely will open issue against Shiro but will fix in tapestry-security immediately. Btw, there's a RegExPatternMatter built-in to Shiro.
Hide
Permalink
Kalle Korhonen added a comment -

As an immediate fix, lowercased requestURI before comparison. 0.3.1-SNAPSHOT should be available shortly. Will merge to 0.2.x and push out releases for both branches

Show
Kalle Korhonen added a comment - As an immediate fix, lowercased requestURI before comparison. 0.3.1-SNAPSHOT should be available shortly. Will merge to 0.2.x and push out releases for both branches

People

  • Assignee:
    Kalle Korhonen
    Reporter:
    Barry Books
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: