opnsense / update

OPNsense update tools
https://opnsense.org/
BSD 2-Clause "Simplified" License
128 stars 79 forks source link

opnsense-update: Support unescaped mirror URLs #96

Closed grembo closed 1 month ago

grembo commented 1 month ago

When running opnsense-update with a custom mirror URL, unescaped slashes (which are part of every URL) will cause the script to fail:

# opnsense-update -ikr whatever -m "http://example.org/"
sed: 1: "/^[[:space:]]*url:[[:sp ...": bad flag in substitute command: '/'

This patch fixes this by using pipe ('|') instead of slash ('/') as delimiters in the affected sed command.

Escaped slashes (as in -m "http:\/\/example.org\/") will still work after applying the patch, so this should not break existing automation.

fichtner commented 1 month ago

Neat, thanks, merged! I think all the sed calls could benefit here, some take a mirror subdir or handle file URLs as well. Let me fix that for the next iteration and then simplify the automated writes as well. :)

https://github.com/opnsense/core/blob/af62c482e2ce75f6fed04633de3171bf8fe23d6f/src/opnsense/scripts/firmware/repos/OPNsense.php#L53

https://github.com/opnsense/core/blob/af62c482e2ce75f6fed04633de3171bf8fe23d6f/src/opnsense/scripts/firmware/repos/OPNsense.php#L57

grembo commented 1 month ago

@fichtner In my larger scripts I use this little trick to reduce the change of escaping issues when using dynamic input in sed statements (this won't protect against abuse, but against problems like this PR addressed). In here pipe was ok, as it shouldn't be part of URLs anyway, but using a control character as separator protects against accidental escape issues in a more generic way :

# shellcheck disable=SC3003
# safe(r) separator for sed
sep=$'\001'

BAR="somestring/|#djhejdh"
sed -i '' "s${sep}FOO${sep}$BAR${sep}g" myfile

or in this specific case:

sep=$'\001'
sed -i '' "/${URL_KEY}/s${sep}\".*\${ABI}${sep}\"${DO_MIRRORURL#"-m "}/\${ABI}${sep}" ${ORIGIN}
fichtner commented 1 month ago

@grembo very nifty, thanks for that. I agree it isn't needed here but it will be handy on more generic input replacements :)

fichtner commented 1 month ago

FYI, fixed the others as well as discussed in 56137de4bff