4 ms·

The code is potentially vulnerable to XSS injections[1]. [1] https://github.com/sbstjn/timesheet.js/blob/master/source/javascripts/timesheet.js#L39-L41 https:/

by gildas 12y ago

The code is potentially vulnerable to XSS injections[1]. [1] https://github.com/sbstjn/timesheet.js/blob/master/source/javascripts/timesheet.js#L39-L41 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!"

Erwin 12y ago
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.
- gildas 12y ago
  Then, I think it should be documented.
  - rpedroso 12y ago
    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/ http://api.jquery.com/html/
    - sukilot 12y ago
      Why doesn't jquery have taint-checking/tagging, at least for data brought in through jquery's methods that read from URLs, etc.
- d23 12y ago
  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.
  - iso8859-1 12y ago
    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.
michaelmior 12y ago
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.
rmrfrmrf 12y ago
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).
mrcactu5 12y ago
Isn't this on Github. Can't you fork and write a better version?
- gildas 12y ago
  I am sorry, I won't fork and fix this code because I don't need it currently.
- konstruktor 12y ago
  Information is also a contribution.
  - mrcactu5 12y ago
    flapping your gums is not a contribution :-/
    - scott_karana 12y ago
      It's not just talking, though: they've outright circled the problem. Saying "Hello, there is a hole in your fence, your cows might escape" is valuable even if the "flapper" isn't going to fix it for you... ;)
tptacek 12y ago
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. :)
- cubano 12y ago
  > 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.
- gildas 12y ago
  Thanks for the advice, I was just a bit disappointed when I saw the downvotes. I promise I won't whine anymore. ;)
  - wahsd 12y ago
    You give 100% more shits about meaningless internet votes than you should
- eitland 12y ago
  > 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.
- angersock 12y ago
  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)

by gildas 12y ago

Erwin 12y ago

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.

gildas 12y ago
Then, I think it should be documented.
- rpedroso 12y ago
  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/ http://api.jquery.com/html/
  - sukilot 12y ago
    Why doesn't jquery have taint-checking/tagging, at least for data brought in through jquery's methods that read from URLs, etc.
d23 12y ago
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.
- iso8859-1 12y ago
  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.

michaelmior 12y ago

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.

rmrfrmrf 12y ago

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).

mrcactu5 12y ago

Isn't this on Github. Can't you fork and write a better version?

gildas 12y ago
I am sorry, I won't fork and fix this code because I don't need it currently.
konstruktor 12y ago
Information is also a contribution.
- mrcactu5 12y ago
  flapping your gums is not a contribution :-/
  - scott_karana 12y ago
    It's not just talking, though: they've outright circled the problem. Saying "Hello, there is a hole in your fence, your cows might escape" is valuable even if the "flapper" isn't going to fix it for you... ;)

tptacek 12y ago

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. :)

cubano 12y ago
> 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.
gildas 12y ago
Thanks for the advice, I was just a bit disappointed when I saw the downvotes. I promise I won't whine anymore. ;)
- wahsd 12y ago
  You give 100% more shits about meaningless internet votes than you should
eitland 12y ago
> 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.
angersock 12y ago
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)