Re: [patch] ftp(1) does not understand "Location"-headers with a relative reference

[Alistair Crooks <agc%pkgsrc.org@localhost>]

Thanks for the patch.

As far as comments go, there's a general lack of checking return
codes(*strchr, *strrchr, strcspn) to make sure they don't fail (both
now, when you *absolutely know 100% guaranteed* that it won't fail,
and in 10 years time when it does fail due to someone's maintenance,
or called from different code).

I have no idea if ftp is written to various levels of libc function
availability, given that it is so widespread, but I'm sure the
(malloc, sprintf) combination would be better done by asprintf. And I
think that an attempt to use sprintf in any way in our brave new world
would run foul of warnings in linkers on some operating systems.

Best,
Al

On 27 May 2016 at 07:58, Timo Buhrmester <fstd.lkml%gmail.com@localhost> wrote:
> The "Location"-Header in a HTTP Redirect used to require a full URL,
> but as of RFC 7231, relative references are also allowed.
>
> ftp(1) does not understand this; the following patch addresses that issue.
>
> Comments?
>
>
> diff --git a/usr.bin/ftp/fetch.c b/usr.bin/ftp/fetch.c
> index d5b13b6..32f0368 100644
> --- a/usr.bin/ftp/fetch.c
> +++ b/usr.bin/ftp/fetch.c
> @@ -115,6 +115,7 @@ static int  parse_url(const char *, const char *, struct urlinfo *,
>  static void    url_decode(char *);
>  static void    freeauthinfo(struct authinfo *);
>  static void    freeurlinfo(struct urlinfo *);
> +static int     isfullurl(const char *);
>
>  static int     redirect_loop;
>
> @@ -1010,9 +1011,29 @@ negotiate_connection(FETCH *fin, const char *url, const char *penv,
>                         getmtime(cp, mtime);
>
>                 } else if (match_token(&cp, "Location:")) {
> -                       location = ftp_strdup(cp);
> +                       if (!isfullurl(cp)) {
> +                               /* Redirect using "relative reference",
> +                                * RFC 7231 7.1.2.  The destination is just a
> +                                * path, which may be absolute or relative */
> +
> +                               /* This is going to be the base URL */
> +                               char *bu = ftp_strdup(url);
> +
> +                               /* Cut off URL at the first slash if we're
> +                                * being redirected using an absolute path, or
> +                                * at the last slash if relative. */
> +                               if (cp[0] == '/')
> +                                       *strchr(strstr(bu, "//")+2, '/') = '\0';
> +                               else
> +                                       *strrchr(bu, '/') = '\0';
> +
> +                               location = malloc(strlen(bu) + strlen(cp) + 1);
> +                               sprintf(location, "%s%s", bu, cp);
> +                               free(bu);
> +                       } else
> +                               location = ftp_strdup(cp);
>                         DPRINTF("%s: parsed location as `%s'\n",
> -                           __func__, cp);
> +                           __func__, location);
>
>                 } else if (match_token(&cp, "Transfer-Encoding:")) {
>                         if (match_token(&cp, "binary")) {
> @@ -2334,3 +2355,14 @@ auto_put(int argc, char **argv, const char *uploadserver)
>         FREEPTR(uargv[2]);
>         return (rval);
>  }
> +
> +/* Determine whether the given string is an URL (starting with <scheme>:)
> + * or a lone path (be it absolute or relative) */
> +static int
> +isfullurl(const char *url)
> +{
> +       size_t slash = strcspn(url, "/");
> +       size_t colon = strcspn(url, ":");
> +
> +       return slash > colon;
> +}
>