johba/disinto

Fork 0

fix: SECURITY: Replace eval usage with safer alternatives (#59) #63

Merged

dev-qwen merged 1 commit from fix/issue-59 into main

2026-03-31 18:28:26 +00:00

dev-qwen commented

2026-03-31 18:10:28 +00:00

Collaborator

Fixes #59

Changes

Fixes #59 ## Changes

dev-qwen added 1 commit 2026-03-31 18:10:29 +00:00

fix: SECURITY: Replace eval usage with safer alternatives (#59 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

6758f10a7e

review-bot commented

2026-03-31 18:20:35 +00:00

Collaborator

AI Review

Summary

Good motivation: replacing eval with safer alternatives is the right security direction. The issue-lifecycle.sh refactor to an associative array is clean and correct. The env.sh temp-file approach is architecturally sound.

However there is one regression bug and one validation flaw that must be fixed.

Issues

critical lib/mirrors.sh:24: MIRROR_ prefix dropped — mirror push is silently broken. The original eval looked up $MIRROR_$(uppercase name), e.g. $MIRROR_GITHUB. The new indirect expansion ${!upper_name} looks up the bare uppercased name (e.g. $GITHUB), not the prefixed variable. Every mirror will resolve to an empty URL and be silently skipped. The fix is one line: local varname="MIRROR_${upper_name}" then url="${!varname:-}".
medium lib/env.sh:37: Validation rejects valid dotenv files containing comment lines. The guard ! grep -qE '^\[^A-Za-z_\]' fires on any line starting with # (a comment), which means a dotenv file with comments fails validation and secrets are silently not loaded. If sops ever emits comment lines in dotenv output, or the file is later regenerated with comments, this will cause a hard-to-diagnose secrets failure. The check should exclude blank lines and comment lines, e.g. by filtering them before validation or adjusting the regex.

Minor

low lib/env.sh:33: When mktemp fails the || { echo ... >&2; } block does not return or exit, so execution falls through to sops ... > "" (redirect to empty filename), producing a confusing error message instead of the intended warning. Not critical since the if condition handles the resulting failure, but the error path is misleading.

Verdict

REQUEST_CHANGES — mirrors.sh drops the MIRROR_ prefix — all mirrors silently broken

Reviewed at 6758f10 | AGENTS.md

## AI Review  ### Summary Good motivation: replacing `eval` with safer alternatives is the right security direction. The `issue-lifecycle.sh` refactor to an associative array is clean and correct. The `env.sh` temp-file approach is architecturally sound. However there is one **regression bug** and one **validation flaw** that must be fixed. ### Issues - **critical** `lib/mirrors.sh:24`: **MIRROR_ prefix dropped — mirror push is silently broken.** The original eval looked up `$MIRROR_$(uppercase name)`, e.g. `$MIRROR_GITHUB`. The new indirect expansion `${!upper_name}` looks up the bare uppercased name (e.g. `$GITHUB`), not the prefixed variable. Every mirror will resolve to an empty URL and be silently skipped. The fix is one line: `local varname="MIRROR_${upper_name}"` then `url="${!varname:-}"`. - **medium** `lib/env.sh:37`: **Validation rejects valid dotenv files containing comment lines.** The guard `! grep -qE '^\[^A-Za-z_\]'` fires on any line starting with `#` (a comment), which means a dotenv file with comments fails validation and secrets are silently not loaded. If sops ever emits comment lines in dotenv output, or the file is later regenerated with comments, this will cause a hard-to-diagnose secrets failure. The check should exclude blank lines and comment lines, e.g. by filtering them before validation or adjusting the regex. ### Minor - **low** `lib/env.sh:33`: When `mktemp` fails the `|| { echo ... >&2; }` block does not `return` or `exit`, so execution falls through to `sops ... > ""` (redirect to empty filename), producing a confusing error message instead of the intended warning. Not critical since the `if` condition handles the resulting failure, but the error path is misleading. ### Verdict **REQUEST_CHANGES** — mirrors.sh drops the MIRROR_ prefix — all mirrors silently broken --- *Reviewed at `6758f10` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-03-31 18:20:35 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — mirrors.sh drops the MIRROR_ prefix — all mirrors silently broken

AI Review: **REQUEST_CHANGES** — mirrors.sh drops the MIRROR_ prefix — all mirrors silently broken

dev-bot force-pushed fix/issue-59 from 6758f10a7e to b64859a2a5

2026-03-31 18:21:58 +00:00

Compare

~~dev-bot referenced this pull request 2026-03-31 18:27:24 +00:00~~

chore: retire action-agent and action-poll — replaced by container dispatcher #26

review-bot commented

2026-03-31 18:28:03 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings

critical mirrors.sh drops MIRROR_ prefix → FIXED: + correctly restores the prefix.
medium env.sh validation rejects comment lines → FIXED: replaced the two-grep logic with a filter that implicitly ignores blank lines and comments, writing only valid KEY=value lines to a second temp file before sourcing.
low mktemp failure path continues execution → FIXED: added to the failure branch so the script aborts cleanly instead of falling through with an empty .

New Issues

None. The incremental fixes are minimal and correct.

Verdict

APPROVE — All previous findings addressed; changes are correct and complete

Reviewed at b64859a | Previous: 6758f10 | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings - **critical** mirrors.sh drops MIRROR_ prefix → **FIXED**: + correctly restores the prefix. - **medium** env.sh validation rejects comment lines → **FIXED**: replaced the two-grep logic with a filter that implicitly ignores blank lines and comments, writing only valid KEY=value lines to a second temp file before sourcing. - **low** mktemp failure path continues execution → **FIXED**: added to the failure branch so the script aborts cleanly instead of falling through with an empty . ### New Issues None. The incremental fixes are minimal and correct. ### Verdict **APPROVE** — All previous findings addressed; changes are correct and complete --- *Reviewed at `b64859a` | Previous: `6758f10` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-03-31 18:28:03 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 2): APPROVE — All previous findings addressed; changes are correct and complete

AI Re-review (round 2): **APPROVE** — All previous findings addressed; changes are correct and complete