General
Always include a shebang, and set options to exit the script when a command fails or when we use a variable that's not set:
#!/bin/bash
set -eu
Test for existence with -e
, not -d
If wifi_pass_dir
exists, but is not a directory (perhaps it's a plain file), it will not be deleted, and the mkdir
will fail. It might be better not to check (rm -f
will succeed if the file doesn't exist):
# Ensure the wifi_pass_dir exists
rm -rf wifi_pass_dir
mkdir wifi_pass_dir
However, I don't believe it's necessary to copy the config files to an insecure location and make them world readable (and writeable) - see later.
sudo
is for interactive use, not scripts
#sudo
is for interactive use, not scripts
AvoidAvoid using sudo
in a script - it expects to be interactive, and won't necessarily do what you want if not connected to a terminal. It's easier to require that the script be run under sudo
(and we can then use su
to change to a non-privileged user where necessary).
Don't parse the output of ls
#Don't parse the output of ls
FilenamesFilenames you don't control can contain all sorts of characters, including newlines. Here, we don't need to do that, as we're reading from a directory. The while read
loop can simply be a for f in *
loop.
Don't over-exercise the cat
#Don't over-exercise the cat
ThisThis cat
isn't needed:
cat "$name" | grep psk=
Just tell grep
to take its input from $name
directly:
grep psk= "$name"
We don't even need a loop, as we can pass all the filenames to grep
in one go.
#Simplified code
Simplified code
#!/bin/bash
exec grep 'psk=' /etc/NetworkManager/system-connections/*
Yes, that's the whole program. Since we only execute one command, we don't need set -e
, and as we use no variables, neither do we need set -u
.
General
Always include a shebang, and set options to exit the script when a command fails or when we use a variable that's not set:
#!/bin/bash
set -eu
Test for existence with -e
, not -d
If wifi_pass_dir
exists, but is not a directory (perhaps it's a plain file), it will not be deleted, and the mkdir
will fail. It might be better not to check (rm -f
will succeed if the file doesn't exist):
# Ensure the wifi_pass_dir exists
rm -rf wifi_pass_dir
mkdir wifi_pass_dir
However, I don't believe it's necessary to copy the config files to an insecure location and make them world readable (and writeable) - see later.
#sudo
is for interactive use, not scripts
Avoid using sudo
in a script - it expects to be interactive, and won't necessarily do what you want if not connected to a terminal. It's easier to require that the script be run under sudo
(and we can then use su
to change to a non-privileged user where necessary).
#Don't parse the output of ls
Filenames you don't control can contain all sorts of characters, including newlines. Here, we don't need to do that, as we're reading from a directory. The while read
loop can simply be a for f in *
loop.
#Don't over-exercise the cat
This cat
isn't needed:
cat "$name" | grep psk=
Just tell grep
to take its input from $name
directly:
grep psk= "$name"
We don't even need a loop, as we can pass all the filenames to grep
in one go.
#Simplified code
#!/bin/bash
exec grep 'psk=' /etc/NetworkManager/system-connections/*
Yes, that's the whole program. Since we only execute one command, we don't need set -e
, and as we use no variables, neither do we need set -u
.
General
Always include a shebang, and set options to exit the script when a command fails or when we use a variable that's not set:
#!/bin/bash
set -eu
Test for existence with -e
, not -d
If wifi_pass_dir
exists, but is not a directory (perhaps it's a plain file), it will not be deleted, and the mkdir
will fail. It might be better not to check (rm -f
will succeed if the file doesn't exist):
# Ensure the wifi_pass_dir exists
rm -rf wifi_pass_dir
mkdir wifi_pass_dir
However, I don't believe it's necessary to copy the config files to an insecure location and make them world readable (and writeable) - see later.
sudo
is for interactive use, not scripts
Avoid using sudo
in a script - it expects to be interactive, and won't necessarily do what you want if not connected to a terminal. It's easier to require that the script be run under sudo
(and we can then use su
to change to a non-privileged user where necessary).
Don't parse the output of ls
Filenames you don't control can contain all sorts of characters, including newlines. Here, we don't need to do that, as we're reading from a directory. The while read
loop can simply be a for f in *
loop.
Don't over-exercise the cat
This cat
isn't needed:
cat "$name" | grep psk=
Just tell grep
to take its input from $name
directly:
grep psk= "$name"
We don't even need a loop, as we can pass all the filenames to grep
in one go.
Simplified code
#!/bin/bash
exec grep 'psk=' /etc/NetworkManager/system-connections/*
Yes, that's the whole program. Since we only execute one command, we don't need set -e
, and as we use no variables, neither do we need set -u
.
General
Always include a shebang, and set options to exit the script when a command fails or when we use a variable that's not set:
#!/bin/bash
set -eu
Test for existence with -e
, not -d
If wifi_pass_dir
exists, but is not a directory (perhaps it's a plain file), it will not be deleted, and the mkdir
will fail. It might be better not to check (rm -f
will succeed if the file doesn't exist):
# Ensure the wifi_pass_dir exists
rm -rf wifi_pass_dir
mkdir wifi_pass_dir
However, I don't believe it's necessary to copy the config files to an insecure location and make them world readable (and writeable) - see later.
#sudo
is for interactive use, not scripts
Avoid using sudo
in a script - it expects to be interactive, and won't necessarily do what you want if not connected to a terminal. It's easier to require that the script be run under sudo
(and we can then use su
to change to a non-privileged user where necessary).
#Don't parse the output of ls
Filenames you don't control can contain all sorts of characters, including newlines. Here, we don't need to do that, as we're reading from a directory. The while read
loop can simply be a for f in *
loop.
#Don't over-exercise the cat
This cat
isn't needed:
cat "$name" | grep psk=
Just tell grep
to take its input from $name
directly:
grep psk= "$name"
We don't even need a loop, as we can pass all the filenames to grep
in one go.
#Simplified code
#!/bin/bash
exec grep 'psk=' /etc/NetworkManager/system-connections/*
Yes, that's the whole program. Since we only execute one command, we don't need set -e
, and as we use no variables, neither do we need set -u
.