From 95f48ad71eba4891ccfd6affe72cbf1a6dd3b970 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftweedal@redhat.com>
Date: Mon, 5 Jun 2023 22:19:06 +1000
Subject: [PATCH] Fix parsing of password-command option (#6268)

The password-command option does not parse its value correctly.
Quotes are ignored, making many kinds of commands impossible to
express (e.g.  `sh -c "foo | bar"`).  Also, `cabal user-config`
treats the argument list as a *list of option values*, rather than a
*value that is a list*.  As a consequence, `cabal user-config
update` corrupts the value in the config file.

Fix these issues by parsing the command as a space separated list of
tokens, and changing the getter to `unwords` the value and return a
*singleton* list.  Also update the argument placeholder from
`PASSWORD` to `COMMAND`.

Fixes: https://github.com/haskell/cabal/issues/6268
---
 .../src/Distribution/Client/Setup.hs          | 11 ++++++++++-
 .../src/Distribution/Deprecated/ParseUtils.hs |  1 +
 .../PackageTests/UserConfig/cabal.out         |  3 +++
 .../PackageTests/UserConfig/cabal.test.hs     |  6 ++++++
 changelog.d/issue-6268                        | 19 +++++++++++++++++++
 doc/cabal-commands.rst                        | 14 +++++++++++++-
 6 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 changelog.d/issue-6268

diff --git a/cabal-install/src/Distribution/Client/Setup.hs b/cabal-install/src/Distribution/Client/Setup.hs
index f550c6e36e8..460decd55cd 100644
--- a/cabal-install/src/Distribution/Client/Setup.hs
+++ b/cabal-install/src/Distribution/Client/Setup.hs
@@ -116,6 +116,8 @@ import Distribution.Client.Targets
   ( UserConstraint
   , readUserConstraint
   )
+import Distribution.Deprecated.ParseUtils (parseSpaceList, parseTokenQ)
+import Distribution.Deprecated.ReadP (readP_to_E)
 import Distribution.Utils.NubList
   ( NubList
   , fromNubList
@@ -2706,7 +2708,14 @@ uploadCommand =
             "Command to get Hackage password."
             uploadPasswordCmd
             (\v flags -> flags{uploadPasswordCmd = v})
-            (reqArg' "PASSWORD" (Flag . words) (fromMaybe [] . flagToMaybe))
+            ( reqArg
+                "COMMAND"
+                ( readP_to_E
+                    ("Cannot parse command: " ++)
+                    (Flag <$> parseSpaceList parseTokenQ)
+                )
+                (flagElim [] (pure . unwords . fmap show))
+            )
         ]
     }
 
diff --git a/cabal-install/src/Distribution/Deprecated/ParseUtils.hs b/cabal-install/src/Distribution/Deprecated/ParseUtils.hs
index 592306727c2..b3b5f8bab9d 100644
--- a/cabal-install/src/Distribution/Deprecated/ParseUtils.hs
+++ b/cabal-install/src/Distribution/Deprecated/ParseUtils.hs
@@ -38,6 +38,7 @@ module Distribution.Deprecated.ParseUtils
   , readFields
   , parseHaskellString
   , parseTokenQ
+  , parseSpaceList
   , parseOptCommaList
   , showFilePath
   , showToken
diff --git a/cabal-testsuite/PackageTests/UserConfig/cabal.out b/cabal-testsuite/PackageTests/UserConfig/cabal.out
index b5e1f5ef9f8..2c6e1a3cd14 100644
--- a/cabal-testsuite/PackageTests/UserConfig/cabal.out
+++ b/cabal-testsuite/PackageTests/UserConfig/cabal.out
@@ -12,3 +12,6 @@ Writing merged config to <ROOT>/cabal.dist/cabal-config.
 # cabal user-config
 Renaming <ROOT>/cabal.dist/cabal-config to <ROOT>/cabal.dist/cabal-config.backup.
 Writing merged config to <ROOT>/cabal.dist/cabal-config.
+# cabal user-config
+Renaming <ROOT>/cabal.dist/cabal-config to <ROOT>/cabal.dist/cabal-config.backup.
+Writing merged config to <ROOT>/cabal.dist/cabal-config.
diff --git a/cabal-testsuite/PackageTests/UserConfig/cabal.test.hs b/cabal-testsuite/PackageTests/UserConfig/cabal.test.hs
index 85d67212d4c..300bcc59ea5 100644
--- a/cabal-testsuite/PackageTests/UserConfig/cabal.test.hs
+++ b/cabal-testsuite/PackageTests/UserConfig/cabal.test.hs
@@ -15,3 +15,9 @@ main = cabalTest $ do
     assertFileDoesContain conf "foo,bar"
     cabalG ["--config-file", conf] "user-config" ["update", "-f", "-a", "extra-prog-path: foo, bar"]
     assertFileDoesContain conf "foo,bar"
+
+    -- regression test for #6268 (password-command parsing)
+    cabalG ["--config-file", conf]
+        "user-config" ["update", "-f", "-a", "password-command: sh -c \"echo secret\""]
+    -- non-quoted tokens do get quoted when writing, but this is expected
+    assertFileDoesContain conf "password-command: \"sh\" \"-c\" \"echo secret\""
diff --git a/changelog.d/issue-6268 b/changelog.d/issue-6268
new file mode 100644
index 00000000000..cc78eecf884
--- /dev/null
+++ b/changelog.d/issue-6268
@@ -0,0 +1,19 @@
+synopsis: Fix parsing of password-command option
+packages: cabal-install
+prs: #9002
+issuesa: #6268
+
+description: {
+
+The password-command option did not parse its value correctly.
+Quotes were ignored, making many kinds of commands impossible to
+express (e.g.  `sh -c "foo | bar"`).  Also, `cabal user-config`
+treated the argument list as a *list of option values*, rather than a
+*value that is a list*.  As a consequence, `cabal user-config
+update` corrupted the value in the config file.
+
+Fixed these issues by parsing the command as a space separated list
+of tokens (which may be enclosed in double quotes), and treating the
+parsed list-of-token as one value (not multiple).
+
+}
diff --git a/doc/cabal-commands.rst b/doc/cabal-commands.rst
index 4ad2fba355d..5b5dfc4269f 100644
--- a/doc/cabal-commands.rst
+++ b/doc/cabal-commands.rst
@@ -1086,7 +1086,19 @@ to Hackage.
 
 .. option:: -P, --password-command
 
-    Command to get your Hackage password.
+    Command to get your Hackage password.  Arguments with whitespace
+    must be quoted (double-quotes only).  For example:
+
+    ::
+
+        --password-command 'sh -c "grep hackage ~/secrets | cut -d : -f 2"'
+
+    Or in the config file:
+
+    ::
+
+        password-command: sh -c "grep hackage ~/secrets | cut -d : -f 2"
+
 
 cabal report
 ^^^^^^^^^^^^