From ee15837244392cc70d36e085f590f58d3b3abab6 Mon Sep 17 00:00:00 2001 From: Otto Sabart Date: Sun, 31 Jul 2022 23:32:12 +0200 Subject: [PATCH 1/5] shellcheck: prevent globbing and word splitting in unit shell scripts --- modules/joinmarket.nix | 26 ++++++++++++++------------ modules/onion-addresses.nix | 14 +++++++------- test/tests.nix | 4 ++-- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/modules/joinmarket.nix b/modules/joinmarket.nix index f5d2324..0034fd6 100644 --- a/modules/joinmarket.nix +++ b/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}' \"\$@\""; - } > $out/bin/$bin + echo "cd '${cfg.dataDir}' && ${cfg.cliExec} ${runAsUser} ${cfg.user} "$jm/$bin" --datadir='${cfg.dataDir}' \"\$@\""; + } > "$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 \ - | (if [[ ! $seed ]]; then + --datadir="${cfg.dataDir}" --wallet-password-stdin "''${seed[@]}" "$walletname" \ + | (if ((! ''${#seed[@]})); then umask u=r,go= grep -ohP '(?<=recovery_seed:).*' > jm-wallet-seed else diff --git a/modules/onion-addresses.nix b/modules/onion-addresses.nix index 9ddd1c0..f2a3565 100644 --- a/modules/onion-addresses.nix +++ b/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 - waitForFile $onionFile - cp $onionFile ${user}/${service} - chown ${user} ${user}/${service} + onionFile='/var/lib/tor/onion/${service}/hostname' + waitForFile "$onionFile" + cp "$onionFile" '${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 - install -D -o ${config.systemd.services.${service}.serviceConfig.User} -m 400 $onionFile services/${service} + waitForFile "$onionFile" + install -D -o ${config.systemd.services.${service}.serviceConfig.User} -m 400 "$onionFile" services/${service} '') cfg.services} ''; }; diff --git a/test/tests.nix b/test/tests.nix index 77129ba..7df63f0 100644 --- a/test/tests.nix +++ b/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 ''; From 01fa900633415079ccad71fcf9d52373e02ccb47 Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Mon, 5 Sep 2022 17:45:45 +0200 Subject: [PATCH 2/5] shellcheck: fix setup-secrets.sh, spark-wallet --- modules/secrets/secrets.nix | 4 +++- modules/spark-wallet.nix | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/secrets/secrets.nix b/modules/secrets/secrets.nix index 3a5a10a..020b33a 100644 --- a/modules/secrets/secrets.nix +++ b/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 diff --git a/modules/spark-wallet.nix b/modules/spark-wallet.nix index adf6c87..ab0f5cb 100644 --- a/modules/spark-wallet.nix +++ b/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 { From c3b97e672854711c3d7b6206f93bbc430a0b15c7 Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Mon, 5 Sep 2022 18:12:09 +0200 Subject: [PATCH 3/5] tests: add `shellcheckServices` --- test/lib/make-test.nix | 6 +- test/lib/shellcheck-services.nix | 128 +++++++++++++++++++++++++++++++ test/lib/test-lib.nix | 4 + test/shellcheck.sh | 2 + 4 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 test/lib/shellcheck-services.nix diff --git a/test/lib/make-test.nix b/test/lib/make-test.nix index 4c95a68..50cfceb 100644 --- a/test/lib/make-test.nix +++ b/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 diff --git a/test/lib/shellcheck-services.nix b/test/lib/shellcheck-services.nix new file mode 100644 index 0000000..bc6d5ba --- /dev/null +++ b/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; +} diff --git a/test/lib/test-lib.nix b/test/lib/test-lib.nix index 1de2b18..9a67f51 100644 --- a/test/lib/test-lib.nix +++ b/test/lib/test-lib.nix @@ -1,6 +1,10 @@ { config, lib, ... }: with lib; { + imports = [ + ./shellcheck-services.nix + ]; + options = { test = { noConnections = mkOption { diff --git a/test/shellcheck.sh b/test/shellcheck.sh index f6b697e..887cc2e 100755 --- a/test/shellcheck.sh +++ b/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 From acd341426a9b296ed7ceb2be9923a38c27b29ae8 Mon Sep 17 00:00:00 2001 From: Otto Sabart Date: Thu, 25 Aug 2022 21:00:00 +0200 Subject: [PATCH 4/5] shellcheck: prevent globbing and word splitting in package shell scripts --- pkgs/clightning-plugins/default.nix | 10 +++++----- pkgs/clightning-rest/default.nix | 4 ++-- pkgs/joinmarket/default.nix | 18 +++++++++--------- pkgs/netns-exec/default.nix | 2 +- pkgs/nixops/default.nix | 4 ++-- .../specific-versions/autobahn.nix | 2 +- pkgs/rtl/default.nix | 4 ++-- 7 files changed, 22 insertions(+), 22 deletions(-) diff --git a/pkgs/clightning-plugins/default.nix b/pkgs/clightning-plugins/default.nix index 46d8022..73447f5 100644 --- a/pkgs/clightning-plugins/default.nix +++ b/pkgs/clightning-plugins/default.nix @@ -60,18 +60,18 @@ let buildInputs = [ python ]; buildCommand = '' - cp --no-preserve=mode -r ${src}/${name} $out - cd $out + cp --no-preserve=mode -r '${src}/${name}' "$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} - patchShebangs ${script} + chmod +x '${script}' + patchShebangs '${script}' ''; passthru.path = "${drv}/${script}"; diff --git a/pkgs/clightning-rest/default.nix b/pkgs/clightning-rest/default.nix index 256cabe..954d1ee 100644 --- a/pkgs/clightning-rest/default.nix +++ b/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 \ - --add-flags $dest/cl-rest.js + makeWrapper ${self.nodejsRuntime}/bin/node "$out/bin/cl-rest" \ + --add-flags "$dest/cl-rest.js" runHook postInstall ''; diff --git a/pkgs/joinmarket/default.nix b/pkgs/joinmarket/default.nix index 457916a..1f40128 100644 --- a/pkgs/joinmarket/default.nix +++ b/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 - patchShebangs $out/bin + chmod +x -R "$out/bin" + patchShebangs "$out/bin" ## ob-watcher obw=$out/libexec/joinmarket-ob-watcher - install -D scripts/obwatch/ob-watcher.py $obw/ob-watcher - patchShebangs $obw/ob-watcher - ln -s $obw/ob-watcher $out/bin/jm-ob-watcher + install -D scripts/obwatch/ob-watcher.py "$obw/ob-watcher" + patchShebangs "$obw/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; { diff --git a/pkgs/netns-exec/default.nix b/pkgs/netns-exec/default.nix index 5998549..6365c97 100644 --- a/pkgs/netns-exec/default.nix +++ b/pkgs/netns-exec/default.nix @@ -5,6 +5,6 @@ stdenv.mkDerivation { buildInputs = [ pkgs.libcap ]; src = ./src; installPhase = '' - cp main $out + cp main "$out" ''; } diff --git a/pkgs/nixops/default.nix b/pkgs/nixops/default.nix index 6af17d6..e7857c7 100644 --- a/pkgs/nixops/default.nix +++ b/pkgs/nixops/default.nix @@ -34,8 +34,8 @@ let }; src = runCommand "src" {} '' - cp --no-preserve=mode -r ${origSrc} $out - cd $out + cp --no-preserve=mode -r '${origSrc}' "$out" + cd "$out" patch -p1 < ${./release.nix.patch} ''; diff --git a/pkgs/python-packages/specific-versions/autobahn.nix b/pkgs/python-packages/specific-versions/autobahn.nix index 2557100..0b2e106 100644 --- a/pkgs/python-packages/specific-versions/autobahn.nix +++ b/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 ''; diff --git a/pkgs/rtl/default.nix b/pkgs/rtl/default.nix index 388e337..2b895fa 100644 --- a/pkgs/rtl/default.nix +++ b/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 \ - --add-flags $dest/rtl.js + makeWrapper ${self.nodejsRuntime}/bin/node "$out/bin/rtl" \ + --add-flags "$dest/rtl.js" runHook postInstall ''; From f3f8d650ab1e588fa6ad3041757493e84be4809c Mon Sep 17 00:00:00 2001 From: Otto Sabart Date: Mon, 8 Aug 2022 14:57:19 +0200 Subject: [PATCH 5/5] shellcheck: fix the lint warnings for other generated scripts --- examples/qemu-vm/minimal-vm.nix | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/examples/qemu-vm/minimal-vm.nix b/examples/qemu-vm/minimal-vm.nix index 6bb9275..c04f27e 100644 --- a/examples/qemu-vm/minimal-vm.nix +++ b/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 '';