Allow annotations in source code to ignore a check

[xntrik]

Is your feature request related to a problem? Please describe. Due to manual review required checks always being directly unaddressable, we can't run electronegativity in CI without ignoring the return code.

Describe the solution you'd like When running Electronegativity in CI it'd be great to have a means to ignore particular checks through adding an annotation in the source code, similar to eslint's ignore annotation. https://eslint.org/docs/user-guide/configuring.html#disabling-rules-with-inline-comments

Describe alternatives you've considered Optionally an exclude flag to allow excluding a particular check from the runtime (although this would be global for the entire scan, which isn't ideal)

[ikkisoft]

I will let speak the current maintainer @phosphore - but it's pretty cool to see you here 👍

[phosphore]

Hello @xntrik and thank you for your feedback!

You were right about the need for an eslint-like annotation system, so we pushed it on d41c8c3ab7fdc908b5e78aa4f2c7a58638e3dd28. For now we only introduced the support for a minimal clone of eslint-disable-line (here called eng-disable). We'll release a new minor version by this EOW including these changes.

Ignoring Lines or Files

Electronegativity lets you disable individual checks using eng-disable comments. For example, if you want a specific check to ignore a line of code, you can disable it as follows:

const res = eval(safeVariable); /* eng-disable DANGEROUS_FUNCTIONS_JS_CHECK */

<webview src="https://doyensec.com/" enableblinkfeatures="DangerousFeature"></webview> <!-- eng-disable BLINK_FEATURES_HTML_CHECK -->

Any eng-disable inline comment (// eng-disable, /* eng-disable */, <!-- eng-disable -->) will disable the specified check for just that line. It is also possible to provide multiple check names using both their snake case IDs (DANGEROUS_FUNCTIONS_JS_CHECK) or their construct names (dangerousFunctionsJSCheck):

shell.openExternal(eval(safeVar)); /* eng-disable OPEN_EXTERNAL_JS_CHECK DANGEROUS_FUNCTIONS_JS_CHECK */

If you put an eng-disable directive before any code at the top of a .js or .html file, that will disable the passed checks for the entire file. For the moment we don't support having an eng-disable directive at the beginning of a function to exclude the whole function, but we'll work on it.

Excluding specific checks

Since this would still leave out GlobalChecks, we also introduced in 1497db664c9468bc244d37cfec38a1c792731a2c a new command line flag (-x) which can be used to exclude a list of checks (similarly to the existing -l option).

Let me know if we're missing some other similar features, we're always open to new ideas on how to improve this tool!

[xntrik]

Awesome!

[phosphore]

I'll leave this issue open for some time in case someone have any feedback on d41c8c3ab7fdc908b5e78aa4f2c7a58638e3dd28 or 1497db664c9468bc244d37cfec38a1c792731a2c.

[avin-shum]

Thank you for working on the eng-disable directive at the beginning of a function, because inline comment is quite long so that it will be placed to the other line very often when using code formatter.

[ghost]

👋 Hi there! We're integrating electronegativity into https://github.com/hashicorp/boundary-ui. Overall this is very helpful! We're now able to selectively annotate lines that have been manually checked so that they no longer alert us every time. We did run into a few potential issues, all of these use the inline disable flag /* eng-disable FLAG */:

• When the CSP cannot be parsed, electronegativity raises an alert and it cannot be disabled inline (CSP_GLOBAL_CHECK).
• When occurrences of navigation limiting are disabled (LIMIT_NAVIGATION_JS_CHECK), electronegativity raises a new alert "Missing navigation limits using .on new-window and will-navigate events" that cannot be disabled inline.
• When occurrences of permissions handling checks are disabled (PERMISSION_REQUEST_HANDLER_GLOBAL_CHECK), electronegativity raises a new alert "Missing PermissionRequestHandler to limit specific permissions (e.g. openExternal) in response to events from particular origins." that cannot be disabled inline.

[phosphore]

Hello @randallmorey! In the latest tag (v1.8.1 and above) you should be able to exclude those checks via CLI arguments (-x LimitNavigationJsCheck,PermissionRequestHandlerJsCheck,CSPJsCheck).

Nonetheless, since I recently received similar feedback from the user base (#88, #84, #85) I decided to change approach. Global checks can now be disabled using inline annotations (9079add37e620d8d580c73ec07e3d04f3ae251c1). Note that using Global Checks annotations may not be applicable for CSP_GLOBAL_CHECK or AVAILABLE_SECURITY_FIXES_GLOBAL_CHECK. For those cases, you might want to use the -x flag to exclude specific checks from your scan.

I will publish a new version including this change (v1.9.0) by this EOW.