[WIP] Implement secure, basic caching #2385
Conversation
This is step one to having a fully-better solution, but will greatly speed up sites for users accessing the same image multiple times. * **Improved file download security**: Added nonce verification for Gravity Forms file downloads accessed through GravityView to prevent unauthorized access and ensure downloads originate from legitimate requests. * **Enhanced file download caching**: Implemented intelligent caching for verified Gravity Forms file downloads, allowing browsers to cache files for up to one year while maintaining security through nonce validation.
WalkthroughThe update introduces new logic to the GravityView plugin to securely handle Gravity Forms file downloads by appending and verifying nonces in download URLs, and by adjusting cache headers to allow caching for up to 48 hours. The readme was updated to reflect these performance and security improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GravityView
participant Gravity Forms
User->>GravityView: Requests file download via View
GravityView->>GravityView: Generates download URL with nonce
User->>Gravity Forms: Accesses file download URL (with nonce)
Gravity Forms->>GravityView: Triggers hook to check nonce
GravityView->>GravityView: Verifies nonce and request
GravityView->>Gravity Forms: Allows file download, sets cache headers
Gravity Forms->>User: Serves file with updated cache headers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
readme.txt (1)
27-27: Avoid redundant “embedded … embedded” phrasing in changelogThe sentence reads awkwardly:
“secure embedded file image downloads embedded in Views”.Consider trimming the second “embedded”:
-* Improved performance of Gravity Forms secure embedded file image downloads embedded in Views by adding caching, while also maintaining security. +* Improved performance of secure Gravity Forms file-image downloads in Views by adding caching, while also maintaining security.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
includes/plugin-and-theme-hooks/class-gravityview-plugin-hooks-gravity-forms.php(2 hunks) -
readme.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: doekenorg
PR: GravityKit/GravityView#2213
File: .circleci/config.yml:54-55
Timestamp: 2025-07-07T15:15:36.761Z
Learning: In CircleCI configuration for GravityView, a daily cache invalidation strategy is intentionally used via a `.date` file in the cache key. This serves as a "safety net" to prevent cache corruption from persisting too long - providing a predictable way to clear potentially corrupted cache daily without waiting for dependency file changes. This is a deliberate trade-off between cache efficiency and cache reliability.
Learnt from: doekenorg
PR: GravityKit/GravityView#2213
File: readme.txt:24-47
Timestamp: 2025-07-08T13:00:10.726Z
Learning: In the GravityView repository, non-standard changelog headers (such as `= search update =`) may be intentionally used in readme.txt to facilitate merging from upstream `develop` updates, even if this breaks the WordPress.org readme parser's expectations.
Learnt from: doekenorg
PR: GravityKit/GravityView#2363
File: includes/widgets/search-widget/settings/class-search-widget-settings-visible-fields-only.php:5-5
Timestamp: 2025-07-07T12:52:36.945Z
Learning: In GravityView development, version placeholders like `$ver$`, `TBD`, or `TODO` are intentionally used in `@since` tags for new features that will be released in future versions when the actual version number is currently unknown. This is their standard practice for unreleased features.
Learnt from: doekenorg
PR: GravityKit/GravityView#2213
File: includes/search/fields/class-search-field-created-by.php:94-94
Timestamp: 2025-07-08T13:01:06.162Z
Learning: When reviewing @since tags in GravityView code, preserve the original version number for existing filters, hooks, or functionality that has been moved from another location, even if the surrounding code is new and uses $ver$ placeholders. Only new functionality should use the $ver$ placeholder.
readme.txt (3)
undefined
<retrieved_learning>
Learnt from: doekenorg
PR: #2213
File: readme.txt:24-47
Timestamp: 2025-07-08T13:00:10.726Z
Learning: In the GravityView repository, non-standard changelog headers (such as = search update =) may be intentionally used in readme.txt to facilitate merging from upstream develop updates, even if this breaks the WordPress.org readme parser's expectations.
</retrieved_learning>
<retrieved_learning>
Learnt from: doekenorg
PR: #2363
File: includes/widgets/search-widget/settings/class-search-widget-settings-visible-fields-only.php:5-5
Timestamp: 2025-07-07T12:52:36.945Z
Learning: In GravityView development, version placeholders like $ver$, TBD, or TODO are intentionally used in @since tags for new features that will be released in future versions when the actual version number is currently unknown. This is their standard practice for unreleased features.
</retrieved_learning>
<retrieved_learning>
Learnt from: doekenorg
PR: #2213
File: includes/search/fields/class-search-field-created-by.php:94-94
Timestamp: 2025-07-08T13:01:06.162Z
Learning: When reviewing @SInCE tags in GravityView code, preserve the original version number for existing filters, hooks, or functionality that has been moved from another location, even if the surrounding code is new and uses
</retrieved_learning>
includes/plugin-and-theme-hooks/class-gravityview-plugin-hooks-gravity-forms.php (4)
Learnt from: doekenorg
PR: GravityKit/GravityView#2213
File: includes/search/fields/class-search-field-created-by.php:94-94
Timestamp: 2025-07-08T13:01:06.162Z
Learning: When reviewing @since tags in GravityView code, preserve the original version number for existing filters, hooks, or functionality that has been moved from another location, even if the surrounding code is new and uses $ver$ placeholders. Only new functionality should use the $ver$ placeholder.
Learnt from: doekenorg
PR: GravityKit/GravityView#2213
File: includes/plugin-and-theme-hooks/class-gravityview-plugin-hooks-gravity-flow.php:0-0
Timestamp: 2025-07-10T07:45:44.881Z
Learning: In GravityView filter hooks that expect array returns, when validating multiple parameters, handle them separately: return empty array if the main data parameter is invalid (to maintain contract), but return original data unchanged if auxiliary parameters are invalid (to preserve existing data flow).
Learnt from: doekenorg
PR: GravityKit/GravityView#2363
File: includes/widgets/search-widget/settings/class-search-widget-settings-visible-fields-only.php:5-5
Timestamp: 2025-07-07T12:52:36.945Z
Learning: In GravityView development, version placeholders like `$ver$`, `TBD`, or `TODO` are intentionally used in `@since` tags for new features that will be released in future versions when the actual version number is currently unknown. This is their standard practice for unreleased features.
Learnt from: doekenorg
PR: GravityKit/GravityView#2213
File: includes/search/fields/class-search-field-gravity-forms.php:332-388
Timestamp: 2025-07-08T13:10:28.236Z
Learning: In GravityView search fields, meta keys like 'workflow_step', 'created_by', 'is_approved' are valid non-numeric field identifiers that can have choices and support sieving functionality. Field ID validation should accommodate both numeric field IDs (1, 2, 3.1) and string-based meta field IDs.
🪛 PHPMD (2.15.0)
includes/plugin-and-theme-hooks/class-gravityview-plugin-hooks-gravity-forms.php
132-132: Avoid unused parameters such as '$context'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test_and_package
🔇 Additional comments (4)
includes/plugin-and-theme-hooks/class-gravityview-plugin-hooks-gravity-forms.php (4)
21-46: Well-defined constants for secure download functionality.The constants are appropriately named and follow GravityView's conventions. The @SInCE TODO placeholders are correct for unreleased features.
101-105: Proper hook registration.The method correctly calls the parent and registers the necessary action hook at an appropriate time.
114-121: Secure implementation of download validation.The method properly validates both the download parameter and nonce. The use of
GV\Utils::get()provides safe parameter retrieval, and the nonce verification adds the necessary security layer.
88-94: Constructor timing is safe as implementedThe call to is_gf_gv_download() and subsequent nonce check in the constructor is safe:
- WordPress pluggable functions (including wp_verify_nonce) are loaded before plugins are instantiated.
- The global $_GET array is fully available during plugin load.
- Registering the nocache_headers filter conditionally in the constructor ensures it’s only added when needed, without any lifecycle issues.
You can safely keep the current implementation without deferring to a later hook.
Likely an incorrect or invalid review comment.
| public static function remove_nocache_headers_from_gf_download( $headers ) { | ||
| // Sanity check (this shouldn't be called if it's not already a GF download from GV). | ||
| if ( ! self::is_gf_gv_download() ) { | ||
| return $headers; | ||
| } | ||
|
|
||
| // Nonces are valid for 24-48 hours. | ||
| $max_age = DAY_IN_SECONDS * 2; | ||
|
|
||
| // Add caching headers to allow caching for as long as the nonce is valid. | ||
| $headers['Cache-Control'] = 'max-age=' . $max_age . ', public, immutable'; | ||
| $headers['Expires'] = gmdate( 'D, d M Y H:i:s', time() + $max_age ) . ' GMT'; | ||
|
|
||
| return $headers; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove conflicting nocache headers before setting cache headers.
The method adds cache headers but doesn't remove existing nocache headers that might conflict. This could lead to inconsistent caching behavior.
public static function remove_nocache_headers_from_gf_download( $headers ) {
// Sanity check (this shouldn't be called if it's not already a GF download from GV).
if ( ! self::is_gf_gv_download() ) {
return $headers;
}
+ // Remove conflicting nocache headers
+ unset( $headers['Cache-Control'] );
+ unset( $headers['Expires'] );
+ unset( $headers['Pragma'] );
+
// Nonces are valid for 24-48 hours.
$max_age = DAY_IN_SECONDS * 2;
// Add caching headers to allow caching for as long as the nonce is valid.
$headers['Cache-Control'] = 'max-age=' . $max_age . ', public, immutable';
$headers['Expires'] = gmdate( 'D, d M Y H:i:s', time() + $max_age ) . ' GMT';
return $headers;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function remove_nocache_headers_from_gf_download( $headers ) { | |
| // Sanity check (this shouldn't be called if it's not already a GF download from GV). | |
| if ( ! self::is_gf_gv_download() ) { | |
| return $headers; | |
| } | |
| // Nonces are valid for 24-48 hours. | |
| $max_age = DAY_IN_SECONDS * 2; | |
| // Add caching headers to allow caching for as long as the nonce is valid. | |
| $headers['Cache-Control'] = 'max-age=' . $max_age . ', public, immutable'; | |
| $headers['Expires'] = gmdate( 'D, d M Y H:i:s', time() + $max_age ) . ' GMT'; | |
| return $headers; | |
| } | |
| public static function remove_nocache_headers_from_gf_download( $headers ) { | |
| // Sanity check (this shouldn't be called if it's not already a GF download from GV). | |
| if ( ! self::is_gf_gv_download() ) { | |
| return $headers; | |
| } | |
| // Remove conflicting nocache headers | |
| unset( $headers['Cache-Control'] ); | |
| unset( $headers['Expires'] ); | |
| unset( $headers['Pragma'] ); | |
| // Nonces are valid for 24-48 hours. | |
| $max_age = DAY_IN_SECONDS * 2; | |
| // Add caching headers to allow caching for as long as the nonce is valid. | |
| $headers['Cache-Control'] = 'max-age=' . $max_age . ', public, immutable'; | |
| $headers['Expires'] = gmdate( 'D, d M Y H:i:s', time() + $max_age ) . ' GMT'; | |
| return $headers; | |
| } |
🤖 Prompt for AI Agents
In
includes/plugin-and-theme-hooks/class-gravityview-plugin-hooks-gravity-forms.php
around lines 155 to 169, the method sets cache headers but does not remove
existing nocache headers that can conflict and cause inconsistent caching. To
fix this, identify and remove any nocache-related headers such as 'Pragma',
'Cache-Control' with no-cache directives, and 'Expires' before setting the new
cache headers. This ensures no conflicting headers remain and caching behavior
is consistent.
| public function add_query_arg_to_gf_download_url( $context ) { | ||
|
|
||
| /** | ||
| * Adds the View ID to the Gravity Forms download URL. | ||
| * | ||
| * @param string $url The existing Gravity Forms download URL. | ||
| * | ||
| * @return string The new Gravity Forms download URL, with the View ID added. | ||
| */ | ||
| add_filter( 'gform_secure_file_download_url', function( $url ) { | ||
| return wp_nonce_url( $url, self::NONCE_ACTION, self::QUERY_ARG_NONCE ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address unused parameter and potential multiple filter registrations.
The method has two issues:
- The
$contextparameter is unused (flagged by static analysis) - The filter is added inside the method, which could lead to multiple registrations if called repeatedly
- public function add_query_arg_to_gf_download_url( $context ) {
+ public function add_query_arg_to_gf_download_url() {
+ // Prevent multiple filter registrations
+ if ( has_filter( 'gform_secure_file_download_url', [ $this, 'add_nonce_to_gf_url' ] ) ) {
+ return;
+ }
- /**
- * Adds the View ID to the Gravity Forms download URL.
- *
- * @param string $url The existing Gravity Forms download URL.
- *
- * @return string The new Gravity Forms download URL, with the View ID added.
- */
- add_filter( 'gform_secure_file_download_url', function( $url ) {
- return wp_nonce_url( $url, self::NONCE_ACTION, self::QUERY_ARG_NONCE );
- } );
+ add_filter( 'gform_secure_file_download_url', [ $this, 'add_nonce_to_gf_url' ] );
+ }
+
+ /**
+ * Adds nonce to Gravity Forms download URL.
+ *
+ * @param string $url The existing Gravity Forms download URL.
+ *
+ * @return string The new Gravity Forms download URL, with nonce added.
+ *
+ * @since TODO
+ */
+ public function add_nonce_to_gf_url( $url ) {
+ return wp_nonce_url( $url, self::NONCE_ACTION, self::QUERY_ARG_NONCE );
}🧰 Tools
🪛 PHPMD (2.15.0)
132-132: Avoid unused parameters such as '$context'. (Unused Code Rules)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In
includes/plugin-and-theme-hooks/class-gravityview-plugin-hooks-gravity-forms.php
around lines 132 to 144, the method add_query_arg_to_gf_download_url has an
unused $context parameter and adds a filter inside the method, risking multiple
registrations if called repeatedly. Remove the unused $context parameter from
the method signature and move the add_filter call outside the method so it is
registered only once, for example in the class constructor or an initialization
method.
mrcasual
changed the title
This is step one to having a fully-better solution, but will greatly speed up sites for users accessing the same image multiple times.
@mrcasual
Summary by CodeRabbit