Fix File Storage Bugs - #326
Merged
Merged
Conversation
* ROCK-9041: Stop SignNowDownload leaking the temp PDF file handle
Read the downloaded SignNow PDF into a MemoryStream instead of assigning
an open FileStream to BinaryFile.ContentStream. The Azure provider did not
dispose the stream on save, so the temp file stayed locked and the
File.Delete afterwards threw. Using a MemoryStream removes the dependency
on the provider's dispose behavior entirely.
Also persist the updated content when the BinaryFile already exists (the
else branch assigned a stream but never called SaveChanges), and delete
the actual downloaded path ({name}.pdf) rather than {name}, which left
temp PDFs behind on every pass.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* ROCK-9041 Harden SignNowDownload temp-file handling per review
Download into a per-call temp directory, check the SDK result, and
delete the directory before any database work so concurrent runs can't
store each other's PDF and cleanup can't throw after the save. Fall back
to the Default file type so new files keep their content, return errors
instead of throwing on SignNow error responses, add the .pdf extension,
and remove dead code. Update the README to match.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* ROCK-9041 Handle SignNowSDK error results and clean up SignNowCreate temp files
RestSharp 105 never throws, so the SDK returns null or the raw error
body. Treat Document.Get/Download/Create results as untyped, report a
clear message for null, and catch non-JSON bodies. Reject downloads that
don't start with %PDF-, since the SDK saves its second response without
a status check. Guard the temp directory delete, reset MimeType on the
existing-file branch, and move SignNowCreate's temp cleanup into a
finally. Update the README to match.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…findings) (#323) * ROCK-9041: Make ConnectionCards utilities storage-provider agnostic ConvertPDFToImage and ChopImage wrote content only to DatabaseData, so under any non-Database provider no content was ever saved, and ChopImage read DatabaseData.Content directly (NullReferenceException for files in Azure). All three utilities now read through ContentStream into memory, dispose the provider stream before the caller deletes the source file, and assign a fresh MemoryStream to ContentStream so the provider gets the content. RotateImage also stops handing the provider a stream positioned at its end, which uploaded a zero-byte blob under Azure. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ROCK-9041: Dispose provider stream in FaceCrop Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ROCK-9041: Dispose provider stream when loading LeagueApps certificate Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ROCK-9041: Dispose provider streams in PDFCombine and PDFFormMerge Read the template bytes into memory and dispose the storage provider stream instead of handing iText a provider stream it never closes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ROCK-9041: Dispose provider streams in SafetyAndSecurity PDF merges Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ROCK-9041: Dispose provider streams in SignNowTest and WellrightRedirect Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ROCK-9041: Add BinaryFile.ReadContentBytes to DevLib Reads a BinaryFile's bytes through a fresh storage-provider stream and disposes it, leaving the entity's cached ContentStream untouched. Disposing ContentStream directly breaks later reads of the same tracked BinaryFile on Azure: the getter only re-fetches when CanSeek is false, and the Azure blob stream still reports CanSeek after Dispose. Null or empty content throws a named InvalidOperationException. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ROCK-9041: Read BinaryFile content via ReadContentBytes in PDF, SafetyAndSecurity, Imaging, LeagueApps, SignNowTest Replaces the using-around-ContentStream blocks, which left a disposed stream cached on the entity under Azure. Missing or empty files now fail with a named error everywhere instead of a NullReferenceException in some paths. LeagueApps no longer returns a null certificate and re-queries on every access. SignNowTest writes with File.WriteAllBytes, which truncates, so a leftover temp file can't leave trailing bytes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ROCK-9041: Fix ConnectionCards chop bounds and bitmap disposal; read via ReadContentBytes ChopImage now iterates exactly cols x rows cells and clamps each inset rectangle to the bitmap. The old step-until-edge loop added a partial column/row when the size wasn't evenly divisible, and Clone threw OutOfMemoryException. All bitmaps are disposed. Drops the private ReadAllBytes helper and corrects the RotateImage comment (the old code leaked the provider stream and Image; it did not upload a zero-byte blob). README updated for provider-agnostic reads and the save-hook resize caveat. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ROCK-9041: Read SignNowCreate's rendered PDF via ReadContentBytes Keeps #321's temp-directory handling (finally cleanup, SDK error handling) and only replaces the ContentStream.CopyTo read with the DevLib helper, so the cached stream on the tracked BinaryFile is never disposed or left open. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ROCK-9041: Stop persisting Wellright signing keys per request; read via ReadContentBytes The PFX was imported with PersistKeySet on every page load and never disposed, leaving a new private-key file in the machine key store each time. It now imports without PersistKeySet and disposes the certificate after signing. Exportable stays because CertificateUtility exports the key. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ROCK-9041: Document storage-provider support in PDF and SafetyAndSecurity READMEs Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ROCK-9041: Reject bad chop grids and zero-page PDFs in ConnectionCards instead of losing the scan ChopImage silently skipped cells when rows/columns made a cell 4px or smaller, so the block deleted the source scan, showed success and launched no workflows (the Rows/Columns NumberUpDowns had no bounds). It now throws InvalidOperationException for fewer than 1 row/column or cells of 4px or less, before anything is saved. The block shows the message and keeps the scan. Rows/Columns get Minimum="1". The no-op Rectangle.Intersect is removed. ConvertPDFToImage returns null for a PDF with no pages (an empty BinaryFile made the save hook throw on its null MimeType), and the block shows an error. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ROCK-9041: Document that ReadContentBytes reads stored bytes (save before reading) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * ROCK-9041: Document ConnectionCards grid validation and zero-page handling Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Release deployment may ship a stale DevLib assembly, and SignNowTest retains a shared temp-file race.
Review effort: Balanced
Findings: 1
Open (5)
What changed in this PR
Improves provider-agnostic binary-file handling and temporary-file cleanup across several Rock plugins.
Changes:
- Adds a shared
BinaryFile.ReadContentBytes()extension. - Updates PDF, image, certificate, and SignNow workflows to use safely buffered streams.
- Improves Connection Card validation, SignNow errors, project references, and documentation.
| File | Description |
|---|---|
SignNowDownload.cs |
Safely downloads, validates, stores, and cleans up signed PDFs. |
SignNowCreate.cs |
Uses provider-safe reads and reliable temp cleanup. |
SignNowWorkflow/README.md |
Documents revised SignNow behavior. |
SignNowWorkflow.csproj |
Adds DevLib reference. |
WellrightRedirect.ascx.cs |
Safely loads and disposes signing certificates. |
SignNowTest.ascx.cs |
Uses provider-safe document reads. |
VolunteerApplicationMerge.cs |
Safely reads PDF templates. |
MinorVolunteerApplicationMerge.cs |
Safely reads PDF templates. |
MedicalIncidentReportMerge.cs |
Buffers template data before iText processing. |
ExternalChurchReferenceMerge.cs |
Safely reads PDF templates. |
DigitalIncidentReportMerge.cs |
Buffers template data before iText processing. |
SafetyAndSecurity/README.md |
Documents storage-provider support. |
PDFFormMerge.cs |
Uses the shared binary-file reader. |
PDFCombine.cs |
Buffers both input PDFs safely. |
PDF/README.md |
Documents provider-agnostic input handling. |
PDF.csproj |
Adds DevLib reference. |
APIClient.cs |
Safely loads the LeagueApps certificate. |
FaceCrop.cs |
Safely buffers source photos before cropping. |
DevLib/README.md |
Documents the new extension. |
DevLib.csproj |
Includes the extension in the assembly. |
BinaryFileExtensions.cs |
Adds provider-agnostic binary-file reading. |
ConnectionCardsUtilties.cs |
Improves image storage, disposal, and grid validation. |
ConnectionCards/README.md |
Documents updated image behavior. |
ConnectionCards.csproj |
Adds DevLib reference. |
ConnectionCardEntry.ascx.cs |
Adds user-visible conversion and crop errors. |
ConnectionCardEntry.ascx |
Adds error UI and numeric minimums. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- ReadContentBytes: decide "unsaved" by Id == 0, not StorageProvider != null, so a new file that already has a storage type is read from ContentStream. - SignNowDownload: a Document attribute whose file type no longer exists is an error, not a silent fallback to the Default type; resolve the destination before downloading; accept the %PDF- header anywhere in the first 1024 bytes. - SignNowCreate: report a missing document, temp-file write failures, and failed signer/token/invite calls as error messages instead of throwing. - CertificateUtility: dispose the RSACryptoServiceProvider per request. - SignNowTest: per-request temp directory removed in a finally. - ConnectionCardEntry: keep the scan and show a message on any ChopImage failure. - FaceCrop: dispose the rotated source and target bitmaps. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…r changes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-lands #321 and #323 (ROCK-9041) against master:
BinaryFile.ReadContentBytesin DevLib for provider-agnostic reads, SignNow temp-file cleanup, the ConnectionCards grid fix, and the WellRight certificate key-store fix. Commits 0abd9dd and 2b2d1dd address the review findings on that code.Review fixes (0abd9dd)
ReadContentBytes: decides "unsaved" byId == 0instead ofStorageProvider != null, so a new file that already has a storage type (e.g. afterSetStorageEntityTypeId) is read fromContentStreamrather than an empty provider stream.binaryFileTypequalifier names a missing type is now an error; the Default type is used only when the qualifier is blank. Destination attribute and file type are resolved before the download.%PDF-is accepted anywhere in the first 1024 bytes.User.Create/RequestToken/Invitecalls are reported as error messages instead of NREs. (The SignNow document is still created before those calls, so a retry after one of them fails uploads a new document; the failure is now visible in the workflow log.)RSACryptoServiceProvideris disposed, completing the key-store fix.finally.ChopImagefailure (storage, corrupt image, GDI+) is logged and shown, keeping the scan.Docs (2b2d1dd)
Security, SignNowWorkflow, Imaging and LeagueApps READMEs updated for the DevLib dependency and the behavior changes.
Deploy notes
org.secc.DevLib.dllwith the plugin DLLs. PDF, SafetyAndSecurity, SignNowWorkflow, ConnectionCards, Imaging and LeagueApps callReadContentBytes, and the RockWeb-compiled Security blocks (WellrightRedirect,SignNowTest) import it, so an old DevLib DLL throwsMissingMethodException/ fails block compilation.bin\Debugpost-build lines in the diff. The branch does not touch those lines, so the merge keeps master's$(TargetDir)version (verified withgit merge-tree).🤖 Generated with Claude Code