Re: [PATCH] Location header recovering to absoluteURI

Hi,

Nice idea. An http patch was long overdue.

In the implementation it might be a little bit faster if you get the leading 
'if' statement outside in the main loop and then call within the if 
recover_absolute_uri. You would save an unnecessary function call and save a 
gw_assert which already has been done in octstr_strip_blanks anyway.

Shouldn't you also check for port 443?

+1 overall
----- Original Message ----- 
From: "Stipe Tolj" <[email protected]>
To: "kannel_dev_mailinglist" <[email protected]>
Sent: Friday, January 23, 2009 7:23 PM
Subject: Re: [PATCH] Location header recovering to absoluteURI


> Alejandro Guerrieri schrieb:
>> Apart from renaming "absulte" to "absolute", patch looks fine and I'm +1
>> with the idea of allowing this behavior.
>
> ehmmm ;) ... yep right, here we go with the typo fixed.
>
> Stipe
>
> -- 
> -------------------------------------------------------------------
> Kφlner Landstrasse 419
> 40589 DΟsseldorf, NRW, Germany
>
> tolj.org system architecture      Kannel Software Foundation (KSF)
> http://www.tolj.org/              http://www.kannel.org/
>
> mailto:st_{at}_tolj.org           mailto:stolj_{at}_kannel.org
> -------------------------------------------------------------------
>


--------------------------------------------------------------------------------


> ### Eclipse Workspace Patch 1.0
> #P gateway-cvs-head
> Index: gwlib/http.c
> ===================================================================
> RCS file: /home/cvs/gateway/gwlib/http.c,v
> retrieving revision 1.253
> diff -u -r1.253 http.c
> --- gwlib/http.c 12 Jan 2009 16:46:55 -0000 1.253
> +++ gwlib/http.c 23 Jan 2009 17:21:57 -0000
> @@ -928,6 +928,49 @@
> }
>
>
> +/*
> + * Recovers a Location header value of format URI /xyz to an
> + * absoluteURI format according to the protocol rules.
> + * This simply implies that we re-create the prefixed scheme,
> + * user/passwd (if any), host and port string and prepend it
> + * to the location URI.
> + */
> +static void recover_absolute_uri(HTTPServer *trans, Octstr *loc)
> +{
> +    Octstr *os;
> +
> +    gw_assert(loc != NULL && trans != NULL);
> +
> +    /* we'll only accept locations with a leading / */
> +    if (octstr_get_char(loc, 0) == '/') {
> +
> +        /* scheme */
> +        os = trans->ssl ? octstr_create("https://") :
> +            octstr_create("http://");
> +
> +        /* credentials, if any */
> +        if (trans->username && trans->password) {
> +            octstr_append(os, trans->username);
> +            octstr_append_char(os, ':');
> +            octstr_append(os, trans->password);
> +            octstr_append_char(os, '@');
> +        }
> +
> +        /* host */
> +        octstr_append(os, trans->host);
> +
> +        /* port, only added if literally not default. */
> +        if (trans->port != 80) {
> +            octstr_format_append(os, ":%ld", trans->port);
> +        }
> +
> +        /* prepend the created octstr to the loc, and destroy then. */
> +        octstr_insert(loc, os, 0);
> +        octstr_destroy(os);
> +    }
> +}
> +
> +
> /*
>  * Read and parse the status response line from an HTTP server.
>  * Fill in trans->persistent and trans->status with the findings.
> @@ -1117,9 +1160,30 @@
>
>         /*
>          * This is a redirected response, we have to follow.
> -         * Clean up all trans stuff for the next request we do.
> +         *
> +         * According to HTTP/1.1 (RFC 2616), section 14.30 any Location
> +         * header value should be 'absoluteURI', which is defined in
> +         * RFC 2616, section 3.2.1 General Syntax, and specifically in
> +         * RFC 2396, section 3 URI Syntactic Components as
> +         *
> +         *   absoluteURI   = scheme ":" ( hier_part | opaque_part )
> +         *
> +         * Some HTTP servers 'interpret' a leading UDI / as that kind
> +         * of absoluteURI, which is not correct, following the protocol 
> in
> +         * detail. But we'll try to recover from that misleaded
> +         * interpreation and try to convert the partly absoluteURI to a
> +         * fully qualified absoluteURI.
> +         *
> +         *   http_URL = "http:" "//" [ userid : password "@"] host
> +         *      [ ":" port ] [ abs_path [ "?" query ]]
> +         *
>          */
>         octstr_strip_blanks(h);
> +        recover_absolute_uri(trans, h);
> +
> +        /*
> +         * Clean up all trans stuff for the next request we do.
> +         */
>         octstr_destroy(trans->url);
>         octstr_destroy(trans->host);
>         trans->port = 0;
>