Add documentation for WordPress.Security.ValidatedSanitizedInput #2587
Conversation
There was a problem hiding this comment.
Thanks for your work on this PR, @rooh-wp311!
I left a few comments with questions and suggestions. I also tagged one of the maintainers in some of my comments for their input, as I'm not sure if they would agree with those specific points that I raised.
| ]]> | ||
| </code> | ||
| </code_comparison> | ||
| </documentation> No newline at end of file |
There was a problem hiding this comment.
Missing newline at the end of the file.
| <code_comparison> | ||
| <code title="Valid: String interpolation with proper validation and sanitization."> | ||
| <![CDATA[ | ||
| if ( isset( $_POST['name'] ) ) { |
There was a problem hiding this comment.
Is there a reason why <em> tags were not used in the examples in this PR?
| > | ||
| <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. |
There was a problem hiding this comment.
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.
| </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. |
There was a problem hiding this comment.
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.
| <code_comparison> | ||
| <code title="Valid: String interpolation with proper validation and sanitization."> | ||
| <![CDATA[ | ||
| if ( isset( $_POST['name'] ) ) { |
There was a problem hiding this comment.
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.
| </code_comparison> | ||
| <standard> | ||
| <![CDATA[ | ||
| All validated input must be sanitized to remove or escape potentially malicious content before processing or output. |
There was a problem hiding this comment.
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:
| 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. |
| </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. |
There was a problem hiding this comment.
Since we mentioned all the superglobals above, maybe it is worth mentioning here the ones that require wp_unslash()?
| </code> | ||
| <code title="Invalid: Missing unslashing before sanitization."> | ||
| <![CDATA[ | ||
| if ( isset( $_POST['data'] ) ) { |
There was a problem hiding this comment.
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).
| > | ||
| <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. |
There was a problem hiding this comment.
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?
| <code title="Valid: String interpolation with proper validation and sanitization."> | ||
| <![CDATA[ | ||
| if ( isset( $_POST['name'] ) ) { | ||
| $safe_name = sanitize_text_field( wp_unslash( $_POST['name'] ) ); |
There was a problem hiding this comment.
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.
Add documentation for WordPress.Security.ValidatedSanitizedInput sniff. Related to #1722