From 224c8182df1b70d297417f3c98b1ce7635995af0 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 18 Feb 2026 13:37:29 +0300 Subject: [PATCH] libstore: Fix pct-encoding issues in store references When the common pattern for store config constructors is to accept an arbitrary string it's unclear whether it needs pct-decoding or not. Prior to c436b7a32afaf01d62f828697ddf5c49d4f8678c the string passed to the constructors was a mix of encoded authority and decoded path concatenated with a `/`. After that commit it accidentally started accepting pct-encoded result of renderAuthorityAndPath, but only in some code paths. This lead to file:///tmp/a+b to be created on disk in /tmp/a%2Bb directory. Similar issue affected the less-known variant with local:///tmp/a+b. Regular store references that are local paths were not affected. This patch changes the constructors to accept different types to signify what is actually needed to let the factory method handle this in a consistent way: - std::filesystem::path - local binary cache store and local store - ParsedURL::Authority - for ssh stores - ParsedURL - for http stores (Some MinGW build fixes by Amaan Qureshi ) --- src/libstore-test-support/https-store.cc | 3 +- src/libstore-tests/http-binary-cache-store.cc | 17 +++++++--- src/libstore-tests/legacy-ssh-store.cc | 3 +- .../local-binary-cache-store.cc | 3 +- src/libstore-tests/local-overlay-store.cc | 3 +- src/libstore-tests/local-store.cc | 5 ++- src/libstore-tests/s3-binary-cache-store.cc | 14 ++++---- src/libstore-tests/ssh-store.cc | 6 ++-- src/libstore-tests/uds-remote-store.cc | 13 +++----- src/libstore/common-ssh-store-config.cc | 10 +----- src/libstore/http-binary-cache-store.cc | 14 ++------ .../nix/store/common-ssh-store-config.hh | 3 +- .../nix/store/http-binary-cache-store.hh | 3 -- .../include/nix/store/legacy-ssh-store.hh | 2 +- .../nix/store/local-binary-cache-store.hh | 2 +- .../include/nix/store/local-fs-store.hh | 2 +- .../include/nix/store/local-overlay-store.hh | 6 ++-- src/libstore/include/nix/store/local-store.hh | 2 +- .../nix/store/s3-binary-cache-store.hh | 4 ++- src/libstore/include/nix/store/ssh-store.hh | 4 +-- .../include/nix/store/store-reference.hh | 1 + .../include/nix/store/store-registration.hh | 12 ++++++- .../include/nix/store/uds-remote-store.hh | 7 +--- src/libstore/legacy-ssh-store.cc | 4 +-- src/libstore/local-binary-cache-store.cc | 33 ++++++++++++++----- src/libstore/local-fs-store.cc | 2 +- src/libstore/local-store.cc | 6 ++-- src/libstore/s3-binary-cache-store.cc | 11 +++++-- src/libstore/ssh-store.cc | 10 +++--- src/libstore/store-registration.cc | 2 +- src/libstore/uds-remote-store.cc | 12 +++---- src/libutil/include/nix/util/url.hh | 2 +- src/nix/nix-copy-closure/nix-copy-closure.cc | 6 +++- tests/functional/binary-cache.sh | 4 +++ tests/functional/chroot-store.sh | 18 ++++++++++ 35 files changed, 137 insertions(+), 112 deletions(-) diff --git a/src/libstore-test-support/https-store.cc b/src/libstore-test-support/https-store.cc index aeba8f0e0f4..79548f61f3f 100644 --- a/src/libstore-test-support/https-store.cc +++ b/src/libstore-test-support/https-store.cc @@ -38,8 +38,7 @@ void HttpsBinaryCacheStoreTest::SetUp() delTmpDir = std::make_unique(tmpDir); localCacheStore = - make_ref("file", cacheDir.string(), LocalBinaryCacheStoreConfig::Params{}) - ->openStore(); + make_ref(cacheDir, LocalBinaryCacheStoreConfig::Params{})->openStore(); caCert = tmpDir / "ca.crt"; caKey = tmpDir / "ca.key"; diff --git a/src/libstore-tests/http-binary-cache-store.cc b/src/libstore-tests/http-binary-cache-store.cc index 33369cd9b76..8495db4b5ab 100644 --- a/src/libstore-tests/http-binary-cache-store.cc +++ b/src/libstore-tests/http-binary-cache-store.cc @@ -7,24 +7,31 @@ namespace nix { +using Authority = ParsedURL::Authority; + TEST(HttpBinaryCacheStore, constructConfig) { - HttpBinaryCacheStoreConfig config{"http", "foo.bar.baz", {}}; + HttpBinaryCacheStoreConfig config{ + { + .scheme = "http", + .authority = Authority{.host = "foo.bar.baz"}, + }, + {}, + }; EXPECT_EQ(config.cacheUri.to_string(), "http://foo.bar.baz"); } TEST(HttpBinaryCacheStore, constructConfigNoTrailingSlash) { - HttpBinaryCacheStoreConfig config{"https", "foo.bar.baz/a/b/", {}}; - + HttpBinaryCacheStoreConfig config{parseURL("https://foo.bar.baz/a/b/"), {}}; EXPECT_EQ(config.cacheUri.to_string(), "https://foo.bar.baz/a/b"); } TEST(HttpBinaryCacheStore, constructConfigWithParams) { StoreConfig::Params params{{"compression", "xz"}}; - HttpBinaryCacheStoreConfig config{"https", "foo.bar.baz/a/b/", params}; + HttpBinaryCacheStoreConfig config{parseURL("https://foo.bar.baz/a/b/"), params}; EXPECT_EQ(config.cacheUri.to_string(), "https://foo.bar.baz/a/b"); EXPECT_EQ(config.getReference().params, params); } @@ -32,7 +39,7 @@ TEST(HttpBinaryCacheStore, constructConfigWithParams) TEST(HttpBinaryCacheStore, constructConfigWithParamsAndUrlWithParams) { StoreConfig::Params params{{"compression", "xz"}}; - HttpBinaryCacheStoreConfig config{"https", "foo.bar.baz/a/b?some-param=some-value", params}; + HttpBinaryCacheStoreConfig config{parseURL("https://foo.bar.baz/a/b?some-param=some-value"), params}; EXPECT_EQ(config.cacheUri.to_string(), "https://foo.bar.baz/a/b?some-param=some-value"); EXPECT_EQ(config.getReference().params, params); } diff --git a/src/libstore-tests/legacy-ssh-store.cc b/src/libstore-tests/legacy-ssh-store.cc index d60ecc424c5..35543cf4006 100644 --- a/src/libstore-tests/legacy-ssh-store.cc +++ b/src/libstore-tests/legacy-ssh-store.cc @@ -7,8 +7,7 @@ namespace nix { TEST(LegacySSHStore, constructConfig) { LegacySSHStoreConfig config( - "ssh", - "me@localhost:2222", + ParsedURL::Authority::parse("me@localhost:2222"), StoreConfig::Params{ { "remote-program", diff --git a/src/libstore-tests/local-binary-cache-store.cc b/src/libstore-tests/local-binary-cache-store.cc index 01f514e89aa..295976488b5 100644 --- a/src/libstore-tests/local-binary-cache-store.cc +++ b/src/libstore-tests/local-binary-cache-store.cc @@ -6,8 +6,7 @@ namespace nix { TEST(LocalBinaryCacheStore, constructConfig) { - LocalBinaryCacheStoreConfig config{"local", "/foo/bar/baz", {}}; - + LocalBinaryCacheStoreConfig config{std::filesystem::path("/foo/bar/baz"), {}}; EXPECT_EQ(config.binaryCacheDir, "/foo/bar/baz"); } diff --git a/src/libstore-tests/local-overlay-store.cc b/src/libstore-tests/local-overlay-store.cc index 175e5d0f44e..c207564255f 100644 --- a/src/libstore-tests/local-overlay-store.cc +++ b/src/libstore-tests/local-overlay-store.cc @@ -7,7 +7,6 @@ namespace nix { TEST(LocalOverlayStore, constructConfig_rootQueryParam) { LocalOverlayStoreConfig config{ - "local-overlay", "", { { @@ -22,7 +21,7 @@ TEST(LocalOverlayStore, constructConfig_rootQueryParam) TEST(LocalOverlayStore, constructConfig_rootPath) { - LocalOverlayStoreConfig config{"local-overlay", "/foo/bar", {}}; + LocalOverlayStoreConfig config{"/foo/bar", {}}; EXPECT_EQ(config.rootDir.get(), std::optional{"/foo/bar"}); } diff --git a/src/libstore-tests/local-store.cc b/src/libstore-tests/local-store.cc index d008888974b..554d42efc81 100644 --- a/src/libstore-tests/local-store.cc +++ b/src/libstore-tests/local-store.cc @@ -13,7 +13,6 @@ namespace nix { TEST(LocalStore, constructConfig_rootQueryParam) { LocalStoreConfig config{ - "local", "", { { @@ -28,14 +27,14 @@ TEST(LocalStore, constructConfig_rootQueryParam) TEST(LocalStore, constructConfig_rootPath) { - LocalStoreConfig config{"local", "/foo/bar", {}}; + LocalStoreConfig config{"/foo/bar", {}}; EXPECT_EQ(config.rootDir.get(), std::optional{"/foo/bar"}); } TEST(LocalStore, constructConfig_to_string) { - LocalStoreConfig config{"local", "", {}}; + LocalStoreConfig config{"", {}}; EXPECT_EQ(config.getReference().to_string(), "local"); } diff --git a/src/libstore-tests/s3-binary-cache-store.cc b/src/libstore-tests/s3-binary-cache-store.cc index 59090a589f0..9aa9b2dd1a3 100644 --- a/src/libstore-tests/s3-binary-cache-store.cc +++ b/src/libstore-tests/s3-binary-cache-store.cc @@ -9,7 +9,7 @@ namespace nix { TEST(S3BinaryCacheStore, constructConfig) { - S3BinaryCacheStoreConfig config{"s3", "foobar", {}}; + S3BinaryCacheStoreConfig config{"foobar", {}}; // The bucket name is stored as the host part of the authority in cacheUri EXPECT_EQ( @@ -23,7 +23,7 @@ TEST(S3BinaryCacheStore, constructConfig) TEST(S3BinaryCacheStore, constructConfigWithRegion) { Store::Config::Params params{{"region", "eu-west-1"}}; - S3BinaryCacheStoreConfig config{"s3", "my-bucket", params}; + S3BinaryCacheStoreConfig config{"my-bucket", params}; EXPECT_EQ( config.cacheUri, @@ -37,7 +37,7 @@ TEST(S3BinaryCacheStore, constructConfigWithRegion) TEST(S3BinaryCacheStore, defaultSettings) { - S3BinaryCacheStoreConfig config{"s3", "test-bucket", {}}; + S3BinaryCacheStoreConfig config{"test-bucket", {}}; EXPECT_EQ( config.cacheUri, @@ -62,7 +62,7 @@ TEST(S3BinaryCacheStore, s3StoreConfigPreservesParameters) params["region"] = "eu-west-1"; params["endpoint"] = "custom.s3.com"; - S3BinaryCacheStoreConfig config("s3", "test-bucket", params); + S3BinaryCacheStoreConfig config("test-bucket", params); // The config should preserve S3-specific parameters EXPECT_EQ( @@ -99,7 +99,7 @@ TEST(S3BinaryCacheStore, parameterFiltering) params["want-mass-query"] = "true"; // Non-S3 store parameter params["priority"] = "10"; // Non-S3 store parameter - S3BinaryCacheStoreConfig config("s3", "test-bucket", params); + S3BinaryCacheStoreConfig config("test-bucket", params); // Only S3-specific params should be in cacheUri.query EXPECT_EQ( @@ -127,7 +127,7 @@ TEST(S3BinaryCacheStore, parameterFiltering) */ TEST(S3BinaryCacheStore, storageClassDefault) { - S3BinaryCacheStoreConfig config{"s3", "test-bucket", {}}; + S3BinaryCacheStoreConfig config{"test-bucket", {}}; EXPECT_EQ(config.storageClass.get(), std::nullopt); } @@ -136,7 +136,7 @@ TEST(S3BinaryCacheStore, storageClassConfiguration) StringMap params; params["storage-class"] = "GLACIER"; - S3BinaryCacheStoreConfig config("s3", "test-bucket", params); + S3BinaryCacheStoreConfig config("test-bucket", params); EXPECT_EQ(config.storageClass.get(), std::optional("GLACIER")); } diff --git a/src/libstore-tests/ssh-store.cc b/src/libstore-tests/ssh-store.cc index a156da52b71..2d4548d6ae2 100644 --- a/src/libstore-tests/ssh-store.cc +++ b/src/libstore-tests/ssh-store.cc @@ -9,8 +9,7 @@ namespace nix { TEST(SSHStore, constructConfig) { SSHStoreConfig config{ - "ssh-ng", - "me@localhost:2222", + ParsedURL::Authority::parse("me@localhost:2222"), StoreConfig::Params{ { "remote-program", @@ -35,8 +34,7 @@ TEST(SSHStore, constructConfig) TEST(MountedSSHStore, constructConfig) { MountedSSHStoreConfig config{ - "mounted-ssh", - "localhost", + {.host = "localhost"}, StoreConfig::Params{ { "remote-program", diff --git a/src/libstore-tests/uds-remote-store.cc b/src/libstore-tests/uds-remote-store.cc index 415dfc4ac94..88af22dbb22 100644 --- a/src/libstore-tests/uds-remote-store.cc +++ b/src/libstore-tests/uds-remote-store.cc @@ -6,26 +6,21 @@ namespace nix { TEST(UDSRemoteStore, constructConfig) { - UDSRemoteStoreConfig config{"unix", "/tmp/socket", {}}; + UDSRemoteStoreConfig config{"/tmp/socket", {}}; EXPECT_EQ(config.path, "/tmp/socket"); } -TEST(UDSRemoteStore, constructConfigWrongScheme) -{ - EXPECT_THROW(UDSRemoteStoreConfig("http", "/tmp/socket", {}), UsageError); -} - TEST(UDSRemoteStore, constructConfig_to_string) { - UDSRemoteStoreConfig config{"unix", "", {}}; + UDSRemoteStoreConfig config{"", {}}; EXPECT_EQ(config.getReference().to_string(), "daemon"); } TEST(UDSRemoteStore, constructConfigWithParams) { StoreConfig::Params params{{"max-connections", "1"}}; - UDSRemoteStoreConfig config{"unix", "/tmp/socket", params}; + UDSRemoteStoreConfig config{"/tmp/socket", params}; auto storeReference = config.getReference(); EXPECT_EQ(storeReference.to_string(), "unix:///tmp/socket?max-connections=1"); EXPECT_EQ(storeReference.render(/*withParams=*/false), "unix:///tmp/socket"); @@ -35,7 +30,7 @@ TEST(UDSRemoteStore, constructConfigWithParams) TEST(UDSRemoteStore, constructConfigWithParamsNoPath) { StoreConfig::Params params{{"max-connections", "1"}}; - UDSRemoteStoreConfig config{"unix", "", params}; + UDSRemoteStoreConfig config{"", params}; auto storeReference = config.getReference(); EXPECT_EQ(storeReference.to_string(), "daemon?max-connections=1"); EXPECT_EQ(storeReference.render(/*withParams=*/false), "daemon"); diff --git a/src/libstore/common-ssh-store-config.cc b/src/libstore/common-ssh-store-config.cc index 12f187b4c9e..fd61b12efee 100644 --- a/src/libstore/common-ssh-store-config.cc +++ b/src/libstore/common-ssh-store-config.cc @@ -1,17 +1,9 @@ -#include - #include "nix/store/common-ssh-store-config.hh" #include "nix/store/ssh.hh" namespace nix { -CommonSSHStoreConfig::CommonSSHStoreConfig(std::string_view scheme, std::string_view authority, const Params & params) - : CommonSSHStoreConfig(scheme, ParsedURL::Authority::parse(authority), params) -{ -} - -CommonSSHStoreConfig::CommonSSHStoreConfig( - std::string_view scheme, const ParsedURL::Authority & authority, const Params & params) +CommonSSHStoreConfig::CommonSSHStoreConfig(const ParsedURL::Authority & authority, const Params & params) : StoreConfig(params) , authority(authority) { diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index c1bee215ce3..b3678ae4fdf 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -20,23 +20,13 @@ StringSet HttpBinaryCacheStoreConfig::uriSchemes() return ret; } -HttpBinaryCacheStoreConfig::HttpBinaryCacheStoreConfig( - std::string_view scheme, std::string_view _cacheUri, const Params & params) - : HttpBinaryCacheStoreConfig( - parseURL( - std::string{scheme} + "://" - + (!_cacheUri.empty() - ? _cacheUri - : throw UsageError("`%s` Store requires a non-empty authority in Store URL", scheme))), - params) -{ -} - HttpBinaryCacheStoreConfig::HttpBinaryCacheStoreConfig(ParsedURL _cacheUri, const Params & params) : StoreConfig(params) , BinaryCacheStoreConfig(params) , cacheUri(std::move(_cacheUri)) { + if (!uriSchemes().contains("file") && (!cacheUri.authority || cacheUri.authority->host.empty())) + throw UsageError("`%s` Store requires a non-empty authority in Store URL", cacheUri.scheme); while (!cacheUri.path.empty() && cacheUri.path.back() == "") cacheUri.path.pop_back(); } diff --git a/src/libstore/include/nix/store/common-ssh-store-config.hh b/src/libstore/include/nix/store/common-ssh-store-config.hh index 7f8fc97b185..1fdb1675bdb 100644 --- a/src/libstore/include/nix/store/common-ssh-store-config.hh +++ b/src/libstore/include/nix/store/common-ssh-store-config.hh @@ -12,8 +12,7 @@ struct CommonSSHStoreConfig : virtual StoreConfig { using StoreConfig::StoreConfig; - CommonSSHStoreConfig(std::string_view scheme, const ParsedURL::Authority & authority, const Params & params); - CommonSSHStoreConfig(std::string_view scheme, std::string_view authority, const Params & params); + CommonSSHStoreConfig(const ParsedURL::Authority & authority, const Params & params); Setting sshKey{ this, "", "ssh-key", "Path to the SSH private key used to authenticate to the remote machine."}; diff --git a/src/libstore/include/nix/store/http-binary-cache-store.hh b/src/libstore/include/nix/store/http-binary-cache-store.hh index 701eb431f6f..7138c56667f 100644 --- a/src/libstore/include/nix/store/http-binary-cache-store.hh +++ b/src/libstore/include/nix/store/http-binary-cache-store.hh @@ -16,9 +16,6 @@ struct HttpBinaryCacheStoreConfig : std::enable_shared_from_this { using CommonSSHStoreConfig::CommonSSHStoreConfig; - LegacySSHStoreConfig(std::string_view scheme, std::string_view authority, const Params & params); + LegacySSHStoreConfig(const ParsedURL::Authority & authority, const Params & params); #ifndef _WIN32 // Hack for getting remote build log output. diff --git a/src/libstore/include/nix/store/local-binary-cache-store.hh b/src/libstore/include/nix/store/local-binary-cache-store.hh index 2438b37155c..8c4e68d2639 100644 --- a/src/libstore/include/nix/store/local-binary-cache-store.hh +++ b/src/libstore/include/nix/store/local-binary-cache-store.hh @@ -12,7 +12,7 @@ struct LocalBinaryCacheStoreConfig : std::enable_shared_from_this> rootDir = makeRootDirSetting(*this, std::nullopt); diff --git a/src/libstore/include/nix/store/local-overlay-store.hh b/src/libstore/include/nix/store/local-overlay-store.hh index 3c035007396..007ef0eed0c 100644 --- a/src/libstore/include/nix/store/local-overlay-store.hh +++ b/src/libstore/include/nix/store/local-overlay-store.hh @@ -8,14 +8,14 @@ namespace nix { struct LocalOverlayStoreConfig : virtual LocalStoreConfig { LocalOverlayStoreConfig(const StringMap & params) - : LocalOverlayStoreConfig("local-overlay", "", params) + : LocalOverlayStoreConfig("", params) { } - LocalOverlayStoreConfig(std::string_view scheme, PathView path, const Params & params) + LocalOverlayStoreConfig(const std::filesystem::path & path, const Params & params) : StoreConfig(params) , LocalFSStoreConfig(path, params) - , LocalStoreConfig(scheme, path, params) + , LocalStoreConfig(path, params) { } diff --git a/src/libstore/include/nix/store/local-store.hh b/src/libstore/include/nix/store/local-store.hh index eb4d971a18a..3d3ca6e589d 100644 --- a/src/libstore/include/nix/store/local-store.hh +++ b/src/libstore/include/nix/store/local-store.hh @@ -82,7 +82,7 @@ struct LocalStoreConfig : std::enable_shared_from_this, { using LocalFSStoreConfig::LocalFSStoreConfig; - LocalStoreConfig(std::string_view scheme, std::string_view authority, const Params & params); + LocalStoreConfig(const std::filesystem::path & path, const Params & params); private: diff --git a/src/libstore/include/nix/store/s3-binary-cache-store.hh b/src/libstore/include/nix/store/s3-binary-cache-store.hh index 4fa3cdcbdb0..0edb1ea3547 100644 --- a/src/libstore/include/nix/store/s3-binary-cache-store.hh +++ b/src/libstore/include/nix/store/s3-binary-cache-store.hh @@ -11,7 +11,9 @@ struct S3BinaryCacheStoreConfig : HttpBinaryCacheStoreConfig { using HttpBinaryCacheStoreConfig::HttpBinaryCacheStoreConfig; - S3BinaryCacheStoreConfig(std::string_view uriScheme, std::string_view bucketName, const Params & params); + S3BinaryCacheStoreConfig(ParsedURL cacheUri, const Params & params); + + S3BinaryCacheStoreConfig(std::string_view bucketName, const Params & params); Setting profile{ this, diff --git a/src/libstore/include/nix/store/ssh-store.hh b/src/libstore/include/nix/store/ssh-store.hh index 2c27bdb13ba..f10068ee4e2 100644 --- a/src/libstore/include/nix/store/ssh-store.hh +++ b/src/libstore/include/nix/store/ssh-store.hh @@ -15,7 +15,7 @@ struct SSHStoreConfig : std::enable_shared_from_this, using CommonSSHStoreConfig::CommonSSHStoreConfig; using RemoteStoreConfig::RemoteStoreConfig; - SSHStoreConfig(std::string_view scheme, std::string_view authority, const Params & params); + SSHStoreConfig(const ParsedURL::Authority & authority, const Params & params); Setting remoteProgram{ this, {"nix-daemon"}, "remote-program", "Path to the `nix-daemon` executable on the remote machine."}; @@ -40,7 +40,7 @@ struct SSHStoreConfig : std::enable_shared_from_this, struct MountedSSHStoreConfig : virtual SSHStoreConfig, virtual LocalFSStoreConfig { MountedSSHStoreConfig(StringMap params); - MountedSSHStoreConfig(std::string_view scheme, std::string_view host, StringMap params); + MountedSSHStoreConfig(const ParsedURL::Authority & authority, StringMap params); static const std::string name() { diff --git a/src/libstore/include/nix/store/store-reference.hh b/src/libstore/include/nix/store/store-reference.hh index b35c27f82ec..37ff2cdee3d 100644 --- a/src/libstore/include/nix/store/store-reference.hh +++ b/src/libstore/include/nix/store/store-reference.hh @@ -56,6 +56,7 @@ struct StoreReference /** * General case, a regular `scheme://authority` URL. + * @todo Consider making this pluggable instead of passing through the encoded authority + path. */ struct Specified { diff --git a/src/libstore/include/nix/store/store-registration.hh b/src/libstore/include/nix/store/store-registration.hh index 8b0f344ba38..8d6578cac05 100644 --- a/src/libstore/include/nix/store/store-registration.hh +++ b/src/libstore/include/nix/store/store-registration.hh @@ -14,6 +14,7 @@ */ #include "nix/store/store-api.hh" +#include "nix/util/url.hh" namespace nix { @@ -65,7 +66,16 @@ struct Implementations .uriSchemes = TConfig::uriSchemes(), .experimentalFeature = TConfig::experimentalFeature(), .parseConfig = ([](auto scheme, auto uri, auto & params) -> ref { - return make_ref(scheme, uri, params); + if constexpr (std::is_constructible_v) { + std::filesystem::path path = percentDecode(uri); + return make_ref(path.empty() ? std::filesystem::path{} : canonPath(path), params); + } else if constexpr (std::is_constructible_v) { + return make_ref(parseURL(concatStrings(scheme, "://", uri)), params); + } else if constexpr (std::is_constructible_v) { + return make_ref(ParsedURL::Authority::parse(uri), params); + } else { + return make_ref(scheme, uri, params); + } }), .getConfig = ([]() -> ref { return make_ref(Store::Config::Params{}); }), }; diff --git a/src/libstore/include/nix/store/uds-remote-store.hh b/src/libstore/include/nix/store/uds-remote-store.hh index 3b8dfadd10c..64672ee3adf 100644 --- a/src/libstore/include/nix/store/uds-remote-store.hh +++ b/src/libstore/include/nix/store/uds-remote-store.hh @@ -11,15 +11,10 @@ struct UDSRemoteStoreConfig : std::enable_shared_from_this virtual LocalFSStoreConfig, virtual RemoteStoreConfig { - // TODO(fzakaria): Delete this constructor once moved over to the factory pattern - // outlined in https://github.com/NixOS/nix/issues/10766 using LocalFSStoreConfig::LocalFSStoreConfig; using RemoteStoreConfig::RemoteStoreConfig; - /** - * @param authority is the socket path. - */ - UDSRemoteStoreConfig(std::string_view scheme, std::string_view authority, const Params & params); + UDSRemoteStoreConfig(const std::filesystem::path & path, const Params & params); UDSRemoteStoreConfig(const Params & params); diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index b2f2cf8b243..81f1facb90d 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -18,9 +18,9 @@ namespace nix { -LegacySSHStoreConfig::LegacySSHStoreConfig(std::string_view scheme, std::string_view authority, const Params & params) +LegacySSHStoreConfig::LegacySSHStoreConfig(const ParsedURL::Authority & authority, const Params & params) : StoreConfig(params) - , CommonSSHStoreConfig(scheme, ParsedURL::Authority::parse(authority), params) + , CommonSSHStoreConfig(authority, params) { } diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index ce4f2358d52..8dba477315f 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -8,8 +8,25 @@ namespace nix { +static std::filesystem::path checkBinaryCachePath(const std::filesystem::path & root, const std::string & path) +{ + auto p = std::filesystem::path(requireCString(path)); + if (p.empty()) + throw Error("local binary cache path must not be empty"); + + if (p.is_absolute()) + throw Error("local binary cache path '%s' must not be absolute", path); + + for (const auto & segment : p) { + if (segment.native() == OS_STR("..") || segment.native() == OS_STR(".")) + throw Error("local binary cache path '%s' must not contain '..' or '.' segments", path); + } + + return root / p.relative_path(); +} + LocalBinaryCacheStoreConfig::LocalBinaryCacheStoreConfig( - std::string_view scheme, PathView binaryCacheDir, const StoreReference::Params & params) + const std::filesystem::path & binaryCacheDir, const StoreReference::Params & params) : Store::Config{params} , BinaryCacheStoreConfig{params} , binaryCacheDir(binaryCacheDir) @@ -29,7 +46,7 @@ StoreReference LocalBinaryCacheStoreConfig::getReference() const .variant = StoreReference::Specified{ .scheme = "file", - .authority = binaryCacheDir.string(), + .authority = encodeUrlPath(pathToUrlPath(binaryCacheDir)), }, }; } @@ -56,23 +73,22 @@ struct LocalBinaryCacheStore : virtual BinaryCacheStore void upsertFile( const std::string & path, RestartableSource & source, const std::string & mimeType, uint64_t sizeHint) override { - assert(!std::filesystem::path(path).is_absolute()); - auto path2 = config->binaryCacheDir / path; + auto path2 = checkBinaryCachePath(config->binaryCacheDir, path); static std::atomic counter{0}; createDirs(path2.parent_path()); auto tmp = path2; tmp += fmt(".tmp.%d.%d", getpid(), ++counter); AutoDelete del(tmp, false); - writeFile(tmp, source); + writeFile(tmp, source); /* TODO: Don't follow symlinks? */ std::filesystem::rename(tmp, path2); del.cancel(); } void getFile(const std::string & path, Sink & sink) override { - assert(!std::filesystem::path(path).is_absolute()); try { - readFile(config->binaryCacheDir / path, sink); + /* TODO: Don't follow symlinks? */ + readFile(checkBinaryCachePath(config->binaryCacheDir, path), sink); } catch (SystemError & e) { if (e.is(std::errc::no_such_file_or_directory)) throw NoSuchBinaryCacheFile("file '%s' does not exist in binary cache", path); @@ -113,8 +129,7 @@ void LocalBinaryCacheStore::init() bool LocalBinaryCacheStore::fileExists(const std::string & path) { - assert(!std::filesystem::path(path).is_absolute()); - return pathExists(config->binaryCacheDir / path); + return pathExists(checkBinaryCachePath(config->binaryCacheDir, path)); } StringSet LocalBinaryCacheStoreConfig::uriSchemes() diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc index 89925098f16..410385ee113 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -18,7 +18,7 @@ std::filesystem::path LocalFSStoreConfig::getDefaultLogDir() return settings.getLogFileSettings().nixLogDir; } -LocalFSStoreConfig::LocalFSStoreConfig(PathView rootDir, const Params & params) +LocalFSStoreConfig::LocalFSStoreConfig(const std::filesystem::path & rootDir, const Params & params) : StoreConfig(params) /* Default `?root` from `rootDir` if non set * NOTE: We would like to just do rootDir.set(...), which would take care of diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 784c5770316..61dcec6f37b 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -56,9 +56,9 @@ namespace nix { -LocalStoreConfig::LocalStoreConfig(std::string_view scheme, std::string_view authority, const Params & params) +LocalStoreConfig::LocalStoreConfig(const std::filesystem::path & path, const Params & params) : StoreConfig(params) - , LocalFSStoreConfig(authority, params) + , LocalFSStoreConfig(path, params) { } @@ -468,11 +468,13 @@ StoreReference LocalStoreConfig::getReference() const /* Back-compatibility kludge. Tools like nix-output-monitor expect 'local' and can't parse 'local://'. */ if (params.empty()) + /* TODO: Add the rootDir here as the authority? */ return {.variant = StoreReference::Local{}}; return { .variant = StoreReference::Specified{ .scheme = *uriSchemes().begin(), + /* TODO: Add the rootDir here as the authority? */ }, .params = std::move(params), }; diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 2a3be047463..8c3ce56fcfc 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -416,10 +416,9 @@ StringSet S3BinaryCacheStoreConfig::uriSchemes() return {"s3"}; } -S3BinaryCacheStoreConfig::S3BinaryCacheStoreConfig( - std::string_view scheme, std::string_view _cacheUri, const Params & params) +S3BinaryCacheStoreConfig::S3BinaryCacheStoreConfig(ParsedURL cacheUri_, const Params & params) : StoreConfig(params) - , HttpBinaryCacheStoreConfig(scheme, _cacheUri, params) + , HttpBinaryCacheStoreConfig(std::move(cacheUri_), params) { assert(cacheUri.query.empty()); assert(cacheUri.scheme == "s3"); @@ -455,6 +454,12 @@ S3BinaryCacheStoreConfig::S3BinaryCacheStoreConfig( } } +S3BinaryCacheStoreConfig::S3BinaryCacheStoreConfig(std::string_view bucketName, const Params & params) + : S3BinaryCacheStoreConfig( + ParsedURL{.scheme = "s3", .authority = ParsedURL::Authority{.host = std::string(bucketName)}}, params) +{ +} + std::string S3BinaryCacheStoreConfig::getHumanReadableURI() const { auto reference = getReference(); diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index aab9741640c..22a023cf569 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -11,10 +11,10 @@ namespace nix { -SSHStoreConfig::SSHStoreConfig(std::string_view scheme, std::string_view authority, const Params & params) +SSHStoreConfig::SSHStoreConfig(const ParsedURL::Authority & authority, const Params & params) : Store::Config{params} , RemoteStore::Config{params} - , CommonSSHStoreConfig{scheme, authority, params} + , CommonSSHStoreConfig{authority, params} { } @@ -97,11 +97,11 @@ MountedSSHStoreConfig::MountedSSHStoreConfig(StringMap params) { } -MountedSSHStoreConfig::MountedSSHStoreConfig(std::string_view scheme, std::string_view host, StringMap params) +MountedSSHStoreConfig::MountedSSHStoreConfig(const ParsedURL::Authority & authority, StringMap params) : StoreConfig(params) , RemoteStoreConfig(params) - , CommonSSHStoreConfig(scheme, host, params) - , SSHStoreConfig(scheme, host, params) + , CommonSSHStoreConfig(authority, params) + , SSHStoreConfig(authority, params) , LocalFSStoreConfig(params) { } diff --git a/src/libstore/store-registration.cc b/src/libstore/store-registration.cc index 42caa819034..e4d5843ed9b 100644 --- a/src/libstore/store-registration.cc +++ b/src/libstore/store-registration.cc @@ -53,7 +53,7 @@ ref resolveStoreConfig(StoreReference && storeURI) } else debug( "%s does not exist, so Nix will use %s as a chroot store", stateDir, PathFmt(chrootStore)); - return make_ref("local", chrootStore.string(), params); + return make_ref(std::filesystem::path(chrootStore), params); } #endif else diff --git a/src/libstore/uds-remote-store.cc b/src/libstore/uds-remote-store.cc index 7c4595c634e..98e6a29332b 100644 --- a/src/libstore/uds-remote-store.cc +++ b/src/libstore/uds-remote-store.cc @@ -19,16 +19,12 @@ namespace nix { -UDSRemoteStoreConfig::UDSRemoteStoreConfig( - std::string_view scheme, std::string_view authority, const StoreReference::Params & params) +UDSRemoteStoreConfig::UDSRemoteStoreConfig(const std::filesystem::path & path, const StoreReference::Params & params) : Store::Config{params} , LocalFSStore::Config{params} , RemoteStore::Config{params} - , path{authority.empty() ? settings.nixDaemonSocketFile : std::filesystem::path{authority}} + , path{path.empty() ? settings.nixDaemonSocketFile : path} { - if (uriSchemes().count(scheme) == 0) { - throw UsageError("Scheme must be 'unix'"); - } } std::string UDSRemoteStoreConfig::doc() @@ -43,7 +39,7 @@ std::string UDSRemoteStoreConfig::doc() // don't we just wire it all through? I believe there are cases where it // will live reload so we want to continue to account for that. UDSRemoteStoreConfig::UDSRemoteStoreConfig(const Params & params) - : UDSRemoteStoreConfig(*uriSchemes().begin(), "", params) + : UDSRemoteStoreConfig("", params) { } @@ -69,7 +65,7 @@ StoreReference UDSRemoteStoreConfig::getReference() const .variant = StoreReference::Specified{ .scheme = *uriSchemes().begin(), - .authority = path.string(), + .authority = encodeUrlPath(pathToUrlPath(path)), }, .params = getQueryParams(), }; diff --git a/src/libutil/include/nix/util/url.hh b/src/libutil/include/nix/util/url.hh index 32c7ee7ecb8..b1cc1155a66 100644 --- a/src/libutil/include/nix/util/url.hh +++ b/src/libutil/include/nix/util/url.hh @@ -280,7 +280,7 @@ std::string encodeQuery(const StringMap & query); /** * Parse a URL into a ParsedURL. * - * @parm lenient Also allow some long-supported Nix URIs that are not quite compliant with RFC3986. + * @param lenient Also allow some long-supported Nix URIs that are not quite compliant with RFC3986. * Here are the deviations: * - Fragments can contain unescaped (not URL encoded) '^', '"' or space literals. * - Queries may contain unescaped '"' or spaces. diff --git a/src/nix/nix-copy-closure/nix-copy-closure.cc b/src/nix/nix-copy-closure/nix-copy-closure.cc index 8141a7f62ac..43cf11e4faf 100644 --- a/src/nix/nix-copy-closure/nix-copy-closure.cc +++ b/src/nix/nix-copy-closure/nix-copy-closure.cc @@ -49,7 +49,11 @@ static int main_nix_copy_closure(int argc, char ** argv) if (sshHost.empty()) throw UsageError("no host name specified"); - auto remoteConfig = make_ref("ssh", sshHost, LegacySSHStoreConfig::Params{}); + auto remoteConfig = + /* FIXME: This doesn't go through the back-compat machinery for IPv6 unbracketed URLs that + is in StoreReference::parse. TODO: Maybe add a authority parsing function specifically + for SSH reference parsing? */ + make_ref(ParsedURL::Authority::parse(sshHost), LegacySSHStoreConfig::Params{}); remoteConfig->compress |= gzip; auto to = toMode ? remoteConfig->openStore() : openStore(); auto from = toMode ? openStore() : remoteConfig->openStore(); diff --git a/tests/functional/binary-cache.sh b/tests/functional/binary-cache.sh index 066fa026ed9..58025b78973 100755 --- a/tests/functional/binary-cache.sh +++ b/tests/functional/binary-cache.sh @@ -38,6 +38,10 @@ nix log --substituters "file://$cacheDir" "$outPath" | grep FOO nix store copy-log --from "file://$cacheDir" "$(nix-store -qd "$outPath")"^'*' nix log "$outPath" | grep FOO +# Test that plus sign in the URL path is handled correctly. +cacheDir2="$TEST_ROOT/binary+cache" +nix copy --to "file://$cacheDir2" "$outPath" && [[ -d "$cacheDir2" ]] + basicDownloadTests() { # No uploading tests bcause upload with force HTTP doesn't work. diff --git a/tests/functional/chroot-store.sh b/tests/functional/chroot-store.sh index 7300f04ba75..cbb80c8710a 100755 --- a/tests/functional/chroot-store.sh +++ b/tests/functional/chroot-store.sh @@ -42,6 +42,24 @@ PATH2=$(nix path-info --store "$TEST_ROOT/x" "$CORRECT_PATH") PATH3=$(nix path-info --store "local?root=$TEST_ROOT/x" "$CORRECT_PATH") [ "$CORRECT_PATH" == "$PATH3" ] +# Test chroot store path with + symbols in it to exercise pct-encoding issues. +cp -r "$TEST_ROOT/x" "$TEST_ROOT/x+chroot" + +PATH4=$(nix path-info --store "local://$TEST_ROOT/x+chroot" "$CORRECT_PATH") +[ "$CORRECT_PATH" == "$PATH4" ] + +PATH5=$(nix path-info --store "$TEST_ROOT/x+chroot" "$CORRECT_PATH") +[ "$CORRECT_PATH" == "$PATH5" ] + +# Params are pct-encoded. +PATH6=$(nix path-info --store "local?root=$TEST_ROOT/x%2Bchroot" "$CORRECT_PATH") +[ "$CORRECT_PATH" == "$PATH6" ] + +PATH7=$(nix path-info --store "local://$TEST_ROOT/x%2Bchroot" "$CORRECT_PATH") +[ "$CORRECT_PATH" == "$PATH7" ] +# Path gets decoded. +[[ ! -d "$TEST_ROOT/x%2Bchroot" ]] + # Ensure store info trusted works with local store nix --store "$TEST_ROOT/x" store info --json | jq -e '.trusted'