Forms: honor webhook destinations for template, template-part and widget forms#49861
Conversation
…get forms The 15.9 SSRF/exfiltration hardening (should_honor_content_destinations) only honored content-declared destinations when the source post's author had `manage_options`. Forms living in block templates, template parts and widgets have a non-numeric source id and no post author, so the function bailed at its `is_numeric()` guard and reconcile_content_destinations() silently wiped their webhooks, postToUrl and Salesforce destinations. Editing any of those surfaces already requires the administrator-tier `edit_theme_options` capability — strictly above what an Editor/Author/ Contributor holds — so destinations declared there are trusted. Pass the source type through and honor block_template, block_template_part and widget sources. The source type is established server-side (signed JWT, or a re-render of the real template on the legacy path) and is not forgeable by the submitter, so this does not reopen the hole for post-content forms authored by non-admins, which remain gated on the author capability. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 4 files.
|
The test helper now also builds template/widget sources, whose ids are non-numeric strings. Phan flagged passing 'mytheme//page' to an `int` parameter; a source id is genuinely int|string (matching Feedback_Source::get_id()), so widen the phpdoc to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The set {block_template, block_template_part, widget} was duplicated as a
literal in should_honor_content_destinations() and again in the test data
provider, with no single source of truth — a new admin-tier source type
could be added to Feedback_Source and silently miss the trust gate.
Define Feedback_Source::ADMIN_TIER_SOURCE_TYPES (on the class that owns
source types), reference it from the gate, and loop the test over it. The
data_admin_tier_source_types provider is no longer needed — the test now
covers exactly the constant's contents, including any future additions.
No behaviour change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
| public static function should_honor_content_destinations( $source_id ) { | ||
| public static function should_honor_content_destinations( $source_id, $source_type = 'single' ) { | ||
| // Block templates, template parts and widgets can only be authored by users with the | ||
| // administrator-tier `edit_theme_options` capability, so their destinations are trusted |
There was a problem hiding this comment.
Big assumption. I'll agree to it, but seems risky to use a derived capability
|
This reopens the dropped SSRF/exfiltration vector for
Submit → Validating test (fails on this branch, passes once the type is server-anchored): /**
* An Editor must not be able to promote a post-content form to an admin-tier
* source by spoofing the block_template / block_template_part attribute.
* Regression guard for the destination-authorization hardening.
*/
public function test_content_supplied_template_markers_are_not_trusted() {
unset( $GLOBALS['grunion_block_template_part_id'] ); // no real template/part render
foreach ( array( 'block_template', 'block_template_part' ) as $marker ) {
$source = Feedback_Source::get_current(
array(
$marker => 'mytheme//evil',
'webhooks' => array(
array(
'webhook_id' => 'w',
'url' => 'https://attacker.example/x',
'enabled' => true,
'format' => 'json',
'method' => 'POST',
),
),
)
);
$this->assertFalse(
\Automattic\Jetpack\Forms\Jetpack_Forms::should_honor_content_destinations(
$source->get_id(),
$source->get_source_type()
),
"A content-supplied $marker attribute must not be trusted as an admin-tier source."
);
}
}Fix — key the source type on server render state, not the attribute:
With the type server-anchored, the |
|
Follow-up review by an agent, adding to the analysis above.
Anchoring the type at Test note. Calling |
…bute The previous commit trusted block_template / block_template_part source types when deciding whether to honor content-declared destinations. But Feedback_Source::get_current() derived those types from a form *content* attribute, which a post author (who need not hold edit_theme_options) can set on a form in ordinary post content. That made the admin-tier trust reachable from non-admin-authored content — reopening the very vector the destination-authorization gate exists to close. The signed JWT did not help: the token is honestly signed from whatever attributes were present at render time. Derive the two template source types from render-scoped globals instead: - block_template_part: key get_current() on the existing $GLOBALS['grunion_block_template_part_id'], which is set only while a template part actually renders (the content attribute is now ignored for type purposes; it is still populated for form-id computation). - block_template: set a new $GLOBALS['grunion_block_template_id'] in the canvas injector when a block template renders, and suspend it while core/post-content renders so a form in the post body is attributed to the post (and gated on the post author), mirroring the template-part lifecycle. widget is unchanged: parse() already overwrites the widget attribute with the server-resolved context before the form is built. Adds a regression test that a content-supplied template marker is not trusted, a positive test for the render-anchored path, and rebuilds the keep-destinations test through that path rather than a synthetic source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collapses the repeated set-global / get_current() / unset dance in the render-anchored source tests into one helper, centralizing the global lifecycle. No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Implemented the render-anchoring you both described — thanks for the catch, and for the detailed follow-up. What changed (latest on the branch, c2a1950):
On the Tests: a guard that a content-supplied |
CGastrell
left a comment
There was a problem hiding this comment.
Approving. Verified the render-anchoring on the branch, not just the description.
get_current()now derivesblock_template/block_template_partfrom render-scoped globals, never from content attributes — so a post-body form (Editor, noedit_theme_options) resolves tosingleand stays gated on the post author. Closes the spoof on both the legacy and JWT paths, and fails closed (any missing or aborted render leaves the type atsingle).- The
block_templatepost-content suspend/restore bracket (pre_render_block/render_block+ stack) is correct for sequential, nested and looped post-content, and is guarded against an empty stack. - Legit delivery is genuinely restored: a form in a real template part sets
grunion_block_template_part_idat render → honored. Confirmed against the original ticket's footer template-part form. - Tests cover the guard (
test_content_supplied_template_markers_are_not_trusted), the positive render-anchored path, and the rebuilt keep-destinations case.
Non-blocking: webhook delivery on free sites configured via raw block attributes returns to pre-15.9 behavior — if runtime should be paid-gated, that's a separate decision. The now-redundant parse() bridge at class-contact-form.php:1225 is fine to defer to a follow-up.
LGTM once CI is green.
The code-coverage check flagged the new render-anchoring helpers in class-util.php (the core/post-content suspend/restore bracket and the canvas-injector global) as uncovered, because the existing tests set the render-scoped globals directly and call get_current(). Cover them directly: suspend/restore around core/post-content (including the nested-post-content case that justifies the stack), the canvas injector marking the block_template global only for template-canvas.php, and the two new hook registrations. Also add a get_current() widget-branch test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Break the two intentional adjacent suspend() calls in the nested post-content test with an intermediate assertion (also a clearer test). - Use the null-coalescing operator instead of isset()-ternaries when saving the template globals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `global $_wp_current_template_content, $_wp_current_template_id;` declaration already defines both (null if unset), so `?? null` is unnecessary (PhanCoalescingNeverUndefined). Assign them directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Proposed changes
Jetpack 15.9 added capability hardening to outbound form destinations (
should_honor_content_destinations): webhooks, the legacy Post to URL, and the Salesforce integration are only honored when the form was placed by an admin-capable author. That gate bailed out for any form whose source id is not a numeric post — which is the case for forms living in block templates, block template parts, and widgets — silently dropping their destinations. Webhooks for those forms stopped firing as of 15.9.This PR honors those three source types. Editing a block template, template part, or widget already requires the administrator-tier
edit_theme_optionscapability — strictly above what an Editor/Author/Contributor holds — so destinations declared there are trusted.Jetpack_Forms::should_honor_content_destinations()now takes the source type and returnstrueforblock_template,block_template_part, andwidgetsources, before the numeric-id guard.Feedback_Source::get_source_type()and pass the type through fromreconcile_content_destinations().manage_optionscheck is unchanged: post-content forms authored by non-admins remain gated.Why this is safe
The source type is established server-side and is not forgeable by the submitter: on the JWT submission path the form (attributes + source) is rebuilt from the signed token; on the legacy path the destinations are loaded from the real server-side template/post-meta, never from request input. A non-admin cannot smuggle a destination into a "trusted" source type, because reaching one requires
edit_theme_options. This does not reopen the exfiltration/SSRF vector the original hardening closed.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
content_destinations_dropped/author_unauthorizedlog entry is recorded.jetpack test php packages/formsstays green.🤖 Generated with Claude Code