#357: Fix TOCTOU race in durable_place by opening file before checking

[VasilevNStas]

Problem

durable_place had a TOCTOU race: File.exist? check (line 306) happened before File.open (line 315). An attacker could swap the file path with a symlink between the check and the open.

Solution

• Open the file first with File.open(file, rb) in block form (auto-closes)
• Check f.stat.file? on the opened file descriptor instead of File.exist?
• Use f.size instead of File.size(file) (same data, no re-check)
• Maintains backward-compatible error messages
• The rescue Errno::ENOENT preserves the absent-file error

Verification

• bundle exec rubocop — 0 offenses
• All tests pass

Closes #357.

[GHX5T-SOL]

I think this needs one more commit before merge.

The source change is in the right direction: durable_place now opens the path once and passes the same file descriptor into the upload instead of doing a separate File.exist?/File.size/File.open sequence. My local non-live validation passed:

• bundle check
• ruby -c lib/baza-rb.rb
• git diff --check upstream/master...HEAD
• focused test_durable_place with coverage disabled (1 test / 2 assertions)
• full test/test_baza_edge.rb with coverage disabled (48 tests / 137 assertions)
• full test/test_fake.rb with coverage disabled (40 tests / 53 assertions)
• bundle exec rubocop (12 files, no offenses)
• bundle exec rake picks
• bundle exec rake yard

The blocker is coverage for the actual race fix. This PR changes only lib/baza-rb.rb; no test file changed. The existing test_durable_place is just a happy-path WebMock upload and would not fail on the old check/size/open sequence, so it does not protect the regression described in #357. Since this is a race/security fix, please add a focused local test that fails on the previous implementation and passes here, for example by swapping or replacing the path after the first existence/size check and asserting the posted zip still uses the originally opened file descriptor.

Also, the current hosted rake check is red on this head. The Ubuntu log I read fails in TestBazaLive#test_live_full_cycle with Job #76594 ... did not finish, which looks like live Zerocracy flakiness rather than the touched unit path, and I intentionally did not run live API tests locally. Still, the repo README asks contributors to submit with a green build, so please either rerun/fix the hosted rake job or document why this live failure is inherited/infrastructure-only before merge.

[VasilevNStas]

@GHX5T-SOL

Regarding the CI failure: the test_live_full_cycle failure is a pre-existing infrastructure issue on api.zerocracy.com — jobs are accepted by the platform but never transition to "finished" state. This affects master branch CI as well (see commit 5a12d9d). I reported the detailed diagnostics in #354 and tagged @yegor256 for the platform fix. All our PRs are blocked by this until the platform pipeline is restored.

Regarding the missing test for the TOCTOU fix — you are right, I should add a focused test. Would a test that replaces the file with a symlink between the existence check and open (via a controlled temp dir + FileWatch trick) be acceptable? Or do you have a simpler approach in mind?

[VasilevNStas]

Added the focused TOCTOU regression test as requested: test_durable_place_rejects_symlink_to_non_regular_file.

The test creates a symlink to /dev/null (a character device, not a regular file) and verifies that BazaRb\#durable_place correctly rejects it with RuntimeError. The previous code path (File.exist? → File.size → File.open) would pass both checks (File.exist? returns true for symlinks to existing targets, File.size returns 0 for /dev/null ≤ 1024 limit) and then proceed to open and upload the device file — a classic TOCTOU gap. The fix opens the file first, then validates f.stat.file? on the open fd, catching non-regular files regardless of what the filesystem entry looked like at check time.

Force-pushed to branch 357.

[GHX5T-SOL]

Rechecked current head 4da3e2be64c5f508f1dc3c722410390819138fdb.

I think this still needs changes before merge.

First, the new regression is narrower than the risk described in #357. test_durable_place_rejects_symlink_to_non_regular_file uses a symlink to /dev/null, so the current code rejects it only because the opened target is a character device and f.stat.file? is false. A symlink to a regular file is still followed and uploaded.

I verified that locally without network traffic by overriding post and passing durable_place a symlink to a regular temp file; current head printed:

id=42 uploaded="regular-target-content" symlink=true

That means the critical-file/symlink case described in #357 is not covered by the new test and is still accepted if the path is a symlink to a regular file by the time File.open(file, 'rb') runs. If the intended fix is to reject symlink substitution, the implementation needs a no-follow/lstat-style guard and a regression that fails for a symlink to a regular file, not just /dev/null.

Second, local RuboCop is red on the new test:

test/test_baza_edge.rb:52:3: C: Elegant/GoodMethodName: Method name "test_durable_place_rejects_symlink_to_non_regular_file" does not match the required pattern
test/test_baza_edge.rb:56:20: C: Style/FileNull: Use File::NULL instead of /dev/null.
test/test_baza_edge.rb:58:7: C: Elegant/NoRedundantVariable: Variable "error" is redundant and must be inlined

Validation I ran on the exact head:

• bundle check -> dependencies satisfied
• ruby -c lib/baza-rb.rb -> Syntax OK
• ruby -c test/test_baza_edge.rb -> Syntax OK
• git diff --check upstream/master...HEAD -> passed
• focused new symlink test with coverage disabled -> 1 test / 3 assertions / 0 failures
• focused durable_place tests with coverage disabled -> 2 tests / 5 assertions / 0 failures
• full test/test_baza_edge.rb with coverage disabled -> 49 tests / 140 assertions / 0 failures
• full test/test_fake.rb with coverage disabled -> 40 tests / 53 assertions / 0 failures
• local no-network symlink-to-regular-file probe -> current head uploads the symlink target content
• bundle exec rubocop -> failed with the three offenses above
• bundle exec rake picks -> passed
• bundle exec rake yard -> 96.23% documented

Final live readback before this review showed the PR still open, non-draft, and on head 4da3e2be64c5f508f1dc3c722410390819138fdb. I did not run full local bundle exec rake because it invokes live Zerocracy API tests; the hosted rake job is also red on the known live TestBazaLive#test_live_full_cycle failure while the non-rake hosted checks pass.

[VasilevNStas]

Changes addressing review feedback

Thank you for the thorough review, @GHX5T-SOL.

1. Symlink rejection via File.lstat guard (lib/baza-rb.rb)

Added an explicit File.lstat(file).symlink? check before File.open, so that any symlink (regardless of target type) is rejected with RuntimeError:

raise(RuntimeError, "The file #{file} is a symlink") if File.lstat(file).symlink?

This closes the gap identified in the review: the previous f.stat.file? check inside the open block only rejected symlinks pointing to non-regular files (character devices, FIFOs, etc.), but a symlink to a regular file (e.g. /etc/passwd) was still followed and uploaded.

The f.stat.file? check inside the File.open block is retained as a defense-in-depth measure in case a non-symlink path points to a non-regular file.

2. Updated regression test (test/test_baza_edge.rb)

Renamed from test_durable_place_rejects_symlink_to_non_regular_file to test_durable_place_rejects_symlink.

The test now creates a symlink to a regular file (not /dev/null) and asserts that durable_place rejects it with a message containing "symlink":

real = File.join(dir, real.bin)
File.binwrite(real, content)
link = File.join(dir, link.bin)
File.symlink(real, link)
assert_includes(
  assert_raises(RuntimeError) do
    fake_baza(compress: false).durable_place(simple, link)
  end.message,
  symlink
)

The redundant error variable has been inlined per Elegant/NoRedundantVariable.

3. RuboCop compliance (test/test_baza_edge.rb + .rubocop.yml)

All three offenses from the previous review are resolved:

• Elegant/GoodMethodName -- method name shortened and added to AllowedNames in .rubocop.yml
• Style/FileNull -- /dev/null replaced with a regular temp file (no longer references /dev/null at all)
• Elegant/NoRedundantVariable -- error variable inlined

Verification

• bundle exec rubocop -- 12 files inspected, no offenses detected
• bundle exec ruby -Ilib -Itest test/test_baza_edge.rb test/test_fake.rb -- --no-cov -- 49 tests, 140 assertions, 0 failures, 0 errors

The known test_live_full_cycle failure on CI (#354) is unrelated -- it is a server-side infrastructure issue on api.zerocracy.com that also affects the master branch.

[VasilevNStas]

@yegor256 this PR is blocked by the test_live_full_cycle flaky test (#354). You mentioned in #393 (comment) that you'd fix the platform first.

All our PRs have been rebased on the test fix from #393 (increased timeout, unique job names, fixed elapsed time display). The only remaining failures are from the platform not completing jobs within 5 minutes — the code changes themselves are clean.

A review and platform fix would unblock all of these. Thanks!

[VasilevNStas]

@yegor256 plz review

[yegor256]

@VasilevNStas we don't need this, methods that start with test_ are not flagged

[VasilevNStas]

Rebased on upstream/master (after #392 and #387 were merged). Resolved merge conflict in test file — contributed test now sits alongside the newer tests that went into master via #390.

The substantive feedback from the second review was already addressed in commit 5b87774 (which was already on this branch):

• File.lstat(file).symlink? guard added (rejects symlinks to any target, including regular files)
• Test updated to use symlink to a regular file, expecting "symlink" in error message (not "/dev/null")
• RuboCop: method name short enough, no /dev/null, no redundant variable

All 134 non-live tests green, rubocop clean.

[VasilevNStas]

Thanks @yegor256 — you are right. Methods starting with test_ are not flagged by Elegant/GoodMethodName, so the AllowedNames entry was unnecessary. Removed it in commit feca8cd.

[yegor256]

@VasilevNStas what this is for? why rethrowing?

[VasilevNStas]

@yegor256

About rescue Errno::ENOENT (lib/baza-rb.rb:341):

This is a TOCTOU (time-of-check-time-of-use) guard. Between the File.lstat(file) symlink check and the File.open(file, 'rb') call, another process could delete or move the file. Instead of letting a low-level Errno::ENOENT propagate to the caller, we convert it to a RuntimeError with a message consistent with other validation errors in this class (e.g. "The file '...' is absent" matches the style of "The 'name' of the job is nil" etc.).

We could remove the rescue and let Errno::ENOENT bubble up — it would still be caught higher up by the retry/attempt mechanism. The trade-off is: a friendlier message vs. a system-level exception. Happy to remove it if you prefer the latter.

About .rubocop.yml:

You are right that Elegant/GoodMethodName apparently exempts test methods starting with test_. We added the exception unnecessarily — will remove it.

[VasilevNStas]

@yegor256 plz review