This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4522

narrowspec: limit patterns to path: and rootfilesin: (BC)
ClosedPublic

Authored by indygreg on Sep 11 2018, 2:52 PM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG0d572769046a: narrowspec: limit patterns to path: and rootfilesin: (BC)
Summary

Some matcher patterns are computationally expensive and may even
have security issues (e.g. evaluating some file sets). For these
reasons, we want to limit the types of matcher patterns that can
be used in narrow specs and by command line arguments used for
defining narrow specs.

This commit teaches `narrowspec.parsepatterns()` to validate the
pattern types against "safe" patterns.

Surprisingly, no existing tests broke. So tests for the feature
have been added.

We also added a function to validate a patterns data structure.
This will be used in future commits.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Sep 11 2018, 2:52 PM
indygreg added a subscriber: martinvonz.Sep 11 2018, 5:56 PM

Looking through this code a bit more, it appears that there is a not-in-core "remote expansion" wire protocol command that recognizes the include: prefix and asks the server to expand it. This series will stop that from working.

I didn't notice this at first because we have no tests for it.

I think Google's server relies on this feature.

@martinvonz how do you want to proceed? Should we nuke the include: feature in core? Or do you want me to refactor this series to support the include: on user-provided commands?

martinvonz added a comment.Sep 11 2018, 6:00 PM
In D4522#69130, @indygreg wrote:

Looking through this code a bit more, it appears that there is a not-in-core "remote expansion" wire protocol command that recognizes the include: prefix and asks the server to expand it. This series will stop that from working.
I didn't notice this at first because we have no tests for it.
I think Google's server relies on this feature.
@martinvonz how do you want to proceed? Should we nuke the include: feature in core? Or do you want me to refactor this series to support the include: on user-provided commands?

Heh, we were talking about that internally and were surprised that this patch didn't mention that and that no tests failed. We realized a month or so ago that while the server has support for it, there was some renaming of a capability that happened when narrow was moved into the hg repo that meant that no one could possibly have used the feature since then. Kyle and I are okay with forbidding include:. We can also add it back, or override it internally, if we decide that we want to let users use that feature again. Thanks for asking :)

martinvonz added inline comments.Sep 11 2018, 6:04 PM
mercurial/narrowspec.py
114–130

Obsolete copy of what you inlined into parsepatterns?

martinvonz requested changes to this revision.Sep 11 2018, 6:10 PM
martinvonz added inline comments.
mercurial/narrowspec.py
114–130

Oh, this gets used in the next patch. Was parsepatterns() supposed to call this function? They seem very similar.

This revision now requires changes to proceed.Sep 11 2018, 6:10 PM
indygreg added inline comments.Sep 11 2018, 6:11 PM
mercurial/narrowspec.py
114–130

It is a copy, but not obsolete.

parsepatterns() normalizes patterns supplied by the user or an untrusted source. validatepatterns is used on "internal" data structures to ensure things are well-formed.

They do similar things but are different. I could extract the common bit to a separate, two-line function and/or update the docstring to reflect the differences. Do you have any preferences?

martinvonz added inline comments.Sep 11 2018, 6:13 PM
mercurial/narrowspec.py
114–130

Can't parsepatterns() just call validatepatterns() just before it returns? If not, at least move this function to a later patch where it's used.

indygreg updated this revision to Diff 10896.Sep 11 2018, 6:25 PM
martinvonz accepted this revision.Sep 11 2018, 6:35 PM
This revision is now accepted and ready to land.Sep 11 2018, 6:35 PM
Closed by commit rHG0d572769046a: narrowspec: limit patterns to path: and rootfilesin: (BC) (authored by indygreg). · Explain WhySep 11 2018, 6:46 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 10896

mercurial/narrowspec.py

match as matchmod, match as matchmod,
repository, repository,
sparse, sparse,
util, util,
) )
FILENAME = 'narrowspec' FILENAME = 'narrowspec'
# Pattern prefixes that are allowed in narrow patterns. This list MUST
# only contain patterns that are fast and safe to evaluate. Keep in mind
# that patterns are supplied by clients and executed on remote servers
# as part of wire protocol commands.
VALID_PREFIXES = (
b'path:',
b'rootfilesin:',
)
def parseserverpatterns(text): def parseserverpatterns(text):
"""Parses the narrowspec format that's returned by the server.""" """Parses the narrowspec format that's returned by the server."""
includepats = set() includepats = set()
excludepats = set() excludepats = set()
# We get one entry per line, in the format "<key> <value>". # We get one entry per line, in the format "<key> <value>".
# It's OK for value to contain other spaces. # It's OK for value to contain other spaces.
for kp in (l.split(' ', 1) for l in text.splitlines()): for kp in (l.split(' ', 1) for l in text.splitlines()):
"""Returns the normalized version of a text-format pattern. """Returns the normalized version of a text-format pattern.
If the pattern has no kind, the default will be added. If the pattern has no kind, the default will be added.
""" """
kind, pat = matchmod._patsplit(pattern, defaultkind) kind, pat = matchmod._patsplit(pattern, defaultkind)
return '%s:%s' % normalizesplitpattern(kind, pat) return '%s:%s' % normalizesplitpattern(kind, pat)
def parsepatterns(pats): def parsepatterns(pats):
"""Parses a list of patterns into a typed pattern set.""" """Parses an iterable of patterns into a typed pattern set.
return set(normalizepattern(p) for p in pats)
Patterns are assumed to be ``path:`` if no prefix is present.
For safety and performance reasons, only some prefixes are allowed.
See ``validatepatterns()``.
This function should be used on patterns that come from the user to
normalize and validate them to the internal data structure used for
representing patterns.
"""
res = {normalizepattern(orig) for orig in pats}
validatepatterns(res)
return res
def validatepatterns(pats):
"""Validate that patterns are in the expected data structure and format.
And that is a set of normalized patterns beginning with ``path:`` or
``rootfilesin:``.
This function should be used to validate internal data structures
and patterns that are loaded from sources that use the internal,
prefixed pattern representation (but can't necessarily be fully trusted).
"""
if not isinstance(pats, set):
raise error.ProgrammingError('narrow patterns should be a set; '
'got %r' % pats)
for pat in pats:
if not pat.startswith(VALID_PREFIXES):
# Use a Mercurial exception because this can happen due to user
# bugs (e.g. manually updating spec file).
raise error.Abort(_('invalid prefix on narrow pattern: %s') % pat,
hint=_('narrow patterns must begin with one of '
'the following: %s') %
', '.join(VALID_PREFIXES))
martinvonzUnsubmitted
Not Done

Obsolete copy of what you inlined into parsepatterns?

martinvonz: Obsolete copy of what you inlined into parsepatterns?
martinvonzUnsubmitted
Not Done

Oh, this gets used in the next patch. Was parsepatterns() supposed to call this function? They seem very similar.

martinvonz: Oh, this gets used in the next patch. Was `parsepatterns()` supposed to call this function?
indygregAuthorUnsubmitted
Not Done

It is a copy, but not obsolete.

parsepatterns() normalizes patterns supplied by the user or an untrusted source. validatepatterns is used on "internal" data structures to ensure things are well-formed.

They do similar things but are different. I could extract the common bit to a separate, two-line function and/or update the docstring to reflect the differences. Do you have any preferences?

indygreg: It is a copy, but not obsolete. `parsepatterns()` normalizes patterns supplied by the user or…
martinvonzUnsubmitted
Not Done

Can't parsepatterns() just call validatepatterns() just before it returns? If not, at least move this function to a later patch where it's used.

martinvonz: Can't parsepatterns() just call validatepatterns() just before it returns? If not, at least…
def format(includes, excludes): def format(includes, excludes):
output = '[include]\n' output = '[include]\n'
for i in sorted(includes - excludes): for i in sorted(includes - excludes):
output += i + '\n' output += i + '\n'
output += '[exclude]\n' output += '[exclude]\n'
for e in sorted(excludes): for e in sorted(excludes):
output += e + '\n' output += e + '\n'
return output return output

tests/test-narrow-clone.t

$ cd dir/src $ cd dir/src
$ for x in `$TESTDIR/seq.py 20`; do echo $x > "f$x"; hg add "f$x"; hg commit -m "Commit src $x"; done $ for x in `$TESTDIR/seq.py 20`; do echo $x > "f$x"; hg add "f$x"; hg commit -m "Commit src $x"; done
$ cd .. $ cd ..
$ mkdir tests $ mkdir tests
$ cd tests $ cd tests
$ for x in `$TESTDIR/seq.py 20`; do echo $x > "t$x"; hg add "t$x"; hg commit -m "Commit test $x"; done $ for x in `$TESTDIR/seq.py 20`; do echo $x > "t$x"; hg add "t$x"; hg commit -m "Commit test $x"; done
$ cd ../../.. $ cd ../../..
Only path: and rootfilesin: pattern prefixes are allowed
$ hg clone --narrow ssh://user@dummy/master badnarrow --noupdate --include 'glob:**'
requesting all changes
abort: invalid prefix on narrow pattern: glob:**
(narrow patterns must begin with one of the following: path:, rootfilesin:)
[255]
$ hg clone --narrow ssh://user@dummy/master badnarrow --noupdate --exclude 'set:ignored'
requesting all changes
abort: invalid prefix on narrow pattern: set:ignored
(narrow patterns must begin with one of the following: path:, rootfilesin:)
[255]
narrow clone a file, f10 narrow clone a file, f10
$ hg clone --narrow ssh://user@dummy/master narrow --noupdate --include "dir/src/f10" $ hg clone --narrow ssh://user@dummy/master narrow --noupdate --include "dir/src/f10"
requesting all changes requesting all changes
adding changesets adding changesets
adding manifests adding manifests
adding file changes adding file changes
added 3 changesets with 1 changes to 1 files added 3 changesets with 1 changes to 1 files
new changesets f93383bb3e99:26ce255d5b5d new changesets f93383bb3e99:26ce255d5b5d
updating to branch default updating to branch default
20 files updated, 0 files merged, 0 files removed, 0 files unresolved 20 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ cd specfile $ cd specfile
$ hg tracked $ hg tracked
I path:dir/tests I path:dir/tests
I path:file:dir/src/f12 I path:file:dir/src/f12
$ cd .. $ cd ..
Narrow spec with invalid patterns is rejected
$ cat > narrowspecs <<EOF
> [include]
> glob:**
> EOF
$ hg clone ssh://user@dummy/master badspecfile --narrowspec narrowspecs
reading narrowspec from '$TESTTMP/narrowspecs'
requesting all changes
abort: invalid prefix on narrow pattern: glob:**
(narrow patterns must begin with one of the following: path:, rootfilesin:)
[255]

tests/test-narrow-patterns.t

o 3 33227af02764... dir2/bar o 3 33227af02764... dir2/bar
| |
o 2 594bc4b13d4a dir1/bar o 2 594bc4b13d4a dir1/bar
| |
o 1 47f480a08324 dir1/foo o 1 47f480a08324 dir1/foo
| |
o 0 2a4f0c3b67da... root o 0 2a4f0c3b67da... root
Illegal patterns are rejected
$ hg tracked --addinclude glob:**
abort: invalid prefix on narrow pattern: glob:**
(narrow patterns must begin with one of the following: path:, rootfilesin:)
[255]
$ hg tracked --addexclude set:ignored
abort: invalid prefix on narrow pattern: set:ignored
(narrow patterns must begin with one of the following: path:, rootfilesin:)
[255]