feat(ruby): AST-based command injection false positive suppression by Adityakk9031 · Pull Request #51 · Corgea/Sighthound

Adityakk9031 · 2026-07-18T09:08:02Z

Problem

The Ruby security rules suffered from false-positives under safe multi-argument executions (e.g., using array literals or splatted array arguments inside command execution methods). These multi-argument formats are securely processed by the OS directly, bypassing the shell. Additionally, there were minor Clippy and formatting warnings that prevented quality harness runs from finishing cleanly.

Solution

AST-Based Suppression: Added a custom AST-based predicate (ruby_unsafe_command_injection) to differentiate safe executions (e.g., multi-argument arrays) from unsafe ones (e.g., interpolated strings, single variables) during command injection scans.
Scanner Integration: Integrated the AST inspection conditions directly into the conditions evaluation logic (src/scanner/conditions.rs) and taint scanner logic (src/scanner/scanning_logic.rs).
Rigorous Testing: Added extensive safe and unsafe Ruby test fixtures along with integration tests (tests/strictness/language_coverage.rs) to ensure false-positive suppression matches correctness requirements.
Code Quality & Clippy Fixes:
- Resolved clippy::items-after-test-module in src/scanner/output.rs by moving the test module to the end of the file.
- Fixed is_empty and unnecessary return statements in tests/unit/django_xss_prevention_tests.rs.
- Replaced redundant clones with slice references in harness.rs.
- Verified that cargo test, cargo clippy, and ./target/debug/harness-run.exe check all pass with zero warnings or errors.

Changes

rules/ruby/command_injection.ron: Integrated custom AST conditions.
src/scanner/conditions.rs: Implemented AST predicate logic and evaluation helpers.
src/scanner/scanning_logic.rs: Incorporated conditions checking during post-taint analysis.
src/scanner/output.rs: Relocated the SARIF tests module to resolve Clippy lints.
tests/unit/django_xss_prevention_tests.rs: Fixed styling lints.
harness.rs: Fixed reference-to-slice clone warning.
tests/test_files/strictness_languages/ruby/: Updated safe.rb and unsafe.rb fixtures.
tests/strictness/language_coverage.rs: Added the ruby_rules_taint_and_search_validation regression test.

Adityakk9031 · 2026-07-18T09:08:42Z

@asadeddin @juangaitanv have a look

Adityakk9031 · 2026-07-20T14:05:10Z

@juangaitanv have a look

….pipeline, and shell paths

Adityakk9031 · 2026-07-21T16:50:02Z

@juangaitanv and @Ibrahimrahhal hey please check this

juangaitanv · 2026-07-22T09:02:44Z

thank you for sticking with this through several rounds 🙏 most of the new comments come down to one question:
should unmodeled call shapes fail closed rather than open, since a miss here becomes a silent false negative?

settling that once likely clears comments 2–5 together. comment 1 is separate: the new condition fallback activates previously-dormant conditions in other languages' rules, which may deserve its own scoping decision.

resolved the two earlier threads, thank you for addressing those.

… namespace normalization

Adityakk9031 · 2026-07-22T11:52:16Z

Thanks @juangaitanv I've updated the PR based on your feedback:

Adopted a fail-closed approach: dynamic executables or array calls invoking shells with -c/-Command/-EncodedCommand (including PowerShell/pwsh) are flagged as unsafe.
Normalized namespace callees (::IO.popen, Open3::pipeline).
Scoped condition fallbacks to pattern and context fields so dormant rules aren't activated.
Added test fixtures for all new cases, and make ci / clippy pass cleanly.

juangaitanv

Thanks for pushing this through so many rounds. The fail-closed Ruby rewrite is the right call and reads accurately: unmodeled shapes returning unsafe is the correct default, and the shell-path, namespace, env-hash, and array/splat handling all model argv semantics well. 8 of my 9 earlier threads are cleanly resolved.

One thing I want to own: the fail-closed direction I steered toward across the earlier rounds is part of what pushed logic into the shared extractors, and that's where the two blockers below come from. Flagging it so it doesn't read as a Ruby-only change.

I built main and this branch and scanned the repo's own fixtures. Two blockers:

~50 JavaScript findings drop (CWE-346 postMessage, 14x CWE-922 storage, 2x DOM XSS). The taint-path condition gate now runs on every visited node against every rule's full condition list, so previously-dormant JS conditions activate and non-call nodes fail the all().
7 new Python SQL false positives on sql_safe_parameterized.py (the issue-#35 safe fixture) from the splat / arg-decomposition changes in common.rs.

Both likely clear together by scoping the two shared-path changes to the Ruby command-injection condition type rather than the global taint path. Happy to pair on that scoping if useful. Everything in the inline threads below the blockers is optional. Also worth a rerun of make complexity: check_ruby_unsafe_command_injection trips CCN 27 against the <=15 gate.

juangaitanv · 2026-07-24T08:00:48Z

            return;
        }
+        if let Some(conditions) = &rule.conditions {
+            if !crate::scanner::conditions::check_ast_conditions(


could the taint-path condition gate be scoped to the ruby command-injection condition type, given ~50 JavaScript findings (CWE-346, CWE-922, DOM XSS) disappear on the current fixtures?

juangaitanv · 2026-07-24T08:00:48Z

                continue; // Skip this finding as it's sanitized
            }
+            if let Some(conditions) = &rule.conditions {
+                if !crate::scanner::conditions::check_ast_conditions(


should the condition check run against every node the taint walk visits, given object literals like cwe_922_config_store_test.js:108 reach check_not_literal_condition, get None args, and drop the finding?

juangaitanv · 2026-07-24T08:00:48Z

        let func_name = crate::scanner::utils::AstUtils::get_function_context(node, ctx.source);

        // Check if this node matches any sink pattern
        let Some(sink_pattern) = ctx.rule_deduplicator.matches_sink_pattern(&node_text) else {


was removing the [SINK_ANALYSIS] Found sink debug log intended?

juangaitanv · 2026-07-24T08:00:48Z

-                .map(|s| s.trim().to_string())
+                .map(|s| {
+                    let mut s_trimmed = s.trim();
+                    if s_trimmed.starts_with('*') {


could the leading */& stripping and the extract_simple_variables decomposition be confined to ruby, given cursor.execute("... %s", [firm_id]) now surfaces firm_id and re-flags the issue-#35 safe fixture?

juangaitanv · 2026-07-24T08:00:48Z

+            if condition.not_in.is_some() {
+                check_in_context_condition(node, condition)
+            } else {
+                evaluate_field_condition(node, source, condition)


should the in_context route to evaluate_field_condition be scoped more tightly, given js-missing-origin-validation-taint-001 has no not_in and its not_contains "origin" check now fires on strings like originalUrl?

juangaitanv · 2026-07-24T08:00:48Z

+            check_ruby_unsafe_command_injection(node, source, language_support)
+        }
+        _ => {
+            if condition.field == "pattern" || condition.field == "context" {


could the generic fallback be limited to condition types designed for it rather than any condition whose field is pattern or context, given the ~30 dormant JS conditions that now activate?

juangaitanv · 2026-07-24T08:00:48Z

+}
+
+/// Extract shell name from a path or executable string, e.g. "/bin/sh" -> "sh", "C:\Windows\cmd.exe" -> "cmd"
+fn extract_shell_name(text: &str) -> Option<String> {


could the shell-name and flag helpers get direct unit tests, e.g. extract_shell_name, is_shell_command_flag, clean_ruby_string?

juangaitanv · 2026-07-24T08:00:49Z

+/// Check if a Ruby call's arguments structure represents an unsafe command execution.
+/// Follows strict FAIL-CLOSED principle: unmodeled, dynamic, or shell-executing calls return `true` (UNSAFE).
+/// Only calls with verified non-shell multi-argument or array structures return `false` (SAFE).
+pub fn check_ruby_unsafe_command_injection(


a few on check_ruby_unsafe_command_injection: should the shell resolver see through wrapper executables (system("/usr/bin/env", "sh", "-c", params[:cmd]) classifies as safe today), could it split into callee dispatch / hash filtering / shell-flag scan given lizard flags CCN 27, and how might we keep the ~200 lines of ruby logic out of the language-agnostic file, maybe a src/language/ruby/ entry?

juangaitanv · 2026-07-24T08:00:49Z

+
+    // All safe patterns (array literals, multiple arguments, escaped) must be CLEAN!
+    assert!(
+        safe_findings.is_empty(),


a few on the assertions: could we add a cross-language before/after finding-count check so a ruby change can't silently move JS and Python counts, could the unsafe cases cover the env/options-hash lines 29 to 31 that the >= 10 aggregate can absorb, and could marker-comment scanning replace the hardcoded line numbers?

…omposition, and reduce Ruby complexity

Adityakk9031 · 2026-07-24T21:28:15Z

@juangaitanv have a look

Adityakk9031 · 2026-07-31T06:52:38Z

@juangaitanv and @Ibrahimrahhal please review this

feat(ruby): AST-based command injection false positive suppression

950834f

juangaitanv requested review from Ibrahimrahhal and juangaitanv July 18, 2026 19:57

juangaitanv reviewed Jul 18, 2026

View reviewed changes

Comment thread src/scanner/conditions.rs Outdated

Comment thread tests/strictness/language_coverage.rs

feat(ruby): AST-based command injection false positive suppression

d4d615f

juangaitanv reviewed Jul 21, 2026

View reviewed changes

Comment thread src/scanner/conditions.rs Outdated

Comment thread src/scanner/conditions.rs Outdated

fix(ruby): refine AST command injection predicate for IO.popen, Open3…

1f7db0a

….pipeline, and shell paths

juangaitanv reviewed Jul 22, 2026

View reviewed changes

Comment thread src/scanner/conditions.rs Outdated

Comment thread src/scanner/conditions.rs Outdated

Comment thread src/scanner/conditions.rs Outdated

Comment thread src/scanner/conditions.rs Outdated

Comment thread src/scanner/conditions.rs Outdated

Adityakk9031 added 2 commits July 22, 2026 17:10

fix(ruby): enforce fail-closed evaluation, shell flag validation, and…

91d9b66

… namespace normalization

fix(clippy): collapse nested if statements in conditions.rs

9be8a80

juangaitanv requested changes Jul 24, 2026

View reviewed changes

fix(scanner): scope taint-path conditions, revert global variable dec…

8e3332f

…omposition, and reduce Ruby complexity

Uh oh!

Conversation

Adityakk9031 commented Jul 18, 2026

Problem

Solution

Changes

Uh oh!

Adityakk9031 commented Jul 18, 2026

Uh oh!

Uh oh!

Uh oh!

Adityakk9031 commented Jul 20, 2026

Uh oh!

Uh oh!

Uh oh!

Adityakk9031 commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juangaitanv commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adityakk9031 commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juangaitanv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adityakk9031 commented Jul 24, 2026

Uh oh!

Adityakk9031 commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Adityakk9031 commented Jul 21, 2026 •

edited

Loading

juangaitanv commented Jul 22, 2026 •

edited

Loading

Adityakk9031 commented Jul 22, 2026 •

edited

Loading