PR#233: Multiple fixes - pagure

		`@@ -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"' %`

pagure/api/__init__.py

file modified

		`@@ -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'))`

pagure/forms.py

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',`

pagure/lib/notify.py

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(`

pagure/ui/filters.py

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)))`
codeblock commented 8 years ago 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.
codeblock commented 8 years ago 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(...)".
codeblock commented 8 years ago 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 ''`

pagure/ui/repo.py

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

tests/test_progit_flask_ui_app.py

file modified

+6 -6

		`@@ -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 "project#1" created</li>'`
		`+ '<li class="message">Project "project-1" 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):`

tests/test_progit_flask_ui_groups.py

file modified

+1 -11

		`@@ -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)`

tests/test_progit_flask_ui_issues.py

file modified

+2 -2

		`@@ -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

pingou commented 8 years ago

Fixes:

ralph commented 8 years ago

:+1: Looks good!

codeblock commented on line 6 of pagure/ui/filters.py 8 years ago

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.

codeblock commented on line 6 of pagure/ui/filters.py 8 years ago

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

codeblock commented on line 6 of pagure/ui/filters.py 8 years ago

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.

pingou commented 8 years ago

``
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?

codeblock commented 8 years ago

@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 commented 8 years ago

@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!