Hacker News Reader

Comment by gildas | Hacker News Reader

The code is potentially vulnerable to XSS injections[1].

[1] https://github.com/sbstjn/timesheet.js/blob/master/source/ja...

edit: Thanks for the downvotes, lesson learned! Never talk about vulnerabilities of a code which is #1 on HN and people will blindly put in production.

edit 2: So here is my new comment: "Awesome library! The fact that users can easily enhance its behaviour by using <script> tags in labels is a great feature!"

View on HN

You're going to be happier and your comments more persuasive if you just ignore downvotes. Like, completely. Your comment was inevitably going to be bolted to the top of the thread regardless of the randos who downvoted you.

I upvoted you because good catch, but winced when I did it because of the downvote whining. :)

Thanks for the advice, I was just a bit disappointed when I saw the downvotes. I promise I won't whine anymore. ;)

> You're going to be happier and your comments more persuasive if you just ignore downvotes.

This is super awesome advice, and I'm thinking of having a one-off t-shirt made with some sort variation of it.

> You're going to be happier and your comments more persuasive if you just ignore downvotes.

Generally yes. That said some people seem to either belong on another forum or have somehow managed to collect enough karma to downvote before learning how to behave on HN.

tptacek speaks the truth--don't worry about how many HN funbucks you have.

I do like their second edit though...I enjoy having users extend my scripts. 8)

You're absolutely right, for what it's worth. Direct <script> tag injection didn't work when I tested it, but the following sure does!

    new Timesheet('timesheet-default', 2002, 2013, [
        ['09/2008', '06/2010', 
         '<img src="666" onerror="alert(\'injection!!\')">',
         'ipsum']
    ]);

Putting HTML elements in the label property itself certainly looks like it's unintended design-wise, and IMO escaping input should definitely be the responsibility of this library (or, as gildas suggested, it should at the very least be documented).

Well, it's a client side library. By that reasoning, jQuery can be said to be vulnerable to XSS if you call .html() with untrusted user input. It seems reasonable to me that the label of each bubble can contain HTML code, it's up to the user to ensure any user input is escaped.

Then, I think it should be documented.

Agreed. In fact, to build on 'Erwin's example, jquery makes note of the potential for XSS when calling .html(), .append(), .after(), etc.[1]

> Do not use these methods to insert strings obtained from untrusted sources such as URL query parameters, cookies, or form inputs. Doing so can introduce cross-site-scripting (XSS) vulnerabilities. Remove or escape any user input before adding content to the document.

[1] http://api.jquery.com/html/

Why doesn't jquery have taint-checking/tagging, at least for data brought in through jquery's methods that read from URLs, etc.

Yeah, I'm having a hard time seeing this as having an XSS vulnerability in and of itself. If you're not escaping user input onto the page, it doesn't matter if some library is taking that data and wrapping it in a span. You're done far before you add a third-party javascript library.

When I call that library, I shouldn't have to worry about the fact that it uses HTML. It is a leaky abstraction since a field that is, intuitively, supposed to be text for the label, is actually just treated as HTML for plain insertion into the page.

It might be a good idea to have separate label and labelHTML properties so users of the library don't have to think about this in the common case of text labels, but they can still use HTML labels if they want.

Reply on news.ycombinator.com