feat: add `disablePlugins` flag for render() options #3701

broofa · 2022-03-14T21:54:50Z

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:

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 · 2022-03-14T21:55:21Z

packages/less/src/less/parser/parser.js

@@ -149,12 +149,22 @@ const Parser = function Parser(context, imports, fileInfo) {
        //
        parse: function (str, callback, additionalData) {
            let root;
-            let error = null;
+            let err = null;


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

matthew-gill · 2022-03-15T09:46:23Z

@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 · 2022-03-15T14:26:08Z

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 · 2022-03-15T14:44:01Z

@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 · 2022-03-15T14:52:20Z

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 · 2022-03-15T17:32:53Z

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 · 2022-03-15T18:36:06Z

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.

packages/less/bin/lessc

edhgoose · 2022-03-28T19:00:43Z

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!!)

Co-authored-by: Edward Hartwell Goose <edhgoose@users.noreply.github.com>

broofa · 2022-04-12T23:03:02Z

@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 · 2022-04-16T17:59:51Z

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 · 2022-04-18T14:14:26Z

@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 · 2022-04-22T18:47:44Z

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 · 2022-05-04T17:29:33Z

@matthew-dean Sweet! Thanks for taking this.

Any idea when the next release will go out?

vosatom · 2023-08-11T10:25:48Z

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.

feat: add disablePlugins support

d893ff4

broofa commented Mar 14, 2022

View reviewed changes

iChenLei requested a review from matthew-dean March 15, 2022 02:09

broofa added 3 commits March 15, 2022 06:54

cleanup eslint warnings in lessc

2665c1e

--disable-plugins command line option

f61bdf9

improve warning message

8cb32de

fixup broken test

43a0a89

edhgoose reviewed Mar 28, 2022

View reviewed changes

packages/less/bin/lessc Outdated Show resolved Hide resolved

Update packages/less/bin/lessc

3f46f52

Co-authored-by: Edward Hartwell Goose <edhgoose@users.noreply.github.com>

rename disablePlugins -> disablePluginRule

60bf94a

matthew-dean approved these changes May 4, 2022

View reviewed changes

matthew-dean merged commit 7491578 into less:master May 4, 2022

broofa deleted the plugin_flag branch May 4, 2022 22:17

ITSecOps-404 mentioned this pull request Jun 25, 2024

[Snyk] Upgrade less from 4.1.3 to 4.2.0 ITSecOps-404/aports#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `disablePlugins` flag for render() options #3701

feat: add `disablePlugins` flag for render() options #3701

broofa commented Mar 14, 2022

broofa Mar 14, 2022 •

edited

Loading

matthew-gill commented Mar 15, 2022

broofa commented Mar 15, 2022 •

edited

Loading

matthew-gill commented Mar 15, 2022

iChenLei commented Mar 15, 2022

broofa commented Mar 15, 2022

broofa commented Mar 15, 2022

edhgoose commented Mar 28, 2022 •

edited

Loading

broofa commented Apr 12, 2022

matthew-dean commented Apr 16, 2022

broofa commented Apr 18, 2022

broofa commented Apr 22, 2022

broofa commented May 4, 2022

vosatom commented Aug 11, 2023 •

edited

Loading

feat: add disablePlugins flag for render() options #3701

feat: add disablePlugins flag for render() options #3701

Conversation

broofa commented Mar 14, 2022

broofa Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

matthew-gill commented Mar 15, 2022

broofa commented Mar 15, 2022 • edited Loading

matthew-gill commented Mar 15, 2022

iChenLei commented Mar 15, 2022

broofa commented Mar 15, 2022

broofa commented Mar 15, 2022

edhgoose commented Mar 28, 2022 • edited Loading

broofa commented Apr 12, 2022

matthew-dean commented Apr 16, 2022

broofa commented Apr 18, 2022

broofa commented Apr 22, 2022

broofa commented May 4, 2022

vosatom commented Aug 11, 2023 • edited Loading

feat: add `disablePlugins` flag for render() options #3701

feat: add `disablePlugins` flag for render() options #3701

broofa Mar 14, 2022 •

edited

Loading

broofa commented Mar 15, 2022 •

edited

Loading

edhgoose commented Mar 28, 2022 •

edited

Loading

vosatom commented Aug 11, 2023 •

edited

Loading