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.

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

URL parameter with colon causes exception #669

Open
davidgiga1993 opened this issue Jul 25, 2019 · 5 comments
Open

URL parameter with colon causes exception #669

davidgiga1993 opened this issue Jul 25, 2019 · 5 comments

Comments

@davidgiga1993
Copy link

davidgiga1993 commented Jul 25, 2019

Error description

When defining a path parameter

router.GET().route(V1 + "/project/byName/{name: .+}").with(ProjectController::getByName);

and opening the url where the name parameter contains a colon will break the decoder

/project/byName/Herramientas%20de%20correcci%c3%b3n%20de%20Microsoft%20Office%202016%3a%20espa%c3%b1ol

Root cause

The %3a part of the url is are already decoded when NinjaServletContext.init is called.
In this case the requestPath contains

/project/byName/Herramientas%20de%20correcci%C3%B3n%20de%20Microsoft%20Office%202016:%20espa%C3%B1ol

Note the decoded : after 2016

This causes URI.create(encodedParameter).getPath(); in AbstractContext to fail since this is an invalid url.
I assume this call is only used to decode the url? If so is it possible to use UrlDecoder instead?

Workaround

As a workaround it is possible to revert the early escaping using a filter.
It's not the most elegant solution but fixes the issue for now without changing the ninja sources

/**
 * Fixes the too early decoding of "%3a" to ":" in the request context
 */
public class ColonUrlFixFilter implements Filter
{
	private Field requestPathField;

	public ColonUrlFixFilter()
	{
		try
		{
			requestPathField = AbstractContext.class.getDeclaredField("requestPath");
			requestPathField.setAccessible(true);
		}
		catch (NoSuchFieldException e)
		{
			e.printStackTrace();
		}
	}

	@Override
	public Result filter(FilterChain filterChain, Context context)
	{
		if (context instanceof AbstractContext)
		{
			try
			{
				String path = (String) requestPathField.get(context);
				path = path.replaceAll(":", "%3a");
				requestPathField.set(context, path);
			}
			catch (IllegalAccessException e)
			{
				// Ignore
			}
		}
		return filterChain.next(context);
	}
}
@raphaelbauer
Copy link
Contributor

Thanks for the nice bug report!
Would you mind fixing it in the Ninja source code?
This'd need a regression test and the change to fix it...

Let us know if we can help!

@davidgiga1993
Copy link
Author

Sure

@davidgiga1993 davidgiga1993 reopened this Jul 29, 2019
davidgiga1993 pushed a commit to davidgiga1993/ninja that referenced this issue Jul 29, 2019
@raphaelbauer
Copy link
Contributor

@jjlauer This issue is quite strange to me... Do you have an idea how to do this in a "better" way. I guess URI.create is not ideal... but I have no idea to do it better... Somehow we can't be the first ones trying to unescape paths of a uri...

@davidgiga1993
Copy link
Author

Well, the correct way of decoding is using URI.create() but the real question is why the %3a is decoded before that in the first place. I'll try to debug this further, I somehow think that it gets already decoded from jetty

@jjlauer
Copy link
Collaborator

jjlauer commented Aug 16, 2019

I haven't looked at this part of the code for awhile, but yes, its something with how Jetty or the servlet spec causes this stuff to be decoded earlier up the chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants