Skip to content

Fix OS command injection via customCliArgs and harden settings validation - #395

Open
R0Wi wants to merge 1 commit into
masterfrom
claude/app-security-code-review-wjabfi
Open

Fix OS command injection via customCliArgs and harden settings validation#395
R0Wi wants to merge 1 commit into
masterfrom
claude/app-security-code-review-wjabfi

Conversation

@R0Wi

@R0Wi R0Wi commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the languages command injection fix (#391): the same class of
issue was still reachable through the customCliArgs workflow setting.

CommandLineUtils::escapeCustomCliArgs() only stripped '&&' and ';', while
the value is concatenated into a command string which is executed by
mikehaertl/php-shellcommand via proc_open() as a raw shell string. Payloads
using '$(...)', backticks, '|', '&', '>' or '<' therefore reached /bin/sh.
Since the OCR operation is available for IManager::SCOPE_USER, any
authenticated user could create a personal flow and execute arbitrary
commands as the web server user once a matching file was processed. The
same string is also forwarded to the remote backend as
ocrmypdf_parameters.

Changes:

  • CommandLineUtils now splits the custom CLI arguments into single tokens
    (honoring single/double quoted values) and quotes every token which
    contains anything but harmless characters via escapeshellarg(). Tokens
    consisting of harmless characters only are passed through unchanged, so
    the resulting commandline is byte identical for all valid inputs (also
    for the remote backend). The sidecar file path uses the same helper.
  • WorkflowSettings validates customCliArgs (string, no control
    characters, max length) and ocrMode (must be a known mode, otherwise
    the commandline parameter mapping is undefined).
  • WorkflowSettings::setProperty() now rejects a present but invalid value
    with an InvalidArgumentException instead of silently falling back to the
    default, and non-object JSON is rejected as invalid JSON. Operation
    surfaces the reason of the failed validation to the user.
  • ControllerBase logs exceptions and returns a generic error message
    instead of the internal exception text, which was reachable by any
    logged in user via the (NoAdminRequired) installed languages endpoint.
  • OcrBackendInfoController uses the #[NoAdminRequired] attribute instead
    of the deprecated annotation.
  • README documents that custom CLI arguments are passed as separate
    literal arguments without shell expansion.

Adds regression tests for shell metacharacter payloads in customCliArgs on
the CommandLineUtils and the PdfOcrProcessor level as well as tests for the
stricter settings validation.

Co-Authored-By: Claude noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01VAjZ1QPPCsKNxxkVN7ZMU8

…tion

Follow-up to the `languages` command injection fix (#391): the same class of
issue was still reachable through the `customCliArgs` workflow setting.

`CommandLineUtils::escapeCustomCliArgs()` only stripped '&&' and ';', while
the value is concatenated into a command string which is executed by
mikehaertl/php-shellcommand via proc_open() as a raw shell string. Payloads
using '$(...)', backticks, '|', '&', '>' or '<' therefore reached /bin/sh.
Since the OCR operation is available for IManager::SCOPE_USER, any
authenticated user could create a personal flow and execute arbitrary
commands as the web server user once a matching file was processed. The
same string is also forwarded to the remote backend as
`ocrmypdf_parameters`.

Changes:

- CommandLineUtils now splits the custom CLI arguments into single tokens
  (honoring single/double quoted values) and quotes every token which
  contains anything but harmless characters via escapeshellarg(). Tokens
  consisting of harmless characters only are passed through unchanged, so
  the resulting commandline is byte identical for all valid inputs (also
  for the remote backend). The sidecar file path uses the same helper.
- WorkflowSettings validates `customCliArgs` (string, no control
  characters, max length) and `ocrMode` (must be a known mode, otherwise
  the commandline parameter mapping is undefined).
- WorkflowSettings::setProperty() now rejects a present but invalid value
  with an InvalidArgumentException instead of silently falling back to the
  default, and non-object JSON is rejected as invalid JSON. Operation
  surfaces the reason of the failed validation to the user.
- ControllerBase logs exceptions and returns a generic error message
  instead of the internal exception text, which was reachable by any
  logged in user via the (NoAdminRequired) installed languages endpoint.
- OcrBackendInfoController uses the #[NoAdminRequired] attribute instead
  of the deprecated annotation.
- README documents that custom CLI arguments are passed as separate
  literal arguments without shell expansion.

Adds regression tests for shell metacharacter payloads in customCliArgs on
the CommandLineUtils and the PdfOcrProcessor level as well as tests for the
stricter settings validation.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VAjZ1QPPCsKNxxkVN7ZMU8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants