simplerisk-minimal: harden config.php substitution against / in values#137
Merged
Conversation
The set_config and set_db_password substitutions used / as the sed delimiter, so any env-var value containing / (cert paths, passwords with special chars) silently broke the substitution and left the bundled default in config.php. The trailing || true on the optional fields hid the failure entirely. - Add sed_escape helper and switch all set_config / set_db_password substitutions to | as the delimiter, escaping \, &, and | in the replacement. - SIMPLERISK_DB_SSL_CERT_PATH: match both the new commented-out sample form and the legacy uncommented form, and rewrite either as an active define when a path is supplied. Leave the commented form in place when no env var is set, so operators don't end up requesting SSL against a non-existent cert path. - Default SIMPLERISK_DB_FOR_SESSIONS to 'true' to match the README and avoid leaving the literal __USE_DATABASE_FOR_SESSIONS__ placeholder in config.php when the env var is omitted with the new sample. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
set_config/set_db_passwordsed substitutions to a|delimiter with proper escaping. The previous/delimiter silently failed whenever an env-var value contained/(cert paths, passwords with special chars), and the trailing|| trueon the optional fields hid the failure — the bundled default ended up inconfig.phpinstead.SIMPLERISK_DB_SSL_CERT_PATHhandling to match both the new commented-out sample form (// define('DB_SSL_CERTIFICATE_PATH', '/path/to/ca-cert.pem');) and the legacy uncommented form. When the env var is set, the line is rewritten as an activedefine; when unset, the commented sample form is left in place so SimpleRisk doesn't request SSL against a non-existent cert path.SIMPLERISK_DB_FOR_SESSIONSto'true'to match the README's documented default and to avoid leaving the literal__USE_DATABASE_FOR_SESSIONS__placeholder inconfig.phpwhen an operator omits the env var alongside the new sample format.Test plan
severity=error)simplerisk-minimalvariants/and a password containing/,&,|,\— all values substituted correctly'true'config.phpfallback (no sample file) + env vars set — substitutions update the existingdefinelines correctlyconfig.phpfallback + no env vars — defaults applied, existing SSL value preservedNotes
USE_DATABASE_FOR_SESSIONS = 'false'and don't setSIMPLERISK_DB_FOR_SESSIONSwill be flipped to'true'on next boot. The README already advertisedtrueas the default; this just brings the entrypoint into line. To preserve'false', setSIMPLERISK_DB_FOR_SESSIONS=false.simplerisk(full-stack) image isn't touched: it generates its DB password fromtr -dc A-Za-z0-9and uses literal hardcoded values for the other fields, so it doesn't hit the/-in-value bug.🤖 Generated with Claude Code