This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

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
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.Sep 11 2018, 2:52 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?

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
This revision was automatically updated to reflect the committed changes.