On Wed, Mar 7, 2018 at 8:49 PM, Matt Harbison <mharbiso...@gmail.com> wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbi...@yahoo.com>
> # Date 1519274700 18000
> #      Wed Feb 21 23:45:00 2018 -0500
> # Node ID 89382cb20bb19e089513b2ce69ef8acfa1f523fd
> # Parent  6dab3bdb1f00932a80ffa07f80ba240c3a4b48df
> hgweb: add a hook for processing LFS Batch API requests
>
> There really isn't a clean way to give LFS a crack at intercepting the
> requests
> without hardcoding some LFS knowledge in the core.  The rationale for this
> URI
> is that the spec for the Batch API[1] defines the URL as the LFS server
> url +
> '/objects/batch'.  The default git URLs are:
>
>     Git remote: https://git-server.com/foo/bar
>     LFS server: https://git-server.com/foo/bar.git/info/lfs
>     Batch API: https://git-server.com/foo/bar.git/info/lfs/objects/batch
>
> '.git/' seems like it's not something a user would normally track.  If we
> adhere
> to how git defines the URLs, then the hg-git extension should be able to
> talk to
> a git based server without any additional work.
>
> I'm not sure if checking the User-Agent is a good idea, but this needs a
> specialized client, and it seems like everyone else is doing it
> (3d48ae1aaa5e,
> e7bb5fc4570c).  We can always back this off if it becomes a nuisance.  A
> web
> browser will see "400: bad method", the same as it would before this
> change.
>
> I'm not sure if None or 'pull' is the proper permission check, but the only
> difference is whether or not `hgweb.allowpull` is checked.  Since nothing
> of
> particular interest is transferred here, and the next phase handles the
> read or
> write, treating this like web interface request seems fine.
>
> [1] https://github.com/git-lfs/git-lfs/blob/master/docs/api/batch.md
>
> diff --git a/mercurial/hgweb/hgweb_mod.py b/mercurial/hgweb/hgweb_mod.py
> --- a/mercurial/hgweb/hgweb_mod.py
> +++ b/mercurial/hgweb/hgweb_mod.py
> @@ -90,6 +90,12 @@
>          urlel = os.path.dirname(urlel)
>      return reversed(breadcrumb)
>
> +def _processlfsbatchreq(repo, req):
> +    """A hook for the LFS extension to wrap that handles requests to the
> Batch
> +    API, and returns the appropriate JSON response.
> +    """
> +    raise ErrorResponse(HTTP_NOT_FOUND)
> +
>  class requestcontext(object):
>      """Holds state/context for an individual request.
>
> @@ -376,6 +382,21 @@
>              except ErrorResponse as inst:
>                  return protohandler['handleerror'](inst)
>
> +        # Route LFS Batch API requests to the appropriate handler
> +
> +        if req.env.get(r'HTTP_USER_AGENT', r'').startswith(r'git-lfs/'):
>

I don't like filtering by the user-agent. It is recommended to not do this.
Unless it will cause problems or tons of code complexity to properly lock
out actual Git clients who may get confused by this, my vote is to remove
it and just rely on path filtering.


> +            try:
> +                path = req.env.get(r'PATH_INFO')
> +                if path == '/.git/info/lfs/objects/batch':
>

Relying on PATH_INFO is super annoying. But our code kind of sucks :/

My only suggestion would be to define `parts` in the `else` block of the
`if r'PATH_INFO' in req.env:` above and check `parts == ['.git', 'info',
'lfs', 'objects', 'batch']`. That's still a big ugly though.

Whatever happens, my guess is I will eventually write some patches to clean
the URL parsing code up.


> +                    self.check_perm(rctx, req, None)
>

Shouldn't this be `pull` instead of `None`? Otherwise, LFS won't honor
web.* to restrict data access.


> +                    return _processlfsbatchreq(rctx.repo, req)
> +                else:
> +                    raise ErrorResponse(HTTP_NOT_FOUND)
> +            except ErrorResponse as inst:
> +                req.respond(inst, 'text/plain; charset=utf-8')
> +                # No body, since only lfs clients are allowed here
> +                return ['']
> +
>          # translate user-visible url structure to internal structure
>
>          args = query.split('/', 2)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to