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.

Sign up for GitHub

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

Throw error if closing tag contains Muststache Statement #982

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

Throw error if closing tag contains Muststache Statement #982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dea60c7
Throw error if closing tag contains Muststache Statement
camerondubas Oct 23, 2019
7f4c379
Throw a SyntaxError if closing tag contains a MustacheStatement
camerondubas Oct 25, 2019
f493c33
Remove debugger
camerondubas Oct 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/@glimmer/syntax/lib/parser/handlebars-node-visitors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,15 @@ export abstract class HandlebarsNodeVisitors extends Parser {
);

case TokenizerState.beforeAttributeName:
if (this.currentNode && this.currentNode.type === 'EndTag') {
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised we enter this state during a close tag 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I was surprised as well. This state is hit in this scenario: <div></div {{some-modifier}}>, and it was actually already throwing this error"Error: expected start tag" which is triggered in the this.currentStartTag call on line 160, but doesn't really describe the error accurately.

currentStartTag snippet:

  get currentStartTag(): Tag<'StartTag'> {
    let node = this.currentNode;
    assert(node && node.type === 'StartTag', 'expected start tag');
    return node as Tag<'StartTag'>;
  }

If you comment out this if check, two of the tests I added will fail, throwing the expected start tag error instead of the one added in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

OK, gotcha, thank you for explaining.

I believe what is going on is that the HTML spec requires the tagname immediately after the </.

See the spec info here:

https://html.spec.whatwg.org/multipage/parsing.html#end-tag-open-state

And our implementation here:

https://github.com/tildeio/simple-html-tokenizer/blob/691662c9f00aec719d0eec8e622ed99935119c55/src/evented-tokenizer.ts#L482-L491

I think essentially we want an else to the if above that throws a useful error (RE: invalid end tag). I think that would look something like:

    endTagOpen() {
      let char = this.consume();

      if (char === '@' || char === ':' || isAlpha(char)) {
        this.transitionTo(TokenizerState.endTagName);
        this.tagNameBuffer = '';
        this.delegate.beginEndTag();
        this.appendToTagName(char);
      } else {
        this.delegate.reportSyntaxError('invalid closing tag');
      }
    }

Would you be interested in working on a fix over in simple-html-tokenizer also?

Copy link
Member

Choose a reason for hiding this comment

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

I also think this is a bit wrong:

https://github.com/tildeio/simple-html-tokenizer/blob/691662c9f00aec719d0eec8e622ed99935119c55/src/evented-tokenizer.ts#L266-L282

We have created the endTagName state, and made it essentially the same as the tagName state (the spec actually says to use the tagName state (see here), but thats a tad bit difficult for us). But honestly, I don't think we actually are allowed to enter those other states (beforeAttributeName and selfClosingStartTag). We should probably add errors there too.

Copy link
Author

Choose a reason for hiding this comment

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

I'll make an attempt at fixing those issues in Simple HTML Tokenizer this weekend. I'll open a PR with the fixes, or an issue, if I'm unable to come up with solutions

Thanks for looking into this!

Copy link
Author

Choose a reason for hiding this comment

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

Made a PR here tildeio/simple-html-tokenizer#73 would love feedback 😄

throw new SyntaxError(
`Cannot use mustaches in an element's closing tag: \`${this.sourceForNode(
rawMustache,
rawMustache.path
)}\` at L${loc.start.line}:C${loc.start.column}`,
loc
);
}
addElementModifier(this.currentStartTag, mustache);
break;
case TokenizerState.attributeName:
Expand All @@ -173,6 +182,14 @@ export abstract class HandlebarsNodeVisitors extends Parser {
case TokenizerState.attributeValueUnquoted:
appendDynamicAttributeValuePart(this.currentAttribute!, mustache);
break;
case TokenizerState.endTagOpen:
throw new SyntaxError(
`Cannot use mustaches in an element's closing tag: \`${this.sourceForNode(
rawMustache,
rawMustache.path
)}\` at L${loc.start.line}:C${loc.start.column}`,
loc
);

// TODO: Only append child when the tokenizer state makes
// sense to do so, otherwise throw an error.
Expand Down
18 changes: 18 additions & 0 deletions packages/@glimmer/syntax/test/parser-node-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,24 @@ test('disallowed mustaches in the tagName space', function(assert) {
}, /Cannot use mustaches in an elements tagname: `{{bar` at L1:C6/);
});

test('disallowed mustaches in closing tag', function(assert) {
assert.throws(() => {
parse('\nbefore <div></ {{some-modifier}} div>');
}, new Error("Cannot use mustaches in an element's closing tag: `{{some-modifier` at L2:C15"));

assert.throws(() => {
parse('\nbefore <div><span></ {{some-modifier}} span></div>');
}, new Error("Cannot use mustaches in an element's closing tag: `{{some-modifier` at L2:C21"));

assert.throws(() => {
parse('\nbefore <div></div {{some-modifier}}>');
}, new Error("Cannot use mustaches in an element's closing tag: `{{some-modifier` at L2:C18"));

assert.throws(() => {
parse('\nbefore <div><span></span {{some-modifier}} ></div>');
}, new Error("Cannot use mustaches in an element's closing tag: `{{some-modifier` at L2:C25"));
});

test('mustache immediately followed by self closing tag does not error', function() {
let ast = parse('<FooBar data-foo={{blah}}/>');
let element = b.element('FooBar/', ['attrs', ['data-foo', b.mustache('blah')]]);
Expand Down