Patch for nginx handling of X-Accel-Redirect URLs

[Maxim Dounin]

Hello!

On Thu, Feb 02, 2012 at 03:27:27AM +0400, vitalif at yourcmc.ru wrote:

> A new version of patch, unescaping in ngx_http_parse_unsafe_uri()...
> Note that it required removing if (ch == ?) blocks that come AFTER
> decoding the character (I don't understand their purpose, they seem
> incorrect, as escaped '?' does not mean the beginning of query
> string).
> And I still don't understand what NGX_UNESCAPE_REDIRECT is... :-)

> This is a patch for nginx 1.1.12 handling of X-Accel-Redirect URLs:
> allow to handle percent-encoded URLs and complex filenames in X-Accel-Redirect
> (for example, filenames which contain '?').
> 
> --- a/src/core/ngx_string.c	2011-11-25 20:36:02.000000000 +0400
> +++ b/src/core/ngx_string.c	2012-02-02 03:21:46.153704057 +0400
> @@ -1590,21 +1590,11 @@ ngx_unescape_uri(u_char **dst, u_char **
>                  ch = (u_char) ((decoded << 4) + c - 'a' + 10);
>  
>                  if (type & NGX_UNESCAPE_URI) {
> -                    if (ch == '?') {
> -                        *d++ = ch;
> -                        goto done;
> -                    }
> -

This looks like a correct change.

>                      *d++ = ch;
>                      break;
>                  }
>  
>                  if (type & NGX_UNESCAPE_REDIRECT) {
> -                    if (ch == '?') {
> -                        *d++ = ch;
> -                        goto done;
> -                    }
> -

This is not correct, as this will break current behaviour of 
redirect unescaping.  Unless you are willing to work on it in 
general, please don't touch it.  In any case this should be a 
separate patch.

>                      if (ch > '%' && ch < 0x7f) {
>                          *d++ = ch;
>                          break;
> --- a/src/http/ngx_http_parse.c	2011-11-28 13:15:33.000000000 +0400
> +++ b/src/http/ngx_http_parse.c	2012-02-02 03:18:42.561709398 +0400
> @@ -1591,7 +1591,7 @@ ngx_int_t
>  ngx_http_parse_unsafe_uri(ngx_http_request_t *r, ngx_str_t *uri,
>      ngx_str_t *args, ngx_uint_t *flags)
>  {
> -    u_char  ch, *p;
> +    u_char  ch, *p, *src, *dst;
>      size_t  len;
>  
>      len = uri->len;
> @@ -1605,6 +1605,23 @@ ngx_http_parse_unsafe_uri(ngx_http_reque
>          goto unsafe;
>      }
>  
> +    /* unescape URI */
> +
> +    if (ngx_strchr(p, '%')) {

This isn't a good idea to assume uri is null-terminated.

> +
> +        dst = uri->data;
> +        src = uri->data;
> +
> +        ngx_unescape_uri(&dst, &src, uri->len, NGX_UNESCAPE_URI);

And it may not be good idea to assume uri->data is writable.

> +
> +        len = (uri->data + uri->len) - src;
> +        if (len) {
> +            dst = ngx_copy(dst, src, len);
> +        }
> +
> +        uri->len = dst - uri->data;
> +    }
> +
>      for ( /* void */ ; len; len--) {
>  
>          ch = *p++;

This also needs changes at least in ssi module, which currently 
does unescaping itself.  The dav module needs some attention too, 
as this change will affect it and it should be made clear it 
doesn't break things.

Maxim Dounin