New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Fetch: Expose nonce on created middleware function #13451

Merged
merged 1 commit into from Jan 25, 2019

Conversation

Projects
None yet
3 participants
@aduth
Copy link
Member

aduth commented Jan 23, 2019

Related: https://core.trac.wordpress.org/ticket/45113#comment:11

This pull request stemmed from a simple desire to rename the rest-nonce heartbeat response value to the core-preferred rest_nonce. It subsequently turned into a bit more of a refactor of how this nonce is assigned and handled within the API fetch middleware.

The proposed changes here avoid having the API Fetch module having any awareness of heartbeat at all, instead using an inline script to create the heartbeat action handler, assigning to the newly-introduced middleware nonce property.

Note: This will require a change to the equivalent inline script handling in core. (patch)

Testing Instructions:

To simplify testing, I found it useful to create a simple plugin at wp-content/mu-plugins/nonce-duration.php to shorten the default lifetime of a nonce dramatically, since otherwise a new nonce won't be included by default with the heartbeat response:

<?php

add_filter( 'nonce_life', function() { 
	return 5;
} );

Verify that a different nonce is used with API requests which are triggered at least 5 seconds apart with the above patch.

@aduth aduth requested review from youknowriad , dd32 and nerrad Jan 23, 2019

@@ -185,10 +185,29 @@ function gutenberg_register_scripts_and_styles() {
gutenberg_register_packages_scripts();
// Inline scripts.
global $wp_scripts;
$wp_scripts->registered['wp-api-fetch']->deps[] = 'wp-hooks';

This comment has been minimized.

@aduth

aduth Jan 23, 2019

Author Member

This is a bit awkward in that the inline script has a dependency which the handle upon which it's attached does not. There's a pretty strong guarantee from the preceding lines that this would be registered, but it might be a good idea to have some protections here all the same, either with isset or by wp_script_is.

We could also just have wp-api-fetch continue to depend on wp-hooks, but I assumed that in the future we might want to have it so that dependencies in core are automatically generated from the packages. Maybe this is thinking too far ahead, or maybe we'd want some filtering to apply to that generated result anyways.

This comment has been minimized.

@nerrad

nerrad Jan 23, 2019

Contributor

Likely not an issue, but I wonder if removing the dependency on wp-hooks from api-fetch runs the risk of breaking plugins that have created a dependency on api-fetch on the assumption it will also queue up wp-hooks. Once could argue that's doing it wrong anyways but I mentioned anyways just for consideration.

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Author Member

We can't realistically hold ourselves hostage to never changing the dependencies of a core script because people aren't declaring their own dependencies correctly 🤷‍♂️

This comment has been minimized.

@aduth

aduth Jan 25, 2019

Author Member

There's a pretty strong guarantee from the preceding lines that this would be registered, but it might be a good idea to have some protections here all the same, either with isset or by wp_script_is.

I decided to be cautious and add a guarded condition in the rebased 65675b4.

@nerrad

nerrad approved these changes Jan 23, 2019

Copy link
Contributor

nerrad left a comment

Looks good to me (note I just reviewed, did not test).

@@ -185,10 +185,29 @@ function gutenberg_register_scripts_and_styles() {
gutenberg_register_packages_scripts();
// Inline scripts.
global $wp_scripts;
$wp_scripts->registered['wp-api-fetch']->deps[] = 'wp-hooks';

This comment has been minimized.

@nerrad

nerrad Jan 23, 2019

Contributor

Likely not an issue, but I wonder if removing the dependency on wp-hooks from api-fetch runs the risk of breaking plugins that have created a dependency on api-fetch on the assumption it will also queue up wp-hooks. Once could argue that's doing it wrong anyways but I mentioned anyways just for consideration.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 24, 2019

I like the simplicity of this approach.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 25, 2019

@aduth aduth force-pushed the update/api-fetch-nonce-assign branch from c133d12 to 65675b4 Jan 25, 2019

@aduth aduth merged commit d28b228 into master Jan 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the update/api-fetch-nonce-assign branch Jan 25, 2019

@youknowriad youknowriad added this to the 5.0 (Gutenberg) milestone Jan 25, 2019

daniloercoli added a commit that referenced this pull request Jan 26, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-enter-key-detection-to-title

* 'master' of https://github.com/WordPress/gutenberg: (29 commits)
  Update for RangeControl documentation (#12564)
  Plugin: Deprecate gutenberg_load_list_reusable_blocks (#13456)
  Update the columns attribute in onSelectImages so that if images are removed via the media modal, the columns can't be higher than the new number of images (#13488)
  Replace the fullscreen "exit" icon with a back arrow (#13403)
  Include :visited links in button color (#12183)
  Amazon Kindle block (#13510)
  Plugin: Deprecate gutenberg_prepare_blocks_for_js (#13457)
  Add watcher on Linux: change fs to node-watch (#13448)
  Plugin: Deprecate `gutenberg` theme support (#13458)
  Datepicker: Add inValidDay support (#12962)
  Block Switcher: Render disabled button even if multi-selection (#13431)
  Plugin: Deprecate gutenberg_register_post_types (#13468)
  Plugin: Deprecate register_tinymce_scripts (#13466)
  Set minimum of words for RSS excerpt (#13502)
  Plugin: Deprecate gutenberg_get_block_categories (#13454)
  Plugin: Deprecate gutenberg_content_block_version (#13469)
  API Fetch: Expose nonce on created middleware function (#13451)
  Plugin: Remove list screens integrations (#13459)
  Plugin: Remove core-defined block detection functions (#13467)
  Spec Parser: Move generated spec parser to package (#13493)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment