</script> isn't escaped in network graph

Created by: davispuh

in Project Network Graph, commit messages are in javascript but there's interesting special case for escaping </script> for example if I've such commit message: fixed missing </script> it will break graph totally

it's because text inside <script> (in HTML) are parsed in specific way and </script> is ending script block and making all next parsed as HTML

(look at "message":"fixed missing </script>")

<script>
  //<![CDATA[
   var chunk1={commits:[{"parents":[["756400df57485ee8df5418628adb404b236fb000",0,0]],"author":"D\u0101vis","time":8,"space":1,"refs":"master","id":"e4dbf8bfc539d46ff33b24f6241cc805608705a6","date":"2012-09-19T16:07:58+00:00","message":"fixed missing </script>","login":"davispuh@local"}]};
    var days=[[19,"Sep"]];
    initGraph();
    $(function(){
      branchGraph($("#holder")[0]);
      initGraphNav();
    });
  //]]>
</script>

interesting is that if page would be served as XHTML (XHTML5) this would perfectly work because CDATA wouldn't be parsed at all..

this can be fixed if we escape </script> with <\/script> or even / with \/

Created by: davispuh

davispuh/UberTest@132c717a46b4a5ae0c289307d1ba7bb8e5edeae2 POC showing XSS in /{project}/graph

push to gitlab and navigate to Project Network Graph

By Administrator on 2012-09-19T23:44:50 (imported from GitLab project)

By Administrator on 2012-09-19T23:44:50 (imported from GitLab)

Created by: davispuh

61049424 kinda fix this, but actually not properly, because it introduces new "bug" (or feature :D)

if I make commit with message: expecting <sometag and="attributes"> including </script> :) and then looking at graph it will be fine, but mouseover commit dot I'll see expecting including :), because attributes and some tags are stripped (eg.<b> isn't)

EDIT

here's one funny: <h1>GitLab</h1> is <NOT> bad, <3 it will become <h1>GitLab</h1> is bad, &lt;3 so I think this needs fixing :D

By Administrator on 2012-09-20T07:12:23 (imported from GitLab project)

By Administrator on 2012-09-20T07:12:23 (imported from GitLab)

Created by: dzaporozhets

LOL

By Administrator on 2012-09-20T07:15:09 (imported from GitLab project)

By Administrator on 2012-09-20T07:15:09 (imported from GitLab)

Created by: dzaporozhets

yeap. at least we fixed a script injection :)

By Administrator on 2012-09-20T07:15:35 (imported from GitLab project)

By Administrator on 2012-09-20T07:15:35 (imported from GitLab)

Created by: davispuh

can't say it's real fix, it's more a workaround, stripping </script> (along with other tags,attributes...) from message doesn't solve real cause, it's actually really tricky place due nature of HTML parser which does parse <script>, but in specific way: ignore everything except </script>

// </script> WTF, not possible to write it even in comment :(
console.log("<\/script>"); // that's how we can bypass...

with XHTML it's so much easier...

By Administrator on 2012-09-20T07:42:06 (imported from GitLab project)

By Administrator on 2012-09-20T07:42:06 (imported from GitLab)

Created by: SaitoWu

@randx seems like fenced code block is broken after some commits in readme.md file.

Origin File:

HOW TO USE IBATOR

Make sure ruby 1.9.x installed.

[sudo] gem install active_support
git clone git@aix:ibator.git
cd ibator
cp model.example.rb model.rb
vim model.rb
ruby ibator.rb [folders | file]

MODEL EXAMPLE

SPUProperty.rb

@package = "item"

@model = "SPUProperty"

@db_attributes = {
  "id" => "BIGINT",
  "spuId" => "BIGINT",
  "propertyKeyId" => "BIGINT",
  "propertyValueId" => "BIGINT",
  "rank" => "INT"
}

By Administrator on 2012-09-20T08:11:16 (imported from GitLab project)

By Administrator on 2012-09-20T08:11:16 (imported from GitLab)

Created by: dzaporozhets

@SaitoWu yeap. I'll fix it

By Administrator on 2012-09-20T08:33:53 (imported from GitLab project)

By Administrator on 2012-09-20T08:33:53 (imported from GitLab)

Created by: dzaporozhets

@SaitoWu @tsigo this caused by github-markup. We should create an issue for this

By Administrator on 2012-09-20T09:24:11 (imported from GitLab project)

By Administrator on 2012-09-20T09:24:11 (imported from GitLab)

Created by: davispuh

didn't noticed ed899a2f but it doesn't solve it any better than previous fix...

<h1 attr="awesome">GitLab</h1> is <NOT> bad, <3 becomes <h1 attr="awesome">GitLab</h1> is <NOT> bad, <3

By Administrator on 2012-09-21T08:37:56 (imported from GitLab project)

By Administrator on 2012-09-21T08:37:56 (imported from GitLab)

Created by: dzaporozhets

the problem we use

 on network graph. So I cannot quickly fix it before 2.9 release.
By Administrator on 2012-09-21T08:41:20 (imported from GitLab project)
By Administrator on 2012-09-21T08:41:20 (imported from GitLab)

Created by: davispuh

then why I can :D

in ./lib/gitlab/graph_commit.rb change line 21 from

@commits_json = commits.map(&:to_graph_hash).to_json

to

@commits_json = commits.map(&:to_graph_hash).to_json.gsub(/"[^"\\]*(?:\\.[^"\\]*)*"/) {|m| m.gsub(/<\//,"<\\/") }

and 169

 h[:message] = escape_once(Gitlab::Encode.utf8(message))

back to original

 h[:message] = Gitlab::Encode.utf8(message)

it does solve it perfectly, maybe it could be solved in different way as that regexp could be considered as a little hack, but it does work way better than any other proposed fix...

any commit messages is shown exactly as it should, including commit messages containing </script>

EDIT

that regexp replaces every </ with <\/ inside double quotes

By Administrator on 2012-09-21T09:00:58 (imported from GitLab project)

By Administrator on 2012-09-21T09:00:58 (imported from GitLab)

Created by: koenpunt

@randx Is this issue still relevant?

By Administrator on 2012-12-04T00:01:55 (imported from GitLab project)

By Administrator on 2012-12-04T00:01:55 (imported from GitLab)

Created by: davispuh

it's workaround'ed... I still don't understand why it couldn't be fixed properly like I said...

with newest version (just took from repository)

notice how HTML is escaped twice... workaround could be ok if it wouldn't introduce side effects...

By Administrator on 2012-12-04T00:38:29 (imported from GitLab project)

By Administrator on 2012-12-04T00:38:29 (imported from GitLab)

Created by: koenpunt

@davispuh Ok, thanks, will look into this today.

By Administrator on 2012-12-04T10:46:43 (imported from GitLab project)

By Administrator on 2012-12-04T10:46:43 (imported from GitLab)

Created by: koenpunt

Submitted a pull request which resolves this issue, but it's still not ideal, using JSON as JS objects. So the next step should be; separately load the commit data as JSON after the page has loaded, this would avoid all the in-page escape/entity issues.

By Administrator on 2012-12-05T00:07:56 (imported from GitLab project)

By Administrator on 2012-12-05T00:07:56 (imported from GitLab)

Created by: koenpunt

Just updated my PR, the graph now loads the JSON data asynchronously, and the explicit escaping of the HTML is no longer necessary, because to_json already escapes enough.

By Administrator on 2012-12-07T21:15:29 (imported from GitLab project)

By Administrator on 2012-12-07T21:15:29 (imported from GitLab)

Created by: koenpunt

@randx See my PR #2175 :)

By Administrator on 2012-12-08T13:58:35 (imported from GitLab project)

By Administrator on 2012-12-08T13:58:35 (imported from GitLab)