Skip to content

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

Merged
matthew-dean merged 7 commits into
less:masterfrom
codepen:plugin_flag
May 4, 2022
Merged

matthew-dean merged 7 commits into
less:masterfrom
codepen:plugin_flag

Conversation

@broofa

@broofa broofa commented Mar 14, 2022 •

Copy link
Copy Markdown
Contributor

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 codebase.

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

parse: function (str, callback, additionalData) {
let root;
let error = null;
let err = null;

@broofa broofa Mar 14, 2022 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown

@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

broofa commented Mar 15, 2022 •

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Member

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

broofa commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

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

broofa commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

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.

Comment thread packages/less/bin/lessc Outdated
@edhgoose

edhgoose commented Mar 28, 2022 •

Copy link
Copy Markdown
Contributor

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

broofa commented Apr 12, 2022

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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

broofa commented Apr 18, 2022

Copy link
Copy Markdown
Contributor Author

@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

broofa commented Apr 22, 2022

Copy link
Copy Markdown
Contributor Author

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

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

broofa commented May 4, 2022

Copy link
Copy Markdown
Contributor Author

@matthew-dean Sweet! Thanks for taking this.

Any idea when the next release will go out?

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

vosatom commented Aug 11, 2023 •

Copy link
Copy Markdown

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.

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.

6 participants