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

Escape-free, error-free, parser-based chalk-template #18

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

Conversation

midzelis
Copy link

Hey @sindresorhus and @Qix- - please check out this enhanced chalk-template function. This started out as a contribution to google/zx#635 - but after thinking about it, it would be more appropriate upstreamed here.

I happen to enjoy writing parsers - weird hobby, I know.

Background: When creating that PR for zx, I realized that I couldn't just send arbitrary text through chalkTemplate - it is very picky about escaping, and matching parenthesis, and styles, etc. I started to write a pre-processor for the template, to adopt a different start/end syntax. I was planning using new new prefix like ct:{ but that wasn't short enough, and it might have been common enough to require escaping, which is a PITA to handle correctly. Then I realized - hey I already have the parser that is pre-processing the template - I could just go all the way and do the actual styling too. This would also solve another problem I had with chalkTemplate - you couldn't use a template as an argument to a template, since it would be automatically escaped by chalkTemplate.

Then I realized - hey - we don't need to handle escapes at all. The syntax of {style or {rgb( or {# was already unique enough to not conflict with most arbitrary text. And, since its a parser, I didn't need to throw errors for mismatching { }s or anything - the fallback would be to just not style the unmatched text.

And so, the parser, AST, and renderer was born. This was written with typescript annotations as well, so I added typescript and prettier as dependencies.

Additionally, I changed the project layout a bit: created /src and /dist. Lastly, the exports were manipulated a little bit. There is no more default-export. There is one that uses chalk, one that uses chalkStderr, and one that you can use to bring your own chalk. There is also an 'experimental' export that exposes the AST. This is really just used to help play/develop the parser, but maybe its useful to someone. Its experimental as this is v1 of the AST and it may need some time to bake before being "API."

Technically, the parser is flexible to allow changing the 'prefix' of the template, currently set to {- this could be configurable, but currently it is not. The suffix is always '}' but that too could probably be configureable without much work - not sure if its a good idea.

Anyways, I'm wondering if there is any appetite for merging this PR, especially since this is basically a rewrite. I'm open to all comments/critiques - especially around the actual exported functions. And naming. Naming is hard. I'm bad at it.

This PR probably needs more documentation, more tests, and more general refinement. But I wanted to start the discussion before deciding if it was worth any more effort.

@Qix-
Copy link
Member

Qix- commented May 30, 2023

Hi there, this is a rather large departure from the current status quo, and it's also breaking a lot of expected functionality (e.g. (chalk`${'{bold escaped}'}`) is now bolded instead of being escaped). Since this might be formatting user input, and chalk might be extended in some cases, this leads to the potential for code injection to be performed, which was expressly why we chose to escape all interpolated tagged template values.

I'm not opposed to a more robust parser implementation but as of now I'm not really seeing the motivation other than the beginning/ending delimiters.

Instead, it might be simpler to construct the regular expression objects with the delimiters interpolated directly into the pattern using \Q and \E. Then the entire template function can be turned into a constructor that returns an instanced templater, with the default having { and } being the default delimiters.

Perhaps a bit of a hack to your particular problem, but as of now the change of base behavior is a non-starter for me, personally. Happy to discuss further, though :)

@midzelis
Copy link
Author

Hey! Thanks for your time and comments. I appreciate and share your concern for security. I’d be happy to work through your concerns to make sure that value is being added in a constructive way. To that end, before I do any more work, I’d like to talk through the use case, the concerns, and the mitigations and proposals - to make sure I'm on the right track.

Use Cases:

  1. Be able to output json in the same template as styles.
import {ct} from 'chalk-template'
log(ct`{bold {“hello":"there","brave":"world”}})
  1. Be able to nest templates
const header = '{bold.bgBlue  UPDATE }'
log(ct`${header} I just updated {green ${thingone}} to {green ${thingtwo}}`)
  1. Paraphrasing your comment: args must continue to be escaped
    1. for security, because the styles directly invoke functions on chalk ${'{bold escaped}'}
    2. because its too different from current behavior

Constraints

Assume the user does not have direct access to chalk or chalk-template, herein known as the "template fn." So all configuration must be via the string. i.e. assume the user just has access to a tagged template function called echo which takes a styled chalk-flavored string and args.

This is playing on hard-mode. Doing it within opts on the template fn is probably fine for most people.

Analysis

Point 1

Changing the delimiters of a style to a different character could work, but that would like require access to the template fn. Unless we take a page from sed, and allow the delimiter to be specified in the text it self. i.e. sed s/replace/me/g === sed s|replace|me|g.
How I solved [1] was to just not treat a single { as the delimiter. Instead, it is the combination of { plus valid chalk templates, verified against the chalkInstance. That means {hello} will just be {hello} but {bold hello} will be hello. However, note {notexisting style} will not be rendered since notexisting is looked up from the chalkInstance (unless, of course, a custom chalk had defined a style called notexisting). I don't think unintentional rendering would happen too often, since {style, {rgb(1,2,3, or {#000000 is pretty uncommon, but you know what they say about assumptions...

Mitigations:

1a. Changing the { to something else, even multi-chars: i.e. c>bold, c>rgb(1,2,3), c>#:000000
1b. Make it (even) more unlikely by adding a leading style: i.e. {chalk.bold, {chalk.rgb(1,2,3), ${chalk.#000000

Point 2

The extra constraint of only using strings is a bummer. I realized it could have been solved if I had just pre-rendered the template to ANSI and then just inserted the ANSI in the next template. i.e.

const header = ct`{bold.bgBlue  UPDATE }`; 
log(ct`${header} I just updated {green ${athing}} to {green ${bthing}}`)
Point 3

I totally agree that invoking arbitrary functions from template strings is risky. But breaking it down - its not always risky. Its not risky if you are in full control of the argument (i.e. did not come user). Its not risky if chalk hadn't been customized. But, lets be secure-by-default and make the default be escaping all arguments - but lets give the user an ability to opt-in to rendering args.

Mitigations:

3a. Create a new function nameTbd, which will cause the template fn to render the selected args.

log(ct`${nameTbd(header)} I just updated {green ${athing}} to {green ${bthing}}`)

or better yet, just use the template again

log(ct`${ct(header)} I just updated {green ${athing}} to {green ${bthing}}`)

-- which is basically an application of [2]
3b. Create a new keyword {nameTbd ${arg1}} but not ${arg2} which will render arg1 but not arg2.

Proposal

I'll do 1a and 1b. These will be options on the template builder fn.

import {ctbuilder} from 'chalk-template'
const ct = ctbuilder({ startDelimeter: 'c>', stylePrefix: 'chalk'})
log(ct`c>chalk.bold {"hello":"there"} c>chalk.green ${thing}}}`)

For 3 I will go ahead and make escaping of args true by default, to match previous behavior and to make it secure.
For 3a, will actually already work with no changes to the PR.
For 3b, this is playing on hard mode - and i think its pretty cool. A new keyword/directive that will cause the template renderer to render the template arg. The user would only use these on args that are known to be secure (i.e. not user input)

// note - no deps on chalk or chalk-template - just a function 
// that hides them and exposes the chalk-flavored template 
// via the third-party tagged template function `echo`
const header = '{bold.bgBlue  UPDATE }'
echo`{render ${header}} I just updated {green ${athing}} to {green ${bthing}}`)

All that being said, this definitely deserves a bump to 2.0.0.

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.

2 participants