fix(plugin-manager): shell-escape hook arguments in pre/post_hooks#2697
fix(plugin-manager): shell-escape hook arguments in pre/post_hooks#2697elibosley wants to merge 1 commit into
Conversation
pre_hooks() and post_hooks() built the hook command by interpolating the plugin
name, method, and error text straight into a string that run() executes via
`sh -c` (popen):
run("$hook plugin $method $plugin $error");
An error containing an apostrophe - e.g. "XML file doesn't exist or xml parse
error" - makes the shell treat it as an unterminated quote and abort:
sh: -c: line 1: unexpected EOF while looking for matching `'`
so the hook never runs. For post-hooks that means post_plugin_checks is skipped,
and normal failure cleanup (e.g. removing the plugin's pending-status marker) is
missed. An error (or name) containing spaces is also split into multiple args,
truncating the message the hook receives.
Escape every interpolated value with escapeshellarg() so each is passed as a
single, literal argument regardless of quotes or whitespace. run()'s streaming
behavior is unchanged. Verified: with an apostrophe-containing error the hook
now runs and receives the full error string as one argument, where before it
failed with the sh EOF error and produced no output.
|
Warning Review limit reached
Next review available in: 47 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
Fixes OS-610.
Summary
pre_hooks()andpost_hooks()build the hook command by concatenating unquoted values into a string thatrun()executes viash -c(popen):A normal Plugin Manager error contains an apostrophe —
XML file doesn't exist or xml parse error— so the shell treats it as the start of a quoted string and aborts:The hook never executes.
Impact
This doesn't cause the original plugin failure, but it prevents Plugin Manager from completing its failure-handling contract — exactly when those hooks matter most:
pre_plugin_checksis never removed (/tmp/plugins/pluginPending/<name>), so the UI can be left showing a stuck "pending" state.Fix
Keep the existing
run()implementation and escape each interpolated argument at the call site withescapeshellarg(), in bothpre_hooks()andpost_hooks(). Surgical, and preserves the existing execution model.post_hooks()keeps passing$error(escaped) sopost_plugin_checks'if ($error) break;success/failure signal is retained.Verification
Using the reproduction from OS-610 (a deliberately malformed
.plg):Before
After
The original XML failure and nonzero status are unchanged —
post_plugin_checksnow executes, clears the pending marker, and emits no secondary shell error. Separately confirmed the hook receives the full error as a single argument ($argv[4]=XML file doesn't exist or xml parse error).php -lclean.Security context
This is not claiming a demonstrated privilege escalation — the inputs here come from a plugin operation that is already privileged. But Plugin Manager runs these commands with elevated authority, so passing values through
sh -cwithout argument escaping is undesirable; escaping is worthwhile defense-in-depth alongside the failure-handling fix.Notes
Independent of the dangling-symlink fix in #2696 (that error string is what surfaced this bug). The two touch different functions and can merge in either order.