Skip to content

(CAT-2637) Enable Security/Open and Security/IoMethods cops#645

Merged
gavindidrichsen merged 1 commit into
mainfrom
CAT-2637
Jun 12, 2026
Merged

(CAT-2637) Enable Security/Open and Security/IoMethods cops#645
gavindidrichsen merged 1 commit into
mainfrom
CAT-2637

Conversation

@david22swan

@david22swan david22swan commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

Enables two RuboCop security cops in the strict profile (the default selected_profile) to detect the pipe-based subprocess creation removed in Ruby 4.0, which ships with Puppet 9 — see Ruby #19630. Parent: CAT-2630 (PDK | Puppet 9 Compatibility Testing).

  • Security/Open — flags open() / URI.open() called with a pipe argument (open("| cmd")) or a dynamic argument (open(var)). Literal file paths such as open("foo.txt") are not flagged. Recommended replacements: File.open, IO.popen, URI.parse(...).open. PDK was previously explicitly disabling this cop (it is enabled-by-default in RuboCop but was excluded from the profile lists); it is now active.
  • Security/IoMethods — flags IO.read / write / binread / binwrite / foreach / readlines used for file access and steers them to the File.* equivalents (autocorrectable). The deliberate IO.read("| cmd") pipe form is intentionally left valid.

hardcore already covered both via all. Both cops remain overridable per module through .sync.yml.

Additional Context

The ticket asked us to investigate before implementing. Empirically verifying the cops (reading the cop sources and running them at rubocop 1.50.2 and 1.73.2, and checking 1.87.0) corrected two claims from the original investigation:

  • Security/Open only catches the literal pipe form (open("| cmd")) from rubocop ~1.71 onward — at 1.50.x the matcher is $!str (dynamic args only). We are fine because the Gemfile pins rubocop ~> 1.73.0, where the matcher is $_ and both literal-pipe and dynamic forms are flagged. It covers bare open / URI.open only — not IO.open or explicit-receiver Kernel.open(...).
  • Security/IoMethods does the inverse of what the ticket title's "IO.open pipe" wording implies (unchanged through 1.87.0): it flags file-access IO.* and skips the IO.read("| cmd") pipe form. This was accepted on the ticket — intentional subprocess use stays valid, and the IO.* -> File.* nudge is a reasonable autocorrectable style enforcement.

Verification (full ERB render of moduleroot/.rubocop.yml.erb + run at rubocop 1.73.2):

Code Result
open("| ls") flagged (literal pipe)
open(var) flagged (dynamic)
open("foo.txt") clean (no false positive)
IO.read("file.txt") flagged -> File.read
IO.read("| cmd") clean (intentional pipe)
IO.popen("ls") clean (replacement API)

Render confirms Security/IoMethods: { Enabled: true }; Security/Open is active via RuboCop's built-in default (same pattern as the existing Security/Eval). bundle exec rubocop on this repo is clean.

Known residual gap (documented in the README, not statically detectable without a bespoke cop — scoped out): IO.read("| cmd") subprocess use and the explicit-receiver Kernel.open(...) form.

Related Issues (if any)

  • CAT-2637 — PDK | Add Rubocop cop for Kernel.open / IO.open subprocess creation
  • Parent: CAT-2630 — PDK | Puppet 9 Compatibility Testing

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified.

Detect the pipe-based subprocess creation removed in Ruby 4.0 (Puppet 9)
per Ruby #19630. Enables both security cops in the strict profile:

- Security/Open flags open()/URI.open() with a pipe or dynamic argument
  (literal pipe detection requires rubocop >= ~1.71; satisfied by the
  pinned ~> 1.73.0). Previously PDK explicitly disabled this cop.
- Security/IoMethods flags IO.read/write/etc. file access, steering to
  File.* equivalents; the deliberate IO.read("| cmd") pipe form is
  intentionally left valid.

Documents the cop->replacement-API mapping and known gaps in README.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@david22swan david22swan requested a review from a team as a code owner June 12, 2026 10:07

@gavindidrichsen gavindidrichsen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LgTM

@gavindidrichsen gavindidrichsen merged commit 6420755 into main Jun 12, 2026
2 checks passed
@gavindidrichsen gavindidrichsen deleted the CAT-2637 branch June 12, 2026 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants