Skip to content
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.

Sign up for GitHub

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

[MNG-6535] Improve test coverage of UrlNormalizer. #264

Closed
wants to merge 3 commits into from
Closed

[MNG-6535] Improve test coverage of UrlNormalizer. #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c6d668
[MNG-6535] Improve test coverage of UrlNormalizer.
josephw May 20, 2019
ad2d94c
[MNG-6535] Switch behaviour on relative URIs to match Path.normalize
josephw Jul 7, 2019
33f1f11
[MNG-6703] Adopt RFC 3968 behaviour
josephw Jul 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions maven-model-builder/src/main/java/org/apache/maven/model/path/DefaultUrlNormalizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,15 @@ public String normalize( String url )
while ( true )
{
int idx = result.indexOf( "/../" );
if ( idx <= 0 )
if ( idx < 0 )
{
break;
}
else if ( idx == 0 )
{
result = result.substring( 3 );
continue;
}
int parent = idx - 1;
while ( parent >= 0 && result.charAt( parent ) == '/' )
{
Expand All @@ -55,9 +60,12 @@ public String normalize( String url )
parent = result.lastIndexOf( '/', parent );
if ( parent < 0 )
{
break;
result = result.substring( idx + 4 );
}
else
{
result = result.substring( 0, parent ) + result.substring( idx + 3 );
}
result = result.substring( 0, parent ) + result.substring( idx + 3 );
}
}

Expand Down
47 changes: 26 additions & 21 deletions maven-model-builder/src/test/java/org/apache/maven/model/path/DefaultUrlNormalizerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,52 +19,39 @@
* under the License.
*/

import junit.framework.TestCase;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;

import org.junit.Test;

/**
* @author Benjamin Bentmann
*/
public class DefaultUrlNormalizerTest
extends TestCase
{

private UrlNormalizer normalizer;

@Override
protected void setUp()
throws Exception
{
super.setUp();

normalizer = new DefaultUrlNormalizer();
}

@Override
protected void tearDown()
throws Exception
{
normalizer = null;

super.tearDown();
}
private UrlNormalizer normalizer = new DefaultUrlNormalizer();

private String normalize( String url )
{
return normalizer.normalize( url );
}

@Test
public void testNullSafe()
{
assertNull( normalize( null ) );
}

@Test
public void testTrailingSlash()
{
assertEquals( "", normalize( "" ) );
assertEquals( "http://server.org/dir", normalize( "http://server.org/dir" ) );
assertEquals( "http://server.org/dir/", normalize( "http://server.org/dir/" ) );
}

@Test
public void testRemovalOfParentRefs()
{
assertEquals( "http://server.org/child", normalize( "http://server.org/parent/../child" ) );
Expand All @@ -74,6 +61,7 @@ public void testRemovalOfParentRefs()
assertEquals( "http://server.org/child", normalize( "http://server.org/parent//../child" ) );
}

@Test
public void testPreservationOfDoubleSlashes()
{
assertEquals( "scm:hg:ssh://localhost//home/user", normalize( "scm:hg:ssh://localhost//home/user" ) );
Expand All @@ -82,4 +70,21 @@ public void testPreservationOfDoubleSlashes()
normalize( "[fetch=]http://server.org/[push=]ssh://server.org/" ) );
}

@Test
public void absolutePathTraversalPastRootIsOmitted()
{
assertEquals( "/", normalize("/../" ) );
}

@Test
public void parentDirectoryRemovedFromRelativeUriReference()
{
assertEquals( "", normalize( "a/../" ) );
}

@Test
public void leadingParentDirectoryNotRemovedFromRelativeUriReference()
{
assertEquals( "../", normalize( "../" ) );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the implementation is wrong, but comparing this with Path this is the result:

    assertEquals( "a/../",  Paths.get("a/../").normalize().toString() ); // fails
    assertEquals( "",  Paths.get("a/../").normalize().toString() ); // succeeds

The Path behavior is also what I would expect.

Copy link
Member

Choose a reason for hiding this comment

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

@rfscholte So our impl is wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it's worth changing the behaviour; this class is used with absolute URLs (or paths), so this is very much an edge case.

However, I think it's (slightly) valuable to test that this case doesn't cause an error -- perhaps a different test name could make that clear?

Copy link
Member

Choose a reason for hiding this comment

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

If we normalize URLs, the test for relative paths must fail. URLs are absolute.

Copy link
Member

Choose a reason for hiding this comment

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

@josephw Please raise an issue that our impl is wrong. Otherwise we will lose the findings here. Then I will add a comment abour the test referring to the new ticket and merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking again; there's no reason to retain the existing behaviour if it's wrong. The change to match Path is simple enough, and doesn't break the existing tests: I've pushed it.

Ideally, the interface would be clearer about its contract, but this is the least surprising behaviour.

}