Enforce that version is a string to prevent exceptions in the case of an all-integer version ID#119
Enforce that version is a string to prevent exceptions in the case of an all-integer version ID#119asaba96 wants to merge 4 commits intovcstools:masterfrom
Conversation
|
lgtm |
test/local/test_config_yaml.py
Outdated
| wrap = get_path_spec_from_yaml(struct) | ||
| self.assertEqual(scmtype, wrap.get_scmtype()) | ||
| self.assertEqual(scmtype, wrap.get_legacy_type()) | ||
| self.assertTrue(isinstance(wrap.get_version(), str)) |
There was a problem hiding this comment.
You might consider using assertIsInstance here.
src/wstool/config_yaml.py
Outdated
| if value is not None: | ||
| version = str(value) | ||
| else: | ||
| version = value |
There was a problem hiding this comment.
is the else branch needed: Version is already None?
| elif key == "version": | ||
| version = value | ||
| if value is not None: | ||
| version = str(value) |
There was a problem hiding this comment.
Additionally, you could add str(arg).strip in the place the exception happened in vcstools, which should not be vulnerable to this. Possibly check other places we use strip without being careful about the type.
There was a problem hiding this comment.
So I addressed the issue here because it seemed to be caused due to the underlying PyYaml versions having different behavior in the way it returned a specific yawl value. So I could fix all exceptions related to strip or I could enforce that version is a string, which already something something above the file parsing layer already expected, so I assume there are other parts that depend on the same expectation of type.
There was a problem hiding this comment.
Sure, here is a good place for a fix. vcstools is a library in it's own right (potentially to be used by other projects), so if you have time, you could also make it less vulnerable, but it's a different story from this PR.
There was a problem hiding this comment.
Cool, I will take a look then to see what I can do with vcstools when I get a chance.
|
Is there anything else I should address for this particular PR? |
|
Closing since the repository is about to be archived: see #154. |
5 participants
If, in your workspace, you specify a specific version and the version id happens to contain all numbers,
yaml.load(stream)will return the value as an integer in the resulting dictionary (see here).This is an issue when you are using the
wstool infocommand, as itsInfoRetrieverclass attempts to build a path spec. However, theget_versioned_path_spec(here) callsrevision = self._get_vcsc().get_version(self.version)on line 416, which is in thevcstoolspackage. This function (here) uses the vcstools utilitysanitizeon line 415, which is expecting the input to be a string (see line 198 here).However, since the PyYaml (version 3.12 on my system, Ubuntu-Gnome) returns the version as an integer, this results in an exception. The stack trace is as follows:
My fix enforces that the version is returned as the expected type, a string.