pfSense

(I recently experienced this on 22.05 snaps, btw)

The inconsistent handling of newlines in text boxes in browsers is one of the reasons the OpenVPN advanced options insists on using semicolons as a line separator. Historically this has been inconsistent at best, varying by OS newline style (e.g. UNIX vs DOS/Windows line endings) and browser vendor.

This may be browser-dependent and not something we can easily solve, but either way we need a reliable way to reproduce the problem before anything can be done.

Alternately, consider separating the options by spaces and not newlines. Both should work in that box.

2 other possible workarounds:

- have each custom option in its own row, with an "add row" button UI similar to defining aliases
- before starting dnsmasq, validate the config and if it fails to validate, start it without the custom options and dispatch an alert. At least this way when the bug happens, you don't completely take down the network due to lack of DNS, and will have an easier time remediating

If you like either of these ideas I can try to work up a PR

→ luckman212 wrote in #note-4:

2 other possible workarounds:
- have each custom option in its own row, with an "add row" button UI similar to defining aliases

That would add too much complexity for this, IMO.

- before starting dnsmasq, validate the config and if it fails to validate, start it without the custom options and dispatch an alert. At least this way when the bug happens, you don't completely take down the network due to lack of DNS, and will have an easier time remediating

It already does this: https://github.com/pfsense/pfsense/blob/master/src/usr/local/pfSense/include/www/services_dnsmasq.inc#L208

Jim Pingle As far as I can tell from looking at the code (and my experience as well) it only validates on SAVE, but not when trying to start or restart the service. So, if the data gets corrupted in the config.xml via whatever other mechanism that's causing this bug (e.g. after a reboot or config restore), then dnsmasq will silently fail and you just lose DNS completely.

A reboot or restore couldn't "corrupt" this. A reboot doesn't alter the configuration. It could only change on save.

Only way a restore could do it is if the user edited the config.xml and maybe messed up the line endings that way.

The way this option stores the text it does so plainly in the config.xml, so it has newlines in place where they are unreliable.

A better fix for all this may be to wrap it in base64 encoding before save and then decode before use, as it done with lots of other similar fields. It would prevent the newlines bare in the config from being a problem.

Oh great idea! Only downside is losing the ability to see the data when directly viewing the XML, but that's a very minor tradeoff. To preserve backward compatibility there would need to be some upgrade functions and the data would need to be tested to see if it was valid base64 and fall back to plaintext interpretation if not. I haven't looked yet but it sounds like this method is already in use elsewhere on other pages. Do you want me to try at a PR?

→ luckman212 wrote in #note-8:

Oh great idea! Only downside is losing the ability to see the data when directly viewing the XML, but that's a very minor tradeoff.

Lots of editors have built-in Base64 encode/decode functions or plugins which makes that less of an issue. A couple extra clicks if it's needed.

To preserve backward compatibility there would need to be some upgrade functions and the data would need to be tested to see if it was valid base64 and fall back to plaintext interpretation if not. I haven't looked yet but it sounds like this method is already in use elsewhere on other pages. Do you want me to try at a PR?

It would need an upgrade code function to change it over to base64 during upgrade, no need to do any kind of conditional test after since it's safe to assume it's base64 in the config at that point. You can try a PR if you'd like. See 86584ded30c27b9ad1b017fb743399dc01180f02 for a similar change in the past.

Jim Pingle I submitted a PR: https://github.com/pfsense/pfsense/pull/4579