Escape Quotes #1511

Closed
btakita opened this Issue Jan 12, 2016 · 29 comments

Projects

None yet

5 participants

@btakita
btakita commented Jan 12, 2016

Strange things happen when there's an unescaped quote in the html

<beverly-hillbillies>
  <p>Y'all come back now...</p>
  <script>
    console.info('I am not run');
  </script>
</beverly-hillbillies>
@GianlucaGuarini
Member

Are you sure? Can you make a live example?

@btakita
btakita commented Jan 12, 2016

I had a live example, but I fixed it. I'll follow up if I can reproduce it.

I'm using browserify, es2015, es2015-riot

On Tue, Jan 12, 2016 at 7:31 AM, Gianluca Guarini notifications@github.com
wrote:

Are you sure? Can you make a live example?


Reply to this email directly or view it on GitHub
#1511 (comment).

Brian Takita
briantakita.com
about.me/brian_takita

@jkirkwood

Just encountered the same thing:

var riot = require('riot');

var tag = `
<app>
  <h1>Compiler doesn't like this...</h1>
  <script>
    var a = 'test';
  </script>
</app>
`;

var compiled = riot.compile(tag);

console.log(compiled);

// riot.tag2('app', '<h1>Compiler doesn\'t like this...</h1> <script> var a = \'test\'; </script>', '', '', function(opts) {
// });
@aMarCruz aMarCruz self-assigned this Jan 13, 2016
@aMarCruz
Member

@jkirkwood are you sure?
I can't reproduce your case. See this jsbin.

maybe an issue with your preprocessor?

@jkirkwood

I'm not using a preprocessor. I'm running the script exactly as written on node 4.2.4 using riot 2.3.13. Is it possible this bug only appears while compiling server-side?

@cognitom
Member

I've confirmed the issue:
http://jsbin.com/zajiqeropo/1/edit?html,js,console

When the template has single quote, <script> block seems not processed correctly.

// with single quote
riot.tag2('app', '<h1>With quote\'</h1> <script> var a = \'test\'; </script>', '', '', function(opts) {
});
// without single quote
riot.tag2('app', '<h1>Without quote</h1>', '', '', function(opts) {
    var a = 'test';
});
@cognitom
Member

@aMarCruz with v2.3.12 it works correctly. It could be related to the change of RegEx on v2.3.13?

@aMarCruz
Member

I'm confirming the issue, it will be fixed soon.

@aMarCruz
Member

@cognitom right, this issue was introduced with the fix of riot#1448. I can do a regression, but deep tests shows other problems. HTML strings are very different from JS strings (i.e. multiline, can't be escaped, entities). E.g. based on riot specs, <tag attr="{'"'}" /> must be a valid syntax, but generates error even if the inner double-quote is escaped, because \ is not significant in HTML so I have to test for expressions in the openning tag.
Also, this is a valid riot tag:

<tag style='
  top:{ a>b?"0":"5px" },
  left:0'><p> Foo's { '-' } bar's baz' </p>
  I'm HTML text
  <p att="<script>out('value')</script>"></p>  // valid script. evaluated? ...depends.
  var x="<script>out('value')</script>"  // I'm JS string? ..depends.
  var z='<!-- removed? -->'
</tag>

but is difficult to parse with regexes, and multiline JS strings need break on ''. I need separated regexes for strings in 1) attributes, 2) html body, 3) expressions and js code.

I will do my best to support most common cases, but there is a limit for regexes, so I'll start to write a mini-parser for riot tags after fixing this issue (perhaps this not slow too much the compilation).

btw: I think riot-checker and lib/server/analyzer.js needs an update.

@cognitom
Member

Oh, so much complicated ><
Thanks for explanation. It's also true that riot-checker needs update. Wondering the way to sync the spec of compiler and checker...

@cognitom
Member

At this point, I think this issue has higher priority than #1448
Too difficult to handle comments and nested <script> with regex. It needs the parser, as you said.

@aMarCruz
Member

@cognitom I think so, will publish a new version of riot-compiler ASAP.

@cognitom
Member

@aMarCruz cool. Thank you!

At least on server-side, I prefer parser than regex, too. I can't wait to see your mini-parser :-)

@aMarCruz aMarCruz added a commit to riot/compiler that closed this issue Jan 14, 2016
@aMarCruz aMarCruz v2.3.21 ac325cb
@aMarCruz
Member

@cognitom thanks, if have time can you test the code in the dev branch of riot-compiler please? seems ok in my tests, the fix 1448 is working as well. I will publish the new version tomorrow.

cc @GianlucaGuarini

@GianlucaGuarini
Member

@aMarCruz cool.. let's fix some other bugs and roll out a new release

@cognitom
Member

@aMarCruz Thank you! LGTM.
The point around this issue is here?
https://github.com/riot/compiler/blob/ac325cbd1e02af6d1d006b40cc83cbbc4701bdae/lib/core.js#L560-L561

Regular expression visualization

Regular expression visualization

Anyway, huuuuge regex. Good job 😲

@aMarCruz
Member

@cognitom exactly! the regex is complicated and does not cover some cases with expressions and literal regexes but it is fast, relatively.

Great tool in debuggex.com, did not know it, I'm using regex101.com in Linux and regexBuddy in Windows. Thanks.

@aMarCruz
Member

The complete regex for <style> is:
Regular expression visualization

this skips tags inside quotes (one-line). The drawback is this slows the search here, because the regex matches all the strings within the tag (and the same for <script> and both js and html comments), so I'm now thinking remove this feature for style and script -- I can't hide all the strings beforehand 'cause the html strings can contain expressions.

@aMarCruz
Member

I will make a regression of riot#1448...

with the fix:

Performance test for the riot-compiler v2.3 against the v2.2.3 release
======================================================================

Testing each compiler with 9 files, 10x5000 iterations (450000 total)
Running the compiler v2.2.4 ...
Running the compiler v2.3.0 ...

Results         old 2.2.4   new 2.3.x
-------------  ----------- -----------
box         :       3686        9008
empty       :       1069         947
input-last  :       2272        3165
mixed-js    :       2598        2955
same        :       2104        2457
scoped      :       4326        3968
timetable   :       2139        3384
treeview    :      13343       24917
oneline     :        416        2152
TOTAL       :      31953       52953

removing the fix:

Results         old 2.2.4   new 2.3.x
-------------  ----------- -----------
box         :       3722        7527
empty       :       1078         941
input-last  :       2268        2503
mixed-js    :       2526        2934
same        :       2036        2422
scoped      :       4312        3918
timetable   :       2057        2581
treeview    :      14017       22141
oneline     :        439        2039
TOTAL       :      32455       47006

users can replace the < for insertion of script/style tags in JS strings. Ex. var s='\x3cscript><\/script>'

@cognitom
Member

Or var s = '<scr' + 'ipt>. Sometimes I do.

@GianlucaGuarini
Member

@aMarCruz @cognitom how many times someone needs to inject a <script> tags in this way? For now I would not invest too much time on this fix and @aMarCruz you are right users can try workaround for this edge case.

@aMarCruz
Member

@GianlucaGuarini right, anyway I found a the way although some minor issues remain, I will update the docs and publish the new version.

@aMarCruz aMarCruz added a commit to riot/compiler that referenced this issue Jan 15, 2016
@aMarCruz aMarCruz v2.3.21
- Code refactorization of the `parsers` module, removed unuseful CSS parser `stylus` for browsers.
- Fix riot/riot#1511 : Escape Quotes - They may be some issues to fix.
- New logic for parsing `script/style` blocks. See doc/guide.md for details.
2a58a9a
@btakita
btakita commented Jan 19, 2016

@GianlucaGuarini @aMarCruz can i use master to get this bugfix? i need it for backticks in es6 to work.

@aMarCruz
Member

@btakita sorry by the delay, I will push the v2.3.22 in ~12 hrs, advances is in the dev branch of riot-compiler.

ADD: perhaps you can use <p>Y&#39;all come back now...</p> for now.

@btakita
btakita commented Jan 19, 2016

@aMarCruz thank you! i appreciate the hard & inspired work

@btakita
btakita commented Jan 19, 2016

@aMarCruz actually, riot/compiler#47 is the root issue that i'm having.

i attempted to use require for an es6 module, which resulted in a parse error.

i'd rather have es6 modules working.

@btakita
btakita commented Jan 19, 2016

Here is the console error when i attempt to use backtick:

console.log(`${logPrefix}|mount`);
(node) util.error is deprecated. Use console.error instead.
WARN: ERROR: Unexpected character '`' [public/dist/app.js:2457,18]

/test-project/node_modules/uglify-js2/lib/parse.js:199
    throw new JS_Parse_Error(message, line, col, pos);
    ^
Error
    at new JS_Parse_Error (/test-project/node_modules/uglify-js2/lib/parse.js:185:18)
    at js_error (/test-project/node_modules/uglify-js2/lib/parse.js:199:11)
    at parse_error (/test-project/node_modules/uglify-js2/lib/parse.js:291:9)
    at Object.next_token [as input] (/test-project/node_modules/uglify-js2/lib/parse.js:523:9)
    at next (/test-project/node_modules/uglify-js2/lib/parse.js:616:25)
    at subscripts (/test-project/node_modules/uglify-js2/lib/parse.js:1258:13)
    at subscripts (/test-project/node_modules/uglify-js2/lib/parse.js:1239:20)
    at expr_atom (/test-project/node_modules/uglify-js2/lib/parse.js:1120:20)
    at maybe_unary (/test-project/node_modules/uglify-js2/lib/parse.js:1278:19)
    at expr_ops (/test-project/node_modules/uglify-js2/lib/parse.js:1313:24)
    at maybe_conditional (/test-project/node_modules/uglify-js2/lib/parse.js:1318:20)
    at maybe_assign (/test-project/node_modules/uglify-js2/lib/parse.js:1349:20)
    at expression (/test-project/node_modules/uglify-js2/lib/parse.js:1368:20)
    at simple_statement (/test-project/node_modules/uglify-js2/lib/parse.js:822:55)
@btakita
btakita commented Jan 19, 2016

@aMarCruz scratch the issue with using require on es6 modules. i had a syntax error.

@aMarCruz
Member

@btakita It happens to all of us all the time 😉

@aMarCruz aMarCruz added a commit to riot/compiler that referenced this issue Jan 23, 2016
@aMarCruz aMarCruz v2.3.22
- Complete code refactorization with comments in preparation for JSDoc.
- Preserves the fix riot/riot#1511 from the deleted v2.3.21, without the single-quote error.
- Removed the "compress" option of the `less` parser, it is deprecated and generates warnings on the console.
- Various tweaks to increase performance and reduce (~55%) memory consumption.
- Files to preprocess are moved from "lib" to the "src" directory, now "lib" has the required node.js files only.
e658252
@aMarCruz aMarCruz added a commit to riot/compiler that referenced this issue Jan 24, 2016
@aMarCruz aMarCruz v2.3.22
- Fix riot/riot#1511 : Escape Quotes - They may be some issues to fix.
- Regression of logic to parse style and script tags, due to loss of performance and other issues.
- Removed the "compress" option of the `less` parser, which is deprecated and generates warnings in the console.
- Removed the unuseful CSS parser `stylus` from the browser version.
- Refactorization of all the code, with more comments in preparation for the automatic documentation of the API.
- Various tweaks to increase performance and reduce (~55%) memory consumption.
- Files to preprocess are moved from "lib" to the "src" directory, now "lib" has the required node.js files only.
132d8af
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment