feat(corpus): lock the ShareX Rfc2898DeriveBytes leak as a regression (recall 9→10)

[PhysShell]

The bug (found by re-mining ShareX)

Re-mining ShareX/ShareX @ ed2a864 on the post-#57 extractor (modeless-Form false positives gone) left a short, mostly-real list of local-disposable findings. The cleanest real bug — ShareX.UploadersLib/FileUploaders/Vault_ooo.cs:216, the Vault.ooo uploader's DeriveCryptoData:

private static Vault_oooCryptoData DeriveCryptoData(byte[] key)
{
    byte[] salt = new byte[8];
    RandomNumberGenerator rng = RandomNumberGenerator.Create();   // leak #2 (factory) — MISSED
    rng.GetBytes(salt);

    Rfc2898DeriveBytes rfcDeriver =                               // leak #1 (new) — CAUGHT (OWN001)
        new Rfc2898DeriveBytes(key, salt, PBKDF2_ITERATIONS, HashAlgorithmName.SHA256);

    return new Vault_oooCryptoData { Salt = salt,
        Key = rfcDeriver.GetBytes(32), IV = rfcDeriver.GetBytes(16) };   // only byte[]s escape, not the deriver
}

Rfc2898DeriveBytes is IDisposable and holds an internal HMAC. It derives the AES key + IV, the method returns, and it is never disposed — a real leak on every Vault.ooo upload. It does not escape (the returned CryptoData holds only the derived byte[]s), so it's a clean local leak → OWN001. An oversight, not a pattern: the sibling EncryptBytes() in the same file already using-scopes its aes/MemoryStream/CryptoStream.

What's captured

corpus/real-world/sharex-rfc2898-derivebytes-leak/, in the established shape:

file	what
before.cs	the leak, reduced + self-contained (real System.Security.Cryptography, no ref pack) → OWN001
after.cs	both crypto disposables using-scoped → silent
case.own	OwnLang reduction (disposable acquire, no release → OWN001), checked by tests/test_corpus.py
expected-diagnostics.txt	OWN001
notes.md	provenance, the smoking-gun sibling, and the recall-gap caveat

Ratchets the benchmark floor 9 → 10 so the catch is pinned (a drop is a regression).

The honest caveat (recorded in notes.md)

The same method leaks a second crypto disposable — rng = RandomNumberGenerator.Create() — which the extractor misses: it's a static factory, and the detector only knows new and File.Open*/Create*, not arbitrary X.Create(). after.cs disposes both, so the fix is genuinely clean; extending the owning-factory set to the BCL crypto factories (RandomNumberGenerator.Create(), Aes.Create(), …) is a separate recall slice.

Validation

Local: case.own → OWN001; tests/test_corpus.py 8/8; modeled before.cs facts → OWN001 "is never disposed"; after.cs (using) → silent. The before.cs→facts extractor step is validated in CI (corpus-benchmark, now gated at --min-recall 10).

🤖 Generated with Claude Code

https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED

---

Generated by Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed a cryptographic key/IV derivation resource leak by ensuring derived cryptographic material is generated using properly scoped disposables.

• Tests

  • Tightened CI benchmark recall gates for stricter quality assurance.
  • Added/updated a real-world regression case and expected diagnostics for a previously detected disposable leak.

• Documentation

  • Added detailed notes explaining the leak, where it occurs in the workflow, what the analyzer reports, and how the resolution addresses the issue.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 975802d0-69aa-4a05-8c2e-5f7cd3b4e3ea

📥 Commits

Reviewing files that changed from the base of the PR and between e77cc8f and a566025.

📒 Files selected for processing (1)

• .github/workflows/ci.yml

🚧 Files skipped from review as they are similar to previous changes (1)

• .github/workflows/ci.yml

---

📝 Walkthrough

Walkthrough

Adds a new real-world corpus test case modeling the undisposed Rfc2898DeriveBytes leak from ShareX's Vault.ooo upload path. The corpus entry includes before.cs (leaking), after.cs (fixed with using), a case.own model, expected OWN001 diagnostics, and a notes.md explaining the finding and a recall gap. The CI benchmark recall gate is raised from 9 to 10.

Changes

ShareX Rfc2898DeriveBytes Corpus Case

Layer / File(s)	Summary
Before/after C# samples and OWN model corpus/real-world/sharex-rfc2898-derivebytes-leak/before.cs, corpus/real-world/sharex-rfc2898-derivebytes-leak/after.cs, corpus/real-world/sharex-rfc2898-derivebytes-leak/case.own	before.cs defines VaultCrypto.DeriveCryptoData with undisposed Rfc2898DeriveBytes and RandomNumberGenerator; after.cs fixes both with scoped using; case.own encodes the Deriver acquire-without-release OWN model.
Expected diagnostics, notes, and CI recall gate corpus/real-world/sharex-rfc2898-derivebytes-leak/expected-diagnostics.txt, corpus/real-world/sharex-rfc2898-derivebytes-leak/notes.md, .github/workflows/ci.yml	expected-diagnostics.txt declares OWN001; notes.md documents the bug location, checker output, and the RandomNumberGenerator.Create() recall gap; CI --min-recall threshold raised from 9 to 10.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• PhysShell/Own.NET#50: Introduced the corpus-benchmark CI job with the scripts/benchmark.py --min-recall gate that this PR tightens.
• PhysShell/Own.NET#51: Also modifies the scripts/benchmark.py --min-recall threshold in the same corpus-benchmark CI job.

Poem

🐇 Hop hop, the deriver leaked away,
No using block to save the day!
Now before and after show the truth,
OWN001 shines as corpus proof.
The recall gate climbs one rung higher—
No more leaks from that key deriver! 🔑

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly identifies the main change: adding a ShareX Rfc2898DeriveBytes leak as a corpus case and updating the recall benchmark threshold from 9 to 10.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/corpus-sharex-rfc2898-leak

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

.github/workflows/ci.yml (1)

667-677: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale benchmark commentary to match the new recall floor.

Line 669 still states “Now 9/11” while Line 677 enforces --min-recall 10. Please align/remove the hard-coded ratio so docs don’t drift on future ratchets.

Suggested patch

-        # improves. Now 9/11 — pooled buffers ride the path-sensitive flow engine
+        # improves. The pinned recall floor is 10 catches — pooled buffers ride the
+        # path-sensitive flow engine

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 667 - 677, The benchmark commentary in
the workflow contains a hard-coded recall ratio "Now 9/11" on line 669 that no
longer matches the actual minimum recall floor specified in the python
scripts/benchmark.py command which uses --min-recall 10. Remove or update the
hard-coded "Now 9/11" reference from the comment block to avoid documentation
drift when the recall floor is ratcheted up in the future, ensuring the comment
documentation stays aligned with the actual minimum recall threshold enforced by
the benchmark script invocation.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 667-677: The benchmark commentary in the workflow contains a
hard-coded recall ratio "Now 9/11" on line 669 that no longer matches the actual
minimum recall floor specified in the python scripts/benchmark.py command which
uses --min-recall 10. Remove or update the hard-coded "Now 9/11" reference from
the comment block to avoid documentation drift when the recall floor is
ratcheted up in the future, ensuring the comment documentation stays aligned
with the actual minimum recall threshold enforced by the benchmark script
invocation.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 69e9dbc1-b0ac-475d-a4d2-f7d3c4656a0a

📥 Commits

Reviewing files that changed from the base of the PR and between 8355251 and e77cc8f.

📒 Files selected for processing (6)

• .github/workflows/ci.yml
• corpus/real-world/sharex-rfc2898-derivebytes-leak/after.cs
• corpus/real-world/sharex-rfc2898-derivebytes-leak/before.cs
• corpus/real-world/sharex-rfc2898-derivebytes-leak/case.own
• corpus/real-world/sharex-rfc2898-derivebytes-leak/expected-diagnostics.txt
• corpus/real-world/sharex-rfc2898-derivebytes-leak/notes.md