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

feat: Introduce a separate parsing_method rule #244

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rpl
Copy link
Member

@rpl rpl commented Aug 22, 2024

This PR move shared bits of the method rule into a base_method.js module and then reuse the helper function exported by that method to create two separate method and parsing_method rules.

The purpose of splitting the method rule into two separate rules is to report violations detected by the parsing_method rule as warnings instead of errors by default, and allow developers to opt-in/opt-out on parsing methods violations separately from the method rule.

@rpl rpl force-pushed the feat/parsing-method-rule-warning branch 2 times, most recently from ceb2644 to 5d1d053 Compare August 22, 2024 10:43
@mozfreddyb
Copy link
Collaborator

Right. This would help with #81, which is a long-standing issue for some users.

docs/rules/parsing_method.md Outdated Show resolved Hide resolved
docs/rules/parsing_method.md Outdated Show resolved Hide resolved
docs/rules/parsing_method.md Outdated Show resolved Hide resolved
tests/rules/parsing_method.js Show resolved Hide resolved
@Rob--W
Copy link
Member

Rob--W commented Aug 26, 2024

Is the purpose of this to add validation of Document.parseHTMLUnsafe?

Don't forget to also add support for setHTMLUnsafe: https://developer.mozilla.org/en-US/docs/Web/API/Element/setHTMLUnsafe

@rpl
Copy link
Member Author

rpl commented Aug 26, 2024

Is the purpose of this to add validation of Document.parseHTMLUnsafe?

Don't forget to also add support for setHTMLUnsafe: https://developer.mozilla.org/en-US/docs/Web/API/Element/setHTMLUnsafe

@Rob--W That is already covered by the 'method' rule, see

@rpl rpl force-pushed the feat/parsing-method-rule-warning branch 4 times, most recently from 5ee194b to b0ed868 Compare August 26, 2024 20:16
index.js Show resolved Hide resolved
@rpl rpl marked this pull request as ready for review August 26, 2024 20:18
@rpl
Copy link
Member Author

rpl commented Aug 26, 2024

@mozfreddyb I opened up this PR for a review pass on your side (in the meantime I applied the fixes for the typos in the markdown file, fixed the recommended config, and the README.md file which had a syntax error, and added a base integration test executed by the github workflow, which would have caught the issue with the typo in the severity level of the new rule in the previous version of this PR).

@rpl rpl requested a review from mozfreddyb August 26, 2024 20:20
@rpl rpl self-assigned this Aug 26, 2024
Copy link
Collaborator

@mozfreddyb mozfreddyb left a comment

Choose a reason for hiding this comment

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

Looks great. Just a few things missing :)

lib/rules/parsing_method.js Outdated Show resolved Hide resolved
lib/rules/parsing_method.js Outdated Show resolved Hide resolved
tests/integration/test-eslint-flat-config.js Outdated Show resolved Hide resolved
tests/rules/parsing_method.js Outdated Show resolved Hide resolved
tests/rules/parsing_method.js Outdated Show resolved Hide resolved
tests/rules/parsing_method.js Outdated Show resolved Hide resolved
errors: [
{
message:
/Unsafe call to DOMParser.parseFromString for argument 0/,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it could be cool to call them "Potentially unsafe call" or even more concrete "Parsing can be unsafe when....? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

That feels like a good point to be too, let me take a look to what additional change may be needed in this PR if we would like to use slightly different message for the separate parsing_method rule.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mozfreddyb I have included in afe2df6 one possible approach to customize the message (for now I went for the simpler "Potentially unsafe call..." one, but if the approach looks reasonable we can tweak the message further).

I have also changed the objectMatches for the DOMParser parseFromString method to 'parser' for now.

I was actually tempted to go even for just 'parse' or 'pars' (so that it would also match other variations, like 'htmlParsing' or 'parseHTML' or 'parseUtils'), wdyt?

@rpl rpl force-pushed the feat/parsing-method-rule-warning branch from ba0ea56 to 98508bd Compare September 2, 2024 14:33
Copy link
Collaborator

@mozfreddyb mozfreddyb left a comment

Choose a reason for hiding this comment

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

Almost ready to go :) Thank you, Luca!

Comment on lines +24 to +31
valid: [
{
code: "var doc = parser.parseFromString('static string');",
},
{
code: "var doc = Document.parseHTMLUnsafe('static string');",
},
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would a sanitized version of this look like?

parseHTMLUnsafe(sanitize(badness)) or would sanitizeDocument(parseHTMLUnsafe(badness)) be more likely? I am leaning to (1) and would like to suggest to add a simplified test case here

Comment on lines +3 to +7
The _parsing_method_ rule in _eslint-plugin-no-unsanitized_ performs basic security
checks for function calls that parse strings into new Document or DocumentFragment
instances. The idea of these checks is to allow developers to opt-in/opt-out of detecting
use of these methods, separately from the `method` rule which is reporting violation
as errors by default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The _parsing_method_ rule in _eslint-plugin-no-unsanitized_ performs basic security
checks for function calls that parse strings into new Document or DocumentFragment
instances. The idea of these checks is to allow developers to opt-in/opt-out of detecting
use of these methods, separately from the `method` rule which is reporting violation
as errors by default.
The _parsing_method_ rule in _eslint-plugin-no-unsanitized_ performs basic security
checks for function calls that parse strings into new Document or DocumentFragment
instances. It is close to impossible to automatically check whether the resulting
Document is actually used insecurely, so this offers a control at the parse step.
Therefore, these checks allow developers to opt-in/opt-out of detecting
use of these methods, separately from the `method` and `property` rule which is
reporting violation as errors by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants