ROCK-9041 Stop SignNowDownload leaking the temp PDF file handle

[stphnlee]

Summary

Plugin-side companion to the core fork patch for ROCK-9041 (secc/Rock#18). The ticket's plugin audit flagged SignNowDownload as the same open-FileStream-then-delete pattern that broke the Process Signature Documents job on the Azure provider. Review follow-ups also harden the rest of the download path.

Changes in org.secc.SignNowWorkflow/Workflows/SignNowDownload.cs

• Read the downloaded PDF into memory (File.ReadAllBytes → MemoryStream) instead of assigning an open FileStream to BinaryFile.ContentStream. No file handle is held, so the fix does not depend on whichever storage provider happens to dispose the stream.
• Per-call temp directory. Every run used to share %TEMP%\{document_name}.pdf, and document names repeat (the PDF merge template name, people with the same name), so two workflows running at once could store each other's signed PDF. The download now goes to Path.GetTempPath() + Path.GetRandomFileName(), following the pattern SignNowCreate already uses.
• Check the download result. The SDK returns the error body instead of a file path when SignNow does not return 200. The action now adds an error message rather than throwing FileNotFoundException or reading a stale file. It also catches non-JSON error bodies and I/O failures.
• Clean up before any database work. The temp directory is removed in a finally right after the bytes are read. A failed delete is logged to the action log and does not fail the action. Before, cleanup deleted {name} while the SDK wrote {name}.pdf, so temp PDFs piled up on every poll.
• Save in both branches. The existing-BinaryFile branch now calls SaveChanges() too. For persisted workflows, WorkflowService.Process already saved this content at the end of processing; the explicit save covers non-persisted workflows and surfaces storage-provider errors inside the action. After the save only attribute values are set, so no file I/O can throw after the commit.
• File type fallback. A new BinaryFile with no BinaryFileTypeId has its content silently dropped by BinaryFile's save hook. If the Document attribute has no binaryFileType qualifier, or it names a missing type, the action now uses the Default file type. A missing Document attribute or file type now returns an error instead of throwing an NRE.
• SignNow error on Document.Get. A missing signatures array (deleted document, rejected token) now returns an error instead of throwing ArgumentNullException.
• .pdf extension is now added to the stored file name.
• Dead code removed: unused PersonAliasService, a redundant null check, an unused Guid.Empty assignment, and a stray comment.

Review follow-ups (SignNowSDK / RestSharp 105 behavior)

RestSharp 105 never throws on transport failures, so the SDK returns null or the raw error body instead of the expected object.

• Document.Get, Download, and Create results are treated as untyped. Each is read with as JObject, null reports "No response from SignNow.", and array or string error bodies are kept in the error text. Get and Create are now wrapped in a JsonException catch for non-JSON bodies (e.g. gateway error pages). The dead WebException filter is removed.
• PDF check on download. Document.Download sends the request twice and saves the second body without a status check. A 401 or 429 on that call used to be stored as the signed document with PDF Signed = True. The action now requires the file to start with %PDF-; otherwise it errors so the workflow keeps polling. ArgumentNullException (the SDK saving a null body after a transport failure) is caught too.
• Temp directory delete is guarded with Directory.Exists, so a failed CreateDirectory doesn't add a misleading log entry.
• Existing-file branch resets MimeType to application/pdf, so a DOCX-sourced document isn't served as Word after its bytes become a PDF.

Changes in org.secc.SignNowWorkflow/Workflows/SignNowCreate.cs

• Temp upload directory is removed in a finally, so error returns and SDK exceptions no longer leave the unsigned PDF behind on each retry. The access token is now fetched before the file is written.
• Document.Create result handled like the others: null or a non-object body is an error message instead of a NullReferenceException, and non-JSON bodies are caught.

README Observations and the Components row are updated to match (Copilot comment). Last updated line added.

Follow-up, not in this PR: SignNowSDK.Document.Download already has the bytes (DownloadData) and sends the request twice. A byte[]-returning method in the secc/Rock SDK fork would remove the temp file entirely.

Testing

• org.secc.SignNowWorkflow.csproj builds clean with standalone MSBuild (0 errors, 0 warnings).
• Needs runtime check on dev: run a SignNow workflow from create through to a signed document. Confirm the Document attribute is populated, the file opens as .pdf and has a file type, and no leftover directories remain in the web server temp directory after either action.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Ensure temporary PDFs are deleted on failure and update the stale README documentation.

Review effort: Lite
Findings: 1

Open (1)

• Update stale README behavior and last-updated date · New

What changed in this PR

Updates SignNowDownload to prevent PDF stream leaks, persist re-downloaded documents, and clean up temporary files.

Changes:

• Reads PDFs into memory before storage.
• Saves updates to existing binary files.
• Deletes the correct .pdf temporary path.

File	Description
Plugins/​org.secc.SignNowWorkflow/​Workflows/​SignNowDownload.cs	Fixes stream handling, persistence, and temporary-file cleanup.

---

💡 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 against RestSharp 105.2.3, SignNowSDK Document.cs, and Rock 1.16.12.1 BinaryFile.SaveHook / GetFile.ashx. The core fix (MemoryStream + per-call temp dir + finally cleanup) is sound.

New in this PR

• SignNowDownload.cs:117
  Problem: RestSharp 105 never throws WebException. RestClient.Execute swallows every exception into ErrorException and returns Content = "", so the SDK returns null and the action logs "SignNow Download Error: " with nothing after it. The ex is System.Net.WebException filter is dead. Same blank message when SignNow returns a JSON-array error body, since as JObject yields null.
  Fix: Drop the WebException clause. Add a specific message when result is null ("No response from SignNow"). Stringify the raw dynamic result before the JObject cast so array bodies are preserved in the error text.

• SignNowDownload.cs:126
  Problem: finally calls Directory.Delete unconditionally. If Directory.CreateDirectory itself threw, the delete throws DirectoryNotFoundException and a misleading forced "Could not delete" log entry lands on top of the real error.
  Fix: Guard the delete with if ( Directory.Exists( tempDirectory ) ).

Pre-existing, but in the path this PR rewrites / claims to harden

• SignNowDownload.cs:109
  Problem: SignNowSDK.Document.Download sends the request twice and writes the second response's RawBytes to signed.pdf with no status check. A 401/429 on the second call saves the JSON error body as the PDF; the new File.Exists check passes, the bytes are stored, and PDF Signed is set to True, so the workflow stops polling. A transport failure leaves RawBytes null and File.WriteAllBytes(path, null) throws ArgumentNullException, outside the catch filter.
  Fix: After ReadAllBytes, check signedPdfBytes.Length >= 5 and that the first bytes are %PDF-; otherwise add an error message and return false. Add ArgumentNullException to the catch filter.

• SignNowDownload.cs:75
  Problem: Document.Get uses the same SDK pattern but sits outside the new catch. A non-JSON body (HTML gateway page) throws JsonReaderException; a JSON-array body throws RuntimeBinderException on the dynamic-to-JObject assignment. The signatures == null guard only covers object-shaped error bodies.
  Fix: Wrap the Get call in the same try/catch, assign to dynamic first, and use as JObject before reading signatures.

• SignNowDownload.cs:187
  Problem: The existing-file branch renames to .pdf and replaces the bytes but leaves MimeType unchanged. GetFile.ashx sends binaryFile.MimeType as Content-Type, so a DOCX-sourced document (the Document attribute allows FileFieldType, and SignNow converts on upload) comes back as PDF bytes served as Word.
  Fix: Set signedPDF.MimeType = "application/pdf" in the existing-file branch as well.

Out of scope, noting for a follow-up

• SignNowCreate.cs:90
  Problem: Directory.Delete runs only on the success path. The error return at line 87 and any SDK exception leave the unsigned PDF in a random temp directory on every retry, the same class of leak ROCK-9041 targets.
  Fix: Move the delete into a finally around the Document.Create call, matching the shape this PR adds to Download.

[stphnlee]

@jwakefield-secc Thanks. I checked each point against the SDK source and all six hold up, so they are all fixed in 3250b07:

• :117 dead WebException filter / blank message: Removed the WebException clause. The Download result is kept untyped until an as JObject cast, so array and string error bodies stay in the error text, and null reports "No response from SignNow."
• :126 delete after a failed CreateDirectory: The delete is now guarded with Directory.Exists.
• :109 second-request body saved as the PDF: After ReadAllBytes the file must start with %PDF-. Otherwise the action adds an error (with the first 500 characters of the body) and returns false, so PDF Signed isn't set and the workflow keeps polling. ArgumentNullException is added to the catch filter.
• :75 Document.Get outside the catch: The call is wrapped in a JsonException catch, and its result is kept as object and cast with as JObject before signatures is read, so neither non-JSON nor array bodies throw.
• :187 MimeType on the existing-file branch: Now set to application/pdf in that branch too.
• SignNowCreate.cs:90 (follow-up): Fixed in this PR. The temp directory delete is in a finally around the file write and Document.Create, and the Create result gets the same untyped handling, so null no longer throws at result.Value. The access token is fetched before the file is written.

The plugin builds clean (0 errors, 0 warnings). The README and PR description are updated. A runtime check on dev is still needed.