unicorn-fail / dreditor

The "Dreditor" browser extension for Drupal.org.
https://www.drupal.org/project/dreditor
GNU General Public License v2.0
72 stars 36 forks source link

Remove Issue Summary templates #262

Closed markhalliwell closed 8 years ago

markhalliwell commented 9 years ago

This is a follow-up from https://github.com/unicorn-fail/dreditor/issues/253#issuecomment-98425848 and the last feedback from a.m.o:

1) Your add-on creates DOM nodes from HTML strings containing unsanitized data, by assigning to innerHTML or through similar means. Aside from being inefficient, this is a major security risk. For more information, see https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion. Here are some examples where you do this:

<file name> <source line>
data/dreditor.js#L1569

Please fix them and submit again. Thank you.

This is specifically talking about the issue summary templates being hosted on d.o:

  insertSummaryTemplate: function ($textarea) {
    $.get('/node/1326662', function (data) {
      // Retrieve the template.
      var $template = $('<div/>').html($(data).find('#node-1326662 code').text());

They do no care that we can inherently trust d.o because we have restricted these nodes to a handful of people who require higher permissions to edit them. This must be moved to drupal.org: https://www.drupal.org/node/1393226

frob commented 8 years ago

Am I wrong in my assumption that we can just remove this file outright?

star-szr commented 8 years ago

@frob we can remove it but unless things have changed or I have completely misunderstood (both very possible) then the functionality needs to be added to drupal.org first before we can commit the removal here (see the d.o issue linked from the original issue post).

Edit: and thank you :)

frob commented 8 years ago

Yup, I understand that to be the case too. I just saw #256 and thought I would check off the lowest hanging fruit. We not have a pull request for this issue, and we can focus on the next. I will have to actually create a dev environment for any others, so that will have to wait till another free moment. :)

star-szr commented 8 years ago

@frob wonderful, thanks again.

markhalliwell commented 8 years ago

The PR was against the 2.x branch, so I went ahead and merged.

2.x is in preparation for all the CKE changes about to made on d.o, which will apparently have "templates" included with it (see https://www.drupal.org/node/2578695).

frob commented 8 years ago

Oh yeah, I planned that. I totally read all the docs and made the changes on the correct branch. ;)

star-szr commented 8 years ago

@markcarver ah I was wanting to ask about the 2.x branch, thanks! FYI I recently changed the default branch from 2.x to 1.x when merging another bugfix (I merged it to both branches). Feel free to change back if 2.x is more appropriate.

@frob ha :) thanks.

markhalliwell commented 8 years ago

@Cottser, yeah 2.x should be default. I just changed it back. 1.x is EOL now considering the changes that will need to be made for everything.

frob commented 8 years ago

So then, should all further contribution be done on the 2.x branch?