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 new PHPCS errors #324

Closed
JDGrimes opened this issue May 1, 2015 · 3 comments
Closed

Fix new PHPCS errors #324

JDGrimes opened this issue May 1, 2015 · 3 comments
Assignees
Labels
Milestone

Comments

@JDGrimes
Copy link
Member

JDGrimes commented May 1, 2015

Updates to WPCS mean we're seeing some new errors.

@JDGrimes JDGrimes added the bug label May 1, 2015
@JDGrimes JDGrimes added this to the 2.0.0 milestone May 1, 2015
@JDGrimes JDGrimes self-assigned this May 1, 2015
JDGrimes added a commit that referenced this issue May 2, 2015
JDGrimes added a commit that referenced this issue May 2, 2015
JDGrimes added a commit that referenced this issue May 2, 2015
This may have been needed before, but now that some query parameters
are automatically cleaned from canonical URLs, we can just let the form
submit to its own page.

See #324, #303
JDGrimes added a commit that referenced this issue May 2, 2015
These values are actually being sanitized, but because `wp_unslashed()`
is (rightly) being used, WPCS is flagging them. We’ll remove these
comments once this is fixed upstream. See
WordPress/WordPress-Coding-Standards#172

See #324
JDGrimes added a commit that referenced this issue May 2, 2015
None of these locations are vulnerable. Two have nonce checks later on,
in the file included within the statement. The other is hooked to an
action that is called only after nonce validation on the settings
screen. However, I opted to add an explicit nonce check here, for good
measure.

See #324
JDGrimes added a commit that referenced this issue May 2, 2015
Also updates a whitelisting comment for points settings. While we might
want to consider performing sanitization of some sort here in the
future, it is no easy task when we are dealing with an arbitrarily deep
array that might contain HTML snippets.

See #324
JDGrimes added a commit that referenced this issue May 2, 2015
JDGrimes added a commit that referenced this issue May 2, 2015
In this case, nonce verification isn’t necessary, because we aren’t
performing a restricted action.

See #324
JDGrimes added a commit that referenced this issue May 2, 2015
In this case, the verification check is performed by the primary
functions which call these methods.

See #324
JDGrimes added a commit that referenced this issue May 2, 2015
So the concern about possible CSRF isn’t valid.

See #324
JDGrimes added a commit that referenced this issue May 7, 2015
JDGrimes added a commit that referenced this issue May 22, 2015
These were pointed out thanks to changes in PHPCS.

See #324
JDGrimes added a commit that referenced this issue May 23, 2015
@JDGrimes
Copy link
Member Author

Fixed for now.

JDGrimes added a commit that referenced this issue Jun 6, 2015
@JDGrimes
Copy link
Member Author

JDGrimes commented Jun 6, 2015

This has broken the Ajax tests:

.F..F...................................

Time: 6.31 seconds, Memory: 76.00Mb

There were 2 failures:

1) WordPoints_Points_Hooks_Order_AJAX_Test::test_as_admin

Failed asserting that two arrays are equal.

--- Expected

+++ Actual

@@ @@

 Array (

     'points' => Array (

-        0 => 'registration_points_hook-1'

-        1 => 'comments_points_hook-1'

+        0 => 'registration_points_hook-1wordpoints_comments_points_hook-1'

     )

 )

/home/travis/build/WordPoints/wordpoints/tests/phpunit/tests/points/ajax/hook-order.php:80

2) WordPoints_Points_Hooks_Order_AJAX_Test::test_network_as_super_admin

Failed asserting that two arrays are equal.

--- Expected

+++ Actual

@@ @@

 Array (

     'points' => Array (

-        0 => 'registration_points_hook-1'

-        1 => 'comments_points_hook-1'

+        0 => 'registration_points_hook-1wordpoints_comments_points_hook-1'

     )

 )

/home/travis/build/WordPoints/wordpoints/tests/phpunit/tests/points/ajax/hook-order.php:169

The problem is this:

        $points_types = array_map( 'sanitize_key', $_POST['points_types'] );

$_POST['points_types'] is an array of arrays, not a simple array. So we need to sanitize differently. I think we should pull the list of points types from the database, and use them to get each sub-array from the POSTed value. The way it works now, the list could contain points types that don't even exist.

@JDGrimes JDGrimes reopened this Jun 6, 2015
@JDGrimes
Copy link
Member Author

JDGrimes commented Jun 6, 2015

$_POST['points_types'] is an array of arrays, not a simple array.

Actually, that is incorrect. It is a simple array. The hooks are a comma-delimited string. The problem is that sanitize_key() is being used, because it strips out the commas. We need to use sanitize_text_field() instead.

I'll still refactor it to use the list of points types from the database.

JDGrimes added a commit that referenced this issue Jun 6, 2015
These lists are an array of comma-delimited hook IDs. We were
sanitizing them with `sanitize_key()`. That is causing the function not
to work correctly, and our tests are failing. The problem is that
`sanitize_key()` will strip out the commas. We need to use
`sanitize_text_field()` instead.

In addition, I’ve refactored the logic to get the list of points types
from the database. Previously, the function was not verifying whether
the points types were valid. Now it will only save the lists for valid
points types.

See #324
JDGrimes added a commit that referenced this issue Jun 6, 2015
@JDGrimes JDGrimes closed this as completed Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant