feat: add disablePlugins flag for render() options

[broofa]

What:

Adds support for a options.disablePlugins flag on the less.render() method. Setting this to true disables parsing of @plugin statements and, instead, throws an error.

Why:

As @plugin statements may be used to run arbitrary system commands. This is a significant security risk for applications that need to process untrusted code. At a minimum, applications should be able to disable parsing of @plugin statements.

As noted by my colleague @shshaw here at CodePen, we ended up forking this repo so we could disable @plugin processing. Adding this option will allow us and our users to stay current with the official less codepase.

See also:

• Prevent use of plugin and imports from untrusted sources #3561
• https://github.com/codepen/cp/issues/1195

How:

Calling less.render(inputString, {disablePlugins: true}) activates the new codepath. With this option enabled, the parsers.plugin() method is replaced with one that failes with an error when an @plugin statement is detected.

Checklist:

• Documentation N/A (where are the docs in this repo?)
• Added/updated unit tests
• Code complete

[broofa]

Note: Had to rename this, as it was shadowing the error function.

[matthew-gill]

@broofa this is amazing! So helpful for our usecase. Forgive my ignorance with JS, but we currently invoke this module via:

node_modules/less/bin/lessc

Would this PR introduce a command line arg like this disablePlugins=true ?
If not can it be added or can I help you add it?

Thanks!

[broofa]

Would this PR introduce a command line arg like this disablePlugins=true ? If not can it be added or can I help you add it?

@matthew-gill heh... I actually misread your comment as "can you help add it" and so just added support for a --disable-plugins flag in the lessc command. For example:

$ cat foo.less
@plugin "autofix";

$ ./bin/lessc --disable-plugins foo.less
SyntaxError: @plugin statements are not allowed when disablePlugins is set to true in /Users/kieffer/codepen/less.js/packages/less/foo.less on line 1, column 9:
1 @plugin "autofix";
2

That said, we (CodePen) don't have an immediate need for this; we're just using the programmatic JS API. I'm happy to revert the CLI flag support. (Might be a good idea, to avoid complicating the CLI interface unnecessarily?)

Thoughts?

[matthew-gill]

@broofa oh wow! Thanks for doing this!
As this is against the upstream less.js, I'd LOVE to keep this in if that's ok? If your PR gets merged, this'll help us tremendously. Appreciate the efforts 👏

[iChenLei]

where are the docs in this repo?

@broofa https://github.com/less/less-docs , docs repo is standalone. Thanks for your contribution, great idea and implemention. 👍

[broofa]

I'd LOVE to keep this in if that's ok?

Of course. Is anything else needed here, then? Would love to see this merged and published sometime in the not-too-distant future, but it's not urgent.

[broofa]

Thoughts on how difficult this would be to backport to less@3? That's the version our CodePen fork is off of. If we can get this on a 3.x release, we'd be able to just do a minor update to remove the fork dependency.

[edhgoose]

We'd love it backported to 3 too - we're in the same boat as you @broofa.

(P.s. thank you for working on this, it's really appreciated!!)

[broofa]

@matthew-dean: Are you the right person to nag about the status of this?

I've discovered yet another down-side to being on our own fork of less: It prevents us from leveraging the community defined TS declarations in @types/less (... because we're importing this as "@codepen/less" instead of "less", which breaks the inferred @type/ type module detection that TS tools use. 😢)

[matthew-dean]

Hi all. A few notes about this.

One is that probably this should be disableInlinePlugins or disablePluginRule (whatever makes sense to people, maybe the latter?). The reason is that there are compiler-level / configuration plugins and it's ambiguous as to which type of plugin inclusion you're referring to. (Unless you truly want to disable all plugins? But then, does this do that?)

Next, yes, I would support it at the lessc level. Ideally, all option flags in lessc should have parity with the JS object options.

Finally, re: backporting to 3.0. The main reason for the 4.0 release is that math=parens-division is the default option, which is technically a breaking change. (See: https://lesscss.org/usage/#less-options-math)

It's basically the same as 3.0 other than that. So, no, I don't think backporting is a good option. What I would do for CodePen, if it were me, is to not make this breaking change, just use 4.0, but set Less's options to be math=all for existing pens, and then use the default setting (parens-division) for new pens, as it results in less developer error, especially as / has been added over the years to more and more CSS prop values.

[broofa]

@matthew-dean

One is that probably this should be disableInlinePlugins or disablePluginRule (whatever makes sense to people, maybe the latter?). The reason is that there are compiler-level / configuration plugins and it's ambiguous as to which type of plugin inclusion you're referring to. (Unless you truly want to disable all plugins? But then, does this do that?)

I've renamed disablePlugins -> disablePluginRule, and the CLI flag to --disable-plugin-rule.

Next, yes, I would support it at the lessc level. Ideally, all option flags in lessc should have parity with the JS object options.

👍

Finally, re: backporting to 3.0. The main reason for the 4.0 release is that math=parens-division is the default option, which is technically a breaking change. (See: https://lesscss.org/usage/#less-options-math)

It's basically the same as 3.0 other than that. So, no, I don't think backporting is a good option.

Sounds good. I'll run this by folks here to see if there are any issues on our end and circle back if so. In the meantime, if we could get this out for 4.x, that would be awesome.

Let me know if you need anything else here.

[broofa]

Sounds good. I'll run this by folks here to see if there are any issues on our end and circle back if so. In the meantime, if we could get this out for 4.x, that would be awesome.

FYI, the consensus here (codepen) is that we can work around the math change on our end. (tl;dr: we'll just set math = 'always' for legacy pens to retain the 3.X behavior.)

[broofa]

@matthew-dean Sweet! Thanks for taking this.

Any idea when the next release will go out?

[vosatom]

Is there any reason to disable both --plugin and --disable-plugin-rule at the same time since it is not disablePlugins anymore?

I would like use plugins enabled from outside (CLI or options), but keep @plugin disabled.