ninjaframework / ninja

Ninja is a full stack web framework for Java. Rock solid, fast and super productive.
http://www.ninjaframework.org
Apache License 2.0
1.91k stars 519 forks source link

URL parameter with colon causes exception #669

Open davidgiga1993 opened 5 years ago

davidgiga1993 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Sure

raphaelbauer commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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.