Thanks for patches! Can you amend the commit messages in order to be formatted as per guidelines at: * https://www.kamailio.org/wiki/devel/github-contributions#commit_message_form... --- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/593#issuecomment-216126893

nu-gui left a comment (kamailio/kamailio#593) Addressed all 4 review findings on `ec13fd4759`. **Major — SQL injection via `GEN_IP` (ext-staging-prep.sh:160)**: Valid, confirmed against current code. `GEN_IP` is an operator CLI arg only checked for non-empty before being interpolated straight into the `INSERT ... values(...,'$GEN_IP',...)` SQL text. Fixed by parameterizing with `psql -v gen_ip="$GEN_IP" ... :'gen_ip'` (psql's safe-literal quoting, escapes via `PQescapeLiteral`) and, as defense in depth, added a `valid_ipv4()` check that rejects anything that isn't 4 dot-separated 0-255 octets — applied before both the `setup` DB/UFW use and the `teardown` UFW-delete use (that second call site also took `$GEN_IP` unvalidated). **Major — predictable `/tmp/uas-ext-err.log` symlink race (ext-staging-prep.sh:174)**: Valid, confirmed. `systemd-run` starts `sipp` as root with a fixed, world-guessable `-error_file` path, so a local attacker could pre-plant a symlink before `setup` runs. Fixed by creating an unpredictable `0700` directory via `mktemp -d` immediately before launching `sipp` and pointing `-error_file` inside it (`mktemp`'s `mkdtemp()` is atomic — nothing to race or pre-create). Also surfaced the resulting log path in the `READY` output line since it's no longer a fixed, discoverable location. **Minor — source-safety test doesn't observe side effects (test-ext-staging-prep.sh:147)**: Valid and cheap, fixed as proposed. `source "$SCRIPT"` output is now captured to a file and required to be empty in addition to `reload_proxy`/`main` being defined, so a regression that let `main()` execute at source time (rather than only under direct invocation) now fails the check instead of passing silently. **Minor — inherited `RELAUNCH` pollutes the propose-only test (test-ext-staging-prep.sh:206)**: Valid, real test-isolation bug. The default-path child (`bash -c "source '$SCRIPT'; reload_proxy 'test'"`) never set `RELAUNCH`, so an exported `RELAUNCH=1` in the caller's shell would silently flip that case onto the redeploy path. Fixed by explicitly clearing it for that child (`RELAUNCH="" ... bash -c "unset RELAUNCH; source ...`). **Guard-discrimination proof** (mutate → red → restore → green), both done locally: - Injected a stray `echo` before the source-guard in `ext-staging-prep.sh` → suite went `7 passed, 1 failed` on the source-safety check (`... produced output: leaked output at source time`); reverted → back to `8 passed, 0 failed`. - Reverted just the `RELAUNCH` isolation fix in the test file, then ran the suite with `RELAUNCH=1` exported in the caller's environment → `7 passed, 1 failed` (`default path invoked redeploy or did not print instructions (rc=0, called=yes)`), proving the pre-fix test was genuinely poisonable; restored the fix → `RELAUNCH=1 bash tests/test-ext-staging-prep.sh` now stays `8 passed, 0 failed`. **Real output — final state:** ``` $ bash -n tests/loadtest/ext-generator/ext-staging-prep.sh && echo OK OK $ bash -n tests/test-ext-staging-prep.sh && echo OK OK $ bash tests/test-ext-staging-prep.sh ok - (a) 'docker compose restart proxy' does not appear in the script ok - sourcing the script defines reload_proxy()/main() without running the CLI ok - (b) image-tag change across relaunch is detected and fails (rc=1) ok - (c) unbound SIP socket after relaunch is detected and fails (rc=1) ok - (d) happy path (image unchanged, socket bound) passes (rc=0) ok - default (RELAUNCH unset) is propose-only: prints the command, never invokes redeploy ok - staging-identity guard still refuses to run off-staging ok - (e) bash -n $SCRIPT is clean == 8 passed, 0 failed == ``` `EXTERNAL-GENERATOR.md` was checked for references to the old `/tmp/uas-ext-err.log` path or the raw SQL form — none found, no doc change needed. Worktree clean after commit + push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P3bDHQ6ouxbM3dNfWUNDBv -- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/593#issuecomment-5192893800 You are receiving this because you are subscribed to this thread. Message ID: <kamailio/kamailio/pull/593/c5192893800@github.com>