#136 enhancement
Fabien Ménager

Script tags evaluated when commented in Ajax requests

Reported by Fabien Ménager | March 13th, 2009 @ 10:52 AM

Hello, I looked at the existing tickets to see if it had been already reported, but didn't find it (sorry if I'm wrong). I've encountered this bug a few times in my scripts, as I sometimes comment parts of my code, and found that the script tags where evaluated even if they were commented. I think the ScriptFragment regex should be modified (I don't know how, but it should be easy) in order not to evaluate script tags between comment tags. I hope this wouldn't make the eval slower. In that case that wouldn't be a good idea.

Comments and changes to this ticket

  • Tobie Langel

    Tobie Langel March 13th, 2009 @ 02:23 PM

    • Milestone cleared.
    • State changed from “new” to “bug”
    • Assigned user set to “Tobie Langel”

    Hi, thanks for your bug report. I'll just poke a bit of fun at your for your classic comment: "I don't know how, but it should be easy." But will look at handling this for next releases.

  • Fabien Ménager

    Fabien Ménager March 13th, 2009 @ 09:12 PM

    The easier solution would be in my opinion to strip the comments before extracting the script tags.

  • Juriy Zaytsev

    Juriy Zaytsev March 13th, 2009 @ 09:23 PM

    I might be missing something, but how are we going to detect comment blocks without a parser? Regex can not do that, as far as understand.

    How would these examples be parsed properly?

    
    (function() {
      return 'foo bar // baz ';
    })();
    
    (function(){
      var x = ' " /* " ';
      alert(1);
      var y = '*/'
    })();
    
  • Fabien Ménager

    Fabien Ménager March 13th, 2009 @ 09:29 PM

    I was talking about HTML comment tags :

    
    <!-- foo - bar -->
    

    that should be removed before eval because thay could contain script tags that we wouldn't want to be evaluated.

  • Juriy Zaytsev

    Juriy Zaytsev March 13th, 2009 @ 09:33 PM

    @Fabien

    My bad, I see what you mean now. I think we had similar request somewhere here. Need to check older tickets.

  • Juriy Zaytsev

    Juriy Zaytsev March 13th, 2009 @ 09:55 PM

    It would be easier to strip comments first, and then extract any scripts, etc.

    I'm thinking of:

    
    // http://www.w3.org/TR/html401/intro/sgmltut.html#h-3.2.4
    var CommentFragment = /<!--.*?--\s*>/g;
    
    // and simply strip comments in `extractScripts` 
    // before extracting anything else:
    ...
    this.replace(CommentFragment, '');
    ...
    
  • Yaffle
  • Yaffle

    Yaffle March 15th, 2009 @ 07:27 AM

    
    <script ...><![CDATA[...
    
    if(x<!--i){
    ...
    }
    
    if(--x>0){
     ...
    }
    
    ]]</script>
    
  • Juriy Zaytsev

    Juriy Zaytsev March 15th, 2009 @ 01:25 PM

    @Yaffle

    Yes. That's because "<--" and "-->" sequences are ignored in CDATA sections, AFAIK.

    The following snippet, for example, would be incorrectly stripped by the CommentFragment regex (even though both sequences are contained within CDATA sections, which proper parser would ignore):

    
    <script type="text/javascript">
      alert('<!--');
    </script>
    foo bar baz
    <script type="text/javascript">
      alert('-->')
    </script>
    

    Any ideas on what we should do about it?

  • Fabien Ménager

    Fabien Ménager March 15th, 2009 @ 01:33 PM

    • Assigned user changed from “Tobie Langel” to “Andrew Dupont”

    @Yaffle Yes, and sadly, there are also things like that :

    
    <script type="text/javascript">
    <!--
    alert('foo bar');
    -->
    </script>
    
    

    The code between comments should be parsed. The solution would be in changing the original script fragment to match script tags that are not between <!-- and --> . I know a little bit Regex, but not enough to make such a Regex :(

  • Juriy Zaytsev

    Juriy Zaytsev March 15th, 2009 @ 02:09 PM

    Yeah, trying to parse an arbitrary chunk of text and break it into proper script sections is silly. Every browser already has a native HTML parser, which we can take advantage of:

    
    function parse(s) {
      var root = document.body || document.documentElement;
      var el = document.createElement('div');
      el.innerHTML = s;
      root.removeChild(root.appendChild(el));
      el = root = null;
    };
    
    var s = '<!--<script type="text/javascript">alert(1)</script>-->';
    parse(s); // not executed
    
    s = '<script type="text/javascript">alert(1)</script>';
    parse(s); // executed
    
    s = '<script type="text/javascript"><!--\nalert(1);\n--></script>';
    parse(s); // executed
    

    The only downside to this is that it's impossible to create a generic container to insert content into; Safari 2.x, for example, only executes script elements from within head.

  • Fabien Ménager

    Fabien Ménager March 15th, 2009 @ 02:22 PM

    That would be a pretty good idea, but we would have to extract the script chunks to insert into the dummy div.

  • Tobie Langel

    Tobie Langel March 15th, 2009 @ 03:14 PM

    The only downside to this is that it's impossible to create a generic container to insert content into; Safari 2.x, for example, only executes script elements from within head.

    Which is I the main reason why this isn't supported. I think this should be marked as a documentation issue, and left alone.

  • Juriy Zaytsev

    Juriy Zaytsev May 22nd, 2009 @ 03:34 PM

    • Assigned user changed from “Andrew Dupont” to “T.J. Crowder”
    • State changed from “bug” to “doc”
    • Milestone cleared.
  • Yaffle

    Yaffle May 27th, 2009 @ 06:26 PM

    scripts like '//<!--\n alert(2); \n//-->' or
    '//<![CDATA[\n alert(2); \n//-->' wouldn't be evaluated in IE 6 with any of eval function ( https://prototype.lighthouseapp.com/projects/8886/tickets/433-provi... )

    so

    
    Prototype.ScriptFragment = '<script[^>]*>(?:\\s*<!\\-\\-)?(?:\\s*<\\!\\[CDATA\\[)?([\\S\\s]*?)(?:\\]\\]>\\s*)?(?:\\-\\->\\s*)?<\/script>';
    
  • Yaffle

    Yaffle May 27th, 2009 @ 06:35 PM

    sorry, eval works fine for

    '// <!--\n alert(2); \n //-'+'->' or '//<![CDATA[ \n alert(2); \n// ]]>' (if CDATA and '<!--' commented in javascript )</p>

  • Juriy Zaytsev

    Juriy Zaytsev May 27th, 2009 @ 08:23 PM

    @Yaffle

    I think OP's issue is related to commented out script elements being evaluated, not the content of script elements themselves:

    var testee = "<!--<script type='text/javascript'>alert(1)</script>-->";
    
    var re = new RegExp('<script[^>]*>([\\S\\s]*?)<\/script>', 'g');
    var re2 = new RegExp('<script[^>]*>(?:\\s*<!\\-\\-)?(?:\\s*<\\!\\[CDATA\\[)?([\\S\\s]*?)(?:\\]\\]>\\s*)?(?:\\-\\->\\s*)?<\/script>', 'g');
    
    
    testee.match(re2); // ["<script type='text/javascript'>alert(1)</script>"]
    
  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 01:54 AM

    • Tag changed from ajax, ajax.request, comments, evalscripts, performance, script to performance, section:ajax

    [not-tagged:"ajax" tagged:"section:ajax" bulk edit command]

  • T.J. Crowder

    T.J. Crowder November 16th, 2009 @ 04:50 PM

    • Assigned user cleared.

    [responsible:none bulk edit command]

  • Tobie Langel

    Tobie Langel November 29th, 2009 @ 07:21 PM

    • State changed from “doc” to “new”
    • Assigned user set to “Samuel Lebeau”
  • Tobie Langel
  • Dan Dean

    Dan Dean February 28th, 2010 @ 11:55 PM

    • Tag set to enhancement, needs:patch
  • Dan Dean

    Dan Dean February 28th, 2010 @ 11:55 PM

    • State changed from “new” to “open”
  • Dan Dean

    Dan Dean March 1st, 2010 @ 12:12 AM

    • State changed from “open” to “enhancement”
    • Tag changed from enhancement, needs:patch to needs:patch

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Pages