From 286d2fe09356fdb95a06ec6265ef9ba3db86b231 Mon Sep 17 00:00:00 2001 From: Karl Dubost Date: Fri, 8 Jun 2018 16:30:17 +0900 Subject: [PATCH 1/7] Issue #2479 - Adds checker for form parameters This adds a function which can be augmented in the future to check form parameters This adds the test for this function. --- tests/unit/test_form.py | 20 ++++++++++++++++++-- webcompat/helpers.py | 19 +++++++++++++++++++ webcompat/views.py | 15 +++++++-------- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/tests/unit/test_form.py b/tests/unit/test_form.py index 1a45e4433..f24732307 100644 --- a/tests/unit/test_form.py +++ b/tests/unit/test_form.py @@ -10,6 +10,7 @@ import webcompat from webcompat import form +from webcompat import helpers FIREFOX_UA = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0' # nopep8 @@ -188,8 +189,7 @@ def test_get_details(self): self.assertEqual(actual_json_arg, expected_json_arg) def test_build_details(self): - """Assert we return the expected HTML, for a json object or a string. - """ + """Assert we return the expected HTML, for a json object or a string.""" actual_json_arg = form.build_details(json.dumps( {'a': 'b', 'c': False})) expected_json_arg = '
\nBrowser Configuration\n\n
' # nopep8 @@ -197,3 +197,19 @@ def test_build_details(self): actual_string_arg = form.build_details("cool") expected_string_arg = '
\nBrowser Configuration\n\n
' # nopep8 self.assertEqual(actual_string_arg, expected_string_arg) + + def test_is_valid_issue_form(self): + """Assert that we get the form parameters we want.""" + incomplete_form = MultiDict([('problem_category', u'unknown_bug')]) + self.assertFalse(helpers.is_valid_issue_form(incomplete_form)) + valid_form = MultiDict([ + ('browser', u'Firefox 61.0'), + ('description', u'streamlining the form.'), + ('details', u''), + ('os', u'Mac OS X 10.13'), + ('problem_category', u'unknown_bug'), + ('submit_type', u'github-auth-report'), + ('url', u'http://2479.example.com'), + ('username', u''), + ]) + self.assertTrue(helpers.is_valid_issue_form(valid_form)) diff --git a/webcompat/helpers.py b/webcompat/helpers.py index 325a5ee04..6de7c530c 100644 --- a/webcompat/helpers.py +++ b/webcompat/helpers.py @@ -552,3 +552,22 @@ def get_milestone_list(milestone_name, params=None): milestone_name, other_params=params) milestone_list = json.loads(raw_response[0]) return milestone_list + + +def is_valid_issue_form(form): + """Check if the issue form follows some requirements. + + To be legit the form needs a couple of parameters + if one essential is missing, it's a bad request. + We may add more constraints in the future. + """ + must_parameters = [ + 'browser', + 'description', + 'os', + 'problem_category', + 'submit_type', + 'url', + 'username', + ] + return set(must_parameters).issubset(form.keys()) diff --git a/webcompat/views.py b/webcompat/views.py index cf7b8029d..f4bedc595 100644 --- a/webcompat/views.py +++ b/webcompat/views.py @@ -28,6 +28,7 @@ from helpers import get_milestone_list from helpers import get_referer from helpers import get_user_info +from helpers import is_valid_issue_form from helpers import set_referer from issues import report_issue from webcompat import app @@ -169,8 +170,11 @@ def show_issues(): def create_issue(): """Create a new issue. - GET will return an HTML response for reporting issues - POST will create a new issue + GET will return an HTML response for reporting issues. + POST will create a new issue. + + Any deceptive requests will be ended as a 400. + See https://tools.ietf.org/html/rfc7231#section-6.5.1 """ if request.method == 'GET': bug_form = get_form(request.headers.get('User-Agent')) @@ -189,12 +193,7 @@ def create_issue(): # copy the form so we can add the full UA string to it. if request.form: form = request.form.copy() - # To be legit the form needs a couple of parameters - # if one essential is missing, it's a bad request - must_parameters = set(['url', 'problem_category', 'description', - 'os', 'browser', - 'username', 'submit_type']) - if not must_parameters.issubset(form.keys()): + if not is_valid_issue_form(form): abort(400) else: # https://tools.ietf.org/html/rfc7231#section-6.5.1 From 5339d389da58da561b91bfec5e6da30d7322af2d Mon Sep 17 00:00:00 2001 From: Karl Dubost Date: Fri, 8 Jun 2018 16:37:04 +0900 Subject: [PATCH 2/7] Issue #2479 - Makes the logging more flexible By starting the logger early up, we can use it to track failures here and there. --- webcompat/views.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/webcompat/views.py b/webcompat/views.py index f4bedc595..8493887b7 100644 --- a/webcompat/views.py +++ b/webcompat/views.py @@ -176,6 +176,10 @@ def create_issue(): Any deceptive requests will be ended as a 400. See https://tools.ietf.org/html/rfc7231#section-6.5.1 """ + # Starting a logger + log = app.logger + log.setLevel(logging.INFO) + # GET Requests if request.method == 'GET': bug_form = get_form(request.headers.get('User-Agent')) if g.user: @@ -190,14 +194,20 @@ def create_issue(): if request.args.get('label'): session['label'] = request.args.getlist('label') return render_template('new-issue.html', form=bug_form) - # copy the form so we can add the full UA string to it. + # POST Requests if request.form: + # Copy the form to add the full UA string. form = request.form.copy() if not is_valid_issue_form(form): abort(400) else: - # https://tools.ietf.org/html/rfc7231#section-6.5.1 + log.info('POST request without form.') abort(400) + # Logging the ip and url for investigation + log.info('{ip} {url}'.format( + ip=request.remote_addr, + url=form['url'].encode('utf-8')) + ) # see https://github.com/webcompat/webcompat.com/issues/1141 # see https://github.com/webcompat/webcompat.com/issues/1237 # see https://github.com/webcompat/webcompat.com/issues/1627 @@ -214,11 +224,6 @@ def create_issue(): form['reported_with'] = session.pop('src', 'web') # Reminder: label is a list, if it exists form['extra_labels'] = session.pop('label', None) - # Logging the ip and url for investigation - log = app.logger - log.setLevel(logging.INFO) - log.info('{ip} {url}'.format(ip=request.remote_addr, - url=form['url'].encode('utf-8'))) # form submission for 3 scenarios: authed, to be authed, anonymous if form.get('submit_type') == AUTH_REPORT: if g.user: # If you're already authed, submit the bug. From b0febe64a68219eca45aff346ffd34ffff6b58ea Mon Sep 17 00:00:00 2001 From: Karl Dubost Date: Fri, 8 Jun 2018 16:56:58 +0900 Subject: [PATCH 3/7] Issue #2479 - Adds helpers for blacklisted domains This creates an helper to check if the domain is blacklisted. Probably this can evolve into something more refined where the blacklist is maintained in the config file and not in the code. --- tests/unit/test_form.py | 5 +++++ webcompat/helpers.py | 9 +++++++++ webcompat/views.py | 21 +++++++++------------ 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/tests/unit/test_form.py b/tests/unit/test_form.py index f24732307..225d94a9a 100644 --- a/tests/unit/test_form.py +++ b/tests/unit/test_form.py @@ -213,3 +213,8 @@ def test_is_valid_issue_form(self): ('username', u''), ]) self.assertTrue(helpers.is_valid_issue_form(valid_form)) + + def test_is_blacklisted_domain(self): + """Assert domains validity in issue reporting.""" + self.assertTrue(helpers.is_blacklisted_domain('coco.fr')) + self.assertFalse(helpers.is_blacklisted_domain('w3.org')) diff --git a/webcompat/helpers.py b/webcompat/helpers.py index 6de7c530c..d29d472aa 100644 --- a/webcompat/helpers.py +++ b/webcompat/helpers.py @@ -571,3 +571,12 @@ def is_valid_issue_form(form): 'username', ] return set(must_parameters).issubset(form.keys()) + + +def is_blacklisted_domain(domain): + """Check if the domain is part of an exclusion list.""" + # see https://github.com/webcompat/webcompat.com/issues/1141 + # see https://github.com/webcompat/webcompat.com/issues/1237 + # see https://github.com/webcompat/webcompat.com/issues/1627 + spamlist = ['qiangpiaoruanjian', 'cityweb.de', 'coco.fr'] + return domain in spamlist diff --git a/webcompat/views.py b/webcompat/views.py index 8493887b7..ec7927881 100644 --- a/webcompat/views.py +++ b/webcompat/views.py @@ -28,6 +28,7 @@ from helpers import get_milestone_list from helpers import get_referer from helpers import get_user_info +from helpers import is_blacklisted_domain from helpers import is_valid_issue_form from helpers import set_referer from issues import report_issue @@ -208,18 +209,14 @@ def create_issue(): ip=request.remote_addr, url=form['url'].encode('utf-8')) ) - # see https://github.com/webcompat/webcompat.com/issues/1141 - # see https://github.com/webcompat/webcompat.com/issues/1237 - # see https://github.com/webcompat/webcompat.com/issues/1627 - spamlist = ['qiangpiaoruanjian', 'cityweb.de', 'coco.fr'] - for spam in spamlist: - if spam in form.get('url'): - msg = (u'Anonymous reporting for domain {0} ' - 'is temporarily disabled. Please contact ' - 'miket@mozilla.com ' - 'for more details.').format(spam) - flash(msg, 'notimeout') - return redirect(url_for('index')) + # Checking blacklisted domains + if is_blacklisted_domain(form['url']): + msg = (u'Anonymous reporting for domain {0} ' + 'is temporarily disabled. Please contact ' + 'miket@mozilla.com ' + 'for more details.').format(form['url']) + flash(msg, 'notimeout') + return redirect(url_for('index')) form['ua_header'] = request.headers.get('User-Agent') form['reported_with'] = session.pop('src', 'web') # Reminder: label is a list, if it exists From bbb3b6002ebce208b0158b6d0118a54b30147a31 Mon Sep 17 00:00:00 2001 From: Karl Dubost Date: Mon, 11 Jun 2018 16:38:47 +0900 Subject: [PATCH 4/7] Issue #2479 - Adds checks for submit values --- tests/unit/test_form.py | 14 ++++++++++++++ webcompat/helpers.py | 6 +++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_form.py b/tests/unit/test_form.py index 225d94a9a..2b84e6654 100644 --- a/tests/unit/test_form.py +++ b/tests/unit/test_form.py @@ -213,6 +213,20 @@ def test_is_valid_issue_form(self): ('username', u''), ]) self.assertTrue(helpers.is_valid_issue_form(valid_form)) + # The value for submit-Type can be only: + # - github-auth-report + # - github-proxy-report + wrong_value_form = MultiDict([ + ('browser', u'Firefox 61.0'), + ('description', u'streamlining the form.'), + ('details', u''), + ('os', u'Mac OS X 10.13'), + ('problem_category', u'unknown_bug'), + ('submit_type', u'wrong-value'), + ('url', u'http://2479.example.com'), + ('username', u''), + ]) + self.assertFalse(helpers.is_valid_issue_form(wrong_value_form)) def test_is_blacklisted_domain(self): """Assert domains validity in issue reporting.""" diff --git a/webcompat/helpers.py b/webcompat/helpers.py index d29d472aa..9b2dfa31d 100644 --- a/webcompat/helpers.py +++ b/webcompat/helpers.py @@ -570,7 +570,11 @@ def is_valid_issue_form(form): 'url', 'username', ] - return set(must_parameters).issubset(form.keys()) + form_submit_values = ['github-auth-report', 'github-proxy-report'] + parameters_check = set(must_parameters).issubset(form.keys()) + if parameters_check: + values_check = form['submit_type'] in form_submit_values + return parameters_check and values_check def is_blacklisted_domain(domain): From 3748ac56b0716123d80b6381c17f6b40f60b6400 Mon Sep 17 00:00:00 2001 From: Karl Dubost Date: Thu, 28 Jun 2018 07:28:07 +0900 Subject: [PATCH 5/7] Issue #2479 - Adds test for HTTP POST on /issues/new --- tests/unit/test_urls.py | 56 +++++++++++++++++++++++++++++++++++++++++ webcompat/views.py | 7 ++++-- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_urls.py b/tests/unit/test_urls.py index 1d79a3a56..6019bcdad 100644 --- a/tests/unit/test_urls.py +++ b/tests/unit/test_urls.py @@ -6,10 +6,18 @@ """Tests for our URL endpoints.""" +import json import os.path import sys import unittest +from mock import MagicMock +from mock import patch +from requests import Response + + +from webcompat.views import report_issue + # Add webcompat module to import path sys.path.append(os.path.realpath(os.pardir)) import webcompat # nopep8 @@ -19,6 +27,17 @@ headers = {'HTTP_USER_AGENT': ('Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; ' 'rv:31.0) Gecko/20100101 Firefox/31.0')} +POST_RESPONSE = { + u'labels': [], + u'number': 1544, + u'title': u'testing-form.example.com - see bug description', + u'state': u'open', + u'body': u'\n\n\n\n**URL**: http://testing-form.example.com/\n\n**Browser / Version**: Firefox 62.0\n**Operating System**: Mac OS X 10.13\n**Tested Another Browser**: Unknown\n\n**Problem type**: Something else\n**Description**: testing form and github response.\n**Steps to Reproduce**:\n\n\n\n\n_From [webcompat.com](https://webcompat.com/) with \u2764\ufe0f_', + u'updated_at': u'2018-06-14T22:50:31Z', + u'milestone': None, + u'created_at': u'2018-06-14T22:50:31Z', + } + class TestURLs(unittest.TestCase): """Test for routes in the the project.""" @@ -42,6 +61,43 @@ def test_new_issue(self): rv = self.app.get('/issues/new', environ_base=headers) self.assertEqual(rv.status_code, 200) + @patch('webcompat.views.report_issue') + def test_successful_post_new_issue(self, mock_proxy): + """Test that post is working on /issues/new.""" + mock_proxy.return_value = POST_RESPONSE + rv = self.app.post( + '/issues/new', + environ_base=headers, + data=dict( + browser='Firefox Mobile 45.0', + description='testing 2971', + os='macos', + problem_category='yada', + submit_type='github-proxy-report', + url='http://testing.example.org', + username='yeeha' + ) + ) + self.assertEqual(rv.status_code, 302) + self.assertEqual( + rv.headers['Location'], 'http://localhost/issues/1544') + self.assertTrue( + '/issues/1544' in rv.data) + + @patch('webcompat.issues.proxy_request') + def test_fail_post_new_issue(self, mock_proxy): + """Test that post is not working on /issues/new.""" + mock_proxy = MagicMock(spec=Response) + mock_proxy = json.dumps(POST_RESPONSE) + rv = self.app.post( + '/issues/new', + environ_base=headers, + data=dict( + browser='Firefox Mobile 45.0', + ) + ) + self.assertEqual(rv.status_code, 400) + def test_about(self): """Test that /about exists.""" rv = self.app.get('/about') diff --git a/webcompat/views.py b/webcompat/views.py index ec7927881..3e4381161 100644 --- a/webcompat/views.py +++ b/webcompat/views.py @@ -180,9 +180,11 @@ def create_issue(): # Starting a logger log = app.logger log.setLevel(logging.INFO) + # Get the User-Agent + user_agent = request.headers.get('User-Agent') # GET Requests if request.method == 'GET': - bug_form = get_form(request.headers.get('User-Agent')) + bug_form = get_form(user_agent) if g.user: get_user_info() # Note: `src` and `label` are special GET params that can pass @@ -217,7 +219,8 @@ def create_issue(): 'for more details.').format(form['url']) flash(msg, 'notimeout') return redirect(url_for('index')) - form['ua_header'] = request.headers.get('User-Agent') + # Feeding the form with request data + form['ua_header'] = user_agent form['reported_with'] = session.pop('src', 'web') # Reminder: label is a list, if it exists form['extra_labels'] = session.pop('label', None) From 6455daa4c3858689e610b3dd9552791a9585d8ef Mon Sep 17 00:00:00 2001 From: Karl Dubost Date: Thu, 28 Jun 2018 08:00:11 +0900 Subject: [PATCH 6/7] Issue #2479 - Fixes syntax kerfuffles --- tests/unit/test_form.py | 8 +++----- tests/unit/test_urls.py | 13 ++++--------- webcompat/helpers.py | 3 +-- webcompat/views.py | 3 +-- 4 files changed, 9 insertions(+), 18 deletions(-) diff --git a/tests/unit/test_form.py b/tests/unit/test_form.py index 2b84e6654..c18950a9b 100644 --- a/tests/unit/test_form.py +++ b/tests/unit/test_form.py @@ -189,7 +189,7 @@ def test_get_details(self): self.assertEqual(actual_json_arg, expected_json_arg) def test_build_details(self): - """Assert we return the expected HTML, for a json object or a string.""" + """Expected HTML is returned for a json object or a string.""" actual_json_arg = form.build_details(json.dumps( {'a': 'b', 'c': False})) expected_json_arg = '
\nBrowser Configuration\n
    \n
  • a: b
  • c: false
    • \n
\n
' # nopep8 @@ -210,8 +210,7 @@ def test_is_valid_issue_form(self): ('problem_category', u'unknown_bug'), ('submit_type', u'github-auth-report'), ('url', u'http://2479.example.com'), - ('username', u''), - ]) + ('username', u''), ]) self.assertTrue(helpers.is_valid_issue_form(valid_form)) # The value for submit-Type can be only: # - github-auth-report @@ -224,8 +223,7 @@ def test_is_valid_issue_form(self): ('problem_category', u'unknown_bug'), ('submit_type', u'wrong-value'), ('url', u'http://2479.example.com'), - ('username', u''), - ]) + ('username', u''), ]) self.assertFalse(helpers.is_valid_issue_form(wrong_value_form)) def test_is_blacklisted_domain(self): diff --git a/tests/unit/test_urls.py b/tests/unit/test_urls.py index 6019bcdad..cab3f249c 100644 --- a/tests/unit/test_urls.py +++ b/tests/unit/test_urls.py @@ -32,11 +32,10 @@ u'number': 1544, u'title': u'testing-form.example.com - see bug description', u'state': u'open', - u'body': u'\n\n\n\n**URL**: http://testing-form.example.com/\n\n**Browser / Version**: Firefox 62.0\n**Operating System**: Mac OS X 10.13\n**Tested Another Browser**: Unknown\n\n**Problem type**: Something else\n**Description**: testing form and github response.\n**Steps to Reproduce**:\n\n\n\n\n_From [webcompat.com](https://webcompat.com/) with \u2764\ufe0f_', + u'body': u'\n\n\n\n**URL**: http://testing-form.example.com/\n\n**Browser / Version**: Firefox 62.0\n**Operating System**: Mac OS X 10.13\n**Tested Another Browser**: Unknown\n\n**Problem type**: Something else\n**Description**: testing form and github response.\n**Steps to Reproduce**:\n\n\n\n\n_From [webcompat.com](https://webcompat.com/) with \u2764\ufe0f_', # noqa u'updated_at': u'2018-06-14T22:50:31Z', u'milestone': None, - u'created_at': u'2018-06-14T22:50:31Z', - } + u'created_at': u'2018-06-14T22:50:31Z', } class TestURLs(unittest.TestCase): @@ -75,9 +74,7 @@ def test_successful_post_new_issue(self, mock_proxy): problem_category='yada', submit_type='github-proxy-report', url='http://testing.example.org', - username='yeeha' - ) - ) + username='yeeha')) self.assertEqual(rv.status_code, 302) self.assertEqual( rv.headers['Location'], 'http://localhost/issues/1544') @@ -93,9 +90,7 @@ def test_fail_post_new_issue(self, mock_proxy): '/issues/new', environ_base=headers, data=dict( - browser='Firefox Mobile 45.0', - ) - ) + browser='Firefox Mobile 45.0', )) self.assertEqual(rv.status_code, 400) def test_about(self): diff --git a/webcompat/helpers.py b/webcompat/helpers.py index 9b2dfa31d..eb15a058c 100644 --- a/webcompat/helpers.py +++ b/webcompat/helpers.py @@ -568,8 +568,7 @@ def is_valid_issue_form(form): 'problem_category', 'submit_type', 'url', - 'username', - ] + 'username', ] form_submit_values = ['github-auth-report', 'github-proxy-report'] parameters_check = set(must_parameters).issubset(form.keys()) if parameters_check: diff --git a/webcompat/views.py b/webcompat/views.py index 3e4381161..c65474f40 100644 --- a/webcompat/views.py +++ b/webcompat/views.py @@ -209,8 +209,7 @@ def create_issue(): # Logging the ip and url for investigation log.info('{ip} {url}'.format( ip=request.remote_addr, - url=form['url'].encode('utf-8')) - ) + url=form['url'].encode('utf-8'))) # Checking blacklisted domains if is_blacklisted_domain(form['url']): msg = (u'Anonymous reporting for domain {0} ' From 02065ef5cc4edeb7136ad031b63f4a882f0c450d Mon Sep 17 00:00:00 2001 From: Karl Dubost Date: Wed, 4 Jul 2018 09:37:35 +0900 Subject: [PATCH 7/7] Issue #2479 - Makes report_issue uniform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before report_issue was reporting either a JSON as dict, either a response object, this makes it uniform so it will only report JSON data as a dict. It simplifies testing and make it less… surprising. :) If in the future we need the access to status, we can revisit this and makes it return response but uniformly. --- webcompat/issues.py | 11 ++++++++--- webcompat/views.py | 13 +++++++------ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/webcompat/issues.py b/webcompat/issues.py index d25d2b016..1b7ee625e 100644 --- a/webcompat/issues.py +++ b/webcompat/issues.py @@ -20,7 +20,12 @@ def report_issue(form, proxy=False): # /repos/:owner/:repo/issues path = 'repos/{0}'.format(REPO_URI) if proxy: - return proxy_request('post', path, - data=json.dumps(build_formdata(form))) + # Return a Response object. + response = proxy_request('post', + path, + data=json.dumps(build_formdata(form))) + json_response = response.json() else: - return github.post(path, build_formdata(form)) + # Return JSON data as a dict + json_response = github.post(path, build_formdata(form)) + return json_response diff --git a/webcompat/views.py b/webcompat/views.py index c65474f40..ef62657aa 100644 --- a/webcompat/views.py +++ b/webcompat/views.py @@ -137,11 +137,11 @@ def file_issue(): abort(401) if session and (form_data is None): abort(403) - response = report_issue(session['form_data']) + json_response = report_issue(session['form_data']) # Get rid of stashed form data session.pop('form_data', None) session['show_thanks'] = True - return redirect(url_for('show_issue', number=response.get('number'))) + return redirect(url_for('show_issue', number=json_response.get('number'))) @app.route('/', methods=['GET']) @@ -226,17 +226,18 @@ def create_issue(): # form submission for 3 scenarios: authed, to be authed, anonymous if form.get('submit_type') == AUTH_REPORT: if g.user: # If you're already authed, submit the bug. - response = report_issue(form) + json_response = report_issue(form) session['show_thanks'] = True return redirect(url_for('show_issue', - number=response.get('number'))) + number=json_response.get('number'))) else: # Stash form data into session, go do GitHub auth session['form_data'] = form return redirect(url_for('login')) elif form.get('submit_type') == PROXY_REPORT: - response = report_issue(form, proxy=True).json() + json_response = report_issue(form, proxy=True) session['show_thanks'] = True - return redirect(url_for('show_issue', number=response.get('number'))) + return redirect(url_for('show_issue', + number=json_response.get('number'))) else: # if anything wrong, we assume it is a bad forged request abort(400)