Security/ValidatedSanitizedInput: add XML documentation#2698
Security/ValidatedSanitizedInput: add XML documentation#2698rodrigoprimo wants to merge 3 commits into
Conversation
- Add introductory standard block explaining what the sniff checks, using language that avoids the overloaded term "validated". - Rewrite standard descriptions to explain why each rule exists. - Add <em> tags to highlight key parts in code examples. - Remove specific mention of XSS as the attack vector. - Focus each section on its specific error without mixing concerns. - List which superglobals require wp_unslash() in MissingUnslash section. - Use varied superglobals and sanitizing functions across code examples. - Keep code examples within 48-character column width.
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
Thanks for picking this up @rodrigoprimo ! I've skimmed through it (my brain is fried), but left some remarks anyway. Hope this helps.
| <standard> | ||
| <![CDATA[ | ||
| Superglobals must not be used directly in string interpolation or heredocs. The array key might not exist, or its value could contain malicious content that gets included in the string without sanitization. | ||
| ]]> | ||
| </standard> | ||
| <code_comparison> | ||
| <code title="Valid: No superglobal in string interpolation."> | ||
| <![CDATA[ | ||
| // No superglobal in string interpolation. | ||
| ]]> | ||
| </code> | ||
| <code title="Invalid: Superglobal used in string interpolation."> | ||
| <![CDATA[ | ||
| echo "Hello <em>{$_GET['name']}</em>"; | ||
| ]]> | ||
| </code> | ||
| </code_comparison> |
There was a problem hiding this comment.
How about moving this block to the end ? The other sections more logically follow the intro. This is kind of the "odd one out", making it more logic to have it at the end IMO.
| <code title="Valid: No superglobal in string interpolation."> | ||
| <![CDATA[ | ||
| // No superglobal in string interpolation. |
There was a problem hiding this comment.
How about having an example of how to do this properly instead ?
| All input must be sanitized to remove potentially malicious content before it is used. | ||
| ]]> | ||
| </standard> | ||
| <code_comparison> |
There was a problem hiding this comment.
Not a must-have, but it may simplify the example code a little to use $_ENV instead (which doesn't need unslashing) ?
Or maybe having an example with both $_POST as well as $_ENV would help users ?
| <![CDATA[ | ||
| if ( <em>isset( $_POST['name'] )</em> ) { | ||
| $name = sanitize_text_field( | ||
| wp_unslash( <em>$_POST['name']</em> ) |
There was a problem hiding this comment.
Not sure these <em>'s are needed - the point of the example is the isset(), isn't it ? If nothing else, it would make the <em> more consistent across the examples.
| > | ||
| <standard> | ||
| <
Description
This PR adds XML documentation for the
WordPress.Security.ValidatedSanitizedInputsniff.The documentation is based on the work started by @rooh-wp311 in #2587. I used the original commit and, in a separate commit, made the following changes to the original version of the documentation:
<em>tags to highlight key parts in code examples.wp_unslash()inMissingUnslashsection.I suggest squashing the two commits before merging. I'm opening the PR without doing that to make it easier to tell my changes apart from the original changes.
Suggested changelog entry
N/A
Related issues/external references
Related to: #1722
Supersedes: #2587
Closes #2587