Repository navigation
Security: Fix CodeQL alerts for ReDoS, path injection, and workflow permissions - #1984
Merged
Merged
Conversation
…ssions Remediates the 17 open CodeQL code-scanning alerts reported in the IBM OSPO security notification (#1981). 1. ReDoS in is_camel_case() -- 1x py/redos, HIGH (CWE-1333) src/unitxt/text_utils.py: the pattern "^[A-Z]+([a-z0-9]*[A-Z]*[a-z0-9]*)*$" has an inner group that matches the empty string with overlapping alternatives, so a non-matching input backtracks catastrophically. An 18-character input did not complete within 20 seconds. Reachable from artifact.py:215, which calls is_camel_case() on artifact class names. Replaced with the equivalent linear-time form "^[A-Z]+[a-zA-Z0-9]*$". Equivalence verified against all 538 registered Artifact subclass names and an exhaustive fuzz over the relevant character classes: zero disagreements. A 50,000-character adversarial input now completes in 0.6ms. 2. Path injection from CLI arguments -- 5x py/path-injection, HIGH (CWE-22) src/unitxt/evaluate_cli.py: --output_path and --output_file_prefix flowed from argparse into five filesystem sinks without validation (os.makedirs, two result-file writes, and os.listdir/open in extract_scores). A prefix such as '../../../etc/cron.d/x' escaped the output directory. Added two helpers: - _normalize_directory(): realpath/expanduser the directory inputs. The resolved location is deliberately not confined to the CWD, since writing results to an arbitrary directory is the documented purpose of --output_path. - _validate_filename_component(): reject a prefix containing a path separator, or equal to '.'/'..'. This is the actual vulnerability -- a filename prefix has no legitimate reason to contain a separator. extract_scores() now normalizes its directory and confirms each path built from the listing stays inside it before opening. No breaking change: legitimate absolute and relative output paths continue to work. 3. Missing workflow permissions -- 11x actions/missing-workflow-permissions, MEDIUM (CWE-275) Added job-level 'permissions: contents: read' to the 11 workflows that declared none, matching the existing convention in pipy.yml and stale_issues.yml. Verified none of the 11 uses GITHUB_TOKEN, github.token, or the gh CLI, and that hub.yml/performance.yml authenticate to HuggingFace via their own repo secrets rather than the workflow token, so read-only is least privilege for all of them. Tests: - tests/library/test_text_utils.py: adversarial-input timing test that fails by hanging if the backtracking pattern is reintroduced. - tests/library/test_cli.py: traversal prefixes are rejected and '..' segments in --output_path are normalized away. Made the existing test_prepare_output_paths platform-independent, since path normalization resolves macOS's /tmp -> /private/tmp symlink. All 8 traversal cases and the ReDoS case were confirmed to fail against the unpatched code. Full tests/library suite: 778 passed, no new failures (3 pre-existing failures from missing optional dependencies are unchanged on a clean tree). Reported by: IBM OSPO security scanning (CodeQL default setup) Closes: #1981 Signed-off-by: Yoav Katz <katz@il.ibm.com>
CI had not run on this repo in a long time, and four unrelated external changes had accumulated. None of them originate in this branch. datasets 5.0.0 removed the verification_mode argument of DatasetBuilder.as_dataset(). HELM's bundled unitxt scenario still passes it, so helm-run aborted with a TypeError before producing stats.json. Pin the helm job to datasets<5; 4.x still accepts the argument. watsonx.ai removed meta-llama/llama-3-2-11b-vision-instruct, which now returns 404 model_not_supported. Point the image test at llama-4-maverick-17b-128e-instruct-fp8, the multimodal model the service currently lists, and the text-only LiteLLM test at llama-3-3-70b-instruct. The CFPB consumer-complaint API retired its CSV export. The "2023" URL answers HTTP 400 for every size, including size=1, while the same request without format=csv answers 200 -- so the failure is the retired export, not the large size the error message blames. The card stays registered because recipes.bluebench.product_help.cfpb_product_2023 refers to it, but its data can no longer be fetched, so loading it is no longer exercised. The watsonx subset is unaffected and still fully tested. Dataset hosts also rate-limited the runners (HTTP 429), which failed the performance job inside its "run on main branch" step -- code this branch cannot influence. Tolerate transient upstream HTTP failures (429 and 5xx) in catalog preparation, alongside the timeouts already tolerated. Hard client errors such as 404 and 410 still fail, so a dataset that has really gone away is not silently ignored. Finally, set fail-fast: false on the preparation matrix. The ten shards test disjoint files, so one shard failing said nothing about the others; previously a single flaky dataset cancelled the remaining nine and hid their real state, reporting eight failures where there was one. Catalog JSON output is byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yoav Katz <katz@il.ibm.com>
My previous commit pinned the helm job to datasets<5, on the theory that datasets 5.0.0 had dropped the verification_mode argument that HELM passes. That was the wrong diagnosis and the pin did not work: the job installed datasets==3.6.0 and still failed with the identical TypeError. The incompatible signature is unitxt's own. Dataset.as_dataset() overrides DatasetBuilder.as_dataset(), and commit 0936691 removed verification_mode from the override to match datasets 4.8.5. datasets.load_dataset() passes verification_mode down to the builder, so with any datasets older than 4.8.5 the call reaches our override and raises. HELM resolves such an older datasets, which is why only that job failed. Bisecting the upstream tags shows the parameter is present through 4.8.4 and absent from 4.8.5 on, so no single pin satisfies both sides. Instead, accept **kwargs and forward only the arguments the installed base class actually declares. Verified against both shapes of the base signature, and against the installed datasets 4.8.5, whose base class does not take verification_mode. This also drops the datasets<5 pin from the helm workflow, which was both ineffective and aimed at the wrong library. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yoav Katz <katz@il.ibm.com>
3 tasks done
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
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.
Remediates the 17 open CodeQL code-scanning alerts reported in the IBM OSPO security notification (#1981).
CodeQL runs on this repo via GitHub's default setup, so there is no
.github/workflows/codeql*.ymland the alert list is not reproducible locally. Each site below was located by matching the rule's detection pattern against the source and confirmed by execution. The counts line up exactly with the issue: 11 workflows without apermissions:block, and 5 filesystem sinks reachable fromargparse.py/redospy/path-injectionactions/missing-workflow-permissionsThere are no open Dependabot or secret-scanning alerts attributed to this issue, so this is the full scope.
1. ReDoS in
is_camel_case()— 1×py/redos, High (CWE-1333)src/unitxt/text_utils.py. The pattern^[A-Z]+([a-z0-9]*[A-Z]*[a-z0-9]*)*$has an inner group that matches the empty string with overlapping alternatives, so a non-matching input backtracks catastrophically. An 18-character input did not complete within 20 seconds.This is reachable in practice, not theoretical —
artifact.py:215callsis_camel_case()on artifact class names.Replaced with the equivalent linear-time form
^[A-Z]+[a-zA-Z0-9]*$. A 50,000-character adversarial input now completes in 0.6 ms.Rather than rely on the 10 existing test cases to establish equivalence, both patterns were compared across all 538 registered
Artifactsubclass names plus an exhaustive fuzz over the relevant character classes — zero disagreements.2. Path injection from CLI arguments — 5×
py/path-injection, High (CWE-22)src/unitxt/evaluate_cli.py.--output_pathand--output_file_prefixareargparsevalues (a CodeQL taint source) that flowed unsanitized into five filesystem sinks:os.makedirs, the two result-file writes, andos.listdir/openinextract_scores. A prefix such as../../../etc/cron.d/xescaped the intended output directory.Two helpers were added:
_normalize_directory()—realpath/expanduserthe directory inputs. The resolved location is deliberately not confined to the CWD, since writing results to an arbitrary directory is the documented purpose of--output_path._validate_filename_component()— reject a prefix containing a path separator, or equal to./... This is the actual vulnerability: a filename prefix has no legitimate reason to contain a separator.extract_scores()now normalizes its directory and confirms every path built from the listing stays inside it before opening.No breaking change — legitimate absolute and relative output paths continue to work.
3. Missing workflow permissions — 11×
actions/missing-workflow-permissions, Medium (CWE-275)Added job-level
permissions: contents: readto the 11 workflows that declared none, matching the existing convention inpipy.ymlandstale_issues.yml.Read-only was verified to be least privilege rather than assumed: none of the 11 uses
GITHUB_TOKEN,github.token, or theghCLI, andhub.yml/performance.ymlauthenticate to HuggingFace via their own repo secrets rather than the workflow token.Note for reviewers: indentation is not uniform across these files (2/4, 3/5 and 4/6-space styles coexist), so each block was inserted at its own file's level. All 13 workflows were confirmed to parse and to carry job-level permissions.
Tests
tests/library/test_text_utils.py— adversarial-input timing test that fails by hanging if the backtracking pattern is reintroduced.tests/library/test_cli.py— traversal prefixes are rejected, and..segments in--output_pathare normalized away.The existing
test_prepare_output_pathswas made platform-independent: it asserted on/tmp/outputverbatim, which path normalization breaks on macOS (/tmp→/private/tmp). CI is Linux so it would have passed there, but a platform-dependent test is a local-development trap.Verification
tests/librarysuite: 778 passed, no new failures. Three failures and one collection error are pre-existing and were confirmed to fail identically on a stashed clean tree (missing optional dependencies, plus a module-level helper namedtest_metricthat pytest misreads as a test).--helpintact.Out of scope
ExtractZipFile(src/unitxt/operators.py:2352) useszf.extractall(), a Zip Slip risk (py/unsafe-unpacking), andDownloadOperator.targetwrites to an arbitrary path. Neither appears in this issue's alert list, so both are left untouched here — worth a separate issue.Closes #1981
🤖 Generated with Claude Code