push_data.sh: no hardcoded deploy user — WOCO_DEPLOY_USER from .env; refuse --import with --dry-run

[reese8272]

Per Michael's request: remove any specific user from push_data.sh and rely on the local .env for the username. wocod remains the owner/runtime user on both boxes — nothing changes server-side.

Changes

• Host resolution: WOCO_HOST env > WOCO_DEPLOY_USER env > WOCO_DEPLOY_USER in the gitignored repo-root .env; fails with a clear message when none is set. No username default remains in the script.
• .env is sed-extracted, not sourced — it's a Django-style env file whose unquoted values (DEFAULT_FROM_EMAIL=WorldCovers <no-reply@...>) are not valid bash and would kill the script under set -e.
• WOCO_DEPLOY_USER= documented in .env.example.
• Safety: --dry-run now refuses --import — previously the rsyncs were previewed but the import still really truncate-reloaded the live DB.
• Header requires the two-command scoped sudoers drop-in instead of endorsing blanket NOPASSWD.

Tests

tools/tests/test_push_data.py (new, 5 tests): missing-user failure, --help without config, .env resolution builds user@hellowoco.app (with a Django-style .env fixture), WOCO_HOST override precedence, and dry-run/import refusal with no rsync executed (PATH-stubbed rsync).

Note for reviewers: I read "wocod user should be who deploys on both boxes" as the ownership/runtime invariant (--chown=wocod:wocod, sudo -u wocod), which is unchanged — not as "SSH in as wocod". Please confirm.

🤖 Generated with Claude Code

[github-actions]

Code review overview

• Syntax Errors - Score - 9/10
• Code Smells - Score - 8/10
• Bugs - Score - 7/10
• Security Vulnerabilities - Score - 8/10

---

Syntax Errors

Finding 1

• File: tools/push_data.sh
• Location: sed -n '2,24p' "$0" (help extraction)
• Why it matters: The help block now spans lines 2–31 (the comment block was expanded considerably in this diff). Hardcoding line 24 means the help output is silently truncated; users won't see the --dry-run note about refusing --import, which is exactly the behaviour that was just added.
• Suggested fix: Use a sentinel comment pattern instead of line numbers, e.g. delimit the block with # --- and extract with sed -n '/^# ---/q; 2,$ { /^#/p }', or at minimum recount and update to the correct end line.

---

Code Smells

Finding 1

• File: tools/push_data.sh
• Location: Flag parsing loop
• Why it matters: Flags are parsed with a simple for arg in "$@" loop, which means flags can appear in any order and all are collected before any validation. This is fine now but will become fragile if positional arguments (e.g. a target environment name) are added later.
• Suggested fix: while [[ $# -gt 0 ]]; do ... shift; done is a more conventional and extensible pattern.

Finding 2

• File: tools/tests/test_push_data.py
• Location: RSYNC_STUB / self.rsync_log
• Why it matters: The stub appends to $RSYNC_LOG across multiple rsync invocations. Because the log file is shared between test methods via self.rsync_log (same path per temp dir but temp dir is per setUp), this is fine in isolation — but if run_script is ever called twice in one test the log accumulates, making assertions on assertNotIn potentially fragile.
• Suggested fix: Either reset/read-then-truncate the log between calls, or have the stub write to a per-invocation file.

Finding 3

• File: tools/tests/test_push_data.py
• Location: run_script method
• Why it matters: cwd is not set in subprocess.run, so the script runs with the test process's working directory rather than self.tmp. The script does cd "$(dirname "$0")/.." which resolves to self.tmp correctly — but only because push_data.sh is placed at self.tmp/tools/push_data.sh. This is a subtle implicit coupling that could silently break if the script's cd logic changes.
• Suggested fix: Pass cwd=self.tmp explicitly to make the intent clear and decouple it from the script's own cd.

---

Bugs

Finding 1

• File: tools/push_data.sh
• Location: .env parsing — sed -n 's/^WOCO_DEPLOY_USER=\([^#]*\).*/\1/p'
• Why it matters: The character class [^#]* is greedy but stops at #. A value containing an = sign (unusual but possible, e.g. a base64 token accidentally placed here) would be captured correctly, but a value with a # that is not a comment — e.g. WOCO_DEPLOY_USER=alice#work — would silently be truncated to alice. More practically, the tail -1 means a duplicate key later in the file wins silently rather than erroring, which can be surprising.
• Suggested fix: Document that # in the value is unsupported, and consider grep -m1 instead of tail -1 so the first occurrence wins (matching typical env-file semantics).

Finding 2

• File: tools/tests/test_push_data.py
• Location: test_env_file_user_builds_host — self.rsync_log.read_text()
• Why it matters: If the script fails before rsync runs (e.g. due to a set -e early exit from a missing directory), rsync_log will not exist and read_text() raises FileNotFoundError, producing a confusing error rather than a clean test failure.
• Suggested fix: Assert self.rsync_log.exists() first, or use self.rsync_log.read_text() if self.rsync_log.exists() else "" with a preceding assertTrue.

Finding 3

• File: tools/push_data.sh
• Location: Top of script — set -euo pipefail with ${WOCO_DEPLOY_USER:-}
• Why it matters: Under set -u, any reference to an unset variable without :- causes immediate exit. The code correctly uses ${WOCO_DEPLOY_USER:-} in the conditionals, but if WOCO_HOST is also unset and the user passes it via a typo'd env var name, the error message will not be reached — set -u will fire first on a bare $WOCO_HOST if it ever appears unguarded in future edits. This is a latent risk introduced by the refactoring.
• Suggested fix: Consistently use ${VAR:-} for all optional variables, which this diff does — but add a comment noting this requirement so future contributors don't regress it.

---

Security Vulnerabilities

Finding 1

• File: tools/push_data.sh
• Location: WOCO_HOST="${WOCO_DEPLOY_USER}@hellowoco.app"
• Why it matters: WOCO_DEPLOY_USER is read from a .env file parsed with sed. If the .env is writable by a different process or user, or if the value contains shell metacharacters (spaces, $(...)), the constructed HOST string is passed directly to ssh via rsync. A value like alice $(evil) would result in command injection in contexts where the shell expands the variable unquoted.
• Suggested fix: Validate WOCO_DEPLOY_USER against an allowlist pattern immediately after extraction: [[ "$WOCO_DEPLOY_USER" =~ ^[a-zA-Z0-9._-]+$ ]] || { echo "Invalid deploy user"; exit 2; }. This is a belt-and-suspenders measure since the value comes from a developer-controlled file, but appropriate for a script with sudo privileges on the remote end.

Finding 2

• File: .env.example / tools/push_data.sh comments
• Location: sudoers drop-in example — <user> ALL=(root) NOPASSWD: /usr/bin/rsync
• Why it matters: Granting passwordless sudo for /usr/bin/rsync to a user is effectively equivalent to root, because rsync can read and write arbitrary paths, including /etc/sudoers. The PR comment acknowledges blanket sudo is worse but doesn't call out that scoped rsync sudo is also near-equivalent to root.
• Suggested fix: Document this explicitly in the comment so operators understand the risk. Consider an alternative: run rsync over a restricted rrsync wrapper (included with rsync) that limits the remote path to /srv/woco/data, which genuinely scopes the privilege.