manifest: relax include name rules for user-specified path
Allow the user to specify relative or absolute or any other funky
path that they want when using `repo init` or `repo sync`. Our
goal is to restrict the paths in the remote manifest git repo we
cloned from the network, not protect the user from themselves.
Bug: https://crbug.com/gerrit/14156
Change-Id: I1ccfb2a6bd1dce2bd765e261bef0bbf0f8a9beb6
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/298823
Reviewed-by: Jonathan Nieder <jrn@google.com>
Tested-by: Mike Frysinger <vapier@google.com>
diff --git a/error.py b/error.py
index 2fb6aa0..25ff80d 100644
--- a/error.py
+++ b/error.py
@@ -22,12 +22,12 @@
"""
-class ManifestInvalidRevisionError(Exception):
+class ManifestInvalidRevisionError(ManifestParseError):
"""The revision value in a project is incorrect.
"""
-class ManifestInvalidPathError(Exception):
+class ManifestInvalidPathError(ManifestParseError):
"""A path used in <copyfile> or <linkfile> is incorrect.
"""
diff --git a/manifest_xml.py b/manifest_xml.py
index cd5954d..e96e062 100644
--- a/manifest_xml.py
+++ b/manifest_xml.py
@@ -624,16 +624,22 @@
b = b[len(R_HEADS):]
self.branch = b
+ # The manifestFile was specified by the user which is why we allow include
+ # paths to point anywhere.
nodes = []
- nodes.append(self._ParseManifestXml(self.manifestFile,
- self.manifestProject.worktree))
+ nodes.append(self._ParseManifestXml(
+ self.manifestFile, self.manifestProject.worktree,
+ restrict_includes=False))
if self._load_local_manifests and self.local_manifests:
try:
for local_file in sorted(platform_utils.listdir(self.local_manifests)):
if local_file.endswith('.xml'):
local = os.path.join(self.local_manifests, local_file)
- nodes.append(self._ParseManifestXml(local, self.repodir))
+ # Since local manifests are entirely managed by the user, allow
+ # them to point anywhere the user wants.
+ nodes.append(self._ParseManifestXml(
+ local, self.repodir, restrict_includes=False))
except OSError:
pass
@@ -651,7 +657,19 @@
self._loaded = True
- def _ParseManifestXml(self, path, include_root, parent_groups=''):
+ def _ParseManifestXml(self, path, include_root, parent_groups='',
+ restrict_includes=True):
+ """Parse a manifest XML and return the computed nodes.
+
+ Args:
+ path: The XML file to read & parse.
+ include_root: The path to interpret include "name"s relative to.
+ parent_groups: The groups to apply to this projects.
+ restrict_includes: Whether to constrain the "name" attribute of includes.
+
+ Returns:
+ List of XML nodes.
+ """
try:
root = xml.dom.minidom.parse(path)
except (OSError, xml.parsers.expat.ExpatError) as e:
@@ -670,10 +688,11 @@
for node in manifest.childNodes:
if node.nodeName == 'include':
name = self._reqatt(node, 'name')
- msg = self._CheckLocalPath(name)
- if msg:
- raise ManifestInvalidPathError(
- '<include> invalid "name": %s: %s' % (name, msg))
+ if restrict_includes:
+ msg = self._CheckLocalPath(name)
+ if msg:
+ raise ManifestInvalidPathError(
+ '<include> invalid "name": %s: %s' % (name, msg))
include_groups = ''
if parent_groups:
include_groups = parent_groups
@@ -681,13 +700,13 @@
include_groups = node.getAttribute('groups') + ',' + include_groups
fp = os.path.join(include_root, name)
if not os.path.isfile(fp):
- raise ManifestParseError("include %s doesn't exist or isn't a file"
- % (name,))
+ raise ManifestParseError("include [%s/]%s doesn't exist or isn't a file"
+ % (include_root, name))
try:
nodes.extend(self._ParseManifestXml(fp, include_root, include_groups))
# should isolate this to the exact exception, but that's
# tricky. actual parsing implementation may vary.
- except (KeyboardInterrupt, RuntimeError, SystemExit):
+ except (KeyboardInterrupt, RuntimeError, SystemExit, ManifestParseError):
raise
except Exception as e:
raise ManifestParseError(
diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py
index f69e9cf..6977b41 100644
--- a/tests/test_manifest_xml.py
+++ b/tests/test_manifest_xml.py
@@ -298,8 +298,8 @@
# Check level2 proj group not removed.
self.assertIn('l2g1', proj.groups)
- def test_bad_name_checks(self):
- """Check handling of bad name attribute."""
+ def test_allow_bad_name_from_user(self):
+ """Check handling of bad name attribute from the user's input."""
def parse(name):
manifest = self.getXmlManifest(f"""
<manifest>
@@ -311,6 +311,34 @@
# Force the manifest to be parsed.
manifest.ToXml()
+ # Setup target of the include.
+ target = os.path.join(self.tempdir, 'target.xml')
+ with open(target, 'w') as fp:
+ fp.write('<manifest></manifest>')
+
+ # Include with absolute path.
+ parse(os.path.abspath(target))
+
+ # Include with relative path.
+ parse(os.path.relpath(target, self.manifest_dir))
+
+ def test_bad_name_checks(self):
+ """Check handling of bad name attribute."""
+ def parse(name):
+ # Setup target of the include.
+ with open(os.path.join(self.manifest_dir, 'target.xml'), 'w') as fp:
+ fp.write(f'<manifest><include name="{name}"/></manifest>')
+
+ manifest = self.getXmlManifest("""
+<manifest>
+ <remote name="default-remote" fetch="http://localhost" />
+ <default remote="default-remote" revision="refs/heads/main" />
+ <include name="target.xml" />
+</manifest>
+""")
+ # Force the manifest to be parsed.
+ manifest.ToXml()
+
# Handle empty name explicitly because a different codepath rejects it.
with self.assertRaises(error.ManifestParseError):
parse('')
