ROCK-9041 Fix storage-provider stream handling across plugins (audit findings)

[stphnlee]

Summary

Remediates the remaining plugin findings from the storage-provider audit on ROCK-9041. Companion to #321 (SignNowDownload, audit CRITICAL #1) and secc/Rock#18. #18 is closed rather than merged because SECC cherry-picks it into our Rock build until it's fixed upstream. It fixes the write side (AzureBlobStorage not disposing the upload stream). This PR covers the read side and the cached-stream reuse, which #18 doesn't touch. Both are needed.

Merge order: #321 is merged; this branch is rebased on hotfix-1.16.12 after it.

Every read now goes through one helper, BinaryFile.ReadContentBytes() in org.secc.DevLib. It opens a fresh storage-provider stream (StorageProvider.GetContentStream), copies the bytes into memory, and disposes the stream. Consumers (iText, GDI+, Ghostscript, temp files) get a MemoryStream or a byte array. Provider streams are never left open, and code never assumes the Database provider. A missing or empty file throws an InvalidOperationException that names the file.

Why a helper instead of using ( var s = binaryFile.ContentStream )

The first revision of this PR wrapped ContentStream in using. That breaks on Azure. BinaryFile.ContentStream caches its stream and only re-fetches when CanSeek is false. Azure's LazyLoadingReadOnlyStream still reports CanSeek = true after Dispose. So any later read of the same tracked BinaryFile in the same RockContext got the disposed stream back and threw ArgumentNullException. Examples: a second merge action on the same template, an email attachment later in the same workflow pass, or PDFCombine of one file with itself. The helper never touches the cached stream.

Changes

org.secc.DevLib: new Extensions/BinaryFileExtensions.cs (ReadContentBytes). README documents it.

org.secc.ConnectionCards (Utilities/ConnectionCardsUtilties.cs, audit CRITICAL #2, #3 and both HIGHs)

• ConvertPDFToImage and ChopImage wrote content only to DatabaseData. Under a non-Database provider SaveContent never ran, so cards were saved with no content. They now assign ContentStream and FileSize.
• ChopImage read DatabaseData.Content directly, which threw a NullReferenceException for Azure files. All three utilities now read via ReadContentBytes.
• The source content is buffered and the provider stream disposed before the block deletes the source BinaryFile, so the FileSystem provider's delete no longer hits an open handle.
• RotateImage now disposes the provider stream and the Image. The old code leaked both.
• ChopImage iterates exactly cols x rows cells; integer division keeps each inset cell inside the bitmap. The old step-until-edge loop added a partial row or column when the size didn't divide evenly (for example 1056 px with rows=5), and Bitmap.Clone threw OutOfMemoryException. All bitmaps are now disposed.
• ChopImage rejects a grid with fewer than 1 row or column, or with cells of 4 px or less, before anything is saved. The block shows the message and keeps the scan. Previously the cells were silently skipped, and the block deleted the scan, reported success and launched no workflows. Rows and Columns now have Minimum="1".
• ConvertPDFToImage returns null for a PDF with no pages, and the block shows an error. The empty BinaryFile it used to return made the save hook throw on its null MimeType.
• Now references DevLib. The private ReadAllBytes helper is gone.

org.secc.PDF PDFCombine.cs, PDFFormMerge.cs; org.secc.SafetyAndSecurity all five *Merge.cs: template and input bytes are read via ReadContentBytes, and iText gets a MemoryStream. PDF now references DevLib.

org.secc.Imaging AI/FaceCrop.cs: crops from an in-memory copy.

org.secc.LeagueApps Utilities/APIClient.cs: a missing or empty service-account file now throws a clear error. Before, it returned a null certificate, failed later inside X509Certificate2, and re-queried on every access.

org.secc.SignNowWorkflow SignNowCreate.cs: on top of #321's temp-directory cleanup and SDK error handling, the rendered PDF is read via ReadContentBytes. Now references DevLib.

org.secc.Security

• WellrightRedirect.ascx.cs: the signing PFX is no longer imported with PersistKeySet, which left a new private-key file in the machine key store on every page load. The certificate is disposed after signing. Exportable stays because CertificateUtility exports the key.
• SignNowTest.ascx.cs: writes with File.WriteAllBytes. File.OpenWrite didn't truncate, so a leftover longer temp file kept its trailing bytes.

READMEs: ConnectionCards, PDF, SafetyAndSecurity and DevLib document provider-agnostic reads (addresses Copilot's review comments). The stale ConnectionCards RotateImage observation is removed.

Not changed, and why

• org.secc.Purchasing Attachments.ascx.cs (audit MEDIUM, cross-provider BinaryFileType reassignment): Rock core's BinaryFile save hook already migrates content between providers when the BinaryFileType navigation property changes, which is what this code sets. No plugin change needed.
• org.secc.Rest GroupExtensionsController (audit LOW): the dispose half is covered by the cherry-picked ROCK-9041 Dispose ContentStream in AzureBlobStorage.SaveContent Rock#18.
• GetStatement.ashx, FontAwesomeSettings, ContributionStatementList: already dispose, via SendFile's using, ZipArchive, and an explicit using.

Deploy notes

• Now that ConnectionCards saves generated images through ContentStream, Rock's save hook resizes them if the block's BinaryFileType sets a max width or height. Check that file type's limits before deploying. If it has limits, the sheet is downscaled before chopping.
• Ghostscript on the web nodes: ConnectionCards PDF upload depends on Ghostscript.NET 1.2.1 reading the page count. Locally that fails on Ghostscript 9.54 and 10.08 (see Testing). My best guess is that Ghostscript 9.50 made -dSAFER the default and broke the old wrapper, but I haven't verified that. Check the version under C:\Program Files\gs on the prod nodes. If it's 9.50 or newer, PDF upload is already broken in production independent of this PR: before, it threw a NullReferenceException; now it shows the "no pages" message. The likely fix, a Ghostscript.NET upgrade or pinning the Ghostscript version, belongs on its own ticket.
• WellrightRedirect and SignNowTest are compiled at runtime and need the new org.secc.DevLib.dll in RockWeb\Bin. The pipeline deploys it with the rest of the repo.

Testing

• DevLib, PDF, ConnectionCards, SignNowWorkflow, SafetyAndSecurity, Imaging and LeagueApps build clean (0 errors) against Rock 1.16.12.

• Rebased onto hotfix-1.16.12 after ROCK-9041 Stop SignNowDownload leaking the temp PDF file handle #321's squash merge. The resulting tree is identical to the previously tested one.

• ConnectionCardEntry.ascx.cs type-checks with the same error profile as master. The only extra errors are 3 references to the new nbError control, which is declared in the .ascx markup the probe doesn't compile.

• The two org.secc.Security .ascx.cs files aren't csproj Compile items. I type-checked them with Roslyn against RockWeb\Bin plus the new DevLib. The error profile is identical to master (only pre-existing missing-reference noise from the probe), so this PR adds no errors there.

• ConnectionCards local smoke test (2026-09-29, Rock 1.16.12 on LocalDB, IIS Express, this branch's DLLs and block): I drove the block with real WebForms postbacks, not by calling the code directly.

  Scenario	Storage	Result
  Rotate right → rotate left → chop 5×2	FileSystem	Sheet 816×1056 → 1056×816 → 816×1056. 10 cards, 369×172 each, read back from disk. Success message shown, source sheet deleted.
  Chop 5×2	Database	10 cards, 369×172 each, content in BinaryFileData, FileSize set. Success message shown.
  rows = 0 / −3 / 500, posted straight to the server so the UI minimum doesn't apply	both	Error message shown ("…must both be at least 1…", "…too fine for this 1056 x 816 pixel sheet"). Scan kept, still in edit mode, 0 cards.
  Zero-page PDF upload	Database	"The uploaded PDF has no pages to convert." shown. Upload kept, no image created, no exception.

  • 1056 px ÷ 5 rows is the uneven split that used to throw OutOfMemoryException. It now yields exactly 10 cards.
  • One no-op workflow launched per card (20 in total).
  • Rows and Columns render with a minimum of 1.
  • No new ExceptionLog rows from the plugin code.

• Not verified: converting a real PDF. The Ghostscript.NET 1.2.1 in RockWeb\Bin returns PageCount 0 for every PDF I tried, on both Ghostscript 10.08.0 and 9.54.0. That held via file path and via stream, for my hand-built PDFs and one written by Ghostscript itself, and with -dNOSAFER added. gswin64c reads the same files correctly. This is an existing environment issue, not a change in this PR; see Deploy notes.

• Still needs runtime checks: two PDFFormMerge actions on the same template in one workflow pass, PDFCombine of a file with itself, and ConnectionCards PDF → image conversion on a server whose Ghostscript works with Ghostscript.NET 1.2.1. Ideally run these on an Azure-backed file type.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Update the affected plugin documentation to describe storage-provider compatibility and buffering behavior.

Review effort: Lite
Findings: 3

Open (3)

• Document provider-backed binary processing support · New
• Document provider-compatible PDF processing · New
• Document provider-backed PDF template support · New

What changed in this PR

Remediates storage-provider stream handling across plugins by buffering content and disposing provider streams safely.

Changes:

• Supports non-Database providers across PDF, image, certificate, and SignNow workflows.
• Fixes Connection Cards persistence and stream positioning.
• Buffers inputs before handing them to downstream processors.

File	Summary
Plugins/​org.secc.SignNowWorkflow/​Workflows/​SignNowCreate.cs	Disposes provider streams after temporary-file copying.
Plugins/​org.secc.Security/​org_secc/​Security/​WellrightRedirect.ascx.cs	Safely reads and disposes certificate streams.
Plugins/​org.secc.Security/​org_secc/​Security/​SignNowTest.ascx.cs	Disposes streams after temporary-file creation.
Plugins/​org.secc.SafetyAndSecurity/​Workflows/​VolunteerApplicationMerge.cs	Buffers and disposes PDF template streams.
Plugins/​org.secc.SafetyAndSecurity/​Workflows/​MinorVolunteerApplicationMerge.cs	Buffers and disposes PDF template streams.
Plugins/​org.secc.SafetyAndSecurity/​Workflows/​MedicalIncidentReportMerge.cs	Buffers PDF templates before processing.
Plugins/​org.secc.SafetyAndSecurity/​Workflows/​ExternalChurchReferenceMerge.cs	Buffers and disposes PDF template streams.
Plugins/​org.secc.SafetyAndSecurity/​Workflows/​DigitalIncidentReportMerge.cs	Buffers PDF templates before processing.
Plugins/​org.secc.PDF/​Workflows/​PDFFormMerge.cs	Buffers template content before merging.
Plugins/​org.secc.PDF/​Workflows/​PDFCombine.cs	Buffers PDF inputs before iText processing.
Plugins/​org.secc.LeagueApps/​Utilities/​APIClient.cs	Safely reads and disposes certificate content.
Plugins/​org.secc.Imaging/​AI/​FaceCrop.cs	Disposes source photo streams after cropping.
Plugins/​org.secc.ConnectionCards/​Utilities/​ConnectionCardsUtilties.cs	Provides provider-safe PDF/image processing and persistence.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[jwakefield-secc]

Review notes

Verified ReadContentBytes against Rock 1.16.12.1 BinaryFile.SaveHook and the Database, FileSystem and Azure providers. The helper and the ContentStream writes are correct. Also checked upstream: Rock develop (as of 2026-09-14) and 1.17.0.33 still have the undisposed AzureBlobStorage.Upload and the same ContentStream cache, so secc/Rock#18 and this helper stay necessary on every upgrade.

Overall: safe to merge once rebased on #321. The only code item worth adding before merge is the cell-skip validation. Everything else is low or nit.

Merge order (do first) — Medium

• SignNowCreate.cs
  Problem: #321 (still open) now has commit 3250b07 that also rewrites this file's temp handling, but it still reads via renderedPDF.ContentStream.CopyTo. git merge-tree of the two heads reports a three-way conflict on SignNowCreate.cs only. Resolving it by taking #321's side drops the ReadContentBytes read, the exact pattern this PR removes.
  Fix: Rebase onto #321 and keep this PR's body (helper read plus try/finally) as the resolution.

New in this PR

• ConnectionCardsUtilties.cs:117 — Medium (low likelihood, data loss when it hits)
  Problem: The new continue silently skips cells. nbCols / nbRows are NumberUpDown controls with no Minimum or Maximum, so a value large enough to make a cell 4 px or smaller skips every cell, ChopImage returns an empty list, the block deletes the source scan, shows success, and launches no workflows. The old loop at least failed loudly with OutOfMemoryException. A value of 0 throws DivideByZeroException at sourceBitmap.Width / cols (pre-existing).
  Fix: Validate cols/rows >= 1 and elementWidth/elementHeight > 4 up front and throw with a message.

• BinaryFileExtensions.cs:46 — Low (theoretical, no caller hits it)
  Problem: The helper always reads from StorageProvider when one exists, so a tracked file with a reassigned but unsaved ContentStream returns the old stored bytes. The remarks only cover the unsaved-file case.
  Fix: Document the constraint (save before reading) in the remarks and README.

• ConnectionCardsUtilties.cs:113 — Nit
  Problem: Rectangle.Intersect can never clip. elementWidth = Width / cols (integer), so a cell's far edge is at most (col+1) * elementWidth <= Width. Only the <= 0 guard does work.
  Fix: Replace with the up-front size validation above and build the rectangle directly.

• ConnectionCardsUtilties.cs:58, 83, 131 — Nit
  Problem: FileSize = data.Length is overwritten by the save hook from the provider's reported size on both the Added and Modified paths.
  Fix: Drop the assignments.

Pre-existing, in a function this PR rewrote

• ConnectionCardsUtilties.cs:65 — Low (error page on a corrupt upload, no data loss, same as today)
  Problem: ConvertPDFToImage still returns new BinaryFile() (no MimeType, no content) when Ghostscript reports zero pages. The block only null-checks, adds it, and Rock's save hook throws on Entity.MimeType.StartsWith(...) (no null guard at BinaryFile.SaveHook.cs:61).
  Fix: Return null or throw a named InvalidOperationException so the caller's null check works.

[stphnlee]

Thanks, @jwakefield-secc. All addressed, and the branch is force-pushed to 423aac48.

Merge order / SignNowCreate.cs: rebased onto #321, and git merge-tree against its head is now clean. For the resolution I took #321's side rather than this PR's: its handling is more thorough (token first, JsonException catch, null-safe SDK result, Directory.Exists guard, AddLogEntry on cleanup failure). The only change on top is swapping ContentStream.CopyTo for File.WriteAllBytes( tempFile, renderedPDF.ReadContentBytes( "Rendered PDF" ) ) (ff51725). My own try/finally is gone, since #321 covers it. Please merge #321 first.

ChopImage cell skip (Medium): fixed in ebfbd52. Before anything is saved, ChopImage throws InvalidOperationException for fewer than 1 row or column (this also covers the pre-existing divide-by-zero) or for cells of 4 px or less. btnCrop_Click catches it, shows the message in a new nbError box, and returns before Delete, so the scan is kept. nbRows/nbCols also get Minimum="1".

Rectangle.Intersect (nit): agreed that it can't clip. It's removed, and the rectangle is built directly after the validation (ebfbd52).

Helper remark (Low): documented in the XML remarks and the DevLib README: save before calling ReadContentBytes if you've reassigned ContentStream on a tracked file (7a28d37).

Zero-page PDF (Low): ConvertPDFToImage returns null, and the block's else shows "The uploaded PDF has no pages to convert." (ebfbd52).

FileSize (nit): I kept these. On the Modified path the hook does overwrite unconditionally (SaveHook.cs:245). On the Added path it only overwrites when the provider returns a size (SaveHook.cs:173: if ( outFileSize.HasValue )). All three current providers return one, so it's redundant today, but it's a cheap guard for a provider that returns null. Happy to drop them if you feel strongly.

secc/Rock#18: agreed, and thanks for checking upstream. The PR description now says it's cherry-picked into our Rock build until upstream fixes it, and that this PR's helper complements it rather than replacing it.