From 9b63051149c94876f8228643483d708959e17bba Mon Sep 17 00:00:00 2001 From: Ben Alex Date: Fri, 28 Apr 2006 08:41:55 +0000 Subject: [PATCH] SEC-204: Improve startup time detection of errors by FilterInvocationDefinitionSourceEditor. --- .../web/FilterInvocationDefinitionMap.java | 8 ++- ...ilterInvocationDefinitionSourceEditor.java | 70 +++++++++++++++---- ...InvocationDefinitionSourceEditorTests.java | 70 ++++++++++++++++--- 3 files changed, 123 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionMap.java b/core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionMap.java index 3172284559..9d409360e8 100644 --- a/core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionMap.java +++ b/core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionMap.java @@ -1,4 +1,4 @@ -/* Copyright 2004 Acegi Technology Pty Limited +/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -28,8 +28,10 @@ import org.acegisecurity.ConfigAttributeDefinition; public interface FilterInvocationDefinitionMap { //~ Methods ================================================================ + public void addSecureUrl(String expression, ConfigAttributeDefinition attr); + + public boolean isConvertUrlToLowercaseBeforeComparison(); + public void setConvertUrlToLowercaseBeforeComparison( boolean convertUrlToLowercaseBeforeComparison); - - public void addSecureUrl(String expression, ConfigAttributeDefinition attr); } diff --git a/core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditor.java b/core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditor.java index 401606711e..abd8d612f4 100644 --- a/core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditor.java +++ b/core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditor.java @@ -55,6 +55,9 @@ public class FilterInvocationDefinitionSourceEditor //~ Static fields/initializers ============================================= private static final Log logger = LogFactory.getLog(FilterInvocationDefinitionSourceEditor.class); + public static final String DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON = + "CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON"; + public static final String DIRECTIVE_PATTERN_TYPE_APACHE_ANT = "PATTERN_TYPE_APACHE_ANT"; //~ Methods ================================================================ @@ -65,14 +68,27 @@ public class FilterInvocationDefinitionSourceEditor // Leave target object empty } else { // Check if we need to override the default definition map - if (s.lastIndexOf("PATTERN_TYPE_APACHE_ANT") != -1) { + if (s.lastIndexOf(DIRECTIVE_PATTERN_TYPE_APACHE_ANT) != -1) { source = new PathBasedFilterInvocationDefinitionMap(); if (logger.isDebugEnabled()) { - logger.debug(("Detected PATTERN_TYPE_APACHE_ANT directive; using Apache Ant style path expressions")); + logger.debug(("Detected " + + DIRECTIVE_PATTERN_TYPE_APACHE_ANT + + " directive; using Apache Ant style path expressions")); } } + if (s.lastIndexOf( + DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON) != -1) { + if (logger.isDebugEnabled()) { + logger.debug("Detected " + + DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON + + " directive; Instructing mapper to convert URLs to lowercase before comparison"); + } + + source.setConvertUrlToLowercaseBeforeComparison(true); + } + BufferedReader br = new BufferedReader(new StringReader(s)); int counter = 0; String line; @@ -100,24 +116,36 @@ public class FilterInvocationDefinitionSourceEditor continue; } - if (line.equals("CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON")) { - if (logger.isDebugEnabled()) { - logger.debug("Line " + counter - + ": Instructing mapper to convert URLs to lowercase before comparison"); + // Attempt to detect malformed lines (as per SEC-204) + if (line.lastIndexOf( + DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON) != -1) { + // Directive found; check for second directive or name=value + if ((line.lastIndexOf(DIRECTIVE_PATTERN_TYPE_APACHE_ANT) != -1) + || (line.lastIndexOf("=") != -1)) { + throw new IllegalArgumentException( + "Line appears to be malformed: " + line); } - - source.setConvertUrlToLowercaseBeforeComparison(true); - - continue; } + // Attempt to detect malformed lines (as per SEC-204) + if (line.lastIndexOf(DIRECTIVE_PATTERN_TYPE_APACHE_ANT) != -1) { + // Directive found; check for second directive or name=value + if ((line.lastIndexOf( + DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON) != -1) + || (line.lastIndexOf("=") != -1)) { + throw new IllegalArgumentException( + "Line appears to be malformed: " + line); + } + } + + // Skip lines that are not directives if (line.lastIndexOf('=') == -1) { continue; } if (line.lastIndexOf("==") != -1) { throw new IllegalArgumentException( - "Only single equals should be used in line " + line); + "Only single equals should be used in line " + line); } // Tokenize the line into its name/value tokens @@ -129,7 +157,25 @@ public class FilterInvocationDefinitionSourceEditor throw new IllegalArgumentException( "Failed to parse a valid name/value pair from " + line); } - + + // Attempt to detect malformed lines (as per SEC-204) + if (source.isConvertUrlToLowercaseBeforeComparison() + && source instanceof PathBasedFilterInvocationDefinitionMap) { + // Should all be lowercase; check each character + // We only do this for Ant (regexp have control chars) + for (int i = 0; i < name.length(); i++) { + String character = name.substring(i, i + 1); + + if (!character.toLowerCase().equals(character)) { + throw new IllegalArgumentException( + "You are using the " + + DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON + + " with Ant Paths, yet you have specified an uppercase character in line: " + + line + " (character '" + character + "')"); + } + } + } + // Convert value to series of security configuration attributes ConfigAttributeEditor configAttribEd = new ConfigAttributeEditor(); configAttribEd.setAsText(value); diff --git a/core/src/test/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditorTests.java b/core/src/test/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditorTests.java index f0d1402747..a42d4e48be 100644 --- a/core/src/test/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditorTests.java +++ b/core/src/test/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditorTests.java @@ -1,4 +1,4 @@ -/* Copyright 2004 Acegi Technology Pty Limited +/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,15 +19,13 @@ import junit.framework.TestCase; import org.acegisecurity.ConfigAttributeDefinition; import org.acegisecurity.MockFilterChain; - - import org.acegisecurity.SecurityConfig; -import java.util.Iterator; - import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; +import java.util.Iterator; + /** * Tests {@link FilterInvocationDefinitionSourceEditor} and its associated @@ -49,14 +47,14 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase { //~ Methods ================================================================ - public final void setUp() throws Exception { - super.setUp(); - } - public static void main(String[] args) { junit.textui.TestRunner.run(FilterInvocationDefinitionSourceEditorTests.class); } + public final void setUp() throws Exception { + super.setUp(); + } + public void testConvertUrlToLowercaseDefaultSettingUnchangedByEditor() { FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); editor.setAsText( @@ -67,6 +65,19 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase { assertFalse(map.isConvertUrlToLowercaseBeforeComparison()); } + public void testConvertUrlToLowercaseDetectsUppercaseEntries() { + FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); + + try { + editor.setAsText( + "CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON\r\nPATTERN_TYPE_APACHE_ANT\r\n\\/secUre/super/**=ROLE_WE_DONT_HAVE"); + fail("Should have thrown IllegalArgumentException"); + } catch (IllegalArgumentException expected) { + assertTrue(expected.getMessage() + .lastIndexOf("you have specified an uppercase character in line") != -1); + } + } + public void testConvertUrlToLowercaseSettingApplied() { FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); editor.setAsText( @@ -87,6 +98,45 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase { assertTrue(map instanceof RegExpBasedFilterInvocationDefinitionMap); } + public void testDetectsDuplicateDirectivesOnSameLineSituation1() { + FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); + + try { + editor.setAsText( + "CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON PATTERN_TYPE_APACHE_ANT\r\n\\/secure/super/**=ROLE_WE_DONT_HAVE"); + fail("Should have thrown IllegalArgumentException"); + } catch (IllegalArgumentException expected) { + assertTrue(expected.getMessage() + .lastIndexOf("Line appears to be malformed") != -1); + } + } + + public void testDetectsDuplicateDirectivesOnSameLineSituation2() { + FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); + + try { + editor.setAsText( + "CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON\r\nPATTERN_TYPE_APACHE_ANT /secure/super/**=ROLE_WE_DONT_HAVE"); + fail("Should have thrown IllegalArgumentException"); + } catch (IllegalArgumentException expected) { + assertTrue(expected.getMessage() + .lastIndexOf("Line appears to be malformed") != -1); + } + } + + public void testDetectsDuplicateDirectivesOnSameLineSituation3() { + FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); + + try { + editor.setAsText( + "PATTERN_TYPE_APACHE_ANT\r\nCONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON /secure/super/**=ROLE_WE_DONT_HAVE"); + fail("Should have thrown IllegalArgumentException"); + } catch (IllegalArgumentException expected) { + assertTrue(expected.getMessage() + .lastIndexOf("Line appears to be malformed") != -1); + } + } + public void testEmptyStringReturnsEmptyMap() { FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor(); editor.setAsText(""); @@ -158,7 +208,7 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase { Class clazz = RegExpBasedFilterInvocationDefinitionMap.EntryHolder.class; try { - clazz.getDeclaredConstructor((Class[])null); + clazz.getDeclaredConstructor((Class[]) null); fail("Should have thrown NoSuchMethodException"); } catch (NoSuchMethodException expected) { assertTrue(true);