kallithea Changeset - 083fbf531a5d

Changeset - 083fbf531a5d

Parent rev.

Child rev.

[Not reviewed]

stable

0 3 0

Mads Kiilerich - 7 years ago 2018-05-29 12:25:41
mads@kiilerich.com

repos: only allow api repo creation in existing groups

Fix problem with '../something' paths being allowed; '..' will always exist and
can't be created.

This also introduce a small API change: Repository groups must now exist before
repositories can be created. This makes the API more explicit and simpler.

This issue was found and reported by
Kacper Szurek
https://security.szurek.pl/

3 files changed with 43 insertions and 23 deletions:

docs/api/api.rst

kallithea/controllers/api/api.py

kallithea/tests/api/api_base.py

0 comments (0 inline, 0 general)

docs/api/api.rst

➞

Show inline comments

@@ @@ -729,52 +729,54 @@ INPUT:: @@
     api_key : "<api_key>"
     method :  "get_repo_nodes"
     args:     {
                 "repoid" : "<reponame or repo_id>"
                 "revision"  : "<revision>",
                 "root_path" : "<root_path>",
                 "ret_type"  : "<ret_type> = Optional('all')"
+              }
 OUTPUT::
     id : <id_given_in_input>
     result: [
+              {
                 "name" :        "<name>"
                 "type" :        "<type>",
               },
               …
+            ]
     error:  null
 create_repo
 -----------
 Create a repository. If the repository name contains "/", all needed repository
 groups will be created. For example "foo/bar/baz" will create repository groups
 "foo", "bar" (with "foo" as parent), and create "baz" repository with
 "bar" as group.
 Create a repository. If the repository name contains "/", the repository will be
 created in the repository group indicated by that path. Any such repository
 groups need to exist before calling this method, or the call will fail.
 For example "foo/bar/baz" will create a repository "baz" inside the repository
 group "bar" which itself is in a repository group "foo", but both "foo" and
 "bar" already need to exist before calling this method.
 This command can only be executed using the api_key of a user with admin rights,
 or that of a regular user with create repository permission.
 Regular users cannot specify owner parameter.
 INPUT::
     id : <id_for_response>
     api_key : "<api_key>"
     method :  "create_repo"
     args:     {
                 "repo_name" :        "<reponame>",
                 "owner" :            "<owner_name_or_id = Optional(=apiuser)>",
                 "repo_type" :        "<repo_type> = Optional('hg')",
                 "description" :      "<description> = Optional('')",
                 "private" :          "<bool> = Optional(False)",
                 "clone_uri" :        "<clone_uri> = Optional(None)",
                 "landing_rev" :      "<landing_rev> = Optional('tip')",
                 "enable_downloads":  "<bool> = Optional(False)",
                 "enable_locking":    "<bool> = Optional(False)",
                 "enable_statistics": "<bool> = Optional(False)",
+              }
 OUTPUT::

kallithea/controllers/api/api.py

➞

Show inline comments

@@ @@ -1380,51 +1380,51 @@ class ApiController(JSONRPCController): @@
                 'all': _d + _f,
                 'files': _f,
                 'dirs': _d,
+            }
             return _map[ret_type]
         except KeyError:
             raise JSONRPCError('ret_type must be one of %s'
                                % (','.join(_map.keys())))
         except Exception:
             log.error(traceback.format_exc())
             raise JSONRPCError(
                 'failed to get repo: `%s` nodes' % repo.repo_name
+            )
     @HasPermissionAnyDecorator('hg.admin', 'hg.create.repository')
     def create_repo(self, apiuser, repo_name, owner=Optional(OAttr('apiuser')),
                     repo_type=Optional('hg'), description=Optional(''),
                     private=Optional(False), clone_uri=Optional(None),
                     landing_rev=Optional('rev:tip'),
                     enable_statistics=Optional(False),
                     enable_locking=Optional(False),
                     enable_downloads=Optional(False),
                     copy_permissions=Optional(False)):
         """
         Creates a repository. If repository name contains "/", all needed repository
         groups will be created. For example "foo/bar/baz" will create groups
         "foo", "bar" (with "foo" as parent), and create "baz" repository with
         Creates a repository. The repository name contains the full path, but the
         parent repository group must exist. For example "foo/bar/baz" require the groups
         "foo" and "bar" (with "foo" as parent), and create "baz" repository with
         "bar" as group. This command can be executed only using api_key
         belonging to user with admin rights or regular user that have create
         repository permission. Regular users cannot specify owner parameter
         :param apiuser: filled automatically from apikey
         :type apiuser: AuthUser
         :param repo_name: repository name
         :type repo_name: str
         :param owner: user_id or username
         :type owner: Optional(str)
         :param repo_type: 'hg' or 'git'
         :type repo_type: Optional(str)
         :param description: repository description
         :type description: Optional(str)
         :param private:
         :type private: bool
         :param clone_uri:
         :type clone_uri: str
         :param landing_rev: <rev_type>:<rev>
         :type landing_rev: str
         :param enable_locking:
         :type enable_locking: bool
         :param enable_downloads:
         :type enable_downloads: bool
@@ @@ -1464,53 +1464,57 @@ class ApiController(JSONRPCController): @@
         owner = get_user_or_error(owner)
         if RepoModel().get_by_repo_name(repo_name):
             raise JSONRPCError("repo `%s` already exist" % repo_name)
         defs = Setting.get_default_repo_settings(strip_prefix=True)
         if isinstance(private, Optional):
             private = defs.get('repo_private') or Optional.extract(private)
         if isinstance(repo_type, Optional):
             repo_type = defs.get('repo_type')
         if isinstance(enable_statistics, Optional):
             enable_statistics = defs.get('repo_enable_statistics')
         if isinstance(enable_locking, Optional):
             enable_locking = defs.get('repo_enable_locking')
         if isinstance(enable_downloads, Optional):
             enable_downloads = defs.get('repo_enable_downloads')
         clone_uri = Optional.extract(clone_uri)
         description = Optional.extract(description)
         landing_rev = Optional.extract(landing_rev)
         copy_permissions = Optional.extract(copy_permissions)
         try:
             repo_name_cleaned = repo_name.split('/')[-1]
             # create structure of groups and return the last group
             repo_group = map_groups(repo_name)
             repo_name_parts = repo_name.split('/')
             repo_group = None
             if len(repo_name_parts) > 1:
                 group_name = '/'.join(repo_name_parts[:-1])
                 repo_group = RepoGroup.get_by_group_name(group_name)
                 if repo_group is None:
                     raise JSONRPCError("repo group `%s` not found" % group_name)
             data = dict(
-                repo_name=repo_name_cleaned,
+                repo_name=repo_name_parts[-1],
                 repo_name_full=repo_name,
                 repo_type=repo_type,
                 repo_description=description,
                 owner=owner,
                 repo_private=private,
                 clone_uri=clone_uri,
                 repo_group=repo_group,
                 repo_landing_rev=landing_rev,
                 enable_statistics=enable_statistics,
                 enable_locking=enable_locking,
                 enable_downloads=enable_downloads,
                 repo_copy_permissions=copy_permissions,
+            )
             task = RepoModel().create(form_data=data, cur_user=owner)
             from celery.result import BaseAsyncResult
             task_id = None
             if isinstance(task, BaseAsyncResult):
                 task_id = task.task_id
             # no commit, it's done in RepoModel, or async via celery
             return dict(
                 msg="Created new repository `%s`" % (repo_name,),
                 success=True,  # cannot return the repo data here since fork
                                # can be done async
@@ @@ -1652,56 +1656,60 @@ class ApiController(JSONRPCController): @@
         if HasPermissionAnyApi('hg.admin')(user=apiuser):
             pass
         elif HasRepoPermissionAnyApi('repository.admin',
                                      'repository.write',
                                      'repository.read')(user=apiuser,
                                                         repo_name=repo.repo_name):
             if not isinstance(owner, Optional):
                 #forbid setting owner for non-admins
                 raise JSONRPCError(
                     'Only Kallithea admin can specify `owner` param'
+                )
             if not HasPermissionAnyApi('hg.create.repository')(user=apiuser):
                 raise JSONRPCError('no permission to create repositories')
         else:
             raise JSONRPCError('repository `%s` does not exist' % (repoid,))
         if isinstance(owner, Optional):
             owner = apiuser.user_id
         owner = get_user_or_error(owner)
         try:
             # create structure of groups and return the last group
             group = map_groups(fork_name)
             fork_base_name = fork_name.rsplit('/', 1)[-1]
             fork_name_parts = fork_name.split('/')
             repo_group = None
             if len(fork_name_parts) > 1:
                 group_name = '/'.join(fork_name_parts[:-1])
                 repo_group = RepoGroup.get_by_group_name(group_name)
                 if repo_group is None:
                     raise JSONRPCError("repo group `%s` not found" % group_name)
             form_data = dict(
-                repo_name=fork_base_name,
+                repo_name=fork_name_parts[-1],
                 repo_name_full=fork_name,
-                repo_group=group,
+                repo_group=repo_group,
                 repo_type=repo.repo_type,
                 description=Optional.extract(description),
                 private=Optional.extract(private),
                 copy_permissions=Optional.extract(copy_permissions),
                 landing_rev=Optional.extract(landing_rev),
                 update_after_clone=False,
                 fork_parent_id=repo.repo_id,
+            )
             task = RepoModel().create_fork(form_data, cur_user=owner)
             # no commit, it's done in RepoModel, or async via celery
             from celery.result import BaseAsyncResult
             task_id = None
             if isinstance(task, BaseAsyncResult):
                 task_id = task.task_id
             return dict(
                 msg='Created fork of `%s` as `%s`' % (repo.repo_name,
                                                       fork_name),
                 success=True,  # cannot return the repo data here since fork
                                # can be done async
                 task=task_id
+            )
         except Exception:
             log.error(traceback.format_exc())
             raise JSONRPCError(

kallithea/tests/api/api_base.py

➞

Show inline comments

@@ @@ -997,48 +997,62 @@ class _BaseTestApi(object): @@
         expected = ret
         self._compare_ok(id_, expected, given=response.body)
         fixture.destroy_repo(repo_name)
     def test_api_create_repo_clone_uri_local(self):
         # cloning from local repo was a mis-feature - it would bypass access control
         # TODO: introduce other test coverage of actual remote cloning
         clone_uri = os.path.join(TESTS_TMP_PATH, self.REPO)
         repo_name = u'api-repo'
         id_, params = _build_data(self.apikey, 'create_repo',
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,
                                   clone_uri=clone_uri,
+        )
         response = api_call(self, params)
         expected = "failed to create repository `%s`" % repo_name
         self._compare_error(id_, expected, given=response.body)
         fixture.destroy_repo(repo_name)
     def test_api_create_repo_and_repo_group(self):
         repo_group_name = 'my_gr'
         repo_name = '%s/api-repo' % repo_group_name
         # repo creation can no longer also create repo group
         id_, params = _build_data(self.apikey, 'create_repo',
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,)
         response = api_call(self, params)
         expected = u'repo group `%s` not found' % repo_group_name
         self._compare_error(id_, expected, given=response.body)
         assert RepoModel().get_by_repo_name(repo_name) is None
         # create group before creating repo
         rg = fixture.create_repo_group(repo_group_name)
         Session().commit()
         id_, params = _build_data(self.apikey, 'create_repo',
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,)
         response = api_call(self, params)
         expected = {
             'msg': 'Created new repository `%s`' % repo_name,
             'success': True,
             'task': None,
+        }
         self._compare_ok(id_, expected, given=response.body)
         repo = RepoModel().get_by_repo_name(repo_name)
         self.assertNotEqual(repo, None)
         fixture.destroy_repo(repo_name)
         fixture.destroy_repo_group(repo_group_name)
     def test_api_create_repo_in_repo_group_without_permission(self):
         repo_group_name = '%s/api-repo-repo' % TEST_REPO_GROUP
         repo_name = '%s/api-repo' % repo_group_name
         rg = fixture.create_repo_group(repo_group_name)
         Session().commit()
         RepoGroupModel().grant_user_permission(repo_group_name,
@@ @@ -1123,62 +1137,58 @@ class _BaseTestApi(object): @@
     def test_api_create_repo_by_non_admin_specify_owner(self):
         repo_name = 'api-repo'
         owner = 'i-dont-exist'
         id_, params = _build_data(self.apikey_regular, 'create_repo',
                                   repo_name=repo_name,
                                   repo_type=self.REPO_TYPE,
                                   owner=owner)
         response = api_call(self, params)
         expected = 'Only Kallithea admin can specify `owner` param'
         self._compare_error(id_, expected, given=response.body)
         fixture.destroy_repo(repo_name)
     def test_api_create_repo_exists(self):
         repo_name = self.REPO
         id_, params = _build_data(self.apikey, 'create_repo',
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,)
         response = api_call(self, params)
         expected = "repo `%s` already exist" % repo_name
         self._compare_error(id_, expected, given=response.body)
     def test_api_create_repo_dot_dot(self):
-        # FIXME: it should only be possible to create repositories in existing repo groups - and '..' can't be used
+        # it is only possible to create repositories in existing repo groups - and '..' can't be used
         group_name = '%s/..' % TEST_REPO_GROUP
         repo_name = '%s/%s' % (group_name, 'could-be-outside')
         id_, params = _build_data(self.apikey, 'create_repo',
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,)
         response = api_call(self, params)
         expected = {
             u'msg': u"Created new repository `%s`" % repo_name,
             u'success': True,
             u'task': None,
+        }
         self._compare_ok(id_, expected, given=response.body)
         expected = u'repo group `%s` not found' % group_name
         self._compare_error(id_, expected, given=response.body)
         fixture.destroy_repo(repo_name)
     @mock.patch.object(RepoModel, 'create', crash)
     def test_api_create_repo_exception_occurred(self):
         repo_name = 'api-repo'
         id_, params = _build_data(self.apikey, 'create_repo',
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,)
         response = api_call(self, params)
         expected = 'failed to create repository `%s`' % repo_name
         self._compare_error(id_, expected, given=response.body)
     @parameterized.expand([
         ('owner', {'owner': TEST_USER_REGULAR_LOGIN}),
         ('description', {'description': 'new description'}),
         ('active', {'active': True}),
         ('active', {'active': False}),
         ('clone_uri', {'clone_uri': 'http://example.com/repo'}), # will fail - pulling from non-existing repo should fail
         ('clone_uri', {'clone_uri': '/repo'}), # will fail - pulling from local repo was a mis-feature - it would bypass access control
         ('clone_uri', {'clone_uri': None}),
         ('landing_rev', {'landing_rev': 'branch:master'}),
         ('enable_statistics', {'enable_statistics': True}),
         ('enable_locking', {'enable_locking': True}),

0 comments (0 inline, 0 general)