Skip to content

Fix/shell injection 1265 - #1283

Open
Sikandar1310291 wants to merge 2 commits into
openml:developfrom
Sikandar1310291:fix/shell-injection-1265
Open

Fix/shell injection 1265#1283
Sikandar1310291 wants to merge 2 commits into
openml:developfrom
Sikandar1310291:fix/shell-injection-1265

Conversation

@Sikandar1310291

@Sikandar1310291 Sikandar1310291 commented Feb 18, 2026

Copy link
Copy Markdown

Summary

Fixes two command-injection vulnerabilities identified in #1265 where user-controlled values were interpolated into shell commands without escaping.

Changes

openml_OS/helpers/api_helper.php

validate_arff() — replaced sed shell command with pure PHP file I/O

The previous code built a sed command using $name (dataset name from user upload), which could contain shell metacharacters like ', ;, $, etc.:

// BEFORE — vulnerable
$command2 = "sed -i -e '$string' $newUrl";
exec(CMD_PREFIX . $command2, $res, $code);
@awais62
feat: add comprehensive code quality tools (closes openml#1272)
4af98df 
- Add .phpstan.neon (level 3, excludes vendor/libraries/third_party)
- Add .php-cs-fixer.php (PSR-12 + PHP74Migration rules)
- Add psalm.xml (error level 8, suppresses CI magic-property issues)
- Add phpmd.xml (codesize, unusedcode, naming rulesets with relaxed thresholds)
- Update openml_OS/composer.json: add PHPStan ^1.10, Psalm ^5.0,
  PHP-CS-Fixer ^3.0, PHPMD ^2.14 as dev dependencies; add composer scripts
- Add .github/workflows/ci-quality.yml: runs on PRs and pushes to main;
  PHP-CS-Fixer and PHPStan are enforced; Psalm and PHPMD are informational
- Update CONTRIBUTING.md: add Code Quality section with setup and usage docs
@awais62
fix: sanitize shell invocations to prevent command injection (closes o…
5934568 
…penml#1265)

openml_OS/helpers/api_helper.php:
- validate_arff(): replace sed shell command with pure PHP file_get_contents/
  file_put_contents to prepend the ARFF comment line. Eliminates the injection
  vector where \ (user-supplied dataset name) could contain shell
  metacharacters. Also strips % / CR / LF from \ and casts \ to int.
- get_arff_features(): wrap \, \, \, and heap flag with
  escapeshellarg() as defence-in-depth.

openml_OS/controllers/Api_splits.php:
- different_predictions(): wrap \->evaluation, \, \ with
  escapeshellarg(). is_safe() check kept as first layer.
- all_wrong(): same treatment as different_predictions().
- challenge(): wrap \->evaluation, \, \, \,
  \ with escapeshellarg(); cast numeric args to int before escaping.
  Whitelist check on \ kept as first layer.
- generate(): wrap \->evaluation, \, \, \, and
  \['custom_testset'] with escapeshellarg(). Also tighten custom_testset
  guard to require is_cs_natural_numbers() before including in command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@Sikandar1310291 @awais62