fix: redact signed-link signatures from the log, keeping Gravity PDF's own query arguments - #1737
Merged
Merged
Conversation
A signed PDF download logged its request URI, a relative path whose query string holds the link's signature. The log redactor only blanked query strings on absolute http(s) URLs, and the 64-hex signature is longer than its hex pattern, so anyone with the log file could download that PDF until the link expired. scrub() now blanks the query string of any whitespace-delimited token holding a / before its first ?, so relative and protocol-relative links lose it too. It matches only from a token's start: an earlier draft that could start at every / took a second on a 100 KB slash-heavy string with PCRE's JIT off, and this takes milliseconds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cting logs Blanking a logged URL's whole query string hid what a request was for. With plain permalinks a PDF link is /?gpdf=1&pid=...&lid=...&action=download, so the log could not say which PDF was requested, or whether a rejected signed link had simply expired. The redactor now keeps each argument's name and masks its value, except for gpdf, pid, lid, action, print and expires, which hold no secret. An argument it does not know by name (signature, token, X-Amz-Signature, ...) is still masked, so it fails closed. Separators an escaped URL uses (&, &) are kept as they are. The hex pattern now covers 28 to 128 characters rather than 28 to 40, so a bare 64-hex signature logged without a path in front of it is masked too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1
added a commit
that referenced
this pull request
Sep 21, 2026
Ports the 6.17.1 release (6.17..6.17.1) to development: - keep mPDF's cache folders through tmp cleanup, and keep font metrics for a week (#1731) - stop concurrent PDFs failing with "Temporary files directory is not writable" (#1733) - note a PDF left off a notification on the entry, linked to its settings and authored as the notification, with the Gravity PDF logo as its avatar (#1732, #1736) - log generation errors with form/entry/PDF IDs instead of the whole object (#1732) - licensing environment type, inactive license status and dead update packages (#1734) - tag mPDF's log records with the form, entry and PDF they belong to (#1738) - keep PDF URL paths and safe query args when redacting logs (#1737) - retry license reactivation soon when the store gives no verdict (#1739) Adapted to development: tests moved to tests/phpunit/integration on the shared TestCase, Context_Logger uses the scoped GFPDF_Vendor\Psr\Log, the generation-error log context lives in development's refactored Model_PDF::process_and_save_pdf(), and pdf_id defaults to '' for a Helper_PDF built without a PDF ID. Adds the 6.17.1 changelog section. Left out: the version bump and the wp-env 11 CI change (development fixed the Debian 11 build its own way in #1724). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1
added a commit
that referenced
this pull request
Sep 21, 2026
Ports the 6.17.1 release (6.17..6.17.1) to development: - keep mPDF's cache folders through tmp cleanup, and keep font metrics for a week (#1731) - stop concurrent PDFs failing with "Temporary files directory is not writable" (#1733) - note a PDF left off a notification on the entry, linked to its settings and authored as the notification, with the Gravity PDF logo as its avatar (#1732, #1736) - log generation errors with form/entry/PDF IDs instead of the whole object (#1732) - licensing environment type, inactive license status and dead update packages (#1734) - tag mPDF's log records with the form, entry and PDF they belong to (#1738) - keep PDF URL paths and safe query args when redacting logs (#1737) - retry license reactivation soon when the store gives no verdict (#1739) Adapted to development: tests moved to tests/phpunit/integration on the shared TestCase, Context_Logger uses the scoped GFPDF_Vendor\Psr\Log, the generation-error log context lives in development's refactored Model_PDF::process_and_save_pdf(), and pdf_id defaults to '' for a Helper_PDF built without a PDF ID. Adds the 6.17.1 changelog section. Left out: the version bump. Also takes 6.17.1's wp-env upgrade (^11.15.0, which repoints Debian 11 sources at archive.debian.org itself) in place of #1724's tools/wp-env/patch-bullseye-apt.mjs, which the yarn wp-env scripts no longer run. Co-Authored-By: Claude Opus 5 (1M context) <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.
Signed PDF download links leaked their signature into the Gravity PDF log. Each signed request logs a
Valid PDF Signing Request(orInvalid …) entry, and itsrequest_urifield held the full?expires=…&signature=…query string. Anyone with the log file, including anyone it was shared with for support, could download that PDF until the link expired.The log redactor now masks query-string values in any link or path, relative or absolute. It keeps each argument's name, and it keeps the values of Gravity PDF's own arguments (
gpdf,pid,lid,action,print,expires), because none of them is secret. That means the log can still show which PDF was requested when permalinks are turned off, and whether a rejected signed link had expired. Any other argument's value is masked, including ones the redactor doesn't know by name.Try it
With Gravity Forms logging enabled for Gravity PDF, open a signed PDF link, then check the newest entry in the Gravity PDF log:
The
Valid PDF Signing Requestentry logsrequest_urias/pdf/<pid>/<lid>/download/?expires=…&signature=[redacted]. With plain permalinks it is/?gpdf=1&pid=…&lid=…&action=download&expires=…&signature=[redacted]. On 6.17.0 the signature is logged in full.Test plan
request_uriandurlwithsignature=[redacted], andexpiresintactgpdf,pid,lidandaction?token=…) is masked, and a plain question mark in a message is left aloneTest_Redact_ProcessorpassesMore info
Why it leaked.
Redact_Processor::scrub()only blanked query strings after an absolutehttp(s)://URL, andrequest_uriis a relative path. The 64-hex signature was also longer than the 28–40 hex pattern, so nothing caught it.URL rule.
scrub()matches#(?<!\S)(?=[^\s?]*/)([^\s?]*+)\?(\S*)#, which finds a whitespace-delimited token with a/before its first?. That covers absolute, root-relative (/pdf/…?) and protocol-relative (//host/…?) links, and a path insidekey="…".mask_query_values()splits the query on&,&and&, keeping each separator, so escaped URLs fromesc_url()are handled and the output matches the input byte for byte apart from masked values. For each argument:SAFE_QUERY_ARGS(case-insensitive): kept as istoken=): kept, since there is nothing to hide?abc123): maskedname=[redacted]scrub()runs on every log message and on every string leaf in the context, so it covers any caller that logs a link, not onlyModel_PDF::middle_signed_url_access().Why an allowlist. A list of sensitive names fails open:
X-Amz-Signature,GoogleAccessIdor a plugin's own token name would be logged in full. Masking every value except a short list of Gravity PDF's own fails closed. The list is a private constant with no filter, because a filter would let any add-on unmask a name likesignaturefor every logger.Hex pattern. It widens from 28–40 to 28–128 characters, so a bare 64-hex signature logged without a path in front of it (e.g.
signature=…&expires=…on its own) is masked too.Behaviour change for absolute URLs. Since 6.16 an absolute URL lost its whole query string (
…file.zip?). It now keeps argument names (…file.zip?token=[redacted]&exp=[redacted]).Performance. The URL pattern starts a match only at a token's start and scans it possessively, which keeps it linear. A draft that could start at any
/took about a second on a 100 KB slash-heavy string withpcre.jitoff. With masking, a 560 KB string holding 5,000 signed links takes about 3 ms with the JIT on and 8 ms with it off.Tests.
test_masks_url_query_valuescovers root-relative, plain-permalink, protocol-relative, mid-sentence and quoted links, an unknown secret name, a safe name in another case, a value with no name, an empty value, escaped separators, an empty query and a plain question mark.provider_message_patternsadds a bare 64-hex signature.Test_Redact_Processor, the logging tests,Test_EDD_SL_Plugin_Updater,Test_Addon,Test_SettingsandTest_PDFpass, and PHPCS is clean. Checked on a local site with a real signed download, with pretty and plain permalinks.Signatures written to existing log files before this change stay there. They expire with their links.