search

hoodiehq-archive / my-first-hoodie

⛔ deprecated
Other
157 stars 35 forks source link

XSS vulnerability in How to get started tutorial JavaScript #82

Closed oliverklee closed 8 years ago

oliverklee commented 8 years ago

The paint() function in main.js has a cross-site scripting vulnerability:

  $el.append(
    '<li data-id="' + collection[i].id + '">' +
      '<input type="checkbox"> <label>' + collection[i].title + '</label>' +
      '<input type="text" value="' + collection[i].title + '"/>' +
    '</li>'
  );

How to reproduce:

Add a to-do item with the following title: "/>

The text needs to be escaped (more correctly: encoded): https://stackoverflow.com/questions/24816/escaping-html-strings-with-jquery

bryanbierce commented 8 years ago

I see this needs done still. I can implement and submit a PR today.

oliverklee commented 8 years ago

I don't think this solution is the right way to go. (HTML) Escaping should not happen when data is stored. Instead, the escaping should be done in context-specific way when the data is used, i.e., the HTML escaping needs to happen when the data is inserted in the HTML.

gr2m commented 8 years ago

yeah I agree, sorry I didn’t pay enough attention when merging it. my-first-hoodie will soon be legacy, it’s not used by the new Hoodie anymore. But we have a new demo app: https://github.com/hoodiehq/hoodie-app-tracker – it might be affected by the same issue :) Wanna have a look?

oliverklee commented 8 years ago

Done. That version also is vulnerable … I'll file a ticket in a minute.