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

pug-codegen: Abstract parts of the code generator that accumulate output #2958

Closed

Conversation

mikesamuel
Copy link

This consolidates code generator code that produces instructions to
append content to the output and allows customizing it.

This enables features like #2895
which distinguishes between strings definitely authored by the
template author and those possibly controlled by an attacker to escape
strings in context.

This approach allows the same hooks to both handle idiomatic pug like

a[href=x]

and inlne html like

<a href="#{x}"></a>

See also #2952

This consolidates code generator code that produces instructions to
append content to the output and allows customizing it.

This enables features like pugjs#2895
which distinguishes between strings definitely authored by the
template author and those possibly controlled by an attacker to escape
strings in context.

This approach allows the same hooks to both handle idiomatic pug like

```pug
a [href=x]
```

and inlne html like

```html
<a href="#{x}">
```

See also pugjs#2952
@ForbesLindesay
Copy link
Member

I'm not sure how broad the use case for this is. a[href=x] can be easily handled by a lexer plugin to simply parse the attrs token.

The reasons I've resisted adding these kinds of hooks to the pug compiler are:

  1. I think we may want to re-write the compiler to generate babel AST nodes and then use babel to generate the actual JS code - this would allow us to apply optimisations to that AST directly, as well as make it relatively easy to transpile newer js features.
  2. We're going to add source maps at some point. If plugins can override the generated code with different strings, that could to get really difficult.
  3. We want to build a compiler plugin API that covers broad use cases (e.g. lets you print custom AST nodes generated by a parser plugin). If we have a more limited plugin API, we could be stuck supporting that instead of the more powerful one we want to eventually build.

It's tempting to accept this as a simple alternative, but I'm not sure how much (if anything) it adds that can't already be done some other way. For example, you could already transform the href attributes in a postParse plugin - look for attributes with the key href test their value with constantinople.isConstant, if they are not constant, replace the original value with 'pug_escape_href(' + originalValue + ')'. You could then define pug_escape_href by pushing an un-buffered code node into the top of the template.

I'm not necessarily saying we should build this as that plugin (I think this should be a core feature), but I'm not sure this is the core API that's needed.

@mikesamuel
Copy link
Author

mikesamuel commented Feb 14, 2018

If I were writing this with no concern to efficiency, I would define a buffer object to accumulate output like

interface Buffer {
  // Appends a string in the output language from a trusted source
  appendStaticContent(content : string);
  // Appends a value from an unknown source.  If it is a string then
  // it should be treated as plain text and not assumed to be a fragment
  // of the output language.
  appendDynamicValue(value : any);
}

and tweak things so that

a[href=x] Foo

did something like

  outputBuffer.appendStaticContent('<a href="');
  outputBuffer.appendDynamicValue(x);
  outputBuffer.appendStaticContent('">');
  outputBuffer.appendStaticContent('Foo');
  outputBuffer.appendStaticContent('</a>');

This patch seeks to enable a buffer object like that but with easy coalescing of subsequent append statements while still allowing += without a buffer object when not using autoescaping.


we may want to re-write the compiler to generate babel AST nodes
If plugins can override the generated code with different strings, that could to get really difficult.
If we have a more limited plugin AP

If this were public, then any clients would need to migrate and/or do work to generate source map info correctly.

If this were not exposed via pug/lib/index.js then the amount of code that would need to be rewritten would be contained.


I'm not necessarily saying we should build this as that plugin (I think this should be a core feature), but I'm not sure this is the core API that's needed.

What kind of core API might enable the interface above?

@ForbesLindesay
Copy link
Member

For a[href=x] Foo, forget the code-gen step. We already have code-gen to produce efficient code for generating HTML, just add a parser step that outputs the appropriate pug AST nodes. You can see something similar done by https://github.com/pugjs/pug-plugin-debug-line and https://github.com/pugjs/pug-plugin-dynamic-include

You can use the preCodeGen step to walk the AST and for each element, check the attributes for any href attributes, and add code to escape. You don't need to plugin to the compiler for this.

@mikesamuel
Copy link
Author

Sorry for not responding sooner. I think I understand what's needed, but haven't found time to work on it yet. I will try to get something done next week.

@mikesamuel
Copy link
Author

I got pulled away by other things. I'm closing this now in favor of https://github.com/mikesamuel/pug-plugin-trusted-types

@mikesamuel mikesamuel closed this Sep 16, 2018
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