From 9d1740ab23e9ed284241122cdbc76d3c94e77183 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Sep 24 2018 20:02:55 +0000
Subject: Enforce that remote PR rely on a remote git repository


Otherwise, potentially, this could lead to leaking out private information
if someone manages to open a remote PR from a private project stored of
this pagure instance.

This commit fixes the CVE: CVE-2018-1002158
Thanks to Patrick Uiterwijk for reporting it!

Signed-off-by: Pierre-Yves Chibon <pingou@pingoured.fr>

---

diff --git a/pagure/forms.py b/pagure/forms.py
index ca098d0..b735567 100644
--- a/pagure/forms.py
+++ b/pagure/forms.py
@@ -288,7 +288,10 @@ class RemoteRequestPullForm(RequestPullForm):
 
     git_repo = wtforms.TextField(
         'Git repo address<span class="error">*</span>',
-        [wtforms.validators.Required()],
+        [
+            wtforms.validators.Required(),
+            wtforms.validators.Regexp(urlpattern, flags=re.IGNORECASE),
+        ],
     )
     branch_from = wtforms.TextField(
         'Git branch<span class="error">*</span>',
diff --git a/pagure/utils.py b/pagure/utils.py
index 0ab261d..142a994 100644
--- a/pagure/utils.py
+++ b/pagure/utils.py
@@ -317,7 +317,7 @@ IN THE SOFTWARE.
 urlregex = re.compile(
     "^"
     # protocol identifier
-    "(?:(?:https?|ftp)://)"
+    "(?:(?:https?|ftp|git)://)"
     # user:pass authentication
     "(?:\S+(?::\S*)?@)?" "(?:" "(?P<private_ip>"
     # IP address exclusion
diff --git a/tests/test_pagure_flask_ui_remote_pr.py b/tests/test_pagure_flask_ui_remote_pr.py
index ad0af76..7aaeb22 100644
--- a/tests/test_pagure_flask_ui_remote_pr.py
+++ b/tests/test_pagure_flask_ui_remote_pr.py
@@ -14,14 +14,16 @@ __requires__ = ['SQLAlchemy >= 0.8']
 import pkg_resources
 
 import json
-import unittest
+import os
+import re
 import shutil
 import sys
 import tempfile
 import time
-import os
+import unittest
 
 import pygit2
+import wtforms
 from mock import patch, MagicMock
 from bs4 import BeautifulSoup
 
@@ -206,62 +208,66 @@ class PagureRemotePRtests(tests.Modeltests):
                 output.get_data(as_text=True))
 
             csrf_token = self.get_csrf(output=output)
-            data = {
-                'csrf_token': csrf_token,
-                'title': 'Remote PR title',
-                'branch_from': 'feature',
-                'branch_to': 'master',
-                'git_repo': os.path.join(self.newpath, 'test'),
-            }
-            output = self.app.post('/test/diff/remote', data=data)
-            self.assertEqual(output.status_code, 200)
-            output_text = output.get_data(as_text=True)
-            self.assertIn('Create Pull Request\n    </div>\n', output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_1">\n', output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_2">\n', output_text)
-            self.assertNotIn(
-                '<div class="card mb-3" id="_3">\n', output_text)
-
-            # Not saved yet
-            self.session = pagure.lib.create_session(self.dbpath)
-            project = pagure.lib.get_authorized_project(self.session, 'test')
-            self.assertEqual(len(project.requests), 0)
-
-            data = {
-                'csrf_token': csrf_token,
-                'title': 'Remote PR title',
-                'branch_from': 'feature',
-                'branch_to': 'master',
-                'git_repo': os.path.join(self.newpath, 'test'),
-                'confirm': 1,
-            }
-            output = self.app.post(
-                '/test/diff/remote', data=data, follow_redirects=True)
-            self.assertEqual(output.status_code, 200)
-            output_text = output.get_data(as_text=True)
-            self.assertIn(
-                '<span class="text-success font-weight-bold">#1',
-                output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_1">\n', output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_2">\n', output_text)
-            self.assertNotIn(
-                '<div class="card mb-3" id="_3">\n', output_text)
-
-            # Show the filename in the Changes summary
-            self.assertIn(
-                '<a href="#_1" class="list-group-item', output_text)
-            self.assertIn(
-                '<div class="ellipsis pr-changes-description">'
-                '\n          <small>.gitignore</small>', output_text)
-            self.assertIn(
-                '<a href="#_2" class="list-group-item', output_text)
-            self.assertIn(
-                '<div class="ellipsis pr-changes-description">'
-                '\n          <small>sources</small>', output_text)
+            with patch(
+                    'pagure.forms.RemoteRequestPullForm.git_repo.args',
+                    MagicMock(return_value=(
+                        u'Git Repo address', [wtforms.validators.Required()]))):
+                data = {
+                    'csrf_token': csrf_token,
+                    'title': 'Remote PR title',
+                    'branch_from': 'feature',
+                    'branch_to': 'master',
+                    'git_repo': os.path.join(self.newpath, 'test'),
+                }
+                output = self.app.post('/test/diff/remote', data=data)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn('Create Pull Request\n    </div>\n', output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_1">\n', output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_2">\n', output_text)
+                self.assertNotIn(
+                    '<div class="card mb-3" id="_3">\n', output_text)
+
+                # Not saved yet
+                self.session = pagure.lib.create_session(self.dbpath)
+                project = pagure.lib.get_authorized_project(self.session, 'test')
+                self.assertEqual(len(project.requests), 0)
+
+                data = {
+                    'csrf_token': csrf_token,
+                    'title': 'Remote PR title',
+                    'branch_from': 'feature',
+                    'branch_to': 'master',
+                    'git_repo': os.path.join(self.newpath, 'test'),
+                    'confirm': 1,
+                }
+                output = self.app.post(
+                    '/test/diff/remote', data=data, follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn(
+                    '<span class="text-success font-weight-bold">#1',
+                    output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_1">\n', output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_2">\n', output_text)
+                self.assertNotIn(
+                    '<div class="card mb-3" id="_3">\n', output_text)
+
+                # Show the filename in the Changes summary
+                self.assertIn(
+                    '<a href="#_1" class="list-group-item', output_text)
+                self.assertIn(
+                    '<div class="ellipsis pr-changes-description">'
+                    '\n          <small>.gitignore</small>', output_text)
+                self.assertIn(
+                    '<a href="#_2" class="list-group-item', output_text)
+                self.assertIn(
+                    '<div class="ellipsis pr-changes-description">'
+                    '\n          <small>sources</small>', output_text)
 
         # Remote PR Created
         self.session = pagure.lib.create_session(self.dbpath)
@@ -325,53 +331,58 @@ class PagureRemotePRtests(tests.Modeltests):
                 output.get_data(as_text=True))
 
             csrf_token = self.get_csrf(output=output)
-            data = {
-                'csrf_token': csrf_token,
-                'title': 'Remote PR title',
-                'branch_from': 'feature',
-                'branch_to': 'master',
-                'git_repo': gitrepo,
-            }
-            output = self.app.post('/test/diff/remote', data=data)
-            self.assertEqual(output.status_code, 200)
-            output_text = output.get_data(as_text=True)
-            self.assertIn('Create Pull Request\n    </div>\n', output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_1">\n', output_text)
-            self.assertNotIn(
-                '<div class="card mb-3" id="_2">\n', output_text)
 
-            # Not saved yet
-            self.session = pagure.lib.create_session(self.dbpath)
-            project = pagure.lib.get_authorized_project(self.session, 'test')
-            self.assertEqual(len(project.requests), 0)
-
-            data = {
-                'csrf_token': csrf_token,
-                'title': 'Remote PR title',
-                'branch_from': 'feature',
-                'branch_to': 'master',
-                'git_repo': gitrepo,
-                'confirm': 1,
-            }
-            output = self.app.post(
-                '/test/diff/remote', data=data, follow_redirects=True)
-            self.assertEqual(output.status_code, 200)
-            output_text = output.get_data(as_text=True)
-            self.assertIn(
-                '<title>PR#1: Remote PR title - test\n - Pagure</title>',
-                output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_1">\n', output_text)
-            self.assertNotIn(
-                '<div class="card mb-3" id="_2">\n', output_text)
-
-            # Show the filename in the Changes summary
-            self.assertIn(
-                '<a href="#_1" class="list-group-item', output_text)
-            self.assertIn(
-                '<div class="ellipsis pr-changes-description">'
-                '\n          <small>sources</small>', output_text)
+            with patch(
+                    'pagure.forms.RemoteRequestPullForm.git_repo.args',
+                    MagicMock(return_value=(
+                        u'Git Repo address', [wtforms.validators.Required()]))):
+                data = {
+                    'csrf_token': csrf_token,
+                    'title': 'Remote PR title',
+                    'branch_from': 'feature',
+                    'branch_to': 'master',
+                    'git_repo': gitrepo,
+                }
+                output = self.app.post('/test/diff/remote', data=data)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn('Create Pull Request\n    </div>\n', output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_1">\n', output_text)
+                self.assertNotIn(
+                    '<div class="card mb-3" id="_2">\n', output_text)
+
+                # Not saved yet
+                self.session = pagure.lib.create_session(self.dbpath)
+                project = pagure.lib.get_authorized_project(self.session, 'test')
+                self.assertEqual(len(project.requests), 0)
+
+                data = {
+                    'csrf_token': csrf_token,
+                    'title': 'Remote PR title',
+                    'branch_from': 'feature',
+                    'branch_to': 'master',
+                    'git_repo': gitrepo,
+                    'confirm': 1,
+                }
+                output = self.app.post(
+                    '/test/diff/remote', data=data, follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn(
+                    '<title>PR#1: Remote PR title - test\n - Pagure</title>',
+                    output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_1">\n', output_text)
+                self.assertNotIn(
+                    '<div class="card mb-3" id="_2">\n', output_text)
+
+                # Show the filename in the Changes summary
+                self.assertIn(
+                    '<a href="#_1" class="list-group-item', output_text)
+                self.assertIn(
+                    '<div class="ellipsis pr-changes-description">'
+                    '\n          <small>sources</small>', output_text)
 
         # Remote PR Created
         self.session = pagure.lib.create_session(self.dbpath)
@@ -437,24 +448,29 @@ class PagureRemotePRtests(tests.Modeltests):
                 'branch_to': 'master',
                 'git_repo': os.path.join(self.newpath, 'test'),
             }
-            output = self.app.post(
-                '/test/diff/remote', data=data, follow_redirects=True)
-            self.assertEqual(output.status_code, 200)
-
-            data['confirm'] = 1
-            output = self.app.post(
-                '/test/diff/remote', data=data, follow_redirects=True)
-            self.assertEqual(output.status_code, 200)
-            output_text = output.get_data(as_text=True)
-            self.assertIn(
-                '<span class="text-success font-weight-bold">#1',
-                output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_1">\n', output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_2">\n', output_text)
-            self.assertNotIn(
-                '<div class="card mb-3" id="_3">\n', output_text)
+            with patch(
+                    'pagure.forms.RemoteRequestPullForm.git_repo.args',
+                    MagicMock(return_value=(
+                        u'Git Repo address', [wtforms.validators.Required()]))):
+
+                output = self.app.post(
+                    '/test/diff/remote', data=data, follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+
+                data['confirm'] = 1
+                output = self.app.post(
+                    '/test/diff/remote', data=data, follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn(
+                    '<span class="text-success font-weight-bold">#1',
+                    output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_1">\n', output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_2">\n', output_text)
+                self.assertNotIn(
+                    '<div class="card mb-3" id="_3">\n', output_text)
 
         # Remote PR Created
         self.session = pagure.lib.create_session(self.dbpath)
@@ -503,24 +519,32 @@ class PagureRemotePRtests(tests.Modeltests):
                 'branch_to': 'master',
                 'git_repo': os.path.join(self.newpath, 'test'),
             }
-            output = self.app.post(
-                '/test/diff/remote', data=data, follow_redirects=True)
-            self.assertEqual(output.status_code, 200)
-
-            data['confirm'] = 1
-            output = self.app.post(
-                '/test/diff/remote', data=data, follow_redirects=True)
-            self.assertEqual(output.status_code, 200)
-            output_text = output.get_data(as_text=True)
-            self.assertIn(
-                '<span class="text-success font-weight-bold">#1',
-                output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_1">\n', output_text)
-            self.assertIn(
-                '<div class="card mb-3" id="_2">\n', output_text)
-            self.assertNotIn(
-                '<div class="card mb-3" id="_3">\n', output_text)
+            # Disables checking the URL pattern for git_repo
+            with patch(
+                    'pagure.forms.RemoteRequestPullForm.git_repo.args',
+                    MagicMock(return_value=(
+                        u'Git Repo address', [wtforms.validators.Required()]))):
+
+                # Do the preview, triggers the cache & all
+                output = self.app.post(
+                    '/test/diff/remote', data=data, follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+
+                # Confirm the PR creation
+                data['confirm'] = 1
+                output = self.app.post(
+                    '/test/diff/remote', data=data, follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn(
+                    '<span class="text-success font-weight-bold">#1',
+                    output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_1">\n', output_text)
+                self.assertIn(
+                    '<div class="card mb-3" id="_2">\n', output_text)
+                self.assertNotIn(
+                    '<div class="card mb-3" id="_3">\n', output_text)
 
         # Remote PR Created
         self.session = pagure.lib.create_session(self.dbpath)