Merge pull request NixOS#11489 from bryanhonof/bryanhonof.warn-on-mal…

…formed-uri-query fix: warn on malformed URI query parameter
bryanhonof · Sep 30, 2024 · 14f029d · 14f029d
2 parents c116030 + 5150a96
commit 14f029d
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 6 deletions.
diff --git a/src/libflake/flake/flakeref.cc b/src/libflake/flake/flakeref.cc
@@ -88,7 +88,7 @@ std::pair<FlakeRef, std::string> parsePathFlakeRefWithFragment(
     if (fragmentStart != std::string::npos) {
         fragment = percentDecode(url.substr(fragmentStart+1));
     }
-    if (pathEnd != std::string::npos && fragmentStart != std::string::npos) {
+    if (pathEnd != std::string::npos && fragmentStart != std::string::npos && url[pathEnd] == '?') {
         query = decodeQuery(url.substr(pathEnd+1, fragmentStart-pathEnd-1));
     }
 

diff --git a/src/libutil/url.cc b/src/libutil/url.cc
@@ -79,10 +79,14 @@ std::map<std::string, std::string> decodeQuery(const std::string & query)
 
     for (auto s : tokenizeString<Strings>(query, "&")) {
         auto e = s.find('=');
-        if (e != std::string::npos)
-            result.emplace(
-                s.substr(0, e),
-                percentDecode(std::string_view(s).substr(e + 1)));
+        if (e == std::string::npos) {
+            warn("dubious URI query '%s' is missing equal sign '%s', ignoring", s, "=");
+            continue;
+        }
+
+        result.emplace(
+            s.substr(0, e),
+            percentDecode(std::string_view(s).substr(e + 1)));
     }
 
     return result;

diff --git a/tests/functional/flakes/dubious-query.sh b/tests/functional/flakes/dubious-query.sh
@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+
+source ./common.sh
+
+requireGit
+
+repoDir="$TEST_ROOT/repo"
+createGitRepo "$repoDir"
+createSimpleGitFlake "$repoDir"
+
+# Check that a flakeref without a query is accepted correctly.
+expectStderr 0 nix --offline build --dry-run "git+file://$repoDir#foo"
+
+# Check that a flakeref with a good query is accepted correctly.
+expectStderr 0 nix --offline build --dry-run "git+file://$repoDir?foo=bar#foo"
+
+# Check that we get the dubious query warning, when passing in a query without an equal sign.
+expectStderr 0 nix --offline build --dry-run "git+file://$repoDir?bar#foo" \
+  | grepQuiet "warning: dubious URI query 'bar' is missing equal sign '=', ignoring"
+
+# Check that the anchor (#) is taken as a whole, not split, and throws an error.
+expectStderr 1 nix --offline build --dry-run "git+file://$repoDir#foo?bar" \
+  | grepQuiet "error: flake 'git+file://$repoDir' does not provide attribute 'packages.$system.foo?bar', 'legacyPackages.$system.foo?bar' or 'foo?bar'"
+
+# Check that a literal `?` in the query  doesn't print dubious query warning.
+expectStderr 0 nix --offline build --dry-run "git+file://$repoDir?#foo" \
+  | grepInverse "warning: dubious URI query "
+
+# Check that a literal `?=` in the query doesn't print dubious query warning.
+expectStderr 0 nix --offline build --dry-run "git+file://$repoDir?=#foo" \
+  | grepInverse "warning: dubious URI query "
diff --git a/tests/functional/flakes/local.mk b/tests/functional/flakes/local.mk
@@ -19,6 +19,7 @@ flake-tests := \
   $(d)/eval-cache.sh \
   $(d)/search-root.sh \
   $(d)/config.sh \
-  $(d)/show.sh
+  $(d)/show.sh \
+  $(d)/dubious-query.sh
 
 install-tests-groups += flake
diff --git a/tests/functional/flakes/meson.build b/tests/functional/flakes/meson.build
@@ -23,6 +23,7 @@ suites += {
     'search-root.sh',
     'config.sh',
     'show.sh',
+    'dubious-query.sh',
   ],
   'workdir': meson.current_build_dir(),
 }