Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions WordPress/Docs/Security/ValidatedSanitizedInputStandard.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="Validated Sanitized Input"
>
<standard>
<![CDATA[
All user input data ($_POST, $_GET, $_REQUEST, $_SERVER, $_COOKIE, $_FILES, $_SESSION, $_ENV) must be validated, unslashed, and sanitized before use to prevent security vulnerabilities like XSS, SQL injection, and code injection attacks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to be improved as not all the superglobals mentioned in the list are related to user input. At the same time, the sniff uses input in several of its messages, so maybe that is fine. Just thinking out loud here. Let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure if this sniff needs a <standard> block summarizing its three errors. Maybe just having the <standard> blocks, one for each error, is enough? What do you think, @jrfnl?


Validation ensures the input key exists (using isset(), empty(), array_key_exists(), or null coalescing operators). Unslashing removes WordPress's automatic backslashes using wp_unslash() or similar functions. Sanitization cleans the data using appropriate functions like sanitize_text_field(), absint(), etc.
]]>
</standard>
<standard>
<![CDATA[
String interpolation with superglobals requires validation and sanitization. Using $_POST, $_GET, etc. directly in strings can lead to XSS attacks if the input contains malicious code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure XSS is the only vector of attack that is prevented here? Without thinking too much about it, I'm inclined to think it is not, but I could be wrong. If we are not sure, this sentence can be rephrased to be something more generic.

]]>
</standard>
<code_comparison>
<code title="Valid: String interpolation with proper validation and sanitization.">
<![CDATA[
if ( isset( $_POST['name'] ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why <em> tags were not used in the examples in this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure if this complete example validating and sanitizing $_POST['name'] should be used here, as this is not checked by this particular sniff error (InputNotValidatedNotSanitized). All that this particular sniff error checks is that superglobals are not interpolated in strings. Maybe the valid example here could be a code comment like:

// No superglobal used in string interpolation.

Happy to hear other suggestions that you might have. Checking other XML docs might help as well, in case there is a similar pattern for another sniff.

$safe_name = sanitize_text_field( wp_unslash( $_POST['name'] ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please ensure that all code examples contain a maximum of 48 characters (not counting the <em> tags). Otherwise, they will break the output (run vendor/bin/phpcs -s --standard=WordPress --sniffs=WordPress.Security.ValidatedSanitizedInput --generator=Text to see what I mean). You can break a line like this one into multiple lines.

echo "Hello " . $safe_name;
}
]]>
</code>
<code title="Invalid: String interpolation without validation or sanitization.">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the text here could be more specific and mention that the problem is about superglobals used in string interpolation and not about string interpolation in general?

<![CDATA[
echo "Hello {$_POST['name']}";
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
All superglobal array access must be validated to ensure the key exists before use. This prevents undefined index notices and potential security issues.
]]>
</standard>
<code_comparison>
<code title="Valid: Input is validated before use.">
<![CDATA[
if ( isset( $_POST['name'] ) ) {
$name = sanitize_text_field( wp_unslash( $_POST['name'] ) );
}
]]>
</code>
<code title="Invalid: Input used without validation.">
<![CDATA[
$name = sanitize_text_field( wp_unslash( $_POST['name'] ) );
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
All validated input must be sanitized to remove or escape potentially malicious content before processing or output.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typically, the description and examples of individual errors focus solely on the specific error. With that in mind, I suggest not mentioning validation here, in title attributes below, and in the code examples (I wouldn't include unleashing as well). cc @jrfnl for your input, as I'm not sure about it in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but I believe only checks for sanitizing function and thus the original input is never escaped. Also, I believe the sniff doesn't really care if the input is being outputted or further processed so I don't think it is necessary to mention it. I would suggest something like:

Suggested change
All validated input must be sanitized to remove or escape potentially malicious content before processing or output.
All input must be sanitized to remove potentially malicious content before it is used.

]]>
</standard>
<code_comparison>
<code title="Valid: Input is validated and sanitized.">
<![CDATA[
if ( isset( $_POST['text'] ) ) {
$text = sanitize_text_field( wp_unslash( $_POST['text'] ) );
}
]]>
</code>
<code title="Invalid: Input validated but not sanitized.">
<![CDATA[
if ( isset( $_POST['text'] ) ) {
$text = wp_unslash( $_POST['text'] );
}
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
WordPress automatically adds backslashes to certain superglobals. These must be removed using wp_unslash() or similar functions before sanitization to prevent double-escaping issues.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we mentioned all the superglobals above, maybe it is worth mentioning here the ones that require wp_unslash()?

]]>
</standard>
<code_comparison>
<code title="Valid: Input is unslashed before sanitization.">
<![CDATA[
if ( isset( $_POST['data'] ) ) {
$clean = sanitize_text_field( wp_unslash( $_POST['data'] ) );
}
]]>
</code>
<code title="Invalid: Missing unslashing before sanitization.">
<![CDATA[
if ( isset( $_POST['data'] ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since validation is not needed to trigger this error, I don't think it is necessary to include it in this example (cc @jrfnl for your opinion).

$clean = sanitize_text_field( $_POST['data'] );
}
]]>
</code>
</code_comparison>
</documentation>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing newline at the end of the file.

Loading