Merge fort-nix/nix-bitcoin#529: ShellCheck on scripts generated by nix

f3f8d650ab shellcheck: fix the lint warnings for other generated scripts (Otto Sabart)
acd341426a shellcheck: prevent globbing and word splitting in package shell scripts (Otto Sabart)
c3b97e6728 tests: add `shellcheckServices` (Erik Arvstedt)
01fa900633 shellcheck: fix setup-secrets.sh, spark-wallet (Erik Arvstedt)
ee15837244 shellcheck: prevent globbing and word splitting in unit shell scripts (Otto Sabart)

Pull request description:

ACKs for top commit:
  erikarvstedt:
    ACK f3f8d650ab
  jonasnick:
    Concept ACK f3f8d650ab

Tree-SHA512: b7a8ae1e8db57e6bb1285832cdd52414913339344e6c6c72621e48404887ae214ed839364d3f2d272cc4b339812ee032e73040b60e6a9f1b9d189ecbae745772

examples/qemu-vm/minimal-vm.nix

@@ -6,8 +6,10 @@ rec {
   mkVMScript = vm: pkgs.writers.writeBash "run-vm" ''
     set -euo pipefail
     export TMPDIR=$(mktemp -d /tmp/nix-bitcoin-vm.XXX)
-    trap "rm -rf $TMPDIR" EXIT
+    trap 'rm -rf $TMPDIR' EXIT
     export NIX_DISK_IMAGE=$TMPDIR/nixos.qcow2
+
+    # shellcheck disable=SC2211
     QEMU_OPTS="-smp $(nproc) -m 1500" ${vm}/bin/run-*-vm
   '';
 

modules/joinmarket.nix

@@ -264,16 +264,16 @@ let
    # The jm scripts create a 'logs' dir in the working dir,
    # so run them inside dataDir.
    cli = pkgs.runCommand "joinmarket-cli" {} ''
-     mkdir -p $out/bin
+     mkdir -p "$out/bin"
      jm=${nbPkgs.joinmarket}/bin
-     cd $jm
+     cd "$jm"
      for bin in jm-*; do
        {
          echo "#!${pkgs.bash}/bin/bash";
-         echo "cd '${cfg.dataDir}' && ${cfg.cliExec} ${runAsUser} ${cfg.user} $jm/$bin --datadir='${cfg.dataDir}' \"\$@\"";
+         echo "cd '${cfg.dataDir}' && ${cfg.cliExec} ${runAsUser} ${cfg.user} "$jm/$bin" --datadir='${cfg.dataDir}' \"\$@\"";
-       } > $out/bin/$bin
+       } > "$out/bin/$bin"
      done
-     chmod -R +x $out/bin
+     chmod -R +x "$out/bin"
    '';
 in {
   inherit options;
@@ -314,7 +314,7 @@ in {
       '';
       postStart = ''
         walletname=wallet.jmdat
-        wallet=${cfg.dataDir}/wallets/$walletname
+        wallet="${cfg.dataDir}/wallets/$walletname"
         if [[ ! -f $wallet ]]; then
           ${optionalString (cfg.rpcWalletFile != null) ''
             echo "Create watch-only wallet ${cfg.rpcWalletFile}"
@@ -330,17 +330,19 @@ in {
               fi
             fi
           ''}
+
           # Restore wallet from seed if available
-          seed=
+          seed=()
           if [[ -e jm-wallet-seed ]]; then
-            seed="--recovery-seed-file jm-wallet-seed"
+            seed=(--recovery-seed-file jm-wallet-seed)
           fi
-          cd ${cfg.dataDir}
+          cd "${cfg.dataDir}"
+
           # Strip trailing newline from password file
-          if ! tr -d "\n" <"${secretsDir}/jm-wallet-password" \
+          if ! tr -d '\n' < '${secretsDir}/jm-wallet-password' \
                | ${nbPkgs.joinmarket}/bin/jm-genwallet \
-                   --datadir=${cfg.dataDir} --wallet-password-stdin $seed $walletname \
+                   --datadir="${cfg.dataDir}" --wallet-password-stdin "''${seed[@]}" "$walletname" \
-               | (if [[ ! $seed ]]; then
+               | (if ((! ''${#seed[@]})); then
                     umask u=r,go=
                     grep -ohP '(?<=recovery_seed:).*' > jm-wallet-seed
                   else

modules/onion-addresses.nix

@@ -74,7 +74,7 @@ in {
         waitForFile /var/lib/tor/state
 
         cd ${cfg.dataDir}
-        rm -rf *
+        rm -rf ./*
 
         ${concatMapStrings
           (user: ''
@@ -82,10 +82,10 @@ in {
             chown ${user} ${user}
             ${concatMapStrings
               (service: ''
-                onionFile=/var/lib/tor/onion/${service}/hostname
+                onionFile='/var/lib/tor/onion/${service}/hostname'
-                waitForFile $onionFile
+                waitForFile "$onionFile"
-                cp $onionFile ${user}/${service}
+                cp "$onionFile" '${user}/${service}'
-                chown ${user} ${user}/${service}
+                chown '${user}' '${user}/${service}'
               '')
               cfg.access.${user}
              }
@@ -95,8 +95,8 @@ in {
 
         ${concatMapStrings (service: ''
           onionFile=/var/lib/tor/onion/${service}/hostname
-          waitForFile $onionFile
+          waitForFile "$onionFile"
-          install -D -o ${config.systemd.services.${service}.serviceConfig.User} -m 400 $onionFile services/${service}
+          install -D -o ${config.systemd.services.${service}.serviceConfig.User} -m 400 "$onionFile" services/${service}
         '') cfg.services}
       '';
     };

modules/secrets/secrets.nix

@@ -210,7 +210,7 @@ in {
         processedFiles=()
         ${
           concatStrings (mapAttrsToList (n: v: ''
-            setupSecret ${n} ${v.user} ${v.group} ${v.permissions} }
+            setupSecret ${n} ${v.user} ${v.group} ${v.permissions}
           '') cfg.secrets)
         }
 
@@ -220,7 +220,9 @@ in {
         )
         if [[ $unprocessedFiles ]]; then
           IFS=$'\n'
+          # shellcheck disable=SC2086
           chown root: $unprocessedFiles
+          # shellcheck disable=SC2086
           chmod 0440 $unprocessedFiles
         fi
 

modules/spark-wallet.nix

@@ -51,14 +51,14 @@ let
   torRateProvider = "--rate-provider wasabi --proxy socks5h://${config.nix-bitcoin.torClientAddressWithPort}";
   startScript = ''
     ${optionalString (cfg.getPublicAddressCmd != "") ''
-      publicURL="--public-url http://$(${cfg.getPublicAddressCmd})"
+      publicURL=(--public-url "http://$(${cfg.getPublicAddressCmd})")
     ''}
     exec ${config.nix-bitcoin.pkgs.spark-wallet}/bin/spark-wallet \
       --ln-path '${clightning.networkDir}'  \
       --host ${cfg.address} --port ${toString cfg.port} \
       --config '${config.nix-bitcoin.secretsDir}/spark-wallet-login' \
       ${optionalString cfg.tor.proxy torRateProvider} \
-      $publicURL \
+      ${optionalString (cfg.getPublicAddressCmd != "") ''"''${publicURL[@]}"''} \
       --pairing-qr --print-key ${cfg.extraArgs}
   '';
 in {

pkgs/clightning-plugins/default.nix

@@ -60,18 +60,18 @@ let
       buildInputs = [ python ];
 
       buildCommand = ''
-        cp --no-preserve=mode -r ${src}/${name} $out
+        cp --no-preserve=mode -r '${src}/${name}' "$out"
-        cd $out
+        cd "$out"
         ${lib.optionalString (plugin ? patchRequirements) ''
           substituteInPlace requirements.txt ${plugin.patchRequirements}
         ''}
 
         # Check that requirements are met
-        PYTHONPATH=${toString python}/${python.sitePackages} \
+        PYTHONPATH='${toString python}/${python.sitePackages}' \
           ${pkgs.python3Packages.pip}/bin/pip install -r requirements.txt --no-cache --no-index
 
-        chmod +x ${script}
+        chmod +x '${script}'
-        patchShebangs ${script}
+        patchShebangs '${script}'
       '';
 
       passthru.path = "${drv}/${script}";

pkgs/clightning-rest/default.nix

@@ -39,8 +39,8 @@ let self = stdenvNoCC.mkDerivation {
       --exclude=/{screenshots,'*.Dockerfile'} \
       $dest
 
-    makeWrapper ${self.nodejsRuntime}/bin/node $out/bin/cl-rest \
+    makeWrapper ${self.nodejsRuntime}/bin/node "$out/bin/cl-rest" \
-      --add-flags $dest/cl-rest.js
+      --add-flags "$dest/cl-rest.js"
 
     runHook postInstall
   '';

pkgs/joinmarket/default.nix

@@ -34,14 +34,14 @@ stdenv.mkDerivation {
   buildInputs = [ pythonEnv ];
 
   installPhase = ''
-    mkdir -p $out/bin
+    mkdir -p "$out/bin"
 
     # add-utxo.py -> bin/jm-add-utxo
     cpBin() {
-      cp scripts/$1 $out/bin/jm-''${1%.py}
+      cp "scripts/$1" "$out/bin/jm-''${1%.py}"
     }
 
-    cp scripts/joinmarketd.py $out/bin/joinmarketd
+    cp scripts/joinmarketd.py "$out/bin/joinmarketd"
     cpBin add-utxo.py
     cpBin convert_old_wallet.py
     cpBin receive-payjoin.py
@@ -52,17 +52,17 @@ stdenv.mkDerivation {
     cpBin yg-privacyenhanced.py
     cpBin genwallet.py
 
-    chmod +x -R $out/bin
+    chmod +x -R "$out/bin"
-    patchShebangs $out/bin
+    patchShebangs "$out/bin"
 
     ## ob-watcher
     obw=$out/libexec/joinmarket-ob-watcher
-    install -D scripts/obwatch/ob-watcher.py $obw/ob-watcher
+    install -D scripts/obwatch/ob-watcher.py "$obw/ob-watcher"
-    patchShebangs $obw/ob-watcher
+    patchShebangs "$obw/ob-watcher"
-    ln -s $obw/ob-watcher $out/bin/jm-ob-watcher
+    ln -s "$obw/ob-watcher" "$out/bin/jm-ob-watcher"
 
     # These files must be placed in the same dir as ob-watcher
-    cp -r scripts/obwatch/{orderbook.html,sybil_attack_calculations.py,vendor} $obw
+    cp -r scripts/obwatch/{orderbook.html,sybil_attack_calculations.py,vendor} "$obw"
   '';
 
   meta = with lib; {

pkgs/netns-exec/default.nix

@@ -5,6 +5,6 @@ stdenv.mkDerivation {
   buildInputs = [ pkgs.libcap ];
   src = ./src;
   installPhase = ''
-    cp main $out
+    cp main "$out"
   '';
 }

pkgs/nixops/default.nix

@@ -34,8 +34,8 @@ let
   };
 
   src = runCommand "src" {} ''
-    cp --no-preserve=mode -r ${origSrc} $out
+    cp --no-preserve=mode -r '${origSrc}' "$out"
-    cd $out
+    cd "$out"
     patch -p1 < ${./release.nix.patch}
   '';
 

pkgs/python-packages/specific-versions/autobahn.nix

@@ -19,7 +19,7 @@ buildPythonPackage rec {
   checkInputs = [ mock pytest ];
   checkPhase = ''
     runHook preCheck
-    USE_TWISTED=true py.test $out
+    USE_TWISTED=true py.test "$out"
     runHook postCheck
   '';
 

pkgs/rtl/default.nix

@@ -58,8 +58,8 @@ let self = stdenvNoCC.mkDerivation {
       ${self.nodeModules}/lib/node_modules \
       $dest
 
-    makeWrapper ${self.nodejsRuntime}/bin/node $out/bin/rtl \
+    makeWrapper ${self.nodejsRuntime}/bin/node "$out/bin/rtl" \
-      --add-flags $dest/rtl.js
+      --add-flags "$dest/rtl.js"
 
     runHook postInstall
   '';

test/lib/make-test.nix

@@ -9,8 +9,9 @@ name: testConfig:
   vm = makeVM {
     name = "nix-bitcoin-${name}";
 
-    nodes.machine = {
+    nodes.machine = { config, ... }: {
       imports = [ testConfig ];
+
       virtualisation = {
         # Needed because duplicity requires 270 MB of free temp space, regardless of backup size
         diskSize = 1024;
@@ -20,6 +21,9 @@ name: testConfig:
 
         cores = lib.mkDefault 2;
       };
+
+      # Run shellcheck on all nix-bitcoin services during machine build time
+      system.extraDependencies = [ config.test.shellcheckServices ];
     };
 
     testScript = nodes: let

test/lib/shellcheck-services.nix

@@ -0,0 +1,128 @@
+{ config, pkgs, lib, extendModules, ... }:
+with lib;
+let
+  options = {
+    test.shellcheckServices = mkOption {
+      readOnly = true;
+      description = ''
+        A derivation that runs shellcheck on all bash scripts included
+        in nix-bitcoin services.
+      '';
+      default = shellcheckServices;
+    };
+  };
+
+  # TODO-EXTERNAL:
+  # This can be removed when https://github.com/NixOS/nixpkgs/pull/189836 is merged.
+  #
+  # A list of all systemd service definitions and their locations, with format
+  # [
+  #   {
+  #     file = ...;
+  #     value = { postgresql = ...; };
+  #   }
+  #   ...
+  # ]
+  systemdServiceDefs =
+    (extendModules {
+      modules = [
+        {
+          # Currently, NixOS modules only allow accessing option definition locations
+          # via type.merge.
+          # Override option `systemd.services` and use it to return the list of service defs.
+          options.systemd.services = lib.mkOption {
+            type = lib.types.anything // {
+              merge = loc: defs: defs;
+            };
+          };
+
+          # Disable all modules that define options.systemd.services so that these
+          # defs don't collide with our definition
+          disabledModules = [
+            "system/boot/systemd.nix"
+            # These files amend option systemd.services
+            "testing/service-runner.nix"
+            "security/systemd-confinement.nix"
+          ];
+
+          config._module.check = false;
+        }
+      ];
+    }).config.systemd.services;
+
+  # A list of all service names that are defined by nix-bitcoin.
+  # [ "bitcoind", "clightning", ... ]
+  #
+  # Algorithm: Parse `systemdServiceDefs` and return all services that
+  # only have definitions located in the nix-bitcoin source.
+  nix-bitcoin-services = let
+    nix-bitcoin-source = toString ../..;
+    nbServices = collectServices true;
+    nonNbServices = collectServices false;
+    # Return set of services ({ service1 = true; service2 = true; ... })
+    # which are either defined or not defined by nix-bitcoin, depending
+    # on `fromNixBitcoin`.
+    collectServices = fromNixBitcoin: lib.listToAttrs (builtins.concatLists (map (def:
+      let
+        isNbSource = lib.hasPrefix nix-bitcoin-source def.file;
+      in
+        # Nix has nor boolean XOR, so use `if`
+        lib.optionals (if fromNixBitcoin then isNbSource else !isNbSource) (
+          (map (service: { name = service; value = true; }) (builtins.attrNames def.value))
+        )
+    ) systemdServiceDefs));
+  in
+    # Set difference: nbServices - nonNbServices
+    builtins.filter (nbService: ! nonNbServices ? ${nbService}) (builtins.attrNames nbServices);
+
+  # The concatenated list of values of ExecStart, ExecStop, ... (`scriptAttrs`) of all `nix-bitcoin-services`.
+  serviceCmds = let
+    scriptAttrs = [
+      "ExecStartPre"
+      "ExecStart"
+      "ExecStartPost"
+      "ExecStop"
+      "ExecStopPost"
+      "ExecCondition"
+      "ExecReload"
+    ];
+    services = config.systemd.services;
+  in
+    builtins.concatMap (serviceName: let
+      serviceCfg = services.${serviceName}.serviceConfig;
+    in
+      builtins.concatMap (attr:
+        lib.optionals (serviceCfg ? ${attr}) (
+          let
+            cmd = serviceCfg.${attr};
+          in
+            if builtins.typeOf cmd == "list" then cmd else [ cmd ]
+        )
+      ) scriptAttrs
+    ) nix-bitcoin-services;
+
+  # A list of all binaries included in `serviceCmds`
+  serviceBinaries = map (cmd: builtins.head (
+    # Extract the first component (the binary).
+    # cmd can start with extra modifiers like `+`
+    builtins.match "[^/]*([^[:space:]]+).*" (toString cmd)
+  )) serviceCmds;
+
+  shellcheckServices = pkgs.runCommand "shellcheck-services" {
+    inherit serviceBinaries;
+    # The `builtins.match` in `serviceBinaries` discards the string context, so we
+    # also have to add `serviceCmds` to the derivation. This ensures that all
+    # referenced nix paths are available to the builder.
+    inherit serviceCmds;
+  } ''
+    echo "Checked binaries:"
+    # Find and check all binaries that have a bash shebang
+    grep -Pl '\A#! *\S+bash\b' $serviceBinaries | while IFS= read -r script; do
+      echo "$script"
+      ${pkgs.shellcheck}/bin/shellcheck --shell bash "$script"
+    done | tee "$out"
+  '';
+in
+{
+  inherit options;
+}

test/lib/test-lib.nix

@@ -1,6 +1,10 @@
 { config, lib, ... }:
 with lib;
 {
+  imports = [
+    ./shellcheck-services.nix
+  ];
+
   options = {
     test = {
       noConnections = mkOption {

test/shellcheck.sh

@@ -2,6 +2,8 @@
 set -euo pipefail
 . "${BASH_SOURCE[0]%/*}/../helper/run-in-nix-env" "shellcheck findutils gnugrep" "$@"
 
+# Shellcheck all bash sources in this repo
+
 cd "${BASH_SOURCE[0]%/*}/.."
 {
     # Skip .git dir in all find commands

test/tests.nix

@@ -281,9 +281,9 @@ let
       systemd.services.bitcoind.postStart = mkAfter ''
         cli=${config.services.bitcoind.cli}/bin/bitcoin-cli
         if ! $cli listwallets | ${pkgs.jq}/bin/jq -e 'index("test")'; then
-          $cli -named createwallet  wallet_name=test load_on_startup=true
+          "$cli" -named createwallet  wallet_name=test load_on_startup=true
           address=$($cli -rpcwallet=test getnewaddress)
-          $cli generatetoaddress ${toString config.test.data.num_blocks} $address
+          "$cli" generatetoaddress ${toString config.test.data.num_blocks} "$address"
         fi
       '';