This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2819

hgweb: refactor repository name URL parsing
ClosedPublic

Authored by indygreg on Mar 12 2018, 5:16 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGd7fd203e36cc: hgweb: refactor repository name URL parsing
Summary

The hgwebdir WSGI application detects when a requested URL is for
a known repository and it effectively forwards the request to the
hgweb WSGI application.

The hgweb WSGI application needs to route the request based on the
base URL for the repository. The way this normally works is
SCRIPT_NAME is used to resolve the base URL and PATH_INFO
contains the path after the script.

But with hgwebdir, SCRIPT_NAME refers to hgwebdir, not the base
URL for the repository. So, there was a hacky REPO_NAME environment
variable being set to convey the part of the URL that represented
the repository so hgweb could ignore this path component for
routing purposes.

The use of the environment variable for passing internal state
is pretty hacky. Plus, it wasn't clear from the perspective of
the URL parsing code what was going on.

This commit improves matters by making the repository name an
explicit argument to the request parser. The logic around
handling of this value has been shored up. We add various checks
that the argument is used properly - that the repository name
does represent the prefix of the PATH_INFO.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Mar 12 2018, 5:16 PM
durin42 accepted this revision.Mar 12 2018, 5:27 PM
This revision is now accepted and ready to land.Mar 12 2018, 5:27 PM
Closed by commit rHGd7fd203e36cc: hgweb: refactor repository name URL parsing (authored by indygreg). · Explain WhyMar 12 2018, 5:38 PM
This revision was automatically updated to reflect the committed changes.
mharbison72 added a subscriber: mharbison72.Apr 4 2018, 1:02 PM

I installed the latest default branch with SCM Manager, and it 404s even simple things like hg id https://.... I bisected back to this. The paths in the access log looks unchanged:

With this commit:

127.0.0.1 - - [04/Apr/2018:12:44:12 -0400] "GET /hook/hg/?ping=true HTTP/1.1" 204 -
10.10.1.36 - - [04/Apr/2018:12:44:12 -0400] "GET /hg/eng/devsetup?cmd=capabilities HTTP/1.1" 404 949

Parent of this commit:

127.0.0.1 - - [04/Apr/2018:12:47:18 -0400] "GET /hook/hg/?ping=true HTTP/1.1" 204 -
10.10.1.36 - - [04/Apr/2018:12:47:19 -0400] "GET /hg/eng/devsetup?cmd=capabilities HTTP/1.1" 200 422
10.10.1.36 - - [04/Apr/2018:12:47:19 -0400] "GET /hg/eng/devsetup?cmd=lookup HTTP/1.1" 200 43
10.10.1.36 - - [04/Apr/2018:12:47:20 -0400] "GET /hg/eng/devsetup?cmd=listkeys HTTP/1.1" 200 30
10.10.1.36 - - [04/Apr/2018:12:47:20 -0400] "GET /hg/eng/devsetup?cmd=listkeys HTTP/1.1" 200 -

I'm going to try to add print statements, but if you have any darts you'd like to throw, I'd be happy to try it.

mharbison72 added a comment.Apr 5 2018, 2:59 PM
In D2819#49519, @mharbison72 wrote:

I installed the latest default branch with SCM Manager, and it 404s even simple things like hg id https://.... I bisected back to this. The paths in the access log looks unchanged:
With this commit:
127.0.0.1 - - [04/Apr/2018:12:44:12 -0400] "GET /hook/hg/?ping=true HTTP/1.1" 204 -
10.10.1.36 - - [04/Apr/2018:12:44:12 -0400] "GET /hg/eng/devsetup?cmd=capabilities HTTP/1.1" 404 949
Parent of this commit:
127.0.0.1 - - [04/Apr/2018:12:47:18 -0400] "GET /hook/hg/?ping=true HTTP/1.1" 204 -
10.10.1.36 - - [04/Apr/2018:12:47:19 -0400] "GET /hg/eng/devsetup?cmd=capabilities HTTP/1.1" 200 422
10.10.1.36 - - [04/Apr/2018:12:47:19 -0400] "GET /hg/eng/devsetup?cmd=lookup HTTP/1.1" 200 43
10.10.1.36 - - [04/Apr/2018:12:47:20 -0400] "GET /hg/eng/devsetup?cmd=listkeys HTTP/1.1" 200 30
10.10.1.36 - - [04/Apr/2018:12:47:20 -0400] "GET /hg/eng/devsetup?cmd=listkeys HTTP/1.1" 200 -
I'm going to try to add print statements, but if you have any darts you'd like to throw, I'd be happy to try it.

Finally figured this out. SCM Manager must be generating this stub hgweb.py (it gets rewritten every time tomcat is restarted):

import os
from mercurial import demandimport
from mercurial.hgweb import hgweb, wsgicgi

repositoryPath = os.environ['SCM_REPOSITORY_PATH']

demandimport.enable()

application = hgweb(repositoryPath)
wsgicgi.launch(application)

Note that it's not using hgwebdir. (I have no idea why not, but it does let you organize repos into virtual directories under '/hg', and git repos under '/git'.) If we simply set the reponame value in request.parserequestfromenv() from env if it wasn't passed in, then everything works. Not real nice, but I assume that we don't want to break existing hosting packages when upgrading just Mercurial? I'll send a patch if this is acceptable.

Revision Contents
Changeset List

Diff 6950

mercurial/hgweb/hgwebdir_mod.py

for p in util.finddirs(virtual): for p in util.finddirs(virtual):
yield p yield p
yield '' yield ''
for virtualrepo in _virtualdirs(): for virtualrepo in _virtualdirs():
real = repos.get(virtualrepo) real = repos.get(virtualrepo)
if real: if real:
wsgireq.env['REPO_NAME'] = virtualrepo # Re-parse the WSGI environment to take into account our
# We have to re-parse because of updated environment # repository path component.
# variable.
# TODO this is kind of hacky and we should have a better
# way of doing this than with REPO_NAME side-effects.
wsgireq.req = requestmod.parserequestfromenv( wsgireq.req = requestmod.parserequestfromenv(
wsgireq.env, wsgireq.req.bodyfh) wsgireq.env, wsgireq.req.bodyfh, reponame=virtualrepo)
try: try:
# ensure caller gets private copy of ui # ensure caller gets private copy of ui
repo = hg.repository(self.ui.copy(), real) repo = hg.repository(self.ui.copy(), real)
return hgweb_mod.hgweb(repo).run_wsgi(wsgireq) return hgweb_mod.hgweb(repo).run_wsgi(wsgireq)
except IOError as inst: except IOError as inst:
msg = encoding.strtolocal(inst.strerror) msg = encoding.strtolocal(inst.strerror)
raise ErrorResponse(HTTP_SERVER_ERROR, msg) raise ErrorResponse(HTTP_SERVER_ERROR, msg)
except error.RepoError as inst: except error.RepoError as inst:

mercurial/hgweb/request.py

# multidict of query string parameters. # multidict of query string parameters.
qsparams = attr.ib() qsparams = attr.ib()
# wsgiref.headers.Headers instance. Operates like a dict with case # wsgiref.headers.Headers instance. Operates like a dict with case
# insensitive keys. # insensitive keys.
headers = attr.ib() headers = attr.ib()
# Request body input stream. # Request body input stream.
bodyfh = attr.ib() bodyfh = attr.ib()
def parserequestfromenv(env, bodyfh): def parserequestfromenv(env, bodyfh, reponame=None):
"""Parse URL components from environment variables. """Parse URL components from environment variables.
WSGI defines request attributes via environment variables. This function WSGI defines request attributes via environment variables. This function
parses the environment variables into a data structure. parses the environment variables into a data structure.
If ``reponame`` is defined, the leading path components matching that
string are effectively shifted from ``PATH_INFO`` to ``SCRIPT_NAME``.
This simulates the world view of a WSGI application that processes
requests from the base URL of a repo.
""" """
# PEP-0333 defines the WSGI spec and is a useful reference for this code. # PEP-0333 defines the WSGI spec and is a useful reference for this code.
# We first validate that the incoming object conforms with the WSGI spec. # We first validate that the incoming object conforms with the WSGI spec.
# We only want to be dealing with spec-conforming WSGI implementations. # We only want to be dealing with spec-conforming WSGI implementations.
# TODO enable this once we fix internal violations. # TODO enable this once we fix internal violations.
#wsgiref.validate.check_environ(env) #wsgiref.validate.check_environ(env)
advertisedfullurl += util.urlreq.quote(env.get('SCRIPT_NAME', '')) advertisedfullurl += util.urlreq.quote(env.get('SCRIPT_NAME', ''))
fullurl += util.urlreq.quote(env.get('PATH_INFO', '')) fullurl += util.urlreq.quote(env.get('PATH_INFO', ''))
advertisedfullurl += util.urlreq.quote(env.get('PATH_INFO', '')) advertisedfullurl += util.urlreq.quote(env.get('PATH_INFO', ''))
if env.get('QUERY_STRING'): if env.get('QUERY_STRING'):
fullurl += '?' + env['QUERY_STRING'] fullurl += '?' + env['QUERY_STRING']
advertisedfullurl += '?' + env['QUERY_STRING'] advertisedfullurl += '?' + env['QUERY_STRING']
# When dispatching requests, we look at the URL components (PATH_INFO # If ``reponame`` is defined, that must be a prefix on PATH_INFO
# and QUERY_STRING) after the application root (SCRIPT_NAME). But hgwebdir # that represents the repository being dispatched to. When computing
# has the concept of "virtual" repositories. This is defined via REPO_NAME. # the dispatch info, we ignore these leading path components.
# If REPO_NAME is defined, we append it to SCRIPT_NAME to form a new app
# root. We also exclude its path components from PATH_INFO when resolving
# the dispatch path.
apppath = env.get('SCRIPT_NAME', '') apppath = env.get('SCRIPT_NAME', '')
if env.get('REPO_NAME'): if reponame:
if not apppath.endswith('/'): repoprefix = '/' + reponame.strip('/')
apppath += '/'
apppath += env.get('REPO_NAME') if not env.get('PATH_INFO'):
raise error.ProgrammingError('reponame requires PATH_INFO')
if 'PATH_INFO' in env: if not env['PATH_INFO'].startswith(repoprefix):
raise error.ProgrammingError('PATH_INFO does not begin with repo '
'name: %s (%s)' % (env['PATH_INFO'],
reponame))
dispatchpath = env['PATH_INFO'][len(repoprefix):]
if dispatchpath and not dispatchpath.startswith('/'):
raise error.ProgrammingError('reponame prefix of PATH_INFO does '
'not end at path delimiter: %s (%s)' %
(env['PATH_INFO'], reponame))
apppath = apppath.rstrip('/') + repoprefix
dispatchparts = dispatchpath.strip('/').split('/')
elif env.get('PATH_INFO', '').strip('/'):
dispatchparts = env['PATH_INFO'].strip('/').split('/') dispatchparts = env['PATH_INFO'].strip('/').split('/')
# Strip out repo parts.
repoparts = env.get('REPO_NAME', '').split('/')
if dispatchparts[:len(repoparts)] == repoparts:
dispatchparts = dispatchparts[len(repoparts):]
else: else:
dispatchparts = [] dispatchparts = []
dispatchpath = '/'.join(dispatchparts) dispatchpath = '/'.join(dispatchparts)
querystring = env.get('QUERY_STRING', '') querystring = env.get('QUERY_STRING', '')
# We store as a list so we have ordering information. We also store as # We store as a list so we have ordering information. We also store as
advertisedurl=advertisedfullurl, advertisedurl=advertisedfullurl,
advertisedbaseurl=advertisedbaseurl, advertisedbaseurl=advertisedbaseurl,
urlscheme=env['wsgi.url_scheme'], urlscheme=env['wsgi.url_scheme'],
remoteuser=env.get('REMOTE_USER'), remoteuser=env.get('REMOTE_USER'),
remotehost=env.get('REMOTE_HOST'), remotehost=env.get('REMOTE_HOST'),
apppath=apppath, apppath=apppath,
dispatchparts=dispatchparts, dispatchpath=dispatchpath, dispatchparts=dispatchparts, dispatchpath=dispatchpath,
havepathinfo='PATH_INFO' in env, havepathinfo='PATH_INFO' in env,
reponame=env.get('REPO_NAME'), reponame=reponame,
querystring=querystring, querystring=querystring,
qsparams=qsparams, qsparams=qsparams,
headers=headers, headers=headers,
bodyfh=bodyfh) bodyfh=bodyfh)
class offsettrackingwriter(object): class offsettrackingwriter(object):
"""A file object like object that is append only and tracks write count. """A file object like object that is append only and tracks write count.

tests/test-wsgirequest.py

from __future__ import absolute_import, print_function from __future__ import absolute_import, print_function
import unittest import unittest
from mercurial.hgweb import ( from mercurial.hgweb import (
request as requestmod, request as requestmod,
) )
from mercurial import (
error,
)
DEFAULT_ENV = { DEFAULT_ENV = {
r'REQUEST_METHOD': r'GET', r'REQUEST_METHOD': r'GET',
r'SERVER_NAME': r'testserver', r'SERVER_NAME': r'testserver',
r'SERVER_PORT': r'80', r'SERVER_PORT': r'80',
r'SERVER_PROTOCOL': r'http', r'SERVER_PROTOCOL': r'http',
r'wsgi.version': (1, 0), r'wsgi.version': (1, 0),
r'wsgi.url_scheme': r'http', r'wsgi.url_scheme': r'http',
r'wsgi.input': None, r'wsgi.input': None,
r'wsgi.errors': None, r'wsgi.errors': None,
r'wsgi.multithread': False, r'wsgi.multithread': False,
r'wsgi.multiprocess': True, r'wsgi.multiprocess': True,
r'wsgi.run_once': False, r'wsgi.run_once': False,
} }
def parse(env, bodyfh=None, extra=None): def parse(env, bodyfh=None, reponame=None, extra=None):
env = dict(env) env = dict(env)
env.update(extra or {}) env.update(extra or {})
return requestmod.parserequestfromenv(env, bodyfh) return requestmod.parserequestfromenv(env, bodyfh, reponame=reponame)
class ParseRequestTests(unittest.TestCase): class ParseRequestTests(unittest.TestCase):
def testdefault(self): def testdefault(self):
r = parse(DEFAULT_ENV) r = parse(DEFAULT_ENV)
self.assertEqual(r.url, b'http://testserver') self.assertEqual(r.url, b'http://testserver')
self.assertEqual(r.baseurl, b'http://testserver') self.assertEqual(r.baseurl, b'http://testserver')
self.assertEqual(r.advertisedurl, b'http://testserver') self.assertEqual(r.advertisedurl, b'http://testserver')
self.assertEqual(r.advertisedbaseurl, b'http://testserver') self.assertEqual(r.advertisedbaseurl, b'http://testserver')
self.assertEqual(r.advertisedurl, b'http://testserver/script/pathinfo') self.assertEqual(r.advertisedurl, b'http://testserver/script/pathinfo')
self.assertEqual(r.advertisedbaseurl, b'http://testserver') self.assertEqual(r.advertisedbaseurl, b'http://testserver')
self.assertEqual(r.apppath, b'/script') self.assertEqual(r.apppath, b'/script')
self.assertEqual(r.dispatchparts, [b'pathinfo']) self.assertEqual(r.dispatchparts, [b'pathinfo'])
self.assertEqual(r.dispatchpath, b'pathinfo') self.assertEqual(r.dispatchpath, b'pathinfo')
self.assertTrue(r.havepathinfo) self.assertTrue(r.havepathinfo)
def testreponame(self): def testreponame(self):
"""REPO_NAME path components get stripped from URL.""" """repository path components get stripped from URL."""
r = parse(DEFAULT_ENV, extra={
r'REPO_NAME': r'repo', with self.assertRaisesRegexp(error.ProgrammingError,
r'PATH_INFO': r'/path1/path2' b'reponame requires PATH_INFO'):
parse(DEFAULT_ENV, reponame=b'repo')
with self.assertRaisesRegexp(error.ProgrammingError,
b'PATH_INFO does not begin with repo '
b'name'):
parse(DEFAULT_ENV, reponame=b'repo', extra={
r'PATH_INFO': r'/pathinfo',
}) })
self.assertEqual(r.url, b'http://testserver/path1/path2') with self.assertRaisesRegexp(error.ProgrammingError,
self.assertEqual(r.baseurl, b'http://testserver') b'reponame prefix of PATH_INFO'):
self.assertEqual(r.advertisedurl, b'http://testserver/path1/path2') parse(DEFAULT_ENV, reponame=b'repo', extra={
self.assertEqual(r.advertisedbaseurl, b'http://testserver') r'PATH_INFO': r'/repoextra/path',
self.assertEqual(r.apppath, b'/repo') })
self.assertEqual(r.dispatchparts, [b'path1', b'path2'])
self.assertEqual(r.dispatchpath, b'path1/path2')
self.assertTrue(r.havepathinfo)
self.assertEqual(r.reponame, b'repo')
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, reponame=b'repo', extra={
r'REPO_NAME': r'repo',
r'PATH_INFO': r'/repo/path1/path2', r'PATH_INFO': r'/repo/path1/path2',
}) })
self.assertEqual(r.url, b'http://testserver/repo/path1/path2') self.assertEqual(r.url, b'http://testserver/repo/path1/path2')
self.assertEqual(r.baseurl, b'http://testserver') self.assertEqual(r.baseurl, b'http://testserver')
self.assertEqual(r.advertisedurl, b'http://testserver/repo/path1/path2') self.assertEqual(r.advertisedurl, b'http://testserver/repo/path1/path2')
self.assertEqual(r.advertisedbaseurl, b'http://testserver') self.assertEqual(r.advertisedbaseurl, b'http://testserver')
self.assertEqual(r.apppath, b'/repo') self.assertEqual(r.apppath, b'/repo')
self.assertEqual(r.dispatchparts, [b'path1', b'path2']) self.assertEqual(r.dispatchparts, [b'path1', b'path2'])
self.assertEqual(r.dispatchpath, b'path1/path2') self.assertEqual(r.dispatchpath, b'path1/path2')
self.assertTrue(r.havepathinfo) self.assertTrue(r.havepathinfo)
self.assertEqual(r.reponame, b'repo') self.assertEqual(r.reponame, b'repo')
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, reponame=b'prefix/repo', extra={
r'REPO_NAME': r'prefix/repo',
r'PATH_INFO': r'/prefix/repo/path1/path2', r'PATH_INFO': r'/prefix/repo/path1/path2',
}) })
self.assertEqual(r.url, b'http://testserver/prefix/repo/path1/path2') self.assertEqual(r.url, b'http://testserver/prefix/repo/path1/path2')
self.assertEqual(r.baseurl, b'http://testserver') self.assertEqual(r.baseurl, b'http://testserver')
self.assertEqual(r.advertisedurl, self.assertEqual(r.advertisedurl,
b'http://testserver/prefix/repo/path1/path2') b'http://testserver/prefix/repo/path1/path2')
self.assertEqual(r.advertisedbaseurl, b'http://testserver') self.assertEqual(r.advertisedbaseurl, b'http://testserver')