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

Security/Underscorejs: bug fixes and enhancements #595

Merged
merged 8 commits into from
Nov 23, 2020

Conversation

jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Nov 1, 2020

Result of a review of the WordPressVIPMinimum.Security.Underscorejs sniff.

Not claiming completeness as JS, like PHP, can be written in lots of different ways - think ternaries, method chaining etc - and my JS is a bit rusty ;-).
Consider this a first round of improvements.

Input regarding underscores syntaxes based on:

Code samples largely based on samples found via:

Fixes #546


Commit details

Security/Underscorejs: sniff all text string tokens

So far, only single quoted text strings, heredocs and inline HTML were sniffed.
However, there are two more text string token types: nowdoc and double quoted strings.

This adds these tokens to the register() method to be sniffed.

This commit also ensures that there is at least one unit test in place for each of these token types and that the tests use a variety of whitespace.

Security/Underscorejs: throw an error for each violation

So far, the sniff would throw one error per text string, independently of how often the unescaped output notation would occur in the text string.

While for simple text strings, this is not much of an issue, for long and complex text strings, it may be more difficult to spot all the unescaped output notations in the text string.

This changes the sniff to:

  • Throw a warning for each occurrence of the unescaped output notation in a text string.
  • Include a snippet from the text string with the details of the unescaped output notation.
    Old: Found Underscore.js unescaped output notation: "<%=".
    New: Found Underscore.js unescaped output notation: "<%= post_url %>".

To allow the sniff to also match on a text string being concatenated together, the regex has to also match when <%= occurs at the end of the text string and will only display <%= in that case.
To this end, we also need to remove the text string quotes (if they exist) from the token content.

Security/Underscorejs: add JS test case file

Add a basic JS test case file.

Tests for interpolate will be added in a follow-up commit.

Security/Underscorejs: improve check for interpolate in JS files

The interpolate property in JS can be set using various syntaxes. While the current check will in no way catch them all, the changes now made should improve the accuracy of detecting a change in the delimiter via interpolate in JS files.

Notes:

  • Add checking for T_STRING to detect _.templateSettings.interpolate = syntax. This was previously not detected (false negative).
  • Add some defensive coding to prevent false positives when the interpolate keyword is used in another context than underscorejs.
  • Limit the check for T_STRING and T_PROPERTY to Javascript files only to prevent false positives when scanning PHP files.
  • Limit the check for interpolate in text strings to PHP only. This prevents false positives when the keyword is used in a text string in JS.

Includes unit tests covering all the above.

Security/Underscorejs: improve check for interpolate in PHP files

Match the interpolate property in PHP files with the similar precision as in JS files.

Includes unit tests.

Security/Underscorejs: bug fix - don't error when variable is escaped

Prevent triggering a warning when the variable being printed is escaped using _.escape().

Includes unit tests.

Fixes #345

Security/Underscorejs: ignore Gruntfile.js files

These are configuration files and not part of the production code.

This check does not verify whether this file is in the project root as we don't know what the project root is. It will plainly ignore any file called Gruntfile.js in a case-insensitive manner.

Includes unit tests.

Security/Underscorejs: enhancement - check for print execution statements

Add a new check for the JS native print command and the _p+= variation, when used without being combined with _.escape().

Includes unit tests.

jrfnl added 8 commits October 31, 2020 19:07
So far, only single quoted text strings, heredocs and inline HTML were sniffed.
However, there are two more text string token types: nowdoc and double quoted strings.

This adds these tokens to the `register()` method to be sniffed.

This commit also ensures that there is at least one unit test in place for each of these token types and that the tests use a variety of whitespace.
So far, the sniff would throw one error per text string, independently of how often the unescaped output notation would occur in the text string.

While for simple text strings, this is not much of an issue, for long and complex text strings, it may be more difficult to spot all the unescaped output notations in the text string.

This changes the sniff to:
* Throw a warning for each occurrence of the unescaped output notation in a text string.
* Include a snippet from the text string with the details of the unescaped output notation.
    Old: `Found Underscore.js unescaped output notation: "<%=".`
    New: `Found Underscore.js unescaped output notation: "<%= post_url %>".`

To allow the sniff to also match on a text string being concatenated together, the regex has to also match when `<%=` occurs at the end of the text string and will only display `<%=` in that case.
To this end, we also need to remove the text string quotes (if they exist) from the token content.
Add a basic JS test case file.

Tests for `interpolate` will be added in a follow-up commit.
The `interpolate` property in JS can be set using various syntaxes. While the current check will in no way catch them all, the changes now made should improve the accuracy of detecting a change in the delimiter via `interpolate` in JS files.

Notes:
* Add checking for `T_STRING` to detect `_.templateSettings.interpolate =` syntax. This was previously not detected (false negative).
* Add some defensive coding to prevent false positives when the `interpolate` keyword is used in another context than underscorejs.
* Limit the check for `T_STRING` and `T_PROPERTY` to Javascript files only to prevent false positives when scanning PHP files.
* Limit the check for `interpolate` in text strings to PHP only. This prevents false positives when the keyword is used in a text string in JS.

Includes unit tests covering all the above.
Match the `interpolate` property in PHP files with the similar precision as in JS files.

Includes unit tests.
Prevent triggering a warning when the variable being printed is escaped using `_.escape()`.

Includes unit tests.

Fixes 345
These are configuration files and not part of the production code.

This check does not verify whether this file is in the project root as we don't know what the project root is. It will plainly ignore any file called `Gruntfile.js` in a case-insensitive manner.

Includes unit tests.
…ents

Add a new check for the JS native `print` command and the `_p+=` variation, when used without being combined with `_.escape()`.

Includes unit tests.
@jrfnl jrfnl added this to the 2.3.0 milestone Nov 1, 2020
@jrfnl jrfnl requested a review from a team as a code owner November 1, 2020 05:45
Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

Love it!

@rebeccahum rebeccahum merged commit 4b3d7f9 into develop Nov 23, 2020
@rebeccahum rebeccahum deleted the fix/345-underscorejs-output-escaping branch November 23, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review the WordPressVIPMinimum.Security.Underscorejs sniff UnderscoreJS output escaping improvement?
3 participants