libstore: Fix pct-encoding issues in store references

src/libstore-test-support/https-store.cc

@@ -38,8 +38,7 @@ void HttpsBinaryCacheStoreTest::SetUp()
     delTmpDir = std::make_unique<AutoDelete>(tmpDir);
 
     localCacheStore =
-        make_ref<LocalBinaryCacheStoreConfig>("file", cacheDir.string(), LocalBinaryCacheStoreConfig::Params{})
-            ->openStore();
+        make_ref<LocalBinaryCacheStoreConfig>(cacheDir, LocalBinaryCacheStoreConfig::Params{})->openStore();
 
     caCert = tmpDir / "ca.crt";
     caKey = tmpDir / "ca.key";

src/libstore-tests/http-binary-cache-store.cc

@@ -7,32 +7,39 @@
 
 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);
 }
 
 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);
 }

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",

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");
 }
 

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"});
 }

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");
 }
 

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<std::string>("GLACIER"));
 }
 

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",

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");

src/libstore/common-ssh-store-config.cc

@@ -1,17 +1,9 @@
-#include <regex>
-
 #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)
 {

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();
 }

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<std::filesystem::path> sshKey{
         this, "", "ssh-key", "Path to the SSH private key used to authenticate to the remote machine."};

src/libstore/include/nix/store/http-binary-cache-store.hh

@@ -16,9 +16,6 @@ struct HttpBinaryCacheStoreConfig : std::enable_shared_from_this<HttpBinaryCache
 {
     using BinaryCacheStoreConfig::BinaryCacheStoreConfig;
 
-    HttpBinaryCacheStoreConfig(
-        std::string_view scheme, std::string_view cacheUri, const Store::Config::Params & params);
-
     HttpBinaryCacheStoreConfig(ParsedURL cacheUri, const Store::Config::Params & params);
 
     ParsedURL cacheUri;

src/libstore/include/nix/store/legacy-ssh-store.hh

@@ -14,7 +14,7 @@ struct LegacySSHStoreConfig : std::enable_shared_from_this<LegacySSHStoreConfig>
 {
     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.

src/libstore/include/nix/store/local-binary-cache-store.hh

@@ -12,7 +12,7 @@ struct LocalBinaryCacheStoreConfig : std::enable_shared_from_this<LocalBinaryCac
      * @param binaryCacheDir `file://` is a short-hand for `file:///`
      * for now.
      */
-    LocalBinaryCacheStoreConfig(std::string_view scheme, PathView binaryCacheDir, const Params & params);
+    LocalBinaryCacheStoreConfig(const std::filesystem::path & binaryCacheDir, const Params & params);
 
     std::filesystem::path binaryCacheDir;
 

src/libstore/include/nix/store/local-fs-store.hh

@@ -31,7 +31,7 @@ public:
      *
      * @todo Make this less error-prone with new store settings system.
      */
-    LocalFSStoreConfig(PathView path, const Params & params);
+    LocalFSStoreConfig(const std::filesystem::path & path, const Params & params);
 
     Setting<std::optional<std::filesystem::path>> rootDir = makeRootDirSetting(*this, std::nullopt);
 

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)
     {
     }
 

src/libstore/include/nix/store/local-store.hh

@@ -82,7 +82,7 @@ struct LocalStoreConfig : std::enable_shared_from_this<LocalStoreConfig>,
 {
     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:
 

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<std::string> profile{
         this,

src/libstore/include/nix/store/ssh-store.hh

@@ -15,7 +15,7 @@ struct SSHStoreConfig : std::enable_shared_from_this<SSHStoreConfig>,
     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<Strings> 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<SSHStoreConfig>,
 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()
     {

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
     {