launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'config.yaml'
> --- config.yaml	2015-03-30 11:11:55 +0000
> +++ config.yaml	2015-03-30 17:01:07 +0000
> @@ -10,3 +10,5 @@
>  repo_store: /var/tmp/git.launchpad.dev
>  turnip_log_dir: ./
>  virtinfo_endpoint: http://xmlrpc-private.launchpad.dev:8087/git
> +cgit_exec_path: /usr/lib/cgit/cgit.cgi
> +cgit_data_path: /usr/share/cgit
> 
> === modified file 'httpserver.tac'
> --- httpserver.tac	2015-02-18 22:53:47 +0000
> +++ httpserver.tac	2015-03-30 17:01:07 +0000
> @@ -19,7 +19,8 @@
>      smarthttp_site = server.Site(
>          SmartHTTPFrontendResource(b'localhost',
>                                    config.get('pack_virt_port'),
> -                                  config.get('virtinfo_endpoint')))
> +                                  config.get('virtinfo_endpoint'),
> +                                  config.get('repo_store')))
>      return internet.TCPServer(config.get('smart_http_port'), smarthttp_site)
>  
>  application = service.Application("Turnip SmartHTTP Service")
> 
> === modified file 'turnip/pack/http.py'
> --- turnip/pack/http.py	2015-03-26 03:50:48 +0000
> +++ turnip/pack/http.py	2015-03-30 17:01:07 +0000
> @@ -5,6 +5,9 @@
>      )
>  
>  from cStringIO import StringIO
> +import os.path
> +import tempfile
> +import textwrap
>  import sys
>  import zlib
>  
> @@ -17,6 +20,8 @@
>      http,
>      resource,
>      server,
> +    static,
> +    twcgi,
>      )
>  
>  from turnip.pack.git import (
> @@ -290,30 +295,156 @@
>          return b''
>  
>  
> +class DirectoryWithoutListings(static.File):
> +    """A static directory resource without support for directory listings."""
> +
> +    def directoryListing(self):
> +        return self.childNotFound
> +
> +
> +class RobotsResource(static.Data):
> +    """HTTP resource to serve our robots.txt."""
> +
> +    robots_txt = textwrap.dedent("""\
> +        User-agent: *
> +        Disallow: /
> +        """).encode('US-ASCII')
> +
> +    def __init__(self):
> +        static.Data.__init__(self, self.robots_txt, 'text/plain')
> +
> +
> +class CGitScriptResource(twcgi.CGIScript):
> +    """HTTP resource to run cgit."""
> +
> +    def __init__(self, root):
> +        twcgi.CGIScript.__init__(self, root.cgit_exec_path)
> +        self.root = root
> +        self.cgit_config = None
> +
> +    def _error(self, request, message, code=http.INTERNAL_SERVER_ERROR):
> +        request.setResponseCode(code)
> +        request.setHeader(b'Content-Type', b'text/plain')
> +        request.write(message)
> +        request.finish()
> +
> +    def _finished(self, ignored):
> +        if self.cgit_config is not None:
> +            self.cgit_config.close()
> +
> +    def _translatePathCallback(self, translated, env, request,
> +                               *args, **kwargs):
> +        if 'path' not in translated:
> +            self._error(
> +                request, b'translatePath response did not include path')
> +            return
> +        repo_url = request.path.rstrip('/')
> +        repo_path = '%s/%s' % (self.root.repo_store, translated['path'])
> +        trailing = translated.get('trailing')
> +        if trailing:
> +            if not trailing.startswith('/'):
> +                trailing = '/' + trailing
> +            if not repo_url.endswith(trailing):
> +                self._error(
> +                    request,
> +                    b'translatePath returned inconsistent response: '
> +                    b'"%s" does not end with "%s"' % (
> +                        repo_url.encode('UTF-8'), trailing.encode('UTF-8')))
> +                return
> +            repo_url = repo_url[:-len(trailing)]
> +        repo_url = repo_url.strip('/')
> +        request.notifyFinish().addBoth(self._finished)
> +        self.cgit_config = tempfile.NamedTemporaryFile(
> +            mode='w+', prefix='turnip-cgit-')
> +        os.chmod(self.cgit_config.name, 0o644)
> +        fmt = {'repo_url': repo_url, 'repo_path': repo_path}
> +        print(textwrap.dedent("""\
> +            # XXX clone-prefix
> +            css=/static/cgit.css
> +            enable-http-clone=0
> +            enable-index-owner=0
> +            logo=/static/cgit.png
> +
> +            repo.url={repo_url}
> +            repo.path={repo_path}
> +            """).format(**fmt), file=self.cgit_config)

Can you perform some validation on repo_url and repo_path? It doesn't seem inconceivable that repo_url could contain metacharacters, and repo_path should be rejected if it doesn't end up as a subtree of repo_store. turnip.helpers.compose_path achieves the latter.

Also, this should probably respect cgit_data_path.

> +        self.cgit_config.flush()
> +        env["CGIT_CONFIG"] = self.cgit_config.name
> +        env["PATH_INFO"] = "/%s%s" % (repo_url, trailing)
> +        env["SCRIPT_NAME"] = "/"
> +        twcgi.CGIScript.runProcess(self, env, request, *args, **kwargs)
> +
> +    def _translatePathErrback(self, failure, request):
> +        e = failure.value
> +        if e.faultCode in (1, 290):
> +            error_code = http.NOT_FOUND
> +        elif e.faultCode in (2, 310):
> +            error_code = http.FORBIDDEN
> +        elif e.faultCode in (3, 410):
> +            # XXX cjwatson 2015-03-30: should be UNAUTHORIZED, but we
> +            # don't implement that yet
> +            error_code = http.FORBIDDEN
> +        else:
> +            error_code = http.INTERNAL_SERVER_ERROR
> +        self._error(request, e.faultString, code=error_code)
> +
> +    def runProcess(self, env, request, *args, **kwargs):
> +        proxy = xmlrpc.Proxy(self.root.virtinfo_endpoint, allowNone=True)
> +        # XXX cjwatson 2015-03-30: authentication
> +        d = proxy.callRemote(
> +            b'translatePath', request.path, b'read', None, False)
> +        d.addErrback(self._translatePathErrback, request)
> +        d.addCallback(
> +            self._translatePathCallback, env, request, *args, **kwargs)

The callback should be added first. A failure causes the first errback to be executed, but if it returns nothing then the error is considered to be handled and subsequent callbacks will be executed.

> +        return server.NOT_DONE_YET
> +
> +
>  class SmartHTTPFrontendResource(resource.Resource):
>      """HTTP resource to translate Git smart HTTP requests to pack protocol."""
>  
>      allowed_services = frozenset((b'git-upload-pack', b'git-receive-pack'))
>  
> -    def __init__(self, backend_host, backend_port, virtinfo_endpoint):
> +    def __init__(self, backend_host, backend_port, virtinfo_endpoint,
> +                 repo_store, cgit_exec_path=None, cgit_data_path=None):
>          resource.Resource.__init__(self)
>          self.backend_host = backend_host
>          self.backend_port = backend_port
>          self.virtinfo_endpoint = virtinfo_endpoint
> +        # XXX cjwatson 2015-03-30: Knowing about the store path here
> +        # violates turnip's layering and may cause scaling problems later,
> +        # but for now cgit needs direct filesystem access.
> +        self.repo_store = repo_store
> +        self.cgit_exec_path = cgit_exec_path
>          self.putChild('', SmartHTTPRootResource())
> +        if cgit_data_path is not None:
> +            static_resource = DirectoryWithoutListings(
> +                cgit_data_path, defaultType='text/plain')
> +            self.putChild('static', static_resource)
> +            self.putChild(
> +                'favicon.ico',
> +                static.File(os.path.join(cgit_data_path, 'favicon.ico')))
> +
> +    @staticmethod
> +    def _isGitRequest(request):
> +        content_type = request.getHeader(b'Content-Type')
> +        if content_type is None:
> +            return False
> +        return content_type.startswith(b'application/x-git-')

Were it only so easy.

This breaks smart HTTP fetch and push, as the info/refs request doesn't have a special Content-Type. You can only distinguish it by knowing when you've hit a repo or by checking that the request ends in /info/refs?service=git-*. The test suite catches this.

>  
>      def getChild(self, path, request):
> -        if request.path.endswith(b'/info/refs'):
> -            # /PATH/TO/REPO/info/refs
> -            return SmartHTTPRefsResource(
> -                self, request.path[:-len(b'/info/refs')])
> -        try:
> -            # /PATH/TO/REPO/SERVICE
> -            path, service = request.path.rsplit(b'/', 1)
> -        except ValueError:
> -            path = request.path
> -            service = None
> -        if service in self.allowed_services:
> -            return SmartHTTPCommandResource(self, service, path)
> -
> +        if self._isGitRequest(request):
> +            if request.path.endswith(b'/info/refs'):
> +                # /PATH/TO/REPO/info/refs
> +                return SmartHTTPRefsResource(
> +                    self, request.path[:-len(b'/info/refs')])
> +            try:
> +                # /PATH/TO/REPO/SERVICE
> +                path, service = request.path.rsplit(b'/', 1)
> +            except ValueError:
> +                path = request.path
> +                service = None
> +            if service in self.allowed_services:
> +                return SmartHTTPCommandResource(self, service, path)
> +        elif self.cgit_exec_path is not None:
> +            return CGitScriptResource(self)
>          return resource.NoResource(b'No such resource')
> 
> === modified file 'turnip/pack/tests/test_functional.py'
> --- turnip/pack/tests/test_functional.py	2015-03-30 09:19:41 +0000
> +++ turnip/pack/tests/test_functional.py	2015-03-30 17:01:07 +0000
> @@ -345,7 +345,7 @@
>          # virtinfo servers started by the mixin.
>          frontend_site = server.Site(
>              SmartHTTPFrontendResource(
> -                b'localhost', self.virt_port, self.virtinfo_url))
> +                b'localhost', self.virt_port, self.virtinfo_url, self.root))
>          self.frontend_listener = reactor.listenTCP(0, frontend_site)
>          self.port = self.frontend_listener.getHost().port
>  
> 
> === modified file 'turnipserver.py'
> --- turnipserver.py	2015-03-30 09:19:41 +0000
> +++ turnipserver.py	2015-03-30 17:01:07 +0000
> @@ -31,6 +31,8 @@
>  PACK_BACKEND_PORT = config.get('pack_backend_port')
>  REPO_STORE = config.get('repo_store')
>  VIRTINFO_ENDPOINT = config.get('virtinfo_endpoint')
> +CGIT_EXEC_PATH = config.get('cgit_exec_path')
> +CGIT_DATA_PATH = config.get('cgit_data_path')
>  
>  # turnipserver.py is preserved for convenience in development, services
>  # in production are run in separate processes.
> @@ -54,7 +56,9 @@
>                    PackFrontendFactory('localhost',
>                                        PACK_VIRT_PORT))
>  smarthttp_site = server.Site(
> -    SmartHTTPFrontendResource(b'localhost', PACK_VIRT_PORT, VIRTINFO_ENDPOINT))
> +    SmartHTTPFrontendResource(
> +        b'localhost', PACK_VIRT_PORT, VIRTINFO_ENDPOINT, REPO_STORE,
> +        cgit_exec_path=CGIT_EXEC_PATH, cgit_data_path=CGIT_DATA_PATH))
>  reactor.listenTCP(config.get('smart_http_port'), smarthttp_site)
>  smartssh_service = SmartSSHService(
>      b'localhost', PACK_VIRT_PORT, config.get('authentication_endpoint'),
> 


-- 
https://code.launchpad.net/~cjwatson/turnip/cgit/+merge/254606
Your team Launchpad code reviewers is subscribed to branch lp:turnip.