Resolve comprehensive TorBox Media Server code review findings#10
Merged
Merged
Conversation
- Fix `configure_arr_auth` syntax error and auth logic skip for fresh installs - Fix admin credential variable naming and `SERVICES_STARTED` uninitialized error - Preserve Decypharr password on re-runs and validate `TORBOX_MEDIA_SERVER` input - Ensure `manage.sh` CI validation fails on syntax errors and test `setup.sh` safely by properly parsing functions - Add `.env.example` and validate `docker-compose.yml` with representative variables in CI - Set `manage.sh keys` to mask secrets by default unless `--show-secrets` is passed - Update `README.md` to reflect pre-seeded credentials and security adjustments - Bind Plex and Jellyfin default ports to `127.0.0.1` for increased security - Add fallback docker wrapper in `uninstall.sh` to use `sudo docker` when appropriate - Increase CI dependency install robustness with `apt-get update` - Use `docker-compose-plugin` on Fedora instead of the legacy `docker-compose` Co-authored-by: nordicnode <128633122+nordicnode@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Review Summary by QodoFix critical bugs and enhance security with credential masking and port binding
WalkthroughsDescription• Fix critical syntax error in configure_arr_auth function blocking authentication setup • Initialize SERVICES_STARTED variable and preserve Decypharr credentials on re-runs • Implement secure secret masking in manage.sh keys command with --show-secrets flag • Bind Plex and Jellyfin ports to 127.0.0.1 for enhanced security by default • Add .env.example file and improve CI validation with docker-compose syntax checking • Fix docker command detection in uninstall.sh to support both direct and sudo execution • Enhance CI robustness with apt-get update and proper heredoc parsing for manage.sh validation • Update documentation to reflect auto-generated credentials and security improvements Diagramflowchart LR
A["setup.sh"] -->|Initialize SERVICES_STARTED| B["Variable Management"]
A -->|Fix configure_arr_auth logic| C["Authentication Setup"]
A -->|Preserve Decypharr credentials| D["Credential Persistence"]
A -->|Add secret masking function| E["manage.sh keys command"]
A -->|Bind to 127.0.0.1| F["docker-compose.yml"]
G[".env.example"] -->|Template variables| H["CI Validation"]
I["uninstall.sh"] -->|Add docker wrapper| J["Docker Command Detection"]
K["lint.yml"] -->|Enhanced checks| L["CI Pipeline"]
L -->|Validate compose config| M["docker-compose.yml"]
N["README.md"] -->|Update security notes| O["Documentation"]
File Changes1. setup.sh
|
Code Review by Qodo
1. manage.sh keys command broken
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1199
to
+1236
| local show_secrets=false | ||
| if [[ "\$2" == "--show-secrets" ]]; then | ||
| show_secrets=true | ||
| fi | ||
|
|
||
| mask_val() { | ||
| local val="\$1" | ||
| if [[ "\$show_secrets" == "true" ]]; then | ||
| echo "\$val" | ||
| elif [[ \${#val} -gt 8 ]]; then | ||
| echo "\${val:0:4}...<hidden>...\${val: -4}" | ||
| else | ||
| echo "***<hidden>***" | ||
| fi | ||
| } | ||
|
|
||
| echo -e "\n${CYAN}━━━━ API Keys ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━${NC}\n" | ||
| echo -e " ${YELLOW}WARNING: Sensitive credentials below. Do not share this output.${NC}\n" | ||
| echo -e " ${BOLD}TorBox${NC} $(env_val TORBOX_API_KEY)" | ||
| echo -e " ${BOLD}Radarr${NC} $(env_val RADARR_API_KEY)" | ||
| echo -e " ${BOLD}Sonarr${NC} $(env_val SONARR_API_KEY)" | ||
| echo -e " ${BOLD}Prowlarr${NC} $(env_val PROWLARR_API_KEY)" | ||
| if [[ "\$show_secrets" == "true" ]]; then | ||
| echo -e " ${YELLOW}WARNING: Sensitive credentials below. Do not share this output.${NC}\n" | ||
| else | ||
| echo -e " ${YELLOW}Secrets masked. Use './manage.sh keys --show-secrets' to reveal.${NC}\n" | ||
| fi | ||
|
|
||
| echo -e " ${BOLD}TorBox${NC} \$(mask_val \"\$(env_val TORBOX_API_KEY)\")" | ||
| echo -e " ${BOLD}Radarr${NC} \$(mask_val \"\$(env_val RADARR_API_KEY)\")" | ||
| echo -e " ${BOLD}Sonarr${NC} \$(mask_val \"\$(env_val SONARR_API_KEY)\")" | ||
| echo -e " ${BOLD}Prowlarr${NC} \$(mask_val \"\$(env_val PROWLARR_API_KEY)\")" | ||
|
|
||
| local _radarr_pass _sonarr_pass _prowlarr_pass | ||
| _radarr_pass="$(env_val RADARR_ADMIN_PASS)" | ||
| _sonarr_pass="$(env_val SONARR_ADMIN_PASS)" | ||
| _prowlarr_pass="$(env_val PROWLARR_ADMIN_PASS)" | ||
| if [[ -n "$_radarr_pass" ]]; then | ||
| _radarr_pass="\$(env_val RADARR_ADMIN_PASS)" | ||
| _sonarr_pass="\$(env_val SONARR_ADMIN_PASS)" | ||
| _prowlarr_pass="\$(env_val PROWLARR_ADMIN_PASS)" | ||
| if [[ -n "\$_radarr_pass" ]]; then | ||
| echo "" | ||
| echo -e " ${BOLD}Admin Credentials:${NC}" | ||
| echo -e " ${BOLD}Radarr${NC} user: $(env_val RADARR_ADMIN_USER) pass: ${_radarr_pass}" | ||
| echo -e " ${BOLD}Sonarr${NC} user: $(env_val SONARR_ADMIN_USER) pass: ${_sonarr_pass}" | ||
| echo -e " ${BOLD}Prowlarr${NC} user: $(env_val PROWLARR_ADMIN_USER) pass: ${_prowlarr_pass}" | ||
| echo -e " ${BOLD}Radarr${NC} user: \$(env_val RADARR_ADMIN_USER) pass: \$(mask_val \"\${_radarr_pass}\")" | ||
| echo -e " ${BOLD}Sonarr${NC} user: \$(env_val SONARR_ADMIN_USER) pass: \$(mask_val \"\${_sonarr_pass}\")" | ||
| echo -e " ${BOLD}Prowlarr${NC} user: \$(env_val PROWLARR_ADMIN_USER) pass: \$(mask_val \"\${_prowlarr_pass}\")" |
There was a problem hiding this comment.
1. Manage.sh keys command broken 🐞 Bug ≡ Correctness
The generated manage.sh keys command will exit immediately because it uses local in a top-level case block (not inside a function) while set -euo pipefail is enabled. Additionally, because the manage.sh content is written via a single-quoted heredoc, the new \$ and \$(...) escapes are preserved literally, so option parsing and key rendering will not work.
Agent Prompt
### Issue description
The `keys)` handler written into `manage.sh` is broken:
- `local` is used at top-level (inside a `case` block), causing `./manage.sh keys` to fail under `set -euo pipefail`.
- The handler unnecessarily escapes `$` and command substitutions even though the heredoc is single-quoted (`<< 'MANAGE_EOF'`), so those escapes become literal characters in the generated `manage.sh`, preventing argument parsing and value rendering.
### Issue Context
`manage.sh` is generated by `setup.sh` using single-quoted heredocs, so content is emitted literally; no variable expansion occurs during generation.
### Fix Focus Areas
- setup.sh[1034-1239]
### Implementation notes
In the generated `manage.sh` content:
- Remove `local show_secrets=false` (use `show_secrets=false`) OR move the `keys` logic into a real function (e.g., `keys_cmd() { ... }`) and call it from the `case`.
- Remove the backslashes in `"\$2"`, `"\$1"`, `\${#val}`, `\$(...)`, etc. Use normal bash: `${2:-}`, `$(env_val TORBOX_API_KEY)`, etc.
- Keep `local` only inside actual function bodies (e.g., inside `mask_val()`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| show_urls | ||
| ;; | ||
| keys) | ||
| local show_secrets=false |
There was a problem hiding this comment.
🔴 local used outside a function in generated manage.sh crashes ./manage.sh keys
The keys) case branch at setup.sh:1199 uses local show_secrets=false at the top level of the generated manage.sh script (not inside any function). The case statement starting at setup.sh:1152 is at the script's top level. Since manage.sh starts with set -euo pipefail (setup.sh:1036), bash will print local: can only be used in a function, return exit code 1, and set -e will immediately terminate the script. This completely breaks ./manage.sh keys and ./manage.sh keys --show-secrets. The same issue exists at line 1227 (local _radarr_pass ...), though line 1199 is hit first.
Prompt for agents
In setup.sh, inside the generate_management_script() function, the keys) case branch of the manage.sh heredoc (around line 1199) uses 'local' outside a function. The case statement at line 1152 is at the top level of the generated manage.sh, not inside a function. 'local' can only be used inside functions in bash, and with set -euo pipefail, this causes an immediate script exit.
Fix: Replace 'local show_secrets=false' with just 'show_secrets=false' at line 1199, and replace 'local _radarr_pass _sonarr_pass _prowlarr_pass' with just '_radarr_pass="" _sonarr_pass="" _prowlarr_pass=""' (or simply remove the 'local' keyword) at line 1227. Top-level variables in the case block don't need 'local' scoping.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+1200
to
+1236
| if [[ "\$2" == "--show-secrets" ]]; then | ||
| show_secrets=true | ||
| fi | ||
|
|
||
| mask_val() { | ||
| local val="\$1" | ||
| if [[ "\$show_secrets" == "true" ]]; then | ||
| echo "\$val" | ||
| elif [[ \${#val} -gt 8 ]]; then | ||
| echo "\${val:0:4}...<hidden>...\${val: -4}" | ||
| else | ||
| echo "***<hidden>***" | ||
| fi | ||
| } | ||
|
|
||
| echo -e "\n${CYAN}━━━━ API Keys ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━${NC}\n" | ||
| echo -e " ${YELLOW}WARNING: Sensitive credentials below. Do not share this output.${NC}\n" | ||
| echo -e " ${BOLD}TorBox${NC} $(env_val TORBOX_API_KEY)" | ||
| echo -e " ${BOLD}Radarr${NC} $(env_val RADARR_API_KEY)" | ||
| echo -e " ${BOLD}Sonarr${NC} $(env_val SONARR_API_KEY)" | ||
| echo -e " ${BOLD}Prowlarr${NC} $(env_val PROWLARR_API_KEY)" | ||
| if [[ "\$show_secrets" == "true" ]]; then | ||
| echo -e " ${YELLOW}WARNING: Sensitive credentials below. Do not share this output.${NC}\n" | ||
| else | ||
| echo -e " ${YELLOW}Secrets masked. Use './manage.sh keys --show-secrets' to reveal.${NC}\n" | ||
| fi | ||
|
|
||
| echo -e " ${BOLD}TorBox${NC} \$(mask_val \"\$(env_val TORBOX_API_KEY)\")" | ||
| echo -e " ${BOLD}Radarr${NC} \$(mask_val \"\$(env_val RADARR_API_KEY)\")" | ||
| echo -e " ${BOLD}Sonarr${NC} \$(mask_val \"\$(env_val SONARR_API_KEY)\")" | ||
| echo -e " ${BOLD}Prowlarr${NC} \$(mask_val \"\$(env_val PROWLARR_API_KEY)\")" | ||
|
|
||
| local _radarr_pass _sonarr_pass _prowlarr_pass | ||
| _radarr_pass="$(env_val RADARR_ADMIN_PASS)" | ||
| _sonarr_pass="$(env_val SONARR_ADMIN_PASS)" | ||
| _prowlarr_pass="$(env_val PROWLARR_ADMIN_PASS)" | ||
| if [[ -n "$_radarr_pass" ]]; then | ||
| _radarr_pass="\$(env_val RADARR_ADMIN_PASS)" | ||
| _sonarr_pass="\$(env_val SONARR_ADMIN_PASS)" | ||
| _prowlarr_pass="\$(env_val PROWLARR_ADMIN_PASS)" | ||
| if [[ -n "\$_radarr_pass" ]]; then | ||
| echo "" | ||
| echo -e " ${BOLD}Admin Credentials:${NC}" | ||
| echo -e " ${BOLD}Radarr${NC} user: $(env_val RADARR_ADMIN_USER) pass: ${_radarr_pass}" | ||
| echo -e " ${BOLD}Sonarr${NC} user: $(env_val SONARR_ADMIN_USER) pass: ${_sonarr_pass}" | ||
| echo -e " ${BOLD}Prowlarr${NC} user: $(env_val PROWLARR_ADMIN_USER) pass: ${_prowlarr_pass}" | ||
| echo -e " ${BOLD}Radarr${NC} user: \$(env_val RADARR_ADMIN_USER) pass: \$(mask_val \"\${_radarr_pass}\")" | ||
| echo -e " ${BOLD}Sonarr${NC} user: \$(env_val SONARR_ADMIN_USER) pass: \$(mask_val \"\${_sonarr_pass}\")" | ||
| echo -e " ${BOLD}Prowlarr${NC} user: \$(env_val PROWLARR_ADMIN_USER) pass: \$(mask_val \"\${_prowlarr_pass}\")" |
There was a problem hiding this comment.
🔴 Incorrect \$ escaping in single-quoted heredoc breaks all command substitutions in keys command
Lines 1200-1236 use \$ for variable references and command substitutions (e.g., \$2, \$(env_val ...), \$(mask_val ...)). However, this code is inside a single-quoted heredoc (<< 'MANAGE_EOF' at setup.sh:1096), which means no expansion or escape processing occurs — content is written literally to manage.sh. The \$ sequences appear as-is in the generated file. When bash later executes manage.sh, "\$2" evaluates to the literal string $2 (not the second positional argument), and "\$(env_val ...)" evaluates to the literal string $(env_val ...) (not a command substitution).
Comparison with old working code
The old code correctly used unescaped $ in the same single-quoted heredoc:
# OLD (correct): command substitution executes at runtime
echo -e " ${BOLD}TorBox${NC} $(env_val TORBOX_API_KEY)"
# NEW (broken): literal text output
echo -e " ${BOLD}TorBox${NC} \$(mask_val \"\$(env_val TORBOX_API_KEY)\")"The fix is to remove all \ before $ and " in the keys section, since single-quoted heredocs don't need escaping.
Prompt for agents
In setup.sh, the keys) case branch (lines 1198-1239) is inside a single-quoted heredoc (<< 'MANAGE_EOF' at line 1096). In single-quoted heredocs, NO expansion or escape processing occurs — everything is written literally to the output file. The PR incorrectly added backslash escaping (\$, \") as if the heredoc were unquoted.
Fix: Remove all backslash escaping of $ and " throughout lines 1200-1236. For example:
- Change '\$2' to '$2'
- Change '\$(env_val ...)' to '$(env_val ...)'
- Change '\$(mask_val ...)' to '$(mask_val ...)'
- Change '\"' to '"'
- Change '\${#val}' to '${#val}'
- Change '\${val:0:4}' to '${val:0:4}'
The old code on the same heredoc correctly used $(env_val ...) without escaping. The mask_val function definition (lines 1204-1213) also needs the same fix for its internal variable references.
Was this helpful? React with 👍 or 👎 to provide feedback.
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
1 participant
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.
This submission addresses a large batch of 16 critical, high-impact, and lower-priority bugs identified during a code review of the
TorBox-Media-Serverrepository. It resolves a blocking syntax error insetup.shthat broke all setups, implements crucial missing logic blocks for authentication handling, bolsters CI by correctly propagating syntax failures, sets safer default network bindings for Plex and Jellyfin, and fixes an uninitialized variable that would crash installations if service startup was delayed.PR created automatically by Jules for task 7776805136999684067 started by @nordicnode