Skip to content
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

fix: resolve asset versions #2387

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

justlevine
Copy link
Collaborator

What does this implement/fix? Explain your changes.

This PR fixes issues with EnqueuedAsset types not always having a set version, providing the default version when none is explicitly set.

Does this close any currently open issues?

#2297

Any relevant logs, error output, GraphiQL screenshots, etc?

Before:
image

After:
image

Any other comments?

Also backfilled tests for some of the other missing EnqueuedAsset fields: extra, handle, src. Unsure how to test dependencies, since we can't query for a specific asset, nor predict what array position in nodes will contain an asset with a set dependency.

Where has this been tested?

Operating System: Ubuntu 20.04 (Wsl2 + devilbox + php 8.0.15)

WordPress Version: 5.9.3

@codeclimate
Copy link

codeclimate bot commented May 18, 2022

Code Climate has analyzed commit e01522c and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

View more on Code Climate.

@justlevine justlevine added type: bug Issue that causes incorrect or unexpected behavior status: in review Awaiting review before merging or closing component: query Relating to GraphQL Queries labels May 18, 2022
@coveralls
Copy link

coveralls commented May 18, 2022

Coverage Status

Coverage increased (+0.3%) to 80.141% when pulling 6692cac on justlevine:fix/asset-versions into 2054b01 on wp-graphql:develop.

@@ -52,7 +52,7 @@ public static function register_type( TypeRegistry $type_registry ) {
],
'version' => [
'type' => 'String',
'description' => __( 'The version of the enqueued asset', 'wp-graphql' ),
'description' => __( 'The version of the enqueued asset. Defaults to the current version of WordPress.', 'wp-graphql' ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should not disclose this as it could leave sites vulnerable.

If a security report were announced about a specific version of WordPress and folks were able to identify that any given site was using that version from this query, it would help bad actors identify which sites are vulnerable. 🤔

Copy link
Collaborator

@jasonbahl jasonbahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments about the version and one idea for it.

global $wp_scripts;

return isset( $script->ver ) && is_string( $script->ver ) ? (string) $script->ver : $wp_scripts->default_version;
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

along the lines of my other comment, when we default to the current WordPress version, perhaps we should hash it to make it an arbitrary value? maybe something like:

$version = \WPGraphQL\Router::$route . $wp_scripts->default_version;
$hashed_version = md5( $version );

@justlevine
Copy link
Collaborator Author

@jasonbahl If we're really worried about security, the version number for the asset could also tell malicious actors about vulnerabilities.

That said, both a 'naked' version number, and using WP's version as a fallback are default WP behavior, so I'll leave the question to you as to how far we want to stray from core WP?

Two possible alternatives:

  • A recipe that replaces version numbers with hashes, for those who want to security harden their installs without a plugin like AIO Security or WordFence that obsfucates things like this.
  • Take a page from RAW/RENDERED, and add a query arg that allows authenticated users to get the version but defaults to the hash. Not sure if this is technically breaking from a GraphQL perspective, but it would break FaustJS (as GQTY converts fields with args to functions).

@jasonbahl
Copy link
Collaborator

Perhaps we leave it as is for now, but leave the comment out of the description. 🤔

@justlevine
Copy link
Collaborator Author

okay I fixed the description

@jasonbahl jasonbahl merged commit cc026f2 into wp-graphql:develop Jun 17, 2022
@justlevine justlevine deleted the fix/asset-versions branch June 17, 2022 03:33
This was referenced Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: query Relating to GraphQL Queries status: in review Awaiting review before merging or closing type: bug Issue that causes incorrect or unexpected behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants