From 4e8178916711bfe30c031b26633aef50ffc16e5e Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Tue, 5 Mar 2019 13:04:51 -0800 Subject: [PATCH] Consistently use flatpak_load_http_uri() Currently Flatpak has a few different implementations of helper functions to download a URI using libsoup, but the best one seems to be in common/flatpak-utils-http.c. So this commit deletes the others and makes use of flatpak_load_http_uri() in place of download_uri() in a few places. This has a couple consequences: 1) It means that we're now properly checking HTTP status codes rather than assuming that the request was successful, in the install command, remote-add command, and in FlatpakTransaction. This fixes an assertion failure seen by a user when they tried to use a flatpakref URL that hit a 404. 2) It means that in the places where we're using flatpak_load_http_uri() we are only supporting http:// and https:// URLs not, say, file:// ones. For the install and remote-add commands this was already being enforced. For the handling of flatpakref files and bundles in FlatpakTransaction, I believe it's just convention because it doesn't make much sense to do anything else; this commit enforces that convention. Also, add a unit test for the case of trying to install a flatpakref at a URL that hits a 404 error. Fixes https://github.com/flatpak/flatpak/issues/2727 Closes: #2740 Approved by: matthiasclasen --- app/flatpak-builtins-install.c | 4 ++- app/flatpak-builtins-remote-add.c | 4 ++- app/flatpak-builtins-utils.c | 59 ------------------------------- app/flatpak-builtins-utils.h | 2 -- common/flatpak-transaction.c | 58 +++++++++++------------------- tests/test-repo.sh | 10 +++++- 6 files changed, 35 insertions(+), 102 deletions(-) diff --git a/app/flatpak-builtins-install.c b/app/flatpak-builtins-install.c index 33d56ccb..9370217d 100644 --- a/app/flatpak-builtins-install.c +++ b/app/flatpak-builtins-install.c @@ -206,7 +206,9 @@ install_from (FlatpakDir *dir, if (g_str_has_prefix (filename, "http:") || g_str_has_prefix (filename, "https:")) { - file_data = download_uri (filename, error); + g_autoptr(SoupSession) soup_session = NULL; + soup_session = flatpak_create_soup_session (PACKAGE_STRING); + file_data = flatpak_load_http_uri (soup_session, filename, 0, NULL, NULL, cancellable, error); if (file_data == NULL) { g_prefix_error (error, "Can't load uri %s: ", filename); diff --git a/app/flatpak-builtins-remote-add.c b/app/flatpak-builtins-remote-add.c index b6374aa9..c69c19a1 100644 --- a/app/flatpak-builtins-remote-add.c +++ b/app/flatpak-builtins-remote-add.c @@ -183,8 +183,10 @@ load_options (const char *filename, { const char *options_data; gsize options_size; + g_autoptr(SoupSession) soup_session = NULL; - bytes = download_uri (filename, &error); + soup_session = flatpak_create_soup_session (PACKAGE_STRING); + bytes = flatpak_load_http_uri (soup_session, filename, 0, NULL, NULL, NULL, &error); if (bytes == NULL) { diff --git a/app/flatpak-builtins-utils.c b/app/flatpak-builtins-utils.c index c3f4d033..39d6c724 100644 --- a/app/flatpak-builtins-utils.c +++ b/app/flatpak-builtins-utils.c @@ -92,65 +92,6 @@ looks_like_branch (const char *branch) return TRUE; } -static SoupSession * -get_soup_session (void) -{ - static SoupSession *soup_session = NULL; - - if (soup_session == NULL) - { - const char *http_proxy; - - soup_session = soup_session_new_with_options (SOUP_SESSION_USER_AGENT, "flatpak-builder ", - SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE, - SOUP_SESSION_USE_THREAD_CONTEXT, TRUE, - SOUP_SESSION_TIMEOUT, 60, - SOUP_SESSION_IDLE_TIMEOUT, 60, - NULL); - http_proxy = g_getenv ("http_proxy"); - if (http_proxy) - { - g_autoptr(SoupURI) proxy_uri = soup_uri_new (http_proxy); - if (!proxy_uri) - g_warning ("Invalid proxy URI '%s'", http_proxy); - else - g_object_set (soup_session, SOUP_SESSION_PROXY_URI, proxy_uri, NULL); - } - } - - return soup_session; -} - -GBytes * -download_uri (const char *url, - GError **error) -{ - SoupSession *session; - g_autoptr(SoupRequest) req = NULL; - g_autoptr(GInputStream) input = NULL; - g_autoptr(GOutputStream) out = NULL; - - session = get_soup_session (); - - req = soup_session_request (session, url, error); - if (req == NULL) - return NULL; - - input = soup_request_send (req, NULL, error); - if (input == NULL) - return NULL; - - out = g_memory_output_stream_new_resizable (); - if (!g_output_stream_splice (out, - input, - G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET | G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE, - NULL, - error)) - return NULL; - - return g_memory_output_stream_steal_as_bytes (G_MEMORY_OUTPUT_STREAM (out)); -} - FlatpakDir * flatpak_find_installed_pref (const char *pref, FlatpakKinds kinds, const char *default_arch, const char *default_branch, gboolean search_all, gboolean search_user, gboolean search_system, char **search_installations, diff --git a/app/flatpak-builtins-utils.h b/app/flatpak-builtins-utils.h index b9c1e202..0fcc64c8 100644 --- a/app/flatpak-builtins-utils.h +++ b/app/flatpak-builtins-utils.h @@ -52,8 +52,6 @@ RemoteDirPair * remote_dir_pair_new (const char *remote_name, FlatpakDir *dir); gboolean looks_like_branch (const char *branch); -GBytes * download_uri (const char *url, - GError **error); GBytes * flatpak_load_gpg_keys (char **gpg_import, GCancellable *cancellable, diff --git a/common/flatpak-transaction.c b/common/flatpak-transaction.c index f5862c45..c7b589a3 100644 --- a/common/flatpak-transaction.c +++ b/common/flatpak-transaction.c @@ -2351,37 +2351,6 @@ flatpak_transaction_get_installation (FlatpakTransaction *self) return g_object_ref (priv->installation); } - -static GBytes * -download_uri (const char *url, - GError **error) -{ - g_autoptr(SoupSession) session = NULL; - g_autoptr(SoupRequest) req = NULL; - g_autoptr(GInputStream) input = NULL; - g_autoptr(GOutputStream) out = NULL; - - session = flatpak_create_soup_session (PACKAGE_STRING); - - req = soup_session_request (session, url, error); - if (req == NULL) - return NULL; - - input = soup_request_send (req, NULL, error); - if (input == NULL) - return NULL; - - out = g_memory_output_stream_new_resizable (); - if (!g_output_stream_splice (out, - input, - G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET | G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE, - NULL, - error)) - return NULL; - - return g_memory_output_stream_steal_as_bytes (G_MEMORY_OUTPUT_STREAM (out)); -} - static gboolean remote_is_already_configured (FlatpakTransaction *self, const char *url, @@ -2465,7 +2434,11 @@ handle_suggested_remote_name (FlatpakTransaction *self, GKeyFile *keyfile, GErro } static gboolean -handle_runtime_repo_deps (FlatpakTransaction *self, const char *id, const char *dep_url, GError **error) +handle_runtime_repo_deps (FlatpakTransaction *self, + const char *id, + const char *dep_url, + GCancellable *cancellable, + GError **error) { FlatpakTransactionPrivate *priv = flatpak_transaction_get_instance_private (self); g_autoptr(GBytes) dep_data = NULL; @@ -2480,6 +2453,7 @@ handle_runtime_repo_deps (FlatpakTransaction *self, const char *id, const char * g_autofree char *group = NULL; g_autoptr(GError) local_error = NULL; g_autofree char *runtime_collection_id = NULL; + g_autoptr(SoupSession) soup_session = NULL; char *t; int i; gboolean res; @@ -2487,7 +2461,11 @@ handle_runtime_repo_deps (FlatpakTransaction *self, const char *id, const char * if (priv->disable_deps) return TRUE; - dep_data = download_uri (dep_url, error); + if (!g_str_has_prefix (dep_url, "http:") && !g_str_has_prefix (dep_url, "https:")) + return flatpak_fail_error (error, FLATPAK_ERROR_INVALID_DATA, _("Flatpakrepo URL %s not HTTP or HTTPS"), dep_url); + + soup_session = flatpak_create_soup_session (PACKAGE_STRING); + dep_data = flatpak_load_http_uri (soup_session, dep_url, 0, NULL, NULL, cancellable, error); if (dep_data == NULL) { g_prefix_error (error, "Can't load dependent file %s", dep_url); @@ -2556,7 +2534,10 @@ handle_runtime_repo_deps (FlatpakTransaction *self, const char *id, const char * } static gboolean -handle_runtime_repo_deps_from_keyfile (FlatpakTransaction *self, GKeyFile *keyfile, GError **error) +handle_runtime_repo_deps_from_keyfile (FlatpakTransaction *self, + GKeyFile *keyfile, + GCancellable *cancellable, + GError **error) { FlatpakTransactionPrivate *priv = flatpak_transaction_get_instance_private (self); g_autofree char *dep_url = NULL; @@ -2574,7 +2555,7 @@ handle_runtime_repo_deps_from_keyfile (FlatpakTransaction *self, GKeyFile *keyfi if (name == NULL) return TRUE; - return handle_runtime_repo_deps (self, name, dep_url, error); + return handle_runtime_repo_deps (self, name, dep_url, cancellable, error); } static gboolean @@ -2595,7 +2576,7 @@ flatpak_transaction_resolve_flatpakrefs (FlatpakTransaction *self, if (!handle_suggested_remote_name (self, flatpakref, error)) return FALSE; - if (!handle_runtime_repo_deps_from_keyfile (self, flatpakref, error)) + if (!handle_runtime_repo_deps_from_keyfile (self, flatpakref, cancellable, error)) return FALSE; if (!flatpak_dir_create_remote_for_ref_file (priv->dir, flatpakref, priv->default_arch, @@ -2618,6 +2599,7 @@ flatpak_transaction_resolve_flatpakrefs (FlatpakTransaction *self, static gboolean handle_runtime_repo_deps_from_bundle (FlatpakTransaction *self, GFile *file, + GCancellable *cancellable, GError **error) { FlatpakTransactionPrivate *priv = flatpak_transaction_get_instance_private (self); @@ -2645,7 +2627,7 @@ handle_runtime_repo_deps_from_bundle (FlatpakTransaction *self, ref_parts = g_strsplit (ref, "/", -1); - return handle_runtime_repo_deps (self, ref_parts[1], dep_url, error); + return handle_runtime_repo_deps (self, ref_parts[1], dep_url, cancellable, error); } static gboolean @@ -2665,7 +2647,7 @@ flatpak_transaction_resolve_bundles (FlatpakTransaction *self, g_autofree char *metadata = NULL; gboolean created_remote; - if (!handle_runtime_repo_deps_from_bundle (self, data->file, error)) + if (!handle_runtime_repo_deps_from_bundle (self, data->file, cancellable, error)) return FALSE; if (!flatpak_dir_ensure_repo (priv->dir, cancellable, error)) diff --git a/tests/test-repo.sh b/tests/test-repo.sh index 4a309760..04b7775c 100755 --- a/tests/test-repo.sh +++ b/tests/test-repo.sh @@ -23,7 +23,7 @@ set -euo pipefail skip_without_bwrap -echo "1..27" +echo "1..28" #Regular repo setup_repo @@ -169,6 +169,14 @@ fi echo "ok missing remote name auto-corrects for install" +port=$(cat httpd-port-main) +if ${FLATPAK} ${U} install -y http://127.0.0.1:${port}/nonexistent.flatpakref 2> install-error-log; then + assert_not_reached "Should not be able to install a nonexistent flatpakref" +fi +assert_file_has_content install-error-log "Server returned status 404: Not Found" + +echo "ok install fails gracefully for 404 URLs" + ${FLATPAK} ${U} uninstall -y org.test.Platform org.test.Hello if ${FLATPAK} ${U} install -y test-missing-gpg-repo org.test.Platform 2> install-error-log; then