| @@ -1,8 +1,9 @@ | ||
| 1 | 1 | <?php |
| 2 | 2 | |
| 3 | 3 | namespace JFB_Modules\Media_Cleanup; |
| 4 | 4 | |
| 5 | +use Jet_Form_Builder\Classes\Tools; | |
| 5 | 6 | use JFB_Components\Module\Base_Module_It; |
| 6 | 7 | |
| 7 | 8 | // If this file is called directly, abort. |
| 8 | 9 | if ( ! defined( 'WPINC' ) ) { |
| @@ -10,8 +11,11 @@ | ||
| 10 | 11 | } |
| 11 | 12 | |
| 12 | 13 | class Module implements Base_Module_It { |
| 13 | 14 | |
| 15 | + /** @var Media_Ownership_Migration_Notice */ | |
| 16 | + private $media_ownership_migration_notice; | |
| 17 | + | |
| 14 | 18 | public function rep_item_id() { |
| 15 | 19 | return 'media-cleanup'; |
| 16 | 20 | } |
| 17 | 21 | |
| @@ -18,9 +22,12 @@ | ||
| 18 | 22 | public function condition(): bool { |
| 19 | 23 | return true; |
| 20 | 24 | } |
| 21 | 25 | |
| 22 | - public function init_hooks() {} | |
| 26 | + public function init_hooks() { | |
| 27 | + $this->media_ownership_migration_notice = new Media_Ownership_Migration_Notice(); | |
| 28 | + $this->media_ownership_migration_notice->init_hooks(); | |
| 29 | + } | |
| 23 | 30 | |
| 24 | 31 | public static function collect_post_meta_attachment_ids( int $post_id, array $meta_keys ): array { |
| 25 | 32 | $attachment_ids = array(); |
| 26 | 33 | |
| @@ -37,9 +44,10 @@ | ||
| 37 | 44 | } |
| 38 | 45 | |
| 39 | 46 | public static function maybe_delete_attachments( |
| 40 | 47 | array $old_attachment_ids, |
| 41 | - array $new_attachment_ids = array() | |
| 48 | + array $new_attachment_ids = array(), | |
| 49 | + int $context_post_id = 0 | |
| 42 | 50 | ): void { |
| 43 | 51 | $attachment_ids_to_delete = self::diff_attachment_ids( |
| 44 | 52 | $old_attachment_ids, |
| 45 | 53 | $new_attachment_ids |
| @@ -46,9 +54,9 @@ | ||
| 46 | 54 | ); |
| 47 | 55 | if ( empty( $attachment_ids_to_delete ) ) { |
| 48 | 56 | return; |
| 49 | 57 | } |
| 50 | - self::delete_attachments( $attachment_ids_to_delete ); | |
| 58 | + self::delete_attachments( $attachment_ids_to_delete, $context_post_id ); | |
| 51 | 59 | } |
| 52 | 60 | |
| 53 | 61 | public static function diff_attachment_ids( array $old_attachment_ids, array $new_attachment_ids ): array { |
| 54 | 62 | $old_attachment_ids = self::normalize_attachment_ids( $old_attachment_ids ); |
| @@ -96,22 +104,286 @@ | ||
| 96 | 104 | ) |
| 97 | 105 | ); |
| 98 | 106 | } |
| 99 | 107 | |
| 100 | - public static function delete_attachments( array $attachment_ids ): void { | |
| 108 | + /** | |
| 109 | + * Deletes plugin-owned attachments (marked with `_jfb_uploaded_by_form` at | |
| 110 | + * upload time) that were removed from a post's media field on this request. | |
| 111 | + * | |
| 112 | + * These attachments may have `post_author = 0` (anonymous form submissions), | |
| 113 | + * so WordPress core's `current_user_can( 'delete_post', $attachment_id )` | |
| 114 | + * cannot express their real ownership: `map_meta_cap()` only recognizes | |
| 115 | + * "author owns it" when `post_author` is truthy, so a `post_author = 0` | |
| 116 | + * attachment can never pass that check for anyone, including the | |
| 117 | + * legitimate logged-in editor of the post it's attached to. Ownership is | |
| 118 | + * therefore asserted via a combination of: | |
| 119 | + * - the request's already-verified authorization to edit the owning post | |
| 120 | + * ($context_post_id), checked the same way `Post_Id_Property::can_attach()` | |
| 121 | + * authorizes the request on the only call path that reaches this method | |
| 122 | + * (post_author match OR edit_post OR webhook) - using only the narrower | |
| 123 | + * edit_post-or-webhook check here would silently skip cleanup for a | |
| 124 | + * legitimate post owner on a post type whose capability mapping doesn't | |
| 125 | + * resolve edit_post true for its own author (e.g. a custom | |
| 126 | + * map_meta_cap callback), even though insert_post already authorized | |
| 127 | + * the very same request to update that post; and | |
| 128 | + * - a per-attachment uploader check ($this->is_owned_by_current_actor()): | |
| 129 | + * a logged-in uploader's attachments can only be deleted by that same | |
| 130 | + * user, so an editor of $context_post_id cannot delete an attachment | |
| 131 | + * another user legitimately uploaded for an unrelated post just because | |
| 132 | + * both happen to carry the plugin's upload marker. Attachments uploaded | |
| 133 | + * before this per-uploader binding existed (no `_jfb_uploaded_by_user` | |
| 134 | + * meta at all) fall back to the pre-existing marker + edit_post check | |
| 135 | + * only - a deliberate, documented compatibility trade-off (see | |
| 136 | + * changelog) since there is no way to retroactively know their real | |
| 137 | + * uploader. Attachments explicitly recorded as anonymously uploaded | |
| 138 | + * (uploader id `0`) are different: that is an ongoing, current-day case | |
| 139 | + * (every anonymous form submission), not a legacy gap, so it is never | |
| 140 | + * allowed via the edit_post fallback - only `Tools::is_webhook()` may | |
| 141 | + * delete them, since no verifiable per-request identity exists to bind | |
| 142 | + * them to. | |
| 143 | + * | |
| 144 | + * @param array $attachment_ids | |
| 145 | + * @param int $context_post_id The post these attachments are being | |
| 146 | + * removed from; the caller must have already | |
| 147 | + * authorized the current request to edit it. | |
| 148 | + */ | |
| 149 | + public static function delete_attachments( array $attachment_ids, int $context_post_id = 0 ): void { | |
| 101 | 150 | |
| 102 | 151 | $attachment_ids = self::normalize_attachment_ids( $attachment_ids ); |
| 103 | 152 | |
| 153 | + if ( empty( $attachment_ids ) ) { | |
| 154 | + return; | |
| 155 | + } | |
| 156 | + | |
| 157 | + $authorization = self::authorize_context_post( $context_post_id ); | |
| 158 | + | |
| 159 | + if ( ! $authorization['authorized'] ) { | |
| 160 | + return; | |
| 161 | + } | |
| 162 | + | |
| 104 | 163 | foreach ( $attachment_ids as $attachment_id ) { |
| 105 | 164 | if ( 'attachment' !== get_post_type( $attachment_id ) ) { |
| 106 | 165 | continue; |
| 107 | 166 | } |
| 108 | 167 | |
| 168 | + if ( ! get_post_meta( $attachment_id, '_jfb_uploaded_by_form', true ) ) { | |
| 169 | + continue; | |
| 170 | + } | |
| 171 | + | |
| 172 | + /** | |
| 173 | + * `authorize_context_post()` above already rejects every logged-out request | |
| 174 | + * outright (see its own docblock for why the anonymous post_author-match branch | |
| 175 | + * `Post_Id_Property::can_attach()` still grants - issues-tracker #20547's | |
| 176 | + * still-open Defect 3 - is deliberately NOT accepted as authorization here), so | |
| 177 | + * `$authorization['verified_identity']` is currently always `=== true` by the | |
| 178 | + * time this line runs. It is still checked explicitly (rather than assuming | |
| 179 | + * `authorized` alone is enough) so this call site stays correct on its own terms | |
| 180 | + * even if `authorize_context_post()` is ever changed to reintroduce a | |
| 181 | + * non-verified authorized branch - `is_owned_by_current_actor()`'s | |
| 182 | + * anonymous-uploader exception must never be unlocked by anything less than a | |
| 183 | + * verified identity, regardless of what else may authorize the request to edit | |
| 184 | + * the post in the future. | |
| 185 | + */ | |
| 186 | + if ( ! self::is_owned_by_current_actor( $attachment_id, $authorization['verified_identity'] ? $context_post_id : 0 ) ) { | |
| 187 | + continue; | |
| 188 | + } | |
| 189 | + | |
| 109 | 190 | wp_delete_attachment( $attachment_id, true ); |
| 110 | 191 | } |
| 111 | 192 | } |
| 112 | 193 | |
| 194 | + /** | |
| 195 | + * Authorizes the current request to run CLEANUP against `$context_post_id`. Unlike | |
| 196 | + * `Post_Id_Property::can_attach()` (which this otherwise mirrors for post_author match / | |
| 197 | + * edit_post / webhook), a logged-out request is deliberately NEVER authorized here, even | |
| 198 | + * though `can_attach()` currently authorizes it via `post_author === 0 === get_current_user_id()` | |
| 199 | + * (issues-tracker #20547's still-open Defect 3, modules/actions-v2/insert-post/properties/ | |
| 200 | + * post-id-property.php - not fixed by this file). That anonymous match is not a | |
| 201 | + * verifiable identity: because Defect 3 lets ANY anonymous visitor get authorized on | |
| 202 | + * ANY `post_author = 0` post, not just one they created, relying on it here would let an | |
| 203 | + * attacker delete a stranger's anonymously-uploaded attachment by first getting | |
| 204 | + * "authorized" on the stranger's own carrier post through that same anonymous match - | |
| 205 | + * reopening the exact "delete a stranger's file" primitive #20547 exists to close, just | |
| 206 | + * laundered through Defect 3 instead of the original PoC's chain. Cleanup for a | |
| 207 | + * logged-out request is therefore only ever performed via `Tools::is_webhook()`, which | |
| 208 | + * `delete_attachments()` still allows. | |
| 209 | + * | |
| 210 | + * The trade-off: an anonymous visitor can no longer have their OWN, legitimately | |
| 211 | + * removed-from-the-field attachment cleaned up automatically either - it is simply left | |
| 212 | + * in the Media Library instead of being deleted. This is the same fail-closed posture | |
| 213 | + * already documented for the anonymous-upload case elsewhere in this fix (see | |
| 214 | + * `is_owned_by_current_actor()`'s docblock) and in `readme.md`'s "Что осознанно осталось | |
| 215 | + * вне скоупа" section: anonymous form submitters never get automatic file cleanup | |
| 216 | + * without going through a webhook or an admin, by design. | |
| 217 | + * | |
| 218 | + * `verified_identity` in the return value is kept (rather than collapsing this to a | |
| 219 | + * plain bool) so `delete_attachments()`'s call to `is_owned_by_current_actor()` stays | |
| 220 | + * self-documenting about WHY it passes `0` vs `$context_post_id` - it is always | |
| 221 | + * `=== authorized` now that the anonymous branch is gone, but keeping the explicit field | |
| 222 | + * name at the call site is clearer than an implicit "true always means verified" rule a | |
| 223 | + * future change could silently invalidate by reintroducing an unverified branch here. | |
| 224 | + * | |
| 225 | + * @param int $context_post_id | |
| 226 | + * | |
| 227 | + * @return array{authorized: bool, verified_identity: bool} | |
| 228 | + */ | |
| 229 | + private static function authorize_context_post( int $context_post_id ): array { | |
| 230 | + if ( Tools::is_webhook() ) { | |
| 231 | + return array( | |
| 232 | + 'authorized' => true, | |
| 233 | + 'verified_identity' => true, | |
| 234 | + ); | |
| 235 | + } | |
| 236 | + | |
| 237 | + $current_user_id = get_current_user_id(); | |
| 238 | + | |
| 239 | + if ( $current_user_id > 0 ) { | |
| 240 | + $post = get_post( $context_post_id ); | |
| 241 | + | |
| 242 | + if ( $post instanceof \WP_Post && absint( $post->post_author ) === $current_user_id ) { | |
| 243 | + return array( | |
| 244 | + 'authorized' => true, | |
| 245 | + 'verified_identity' => true, | |
| 246 | + ); | |
| 247 | + } | |
| 248 | + | |
| 249 | + if ( current_user_can( 'edit_post', $context_post_id ) ) { | |
| 250 | + return array( | |
| 251 | + 'authorized' => true, | |
| 252 | + 'verified_identity' => true, | |
| 253 | + ); | |
| 254 | + } | |
| 255 | + } | |
| 256 | + | |
| 257 | + return array( | |
| 258 | + 'authorized' => false, | |
| 259 | + 'verified_identity' => false, | |
| 260 | + ); | |
| 261 | + } | |
| 262 | + | |
| 263 | + /** | |
| 264 | + * Shared ownership check for the plugin's `_jfb_uploaded_by_user` marker, used by BOTH | |
| 265 | + * `delete_attachments()` here (may this attachment be deleted?) and | |
| 266 | + * `Media_Field_Parser::is_allowed_submitted_attachment()` (may this attachment ID be | |
| 267 | + * accepted into a post's meta?). The two questions apply the exact same trust model - | |
| 268 | + * "does the current actor match who the plugin recorded as having uploaded this file?" | |
| 269 | + * - so they share this one implementation instead of two independently maintained | |
| 270 | + * copies that could silently drift apart. | |
| 271 | + * | |
| 272 | + * An attachment uploaded by a logged-in user may only be claimed (deleted OR newly | |
| 273 | + * attached to a post) by that same user. Attachments carrying `_jfb_uploaded_by_form` | |
| 274 | + * but NO `_jfb_uploaded_by_user` meta at all fall through to an unconditional allow | |
| 275 | + * below, since there is nothing to bind them to. Both `Uploaded_File::add_attachment()` | |
| 276 | + * (live uploads) and `Version_3_6_6::maybe_backfill_attachment()` (the migration) always | |
| 277 | + * write BOTH markers together - including an explicit `_jfb_uploaded_by_user = 0` for | |
| 278 | + * the anonymous case, precisely so a marked attachment never ends up in this | |
| 279 | + * unconditional branch just because its real uploader happens to be "nobody" (see | |
| 280 | + * `maybe_backfill_attachment()`'s docblock for why an earlier revision that left this | |
| 281 | + * meta unset for `post_author = 0` was itself a security bug: this branch is meant only | |
| 282 | + * for attachments that predate the marker system outright, and granting it to a marked | |
| 283 | + * anonymous upload let any authenticated user claim/delete it via any post they | |
| 284 | + * control). This branch is therefore not expected to be reachable through this fix's | |
| 285 | + * own code paths going forward; it remains only as a documented fallback for an | |
| 286 | + * attachment some other, unrelated code path marked `_jfb_uploaded_by_form` without | |
| 287 | + * also setting an uploader value. | |
| 288 | + * | |
| 289 | + * An anonymously-uploaded attachment (uploader id `0`) cannot be told apart from a | |
| 290 | + * stranger's anonymous upload by identity alone, since `0` is also what any logged-out | |
| 291 | + * visitor's `get_current_user_id()` returns - claiming it on that basis would let any | |
| 292 | + * anonymous visitor claim any other anonymous visitor's upload. `$owning_post_id` is | |
| 293 | + * the one exception a caller can supply: when the caller has ALREADY (a) verified the | |
| 294 | + * attachment ID was found in that specific post's own pre-save meta (proving it, not | |
| 295 | + * some other post's anonymous upload, is what's being claimed) and (b) authorized the | |
| 296 | + * current request to edit that same post via a VERIFIED IDENTITY - `Tools::is_webhook()`, | |
| 297 | + * or a logged-in user's own `post_author` match / `edit_post` capability - never the | |
| 298 | + * anonymous `post_author === 0 === get_current_user_id()` match alone. That last | |
| 299 | + * distinction is load-bearing: `Post_Id_Property::can_attach()` (issues-tracker #20547's | |
| 300 | + * still-open Defect 3) currently authorizes ANY anonymous visitor to edit ANY post with | |
| 301 | + * `post_author = 0`, not just the post its own submission created, so an anonymous | |
| 302 | + * post_author match proves nothing about who "the requester" actually is - passing it | |
| 303 | + * through as ownership proof would let an attacker claim a stranger's anonymous upload | |
| 304 | + * by first getting `Post_Id_Property::can_attach()` to authorize them on that stranger's | |
| 305 | + * carrier post (trivial today, see Defect 3), then deleting the attachment through that | |
| 306 | + * post's own cleanup. `delete_attachments()` is the only caller that can supply a | |
| 307 | + * verified identity (via `authorize_context_post()` and the diff in | |
| 308 | + * `maybe_delete_attachments()`), and only passes `$owning_post_id` when | |
| 309 | + * `authorize_context_post()` reports `verified_identity => true`. `Media_Field_Parser:: | |
| 310 | + * is_allowed_submitted_attachment()` has no post context at all at the point it runs | |
| 311 | + * (see `Uploaded_File::add_attachment()`'s docblock on why no post is authorized yet | |
| 312 | + * when a field is parsed), so it always omits this parameter and an anonymous upload | |
| 313 | + * remains claimable there only via `Tools::is_webhook()` - including by the very same | |
| 314 | + * anonymous submitter resubmitting their own not-yet-authorized form. Concretely: if that | |
| 315 | + * form's Media field has "Delete removed attachments" enabled, an anonymous visitor who | |
| 316 | + * uploads a file and then resubmits the SAME form (e.g. multi-step form, validation | |
| 317 | + * retry, or simply re-saving the post) WITHOUT changing that field will have their own | |
| 318 | + * just-uploaded, still-wanted attachment physically deleted - `sanitize_submitted_ | |
| 319 | + * attachment_ids()` drops the ID from the "new" snapshot, `Media_Cleanup:: | |
| 320 | + * maybe_delete_attachments()` sees it missing from the diff, and `wp_delete_attachment()` | |
| 321 | + * runs. This is a known, deliberate limitation, not an oversight - the request has no | |
| 322 | + * way to prove "still the same visitor" beyond a session this check does not rely on - | |
| 323 | + * but it is a real, user-facing data-loss risk for anonymous-upload + cleanup-enabled | |
| 324 | + * forms, not merely a cosmetic "field forgets its value" annoyance. Closing it properly | |
| 325 | + * needs a verifiable, request-scoped identity for anonymous uploads (e.g. binding the | |
| 326 | + * upload to the plugin's own CSRF/session token infrastructure in modules/security/csrf/, | |
| 327 | + * available at field-parse time) rather than `get_current_user_id()`, which cannot | |
| 328 | + * distinguish anonymous visitors from each other; tracked as a follow-up, not fixed by | |
| 329 | + * this parameter. | |
| 330 | + * | |
| 331 | + * @param int $attachment_id | |
| 332 | + * @param int $owning_post_id Optional. A post ID the caller has already verified this | |
| 333 | + * attachment ID came from AND authorized the current request | |
| 334 | + * to edit via a verified (non-anonymous-match) identity. Only | |
| 335 | + * ever passed by `delete_attachments()`. | |
| 336 | + * | |
| 337 | + * @return bool | |
| 338 | + */ | |
| 339 | + public static function is_owned_by_current_actor( int $attachment_id, int $owning_post_id = 0 ): bool { | |
| 340 | + $has_uploader_meta = metadata_exists( 'post', $attachment_id, '_jfb_uploaded_by_user' ); | |
| 341 | + | |
| 342 | + if ( ! $has_uploader_meta ) { | |
| 343 | + return true; | |
| 344 | + } | |
| 345 | + | |
| 346 | + $uploader_id = (int) get_post_meta( $attachment_id, '_jfb_uploaded_by_user', true ); | |
| 347 | + | |
| 348 | + if ( ! $uploader_id ) { | |
| 349 | + return Tools::is_webhook() || $owning_post_id > 0; | |
| 350 | + } | |
| 351 | + | |
| 352 | + if ( get_current_user_id() === $uploader_id || Tools::is_webhook() ) { | |
| 353 | + return true; | |
| 354 | + } | |
| 355 | + | |
| 356 | + /** | |
| 357 | + * A non-zero `_jfb_uploaded_by_user` is normally ground truth - `Uploaded_File:: | |
| 358 | + * add_attachment()` records the actual `get_current_user_id()` at upload time, so | |
| 359 | + * requiring an exact match is correct and intentionally has no `$owning_post_id` | |
| 360 | + * fallback (unlike the anonymous-uploader branch above): that strictness is exactly | |
| 361 | + * what stops one user from claiming another user's real upload. | |
| 362 | + * | |
| 363 | + * `Version_3_6_6::maybe_backfill_attachment()` is the one exception: it writes this | |
| 364 | + * same meta key from `post_author` as a best-effort GUESS for pre-fix attachments, | |
| 365 | + * not an observed fact, and marks that with `_jfb_uploaded_by_user_heuristic` (see | |
| 366 | + * that method's docblock for why `post_author` can be stale/reassigned and is not | |
| 367 | + * reliable enough to lock an attachment to forever). Without this exception, a | |
| 368 | + * legacy attachment whose `post_author` no longer matches its post's actual current | |
| 369 | + * editor would become permanently unmanageable by anyone but that one guessed user | |
| 370 | + * or a webhook - worse than the pre-fix state, where an unmarked attachment was | |
| 371 | + * freely manageable by any `edit_post`-authorized editor of its post. For a | |
| 372 | + * heuristic value specifically, the same verified-identity `$owning_post_id` | |
| 373 | + * exception the anonymous branch above uses also applies here. | |
| 374 | + */ | |
| 375 | + if ( $owning_post_id > 0 && get_post_meta( $attachment_id, '_jfb_uploaded_by_user_heuristic', true ) ) { | |
| 376 | + return true; | |
| 377 | + } | |
| 378 | + | |
| 379 | + return false; | |
| 380 | + } | |
| 381 | + | |
| 113 | 382 | public function remove_hooks() { |
| 383 | + if ( $this->media_ownership_migration_notice ) { | |
| 384 | + $this->media_ownership_migration_notice->remove_hooks(); | |
| 385 | + } | |
| 114 | 386 | } |
| 115 | 387 | |
| 116 | 388 | public static function get_post_meta_keys_for_cleanup( $modifier ): array { |
| 117 | 389 | $form_id = jet_fb_action_handler()->get_form_id(); |
