Four real defects hidden by the RuboCop todo baseline

[tas50]

Context

#553 adds a .rubocop_todo.yml baseline so that linting can pass on CI while the ~2500 accumulated offences are worked through gradually. The overwhelming majority of that baseline is genuine style debt — hash alignment, line length, symbol arrays.

Four entries are not style debt. They are latent defects that the baseline now hides. They are deliberately left excluded in #553 so that PR stays a CI-only change; this issue tracks fixing them properly.

Findings below were each verified by reading the code and reproducing the behaviour, not just taken from the RuboCop output.

---

1. Remote Heat template fetching is broken on Ruby >= 3.0

File: lib/fog/openstack/orchestration/util/recursive_hot_file_loader.rb:163
Cop that surfaced it: Security/Open

read_uri validates that a template reference is a remote http / https / ftp URL with a host, and then reads it with bare Kernel#open:

remote_schemes = %w[http https ftp]
# ...
url = URI(uri_or_filename)
if remote_schemes.include?(url.scheme)
  # Remote schemes must contain an host.
  raise ArgumentError if url.host.nil?
# ...
open(uri_or_filename) { |f| content = f.read }

Ruby 2.7 deprecated, and Ruby 3.0 removed, open-uri's monkey-patch that let Kernel#open accept URLs. require 'open-uri' at the top of the file no longer changes Kernel#open at all. Reproduced:

$ ruby -ropen-uri -e 'open("https://example.com") { |f| }'
Errno::ENOENT: No such file or directory @ rb_sysopen - https://example.com

So any nested template referenced by URL fails with a confusing Errno::ENOENT naming the URL as though it were a file path. This affects every Ruby the gem currently supports.

Fix: use URI.open (or better, branch to URI.parse(...).open for remote and File.read for local). That also resolves the cop, since Security/Open targets Kernel#open specifically.

Worth noting: the security risk the cop warns about — open("|ls") executing a shell command — is already mitigated here by the URI() call above it, which raises URI::InvalidURIError on |ls, | ls and -|ls. The comment at line 136 shows this was a deliberate defence. The bug is functional, not a vulnerability.

---

2. Image::V2::Mock#upload_image has the wrong signature and return value

File: lib/fog/openstack/image/v2/requests/upload_image.rb:22-25
Cop that surfaced it: Lint/UselessSetterCall

class Mock
  def upload_image(_image_id, _body)
    response = Excon::Response.new
    response.status = 204
  end
end

Two distinct problems:

a) Arity mismatch — the mock raises ArgumentError on a real call path. Image::V2::Real#upload_image takes three parameters, the mock takes two:

Mock params=[[:req, :_image_id], [:req, :_body]]
Real params=[[:req, :image_id], [:req, :body], [:opt, :params]]

Image#upload_data calls it with three arguments when handed a Hash:

# lib/fog/openstack/image/v2/models/image.rb:106-113
def upload_data(io_obj)
  requires :id
  if io_obj.kind_of? Hash
    service.upload_image(id, nil, io_obj)   # <- 3 args, mock accepts 2
  else
    service.upload_image(id, io_obj)
  end
end

So upload_data with a Hash blows up under Fog.mock!.

b) The mock returns 204, not a response. This is what Lint/UselessSetterCall flags: the method's last expression is an assignment, so it evaluates to the assigned 204 and the Excon::Response is discarded. Real#upload_image returns request(request_hash).body.

Fix: match the real signature (_image_id, _body, _params = {}) and return response explicitly.

---

3. Duplicate attr_writer declarations in Compute::Server

File: lib/fog/openstack/compute/models/server.rb:185 and :189
Cop that surfaced it: Lint/DuplicateMethods

image_ref= and flavor_ref= are each defined twice:

# line 53
attr_writer :image_ref, :flavor_ref, :nics, :os_scheduler_hints

# lines 183-189
attr_reader :image_ref
attr_writer :image_ref     # duplicate of line 53
attr_reader :flavor_ref
attr_writer :flavor_ref    # duplicate of line 53

Benign in behaviour — the second definition is identical to the first — but it emits "method redefined" warnings under ruby -W and is the kind of thing that hides a real conflict later. The attr_readers at 183 and 187 are not duplicates and must stay.

Fix: drop lines 185 and 189, or drop :image_ref, :flavor_ref from line 53. Lowest risk of the four.

---

4. YAML.load in the introspection mock

File: lib/fog/openstack/introspection.rb:48
Cop that surfaced it: Security/YAMLLoad

hash[key] = YAML.load(File.read(file))

Lowest priority of the four, and the security framing overstates it: this is Mock code reading a fixture committed to this repo, and I confirmed test/fixtures/introspection.yaml contains no aliases, symbols or !ruby/ tags. On Psych 4+ (Ruby 3.1+) YAML.load is safe-by-default anyway, so there is no live risk.

Fix: YAML.safe_load(File.read(file)) for clarity and to clear the cop.

Separate latent issue in the same three lines: the fixture path is relative —

file = "test/fixtures/introspection.yaml"

— so Mock.data only resolves when the process CWD happens to be the repository root. Worth making it relative to __dir__ while touching this.

---

Verifying a fix

Each of these is currently listed in .rubocop_todo.yml. After fixing one, remove its entry (or the whole file entry if it was the only offence) and confirm:

bundle exec rubocop

To see the offences while they are still baselined:

bundle exec rubocop --force-default-config \
  --only Lint/DuplicateMethods,Lint/UselessSetterCall,Security/YAMLLoad,Security/Open \
  lib/fog/openstack/compute/models/server.rb \
  lib/fog/openstack/image/v2/requests/upload_image.rb \
  lib/fog/openstack/introspection.rb \
  lib/fog/openstack/orchestration/util/recursive_hot_file_loader.rb