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
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: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 6949

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__)