WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 weeks ago

#42450 closed defect (bug) (fixed)

Customize: Ensure customize_autosaved requests only use revision of logged-in user

Reported by: westonruter Owned by: westonruter
Milestone: 4.9.3 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description (last modified by westonruter)

To reproduce:

  1. Make a change in the customizer to the site title.
  2. Save draft.
  3. Open the preview link in another tab, but then append with customize_autosaved=on to the URL.
  4. Make a second change to the site title, but do not Save Draft.
  5. Switch to other tab (and reload) and see your second change appearing in the tab even though you did't save draft.
  6. Now open the preview URL from that other tab in an incognito window, and you'll see the user's autosave revision also applying there unexpectedly.

Previously #42433.

The logic for adding the customize_autosaved param to the frontend preview URL (#39896) should get improved, in case a plugin does want to preview the autosaved state. In the mean time, the preview link feature is only intended for previewing the fully saved state, not autosaves. Nevertheless, the customize_autosaved=on preview URL may not ultimately have the changeset autosave revision fully populated yet since pending changes are sent in POST requests before being written into the changeset at the autosave interval.

Attachments (2)

42450.0.diff (2.4 KB) - added by westonruter 3 months ago.
42450.2.diff (4.2 KB) - added by dlh 5 weeks ago.

Download all attachments as: .zip

Change History (19)

#1 @westonruter
3 months ago

  • Description modified (diff)

#2 @westonruter
3 months ago

  • Description modified (diff)

#3 @westonruter
3 months ago

In 42118:

Customize: Prevent customize_autosaved=on from getting added to frontend preview URLs.

Amends [41969].
See #39896, #42450.
Fixes #42433.

@westonruter
3 months ago

#4 @westonruter
3 months ago

  • Keywords has-patch needs-unit-tests added

#5 @johnbillion
3 months ago

  • Milestone changed from 4.9.1 to 4.9.2

#6 @obenland
2 months ago

@bpayton Is this something you could get over the finish line?

#7 @bpayton
2 months ago

Sure, I can take a look at this on Monday. Do I understand correctly that we just need unit tests?

#8 @westonruter
2 months ago

Yes, tests and testing.

#9 @bpayton
6 weeks ago

I was wrong about being able to take this. This is unfortunately late notice, and I apologize for that.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


5 weeks ago

@dlh
5 weeks ago

#11 @dlh
5 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed

42450.2.diff adds a couple of test assertions for the changes in the patch. Also:

  • Moves the is_user_logged_in() check to the top of WP_Customize_Manager::handle_dismiss_autosave_or_lock_request(). This would provide parity with the order of similar checks in WP_Customize_Manager::handle_changeset_trash_request() and ::save(). Additionally, if there is no user, then it seems all but certain that the nonce check would fail before is_user_logged_in() ran, unless there are cases I'm not thinking of.
  • Updates the new Ajax error code in handle_dismiss_autosave_or_lock_request() to unauthenticated to match similar responses elsewhere in WP_Customize_Manager.

In my testing, I found that the patch didn't change anything about step (5) above ("see your second change appearing in the tab even though you didn't save a draft") because the user is still authenticated in the second tab. The unchanged behavior seems expected given the title of this ticket, but I wanted to double-check just because it was mentioned in the steps.

One other small comment: The new Ajax error in uses a 401 status code, which I think, technically, also requires a WWW-Authenticate header. The other unauthenticated responses omit a status code.

#12 @dd32
5 weeks ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 weeks ago

#15 @westonruter
3 weeks ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 42615:

Customize: Ensure customize_autosaved requests only use revision of logged-in user.

Props dlh, westonruter.
See #42433, #39896.
Fixes #42450.

#16 @westonruter
3 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#17 @SergeyBiryukov
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 42620:

Customize: Ensure customize_autosaved requests only use revision of logged-in user.

Props dlh, westonruter.
See #42433, #39896.
Merges [42615] to the 4.9 branch.
Fixes #42450.

Note: See TracTickets for help on using tickets.