Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#42477 closed defect (bug) (wontfix)

Cannot save theme customizer changes if nonce_life value is filtered in the active theme

Reported by: figureone's profile figureone Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.8.3
Component: Customize Keywords:
Focuses: administration Cc:

Description

Summary:

If the nonce_life filter is used in the active theme to change the nonce lifetime from its default 86400 (e.g., to 3600), then changes made in the Theme Customizer cannot be saved.

Details:

WordPress 4.8 introduced some changes to the theme customizer, including this security-related commit that clears submitted post data if the nonce check fails:
https://github.com/WordPress/WordPress/commit/3d10fef22d788f29aed745b0f5ff6f6baea69af3#diff-681a4f686cf806e75ff8cd4c6d2fe5b7

This introduces an unintended side effect in a certain context:

  • Since this code is hooked into setup_theme, it fires before the theme is loaded.
  • If a theme developer hooks into the nonce_life filter that can be used to adjust the lifespan of nonces, it will cause the nonce check above to fail (since the nonce ticks will not match).

https://developer.wordpress.org/reference/hooks/nonce_life/
https://github.com/WordPress/WordPress/blob/master/wp-includes/pluggable.php#L1999

This specific context happens in the theme customizer, when trying to Save & Publish changes. Interestingly, changesets can still be saved as an auto-draft on window blur (clicking outside of the browser window). I believe this is because changeset data is in $_POST['customize_changeset_data'], while the same data is in $_POST['customized'] when the Save & Publish button is clicked.

I think the best solution here may be to remove the nonce_life filter, but I don't know how widely it's used. Another possibility may be moving the nonce check out of WP_Customize_Manager::setup_theme(), but I don't have a good idea at the moment for where it should go.

Change History (9)

#1 follow-up: @westonruter
6 years ago

@figureone Interesting scenario. Why would a theme be adding a nonce_life filter in the first place?

#2 in reply to: ↑ 1 @figureone
6 years ago

Replying to westonruter:

@figureone Interesting scenario. Why would a theme be adding a nonce_life filter in the first place?

Good question, and I don't have a great answer. :)

Once I diagnosed this issue, I simply removed the nonce_life filter that a previous theme developer had written; their comments only pointed to:
https://wordpress.stackexchange.com/questions/94585/is-it-safe-to-assume-that-a-nonce-may-be-validated-more-than-once

So I gather someone might use it if they are worried about nonces being stolen and reused, but I don't think that is a great idea in a theme context.

The fix here could simply be to add some notes to the documentation for the filter; I just wanted to seek some input first to see if there were other uses for the nonce_life filter that I wasn't thinking of.

#3 @dd32
6 years ago

This was brought up in #41617 in a different way.

I think it's OK as-is, and the breakage is expected. If the theme filters nonce_life globally and not per-action then it's deliberately changing how WordPress works, and it's similar to a theme replacing the Customiser with it's own version.

There's also #35188 to pass the nonce name through to aid in per-action filtering.

If you have a link to a theme (public, or paid) that triggers this, I'd be interested, feel free to post here or DM me on slack (@dd32)

I think the best solution here may be to remove the nonce_life filter

Personally I think the filter should be removed, or at least changed to only allow increasing the lifetime.

#4 follow-up: @figureone
6 years ago

Thanks @dd32, here's a simple demonstration theme (child theme of twentyseventeen) that simply changes the nonce_life value:
https://github.com/uhm-coe/twentyseventeen-child-demonstrate-nonce_life-bug

If you activate that theme and then go into the theme customizer, you'll see the behavior:

  • Changing a value (e.g., Site Title), then clicking Save & Publish, then refreshing the page will show that the change was not made (it fails silently).
  • Changing a value (e.g., Site Title), then blurring the page by clicking outside the window will successfully create a changeset. If you then click Save & Publish, the changeset does get published.

In tracing the code, I discovered the inconsistent behavior was because the ajax action for submitting changeset data uses $_POST['customize_changeset_data'], while the ajax action for the Save & Publish button uses $_POST['customized']. The core commit referenced in the OP only clears $_POST['customized'] if the nonce check fails.

#5 @westonruter
6 years ago

In regards to failing silently, this should be fixed in 4.9: a notification will appear when save errors occur.

#6 in reply to: ↑ 4 @dd32
6 years ago

Replying to figureone:

Thanks @dd32, here's a simple demonstration theme (child theme of twentyseventeen) that simply changes the nonce_life value:
https://github.com/uhm-coe/twentyseventeen-child-demonstrate-nonce_life-bug

If you find any real-world example themes or plugins I'd be interested in those :)

#7 @figureone
6 years ago

Ah, sorry, thought you were looking for a POC. :)

I don't have any real-world examples. I removed the filter from 8 of our private themes (mainly online courses for the University of Hawai‘i). They were added by a previous developer, and unnecessary in my opinion.

#8 @westonruter
6 years ago

  • Keywords close added

#9 @pento
5 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing this issue, as overriding the nonce life globally is not supported.

Note: See TracTickets for help on using tickets.