This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2820

hgweb: change how dispatch path is reported
ClosedPublic

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

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGd0b0fedbfb53: hgweb: change how dispatch path is reported
Summary

When I implemented the new request object, I carried forward some
ugly hacks until I could figure out what was happening. One of those
was the handling of PATH_INFO to determine how to route hgweb
requests.

Essentially, if we have PATH_INFO data, we route according to
that. But if we don't, we route by the query string. I question
if we still need to support query string routing. But that's for
another day, I suppose.

In this commit, we clean up the ugly "havepathinfo" hack and
replace it with a "dispatchpath" attribute that can hold None or
empty string to differentiate between the presence of PATH_INFO.
This is still a bit hacky. But at least the request parsing
and routing code is explicit about the meaning now.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Mar 12 2018, 5:16 PM
durin42 accepted this revision.Mar 12 2018, 5:28 PM
This revision is now accepted and ready to land.Mar 12 2018, 5:28 PM
Closed by commit rHGd0b0fedbfb53: hgweb: change how dispatch path is reported (authored by indygreg). · Explain WhyMar 12 2018, 5:38 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 6886

mercurial/hgweb/hgweb_mod.py

# replace it. # replace it.
res.headers['Content-Security-Policy'] = rctx.csp res.headers['Content-Security-Policy'] = rctx.csp
handled = wireprotoserver.handlewsgirequest( handled = wireprotoserver.handlewsgirequest(
rctx, req, res, self.check_perm) rctx, req, res, self.check_perm)
if handled: if handled:
return res.sendresponse() return res.sendresponse()
if req.havepathinfo: # Old implementations of hgweb supported dispatching the request via
# the initial query string parameter instead of using PATH_INFO.
# If PATH_INFO is present (signaled by ``req.dispatchpath`` having
# a value), we use it. Otherwise fall back to the query string.
if req.dispatchpath is not None:
query = req.dispatchpath query = req.dispatchpath
else: else:
query = req.querystring.partition('&')[0].partition(';')[0] query = req.querystring.partition('&')[0].partition(';')[0]
# translate user-visible url structure to internal structure # translate user-visible url structure to internal structure
args = query.split('/', 2) args = query.split('/', 2)
if 'cmd' not in req.qsparams and args and args[0]: if 'cmd' not in req.qsparams and args and args[0]:

mercurial/hgweb/request.py

# Value of REMOTE_USER, if set, or None. # Value of REMOTE_USER, if set, or None.
remoteuser = attr.ib() remoteuser = attr.ib()
# Value of REMOTE_HOST, if set, or None. # Value of REMOTE_HOST, if set, or None.
remotehost = attr.ib() remotehost = attr.ib()
# WSGI application path. # WSGI application path.
apppath = attr.ib() apppath = attr.ib()
# List of path parts to be used for dispatch. # List of path parts to be used for dispatch.
dispatchparts = attr.ib() dispatchparts = attr.ib()
# URL path component (no query string) used for dispatch. # URL path component (no query string) used for dispatch. Can be
# ``None`` to signal no path component given to the request, an
# empty string to signal a request to the application's root URL,
# or a string not beginning with ``/`` containing the requested
# path under the application.
dispatchpath = attr.ib() dispatchpath = attr.ib()
# Whether there is a path component to this request. This can be true
# when ``dispatchpath`` is empty due to REPO_NAME muckery.
havepathinfo = attr.ib()
# The name of the repository being accessed. # The name of the repository being accessed.
reponame = attr.ib() reponame = attr.ib()
# Raw query string (part after "?" in URL). # Raw query string (part after "?" in URL).
querystring = attr.ib() querystring = attr.ib()
# 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.
if dispatchpath and not dispatchpath.startswith('/'): if dispatchpath and not dispatchpath.startswith('/'):
raise error.ProgrammingError('reponame prefix of PATH_INFO does ' raise error.ProgrammingError('reponame prefix of PATH_INFO does '
'not end at path delimiter: %s (%s)' % 'not end at path delimiter: %s (%s)' %
(env['PATH_INFO'], reponame)) (env['PATH_INFO'], reponame))
apppath = apppath.rstrip('/') + repoprefix apppath = apppath.rstrip('/') + repoprefix
dispatchparts = dispatchpath.strip('/').split('/') dispatchparts = dispatchpath.strip('/').split('/')
elif env.get('PATH_INFO', '').strip('/'): dispatchpath = '/'.join(dispatchparts)
elif 'PATH_INFO' in env:
if env['PATH_INFO'].strip('/'):
dispatchparts = env['PATH_INFO'].strip('/').split('/') dispatchparts = env['PATH_INFO'].strip('/').split('/')
dispatchpath = '/'.join(dispatchparts)
else: else:
dispatchparts = [] dispatchparts = []
dispatchpath = ''
dispatchpath = '/'.join(dispatchparts) else:
dispatchparts = []
dispatchpath = None
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
# a dict to facilitate fast lookup. # a dict to facilitate fast lookup.
qsparams = multidict() qsparams = multidict()
for k, v in util.urlreq.parseqsl(querystring, keep_blank_values=True): for k, v in util.urlreq.parseqsl(querystring, keep_blank_values=True):
qsparams.add(k, v) qsparams.add(k, v)
url=fullurl, baseurl=baseurl, url=fullurl, baseurl=baseurl,
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,
reponame=reponame, 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

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.urlscheme, b'http') self.assertEqual(r.urlscheme, b'http')
self.assertEqual(r.method, b'GET') self.assertEqual(r.method, b'GET')
self.assertIsNone(r.remoteuser) self.assertIsNone(r.remoteuser)
self.assertIsNone(r.remotehost) self.assertIsNone(r.remotehost)
self.assertEqual(r.apppath, b'') self.assertEqual(r.apppath, b'')
self.assertEqual(r.dispatchparts, []) self.assertEqual(r.dispatchparts, [])
self.assertEqual(r.dispatchpath, b'') self.assertIsNone(r.dispatchpath)
self.assertFalse(r.havepathinfo)
self.assertIsNone(r.reponame) self.assertIsNone(r.reponame)
self.assertEqual(r.querystring, b'') self.assertEqual(r.querystring, b'')
self.assertEqual(len(r.qsparams), 0) self.assertEqual(len(r.qsparams), 0)
self.assertEqual(len(r.headers), 0) self.assertEqual(len(r.headers), 0)
def testcustomport(self): def testcustomport(self):
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'SERVER_PORT': r'8000', r'SERVER_PORT': r'8000',
}) })
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.apppath, b'') self.assertEqual(r.apppath, b'')
self.assertEqual(r.dispatchparts, []) self.assertEqual(r.dispatchparts, [])
self.assertEqual(r.dispatchpath, b'') self.assertIsNone(r.dispatchpath)
self.assertFalse(r.havepathinfo)
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'SCRIPT_NAME': r'/script', r'SCRIPT_NAME': r'/script',
}) })
self.assertEqual(r.url, b'http://testserver/script') self.assertEqual(r.url, b'http://testserver/script')
self.assertEqual(r.baseurl, b'http://testserver') self.assertEqual(r.baseurl, b'http://testserver')
self.assertEqual(r.advertisedurl, b'http://testserver/script') self.assertEqual(r.advertisedurl, b'http://testserver/script')
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, []) self.assertEqual(r.dispatchparts, [])
self.assertEqual(r.dispatchpath, b'') self.assertIsNone(r.dispatchpath)
self.assertFalse(r.havepathinfo)
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'SCRIPT_NAME': r'/multiple words', r'SCRIPT_NAME': r'/multiple words',
}) })
self.assertEqual(r.url, b'http://testserver/multiple%20words') self.assertEqual(r.url, b'http://testserver/multiple%20words')
self.assertEqual(r.baseurl, b'http://testserver') self.assertEqual(r.baseurl, b'http://testserver')
self.assertEqual(r.advertisedurl, b'http://testserver/multiple%20words') self.assertEqual(r.advertisedurl, b'http://testserver/multiple%20words')
self.assertEqual(r.advertisedbaseurl, b'http://testserver') self.assertEqual(r.advertisedbaseurl, b'http://testserver')
self.assertEqual(r.apppath, b'/multiple words') self.assertEqual(r.apppath, b'/multiple words')
self.assertEqual(r.dispatchparts, []) self.assertEqual(r.dispatchparts, [])
self.assertEqual(r.dispatchpath, b'') self.assertIsNone(r.dispatchpath)
self.assertFalse(r.havepathinfo)
def testpathinfo(self): def testpathinfo(self):
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'PATH_INFO': r'', r'PATH_INFO': r'',
}) })
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.apppath, b'') self.assertEqual(r.apppath, b'')
self.assertEqual(r.dispatchparts, []) self.assertEqual(r.dispatchparts, [])
self.assertEqual(r.dispatchpath, b'') self.assertEqual(r.dispatchpath, b'')
self.assertTrue(r.havepathinfo)
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'PATH_INFO': r'/pathinfo', r'PATH_INFO': r'/pathinfo',
}) })
self.assertEqual(r.url, b'http://testserver/pathinfo') self.assertEqual(r.url, b'http://testserver/pathinfo')
self.assertEqual(r.baseurl, b'http://testserver') self.assertEqual(r.baseurl, b'http://testserver')
self.assertEqual(r.advertisedurl, b'http://testserver/pathinfo') self.assertEqual(r.advertisedurl, b'http://testserver/pathinfo')
self.assertEqual(r.advertisedbaseurl, b'http://testserver') self.assertEqual(r.advertisedbaseurl, b'http://testserver')
self.assertEqual(r.apppath, b'') self.assertEqual(r.apppath, b'')
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)
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'PATH_INFO': r'/one/two/', r'PATH_INFO': r'/one/two/',
}) })
self.assertEqual(r.url, b'http://testserver/one/two/') self.assertEqual(r.url, b'http://testserver/one/two/')
self.assertEqual(r.baseurl, b'http://testserver') self.assertEqual(r.baseurl, b'http://testserver')
self.assertEqual(r.advertisedurl, b'http://testserver/one/two/') self.assertEqual(r.advertisedurl, b'http://testserver/one/two/')
self.assertEqual(r.advertisedbaseurl, b'http://testserver') self.assertEqual(r.advertisedbaseurl, b'http://testserver')
self.assertEqual(r.apppath, b'') self.assertEqual(r.apppath, b'')
self.assertEqual(r.dispatchparts, [b'one', b'two']) self.assertEqual(r.dispatchparts, [b'one', b'two'])
self.assertEqual(r.dispatchpath, b'one/two') self.assertEqual(r.dispatchpath, b'one/two')
self.assertTrue(r.havepathinfo)
def testscriptandpathinfo(self): def testscriptandpathinfo(self):
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'SCRIPT_NAME': r'/script', r'SCRIPT_NAME': r'/script',
r'PATH_INFO': r'/pathinfo', r'PATH_INFO': r'/pathinfo',
}) })
self.assertEqual(r.url, b'http://testserver/script/pathinfo') self.assertEqual(r.url, b'http://testserver/script/pathinfo')
self.assertEqual(r.baseurl, b'http://testserver') self.assertEqual(r.baseurl, 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)
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'SCRIPT_NAME': r'/script1/script2', r'SCRIPT_NAME': r'/script1/script2',
r'PATH_INFO': r'/path1/path2', r'PATH_INFO': r'/path1/path2',
}) })
self.assertEqual(r.url, self.assertEqual(r.url,
b'http://testserver/script1/script2/path1/path2') b'http://testserver/script1/script2/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/script1/script2/path1/path2') b'http://testserver/script1/script2/path1/path2')
self.assertEqual(r.advertisedbaseurl, b'http://testserver') self.assertEqual(r.advertisedbaseurl, b'http://testserver')
self.assertEqual(r.apppath, b'/script1/script2') self.assertEqual(r.apppath, b'/script1/script2')
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)
r = parse(DEFAULT_ENV, extra={ r = parse(DEFAULT_ENV, extra={
r'HTTP_HOST': r'hostserver', r'HTTP_HOST': r'hostserver',
r'SCRIPT_NAME': r'/script', r'SCRIPT_NAME': r'/script',
r'PATH_INFO': r'/pathinfo', r'PATH_INFO': r'/pathinfo',
}) })
self.assertEqual(r.url, b'http://hostserver/script/pathinfo') self.assertEqual(r.url, b'http://hostserver/script/pathinfo')
self.assertEqual(r.baseurl, b'http://hostserver') self.assertEqual(r.baseurl, b'http://hostserver')
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)
def testreponame(self): def testreponame(self):
"""repository path components get stripped from URL.""" """repository path components get stripped from URL."""
with self.assertRaisesRegexp(error.ProgrammingError, with self.assertRaisesRegexp(error.ProgrammingError,
b'reponame requires PATH_INFO'): b'reponame requires PATH_INFO'):
parse(DEFAULT_ENV, reponame=b'repo') parse(DEFAULT_ENV, reponame=b'repo')
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.assertEqual(r.reponame, b'repo') self.assertEqual(r.reponame, b'repo')
r = parse(DEFAULT_ENV, reponame=b'prefix/repo', extra={ r = parse(DEFAULT_ENV, reponame=b'prefix/repo', extra={
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')
self.assertEqual(r.apppath, b'/prefix/repo') self.assertEqual(r.apppath, b'/prefix/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.assertEqual(r.reponame, b'prefix/repo') self.assertEqual(r.reponame, b'prefix/repo')
if __name__ == '__main__': if __name__ == '__main__':
import silenttestrunner import silenttestrunner
silenttestrunner.main(__name__) silenttestrunner.main(__name__)