pidgin/pidgin

Stop using g_uri_escape_string() to escape the URI before launching it.
release-2.x.y
2014-01-13, Mark Doliner
41e1147347a5
Parents 4577ec83135a
Children dc08416b884f
Stop using g_uri_escape_string() to escape the URI before launching it.

This was wrong. Take this URL as an example:
https://developer.pidgin.im/search?q=brains&noquickjump=1&wiki=on

When escaped with g_uri_escape_string() it becomes:
https://developer.pidgin.im/search%3Fq%3Dbrains%26noquickjump%3D1%26wiki%3Don

?, = and & are replaced with %3F, %3D and %26 which means they are considered part of the path component rather than query args. I tested and I get 404s when launching that URL with Firefox, Google Chrome, and these manual commands: gnome-open, xdg-open, firefox, google-chrome.

Strangely I DON'T get a 404 when I launch the URL with Konqueror. The original unescaped URL loads. I consider this to be a bug in Konqueror. They would fail to load when launched with a URL that has a question mark as part of the path component because they would convert the remaining path into the query string.

So I ripped out uri_escaped and used uri in its place everywhere.

This bug never got released. We changed the behavior because someone reported
to us that this URL:
http://example.org/$(xterm)
caused xterm to be executed on his system. Obviously that's bad if that
happens, but I don't think it's a bug in Pidgin. We're correctly escaping
all arguments that we pass to the browser command. If a system unescapes those
at some point and execs them, then that system is dangerously broken.

I tested this newest code with Firefox, Google Chrome, Konqueror, and the
manual commands gnome-open and xdg-open and they all work perfectly for me.
1 files changed, 12 insertions(+), 23 deletions(-)
  • +12 -23
    pidgin/gtknotify.c
    • --- a/pidgin/gtknotify.c Mon Jan 13 19:59:51 2014 -0800
    +++ b/pidgin/gtknotify.c Mon Jan 13 21:43:50 2014 -0800
    @@ -1280,12 +1280,10 @@
    #ifndef _WIN32
    const char *web_browser;
    int place;
    - gchar *uri_escaped, *uri_custom = NULL;
    + gchar *uri_custom = NULL;
    GSList *argv = NULL, *argv_remote = NULL;
    gchar **usercmd_argv = NULL;
    - uri_escaped = g_uri_escape_string(uri, ":/%#,+", FALSE);
    -
    web_browser = purple_prefs_get_string(PIDGIN_PREFS_ROOT
    "/browsers/browser");
    place = purple_prefs_get_int(PIDGIN_PREFS_ROOT "/browsers/place");
    @@ -1298,7 +1296,7 @@
    else
    argv = g_slist_append(argv, "xdg-open");
    g_free(tmp);
    - argv = g_slist_append(argv, uri_escaped);
    + argv = g_slist_append(argv, (gpointer)uri);
    } else if (purple_running_osx() == TRUE) {
    argv = g_slist_append(argv, "open");
    argv = g_slist_append(argv, (gpointer)uri);
    @@ -1315,10 +1313,10 @@
    argv = g_slist_append(argv, (gpointer)uri);
    } else if (!strcmp(web_browser, "xdg-open")) {
    argv = g_slist_append(argv, "xdg-open");
    - argv = g_slist_append(argv, uri_escaped);
    + argv = g_slist_append(argv, (gpointer)uri);
    } else if (!strcmp(web_browser, "gnome-open")) {
    argv = g_slist_append(argv, "gnome-open");
    - argv = g_slist_append(argv, uri_escaped);
    + argv = g_slist_append(argv, (gpointer)uri);
    } else if (!strcmp(web_browser, "kfmclient")) {
    argv = g_slist_append(argv, "kfmclient");
    argv = g_slist_append(argv, "openURL");
    @@ -1333,18 +1331,15 @@
    !strcmp(web_browser, "seamonkey"))
    {
    argv = g_slist_append(argv, (gpointer)web_browser);
    - argv = g_slist_append(argv, uri_escaped);
    + argv = g_slist_append(argv, (gpointer)uri);
    g_assert(uri_custom == NULL);
    if (place == PIDGIN_BROWSER_NEW_WINDOW) {
    - uri_custom = g_strdup_printf("openURL(%s,new-window)",
    - uri_escaped);
    + uri_custom = g_strdup_printf("openURL(%s,new-window)", uri);
    } else if (place == PIDGIN_BROWSER_NEW_TAB) {
    - uri_custom = g_strdup_printf("openURL(%s,new-tab)",
    - uri_escaped);
    + uri_custom = g_strdup_printf("openURL(%s,new-tab)", uri);
    } else if (place == PIDGIN_BROWSER_CURRENT) {
    - uri_custom = g_strdup_printf("openURL(%s)",
    - uri_escaped);
    + uri_custom = g_strdup_printf("openURL(%s)", uri);
    }
    if (uri_custom != NULL) {
    @@ -1373,11 +1368,9 @@
    argv = g_slist_append(argv, (gpointer)uri);
    if (place == PIDGIN_BROWSER_NEW_WINDOW) {
    - uri_custom = g_strdup_printf("openURL(%s,new-window)",
    - uri_escaped);
    + uri_custom = g_strdup_printf("openURL(%s,new-window)", uri);
    } else if (place == PIDGIN_BROWSER_CURRENT) {
    - uri_custom = g_strdup_printf("openURL(%s)",
    - uri_escaped);
    + uri_custom = g_strdup_printf("openURL(%s)", uri);
    }
    if (uri_custom) {
    @@ -1433,7 +1426,6 @@
    purple_notify_error(NULL, NULL, _("Unable to open URL"),
    _("The 'Manual' browser command has been "
    "chosen, but no command has been set."));
    - g_free(uri_escaped);
    return NULL;
    }
    @@ -1444,7 +1436,6 @@
    "the 'Manual' browser command seems invalid."),
    error ? error->message : NULL);
    g_error_free(error);
    - g_free(uri_escaped);
    return NULL;
    }
    @@ -1456,8 +1447,7 @@
    continue;
    }
    - uri_custom = purple_strreplace(cmd_part, "%s",
    - uri_escaped);
    + uri_custom = purple_strreplace(cmd_part, "%s", uri);
    argv = g_slist_append(argv, uri_custom);
    uri_added = TRUE;
    }
    @@ -1466,7 +1456,7 @@
    * wanted the URL tacked on to the end of the command.
    */
    if (!uri_added)
    - argv = g_slist_append(argv, uri_escaped);
    + argv = g_slist_append(argv, (gpointer)uri);
    }
    if (argv_remote != NULL) {
    @@ -1477,7 +1467,6 @@
    uri_command(argv, FALSE);
    g_strfreev(usercmd_argv);
    - g_free(uri_escaped);
    g_free(uri_custom);
    g_slist_free(argv);
    g_slist_free(argv_remote);