#233 Multiple fixes
Merged 5 years ago by . Opened 5 years ago by pingou.

file modified
+3 -2

@@ -205,13 +205,14 @@ 

                      row = 'command="/usr/bin/gl-auth-command %s",' \

                          'no-port-forwarding,no-X11-forwarding,'\

                          'no-agent-forwarding,no-pty %s' % (

-                             user.user, user.public_ssh_key)

+                             user.user, user.public_ssh_key.strip())

                  elif gitolite_version == 3:

                      row = 'command="HOME=%s '\

                          '/usr/share/gitolite3/gitolite-shell %s",' \

                          'no-port-forwarding,no-X11-forwarding,'\

                          'no-agent-forwarding,no-pty %s' % (

-                             gitolite_home, user.user, user.public_ssh_key)

+                             gitolite_home, user.user,

+                             user.public_ssh_key.strip())

                  else:

                      raise pagure.exceptions.PagureException(

                          'Non-supported gitolite version "%s"' %

file modified
+9

@@ -400,3 +400,12 @@ 

              api_error_codes_doc,

          ],

      )

+ 

+ 

+ @APP.route('/api/')

+ @APP.route('/api')

+ def api_redirect():

+     ''' Redirects the user to the API documentation page.

+ 

+     '''

+     return flask.redirect(flask.url_for('api_ns.api'))

file modified
+36 -19

@@ -8,11 +8,16 @@ 

  

  """

  

+ import re

  from flask.ext import wtf

  import wtforms

  # pylint: disable=R0903,W0232,E1002

  

  

+ STRICT_REGEX = '^[a-zA-Z0-9-_]+$'

+ RELAXED_REGEX = '^[a-zA-Z0-9 #]+$'

+ 

+ 

  class ProjectFormSimplified(wtf.Form):

      ''' Form to edit the description of a project. '''

      description = wtforms.TextField(

@@ -33,7 +38,10 @@ 

      ''' Form to create or edit project. '''

      name = wtforms.TextField(

          'Project name <span class="error">*</span>',

-         [wtforms.validators.Required()]

+         [

+             wtforms.validators.Required(),

+             wtforms.validators.Regexp(STRICT_REGEX, flags=re.IGNORECASE)

+         ]

      )

  

  

@@ -41,7 +49,10 @@ 

      ''' Form to create or edit an issue. '''

      title = wtforms.TextField(

          'Title<span class="error">*</span>',

-         [wtforms.validators.Required()]

+         [

+             wtforms.validators.Required(),

+             wtforms.validators.Regexp(RELAXED_REGEX, flags=re.IGNORECASE)

+         ]

      )

      issue_content = wtforms.TextAreaField(

          'Content<span class="error">*</span>',

@@ -55,23 +66,11 @@ 

  

  class IssueForm(IssueFormSimplied):

      ''' Form to create or edit an issue. '''

-     title = wtforms.TextField(

-         'Title<span class="error">*</span>',

-         [wtforms.validators.Required()]

-     )

-     issue_content = wtforms.TextAreaField(

-         'Content<span class="error">*</span>',

-         [wtforms.validators.Required()]

-     )

      status = wtforms.SelectField(

          'Status',

          [wtforms.validators.Required()],

          choices=[(item, item) for item in []]

      )

-     private = wtforms.BooleanField(

-         'Private',

-         [wtforms.validators.optional()],

-     )

  

      def __init__(self, *args, **kwargs):

          """ Calls the default constructor with the normal argument but

@@ -89,14 +88,21 @@ 

      ''' Form to create a request pull. '''

      title = wtforms.TextField(

          'Title<span class="error">*</span>',

-         [wtforms.validators.Required()]

+         [

+             wtforms.validators.Required(),

+             wtforms.validators.Regexp(RELAXED_REGEX, flags=re.IGNORECASE)

+         ]

      )

  

  

  class AddIssueTagForm(wtf.Form):

      ''' Form to add a comment to an issue. '''

      tag = wtforms.TextField(

-         'tag', [wtforms.validators.Optional()]

+         'tag',

+         [

+             wtforms.validators.Optional(),

+             wtforms.validators.Regexp(STRICT_REGEX, flags=re.IGNORECASE)

+         ]

      )

  

  

@@ -143,7 +149,11 @@ 

  class UpdateIssueForm(wtf.Form):

      ''' Form to add a comment to an issue. '''

      tag = wtforms.TextField(

-         'tag', [wtforms.validators.Optional()]

+         'tag',

+         [

+             wtforms.validators.Optional(),

+             wtforms.validators.Regexp(STRICT_REGEX, flags=re.IGNORECASE)

+         ]

      )

      depends = wtforms.TextField(

          'dependency issue', [wtforms.validators.Optional()]

@@ -221,7 +231,10 @@ 

      ''' Form to add a group to a project. '''

      group = wtforms.TextField(

          'Group <span class="error">*</span>',

-         [wtforms.validators.Required()]

+         [

+             wtforms.validators.Required(),

+             wtforms.validators.Regexp(STRICT_REGEX, flags=re.IGNORECASE)

+         ]

      )

  

  

@@ -267,7 +280,11 @@ 

      """ Form to ask for a password change. """

      group_name = wtforms.TextField(

          'Group name  <span class="error">*</span>',

-         [wtforms.validators.Required(), wtforms.validators.Length(max=16)]

+         [

+             wtforms.validators.Required(),

+             wtforms.validators.Length(max=16),

+             wtforms.validators.Regexp(STRICT_REGEX, flags=re.IGNORECASE)

+         ]

      )

      group_type = wtforms.SelectField(

          'Group type',

file modified
+4 -23

@@ -372,11 +372,7 @@ 

             request.project.name,

             'pull-request',

             request.id))

-     mail_to = set([cmt.user.default_email for cmt in request.comments])

-     mail_to.add(request.project.user.default_email)

-     for prouser in request.project.users:

-         if prouser.default_email:

-             mail_to.add(prouser.default_email)

+     mail_to = _get_emails_for_issue(request)

  

      send_email(

          text,

@@ -408,11 +404,7 @@ 

             request.project.name,

             'pull-request',

             request.id))

-     mail_to = set([cmt.user.default_email for cmt in request.comments])

-     mail_to.add(request.project.user.default_email)

-     for prouser in request.project.users:

-         if prouser.default_email:

-             mail_to.add(prouser.default_email)

+     mail_to = _get_emails_for_issue(request)

  

      uid = time.mktime(datetime.datetime.now().timetuple())

      send_email(

@@ -446,11 +438,7 @@ 

             request.project.name,

             'pull-request',

             request.id))

-     mail_to = set([cmt.user.default_email for cmt in request.comments])

-     mail_to.add(request.project.user.default_email)

-     for prouser in request.project.users:

-         if prouser.default_email:

-             mail_to.add(prouser.default_email)

+     mail_to = _get_emails_for_issue(request)

  

      uid = time.mktime(datetime.datetime.now().timetuple())

      send_email(

@@ -482,14 +470,7 @@ 

             comment.pull_request.project.name,

             'pull-request',

             comment.pull_request.id))

-     mail_to = set([

-         cmt.user.default_email

-         for cmt in comment.pull_request.comments])

-     mail_to.add(comment.pull_request.project.user.default_email)

-     for prouser in comment.pull_request.project.users:

-         if prouser.default_email:

-             mail_to.add(prouser.default_email)

- 

+     mail_to = _get_emails_for_issue(comment.pull_request)

      mail_to = _clean_emails(mail_to, user)

  

      send_email(

file modified
+1 -1

@@ -224,7 +224,7 @@ 

              if indent:

                  line = '    %s' % line

              ntext.append(line)

-         return markdown.markdown('\n'.join(ntext))

+         return no_js(markdown.markdown('\n'.join(ntext)))

This method (no_js(...)) feels very error prone to me. It misses a lot of cases, for example when the script tag is capitalized or mixed-case. Or when I write <iframe src="javascript:alert('hi');"></iframe> instead of a plain script tag. Or if I use an <embed> tag which contains a malicious SVG. Or if I use an <img> tag with a malicious onmouseover attribute and make it really large so people mouse over it on accident.

I am still in favor of disabling html input altogether. It is not worth the security risks involved.

Ugh. I thought if I put it in backticks that wouldn't happen. The XSS above is caused by an iframe tag with a src="javascript:alert(...)".

Oh, that's weird. When I submitted, the backticked code triggered an XSS (the iframe one, specifically), but when I refresh, it doesn't, it renders it as a code block. So it sounds like there is another bug where submitting a comment for the first time, vs rendering it in the future, has potentially different output.

  

      return ''

  

file modified
+38 -5

@@ -704,6 +704,8 @@ 

      """ Presents the settings of the project.

      """

      if admin_session_timedout():

+         if flask.request.method == 'POST':

+             flask.flash('Action canceled, try it again', 'error')

          return flask.redirect(

              flask.url_for('auth_login', next=flask.request.url))

  

@@ -742,6 +744,9 @@ 

              flask.flash(message)

              return flask.redirect(flask.url_for(

                  'view_repo', username=username, repo=repo.name))

+         except pagure.exceptions.PagureException as msg:

+             SESSION.rollback()

+             flask.flash(msg, 'error')

          except SQLAlchemyError, err:  # pragma: no cover

              SESSION.rollback()

              flask.flash(str(err), 'error')

@@ -764,6 +769,13 @@ 

  def update_project(repo, username=None):

      """ Update the description of a project.

      """

+     if admin_session_timedout():

+         flask.flash('Action canceled, try it again', 'error')

+         url = flask.url_for(

+             'view_settings', username=username, repo=repo.name)

+         return flask.redirect(

+             flask.url_for('auth_login', next=url))

+ 

      repo = pagure.lib.get_project(SESSION, repo, user=username)

  

      if not repo:

@@ -799,8 +811,11 @@ 

      """ Delete the present project.

      """

      if admin_session_timedout():

+         flask.flash('Action canceled, try it again', 'error')

+         url = flask.url_for(

+             'view_settings', username=username, repo=repo)

          return flask.redirect(

-             flask.url_for('auth_login', next=flask.request.url))

+             flask.url_for('auth_login', next=url))

  

      repo = pagure.lib.get_project(SESSION, repo, user=username)

  

@@ -853,8 +868,11 @@ 

      """ Re-generate a hook token for the present project.

      """

      if admin_session_timedout():

+         flask.flash('Action canceled, try it again', 'error')

+         url = flask.url_for(

+             'view_settings', username=username, repo=repo)

          return flask.redirect(

-             flask.url_for('auth_login', next=flask.request.url))

+             flask.url_for('auth_login', next=url))

  

      repo = pagure.lib.get_project(SESSION, repo, user=username)

  

@@ -890,8 +908,11 @@ 

      """ Remove the specified user from the project.

      """

      if admin_session_timedout():

+         flask.flash('Action canceled, try it again', 'error')

+         url = flask.url_for(

+             'view_settings', username=username, repo=repo)

          return flask.redirect(

-             flask.url_for('auth_login', next=flask.request.url))

+             flask.url_for('auth_login', next=url))

  

      repo = pagure.lib.get_project(SESSION, repo, user=username)

  

@@ -942,6 +963,8 @@ 

      """ Add the specified user from the project.

      """

      if admin_session_timedout():

+         if flask.request.method == 'POST':

+             flask.flash('Action canceled, try it again', 'error')

          return flask.redirect(

              flask.url_for('auth_login', next=flask.request.url))

  

@@ -996,6 +1019,8 @@ 

      """ Add the specified group from the project.

      """

      if admin_session_timedout():

+         if flask.request.method == 'POST':

+             flask.flash('Action canceled, try it again', 'error')

          return flask.redirect(

              flask.url_for('auth_login', next=flask.request.url))

  

@@ -1048,8 +1073,11 @@ 

      """ Regenerate the specified git repo with the content in the project.

      """

      if admin_session_timedout():

+         flask.flash('Action canceled, try it again', 'error')

+         url = flask.url_for(

+             'view_settings', username=username, repo=repo)

          return flask.redirect(

-             flask.url_for('auth_login', next=flask.request.url))

+             flask.url_for('auth_login', next=url))

  

      repo = pagure.lib.get_project(SESSION, repo, user=username)

  

@@ -1095,6 +1123,8 @@ 

      """ Add a token to a specified project.

      """

      if admin_session_timedout():

+         if flask.request.method == 'POST':

+             flask.flash('Action canceled, try it again', 'error')

          return flask.redirect(

              flask.url_for('auth_login', next=flask.request.url))

  

@@ -1147,8 +1177,11 @@ 

      """ Revokie a token to a specified project.

      """

      if admin_session_timedout():

+         flask.flash('Action canceled, try it again', 'error')

+         url = flask.url_for(

+             'view_settings', username=username, repo=repo)

          return flask.redirect(

-             flask.url_for('auth_login', next=flask.request.url))

+             flask.url_for('auth_login', next=url))

  

      repo = pagure.lib.get_project(SESSION, repo, user=username)

  

@@ -154,7 +154,7 @@ 

                  '<td class="errors">This field is required.</td>'

                  in output.data)

  

-             data['name'] = 'project#1'

+             data['name'] = 'project-1'

              output = self.app.post('/new/', data=data)

              self.assertEqual(output.status_code, 200)

              self.assertTrue('<h2>New project</h2>' in output.data)

@@ -177,20 +177,20 @@ 

              self.assertEqual(output.status_code, 200)

              self.assertTrue('<p>Project #1</p>' in output.data)

              self.assertTrue(

-                 '<li class="message">Project &#34;project#1&#34; created</li>'

+                 '<li class="message">Project &#34;project-1&#34; created</li>'

                  in output.data)

  

          # After

          projects = pagure.lib.search_projects(self.session)

          self.assertEqual(len(projects), 1)

          self.assertTrue(os.path.exists(

-             os.path.join(tests.HERE, 'project#1.git')))

+             os.path.join(tests.HERE, 'project-1.git')))

          self.assertTrue(os.path.exists(

-             os.path.join(tests.HERE, 'tickets', 'project#1.git')))

+             os.path.join(tests.HERE, 'tickets', 'project-1.git')))

          self.assertTrue(os.path.exists(

-             os.path.join(tests.HERE, 'docs', 'project#1.git')))

+             os.path.join(tests.HERE, 'docs', 'project-1.git')))

          self.assertTrue(os.path.exists(

-             os.path.join(tests.HERE, 'requests', 'project#1.git')))

+             os.path.join(tests.HERE, 'requests', 'project-1.git')))

  

      @patch('pagure.ui.app.admin_session_timedout')

      def test_user_settings(self, ast):

@@ -88,7 +88,7 @@ 

                  'This field is required.'), 1)

  

              data = {

-                 'group_name': 'test group',

+                 'group_name': 'test_group',

              }

  

              # Missing CSRF

@@ -100,17 +100,7 @@ 

  

              data['csrf_token'] = csrf_token

  

-             # Space in the group

-             output = self.app.post(

-                 '/group/add', data=data)

-             self.assertEqual(output.status_code, 200)

-             self.assertIn('<h2>Create group</h2>', output.data)

-             self.assertIn(

-                 '<li class="error">Spaces are not allowed in group names: '

-                 'test group</li>' , output.data)

- 

              # All good

-             data['group_name'] = 'test_group'

              output = self.app.post(

                  '/group/add', data=data, follow_redirects=True)

              self.assertEqual(output.status_code, 200)

@@ -486,7 +486,7 @@ 

              data = {

                  'csrf_token': csrf_token,

                  'status': 'Fixed',

-                 'tag': 'tag#2',

+                 'tag': 'tag2',

              }

              output = self.app.post(

                  '/test/issue/1/update', data=data, follow_redirects=True)

@@ -495,7 +495,7 @@ 

                  '<p>test project<a href="/test/issue/1"> #1</a></p>'

                  in output.data)

              self.assertTrue(

-                 '<li class="message">Tag added: tag#2</li>' in output.data)

+                 '<li class="message">Tag added: tag2</li>' in output.data)

              self.assertFalse(

                  'li class="message">No changes to edit</li>' in output.data)

              self.assertTrue(

no initial comment

This method (no_js(...)) feels very error prone to me. It misses a lot of cases, for example when the script tag is capitalized or mixed-case. Or when I write <iframe src="javascript:alert('hi');"></iframe> instead of a plain script tag. Or if I use an <embed> tag which contains a malicious SVG. Or if I use an <img> tag with a malicious onmouseover attribute and make it really large so people mouse over it on accident.

I am still in favor of disabling html input altogether. It is not worth the security risks involved.

Ugh. I thought if I put it in backticks that wouldn't happen. The XSS above is caused by an iframe tag with a src="javascript:alert(...)".

Oh, that's weird. When I submitted, the backticked code triggered an XSS (the iframe one, specifically), but when I refresh, it doesn't, it renders it as a code block. So it sounds like there is another bug where submitting a comment for the first time, vs rendering it in the future, has potentially different output.

``
Oh, that's weird. When I submitted, the backticked code triggered an XSS (the iframe one, specifically), but when I refresh, it doesn't, it renders it as a code block. So it sounds like there is another bug where submitting a comment for the first time, vs rendering it in the future, has potentially different output.

That's because the inline comments code does not run via the same path as the
rest, that's something Patrick worked on to avoid having to reload the page at
every comment (making commenting on PR harder) but that we should still address.

As for the root cause, I'm still interesting to know you thought on allowing
markdown in comments while being secure.
I don't want to take markdown out of the picture, I find it an interesting
feature, but XSS is a problem definitely.
Do you think you could come up with a patch of some sort?

@pingou https://pythonhosted.org/Markdown/release-2.6.html suggests using the bleach library's clean method after converting the markdown to HTML. Maybe give that a shot?

@pingou https://pythonhosted.org/Markdown/release-2.6.html suggests using the bleach library's clean method after converting the markdown to HTML. Maybe give that a shot?

Ok done via: https://pagure.io/pagure/pull-request/235
Let's see in the next release if it helps :)

Thanks for the input!