Repository navigation
Add stricter TARGETPATH and PATHPATTERN checks #1018
Description
Activity
- added this to the milestone
on Jul 7, 2020 - removed this from the milestone
on Sep 8, 2020 I wonder if there's anything more to do here?
I think the big question is how much input validation should be performed in the Metadata API, which still remains unanswered (#1130)
The checks requested in this ticket are currently not performed in the relevant interfaces:
TARGETPATH not checked at all
python-tuf/tuf/api/metadata.py
Line 1503 in 8ac7167
PATHPATTERN only checked to be strings
python-tuf/tuf/api/metadata.py
Lines 1278 to 1279 in 8ac7167
I lean towards implementing the requested checks, also to be consistent with places where we do check more strictly eg.
python-tuf/tuf/api/metadata.py
Lines 1498 to 1499 in 8ac7167
cc @MVrachev (head of validation)
We have implemented validation for quite many attributes of the Metadata classes (see #1140 (comment)).
For some of them, we have decided there is no sense in checking their type or content (as forRoot.keysfor example) for others we have added as forpaths.
I am open to rediscussing why some of the attributes are missing validation.
We have generally decided to not apply the same approach for all attributes as the different attributes are used differently.In this particular case:
TARGETPATH not checked at all
Together with @jku we had discussions about how we should handle
TARGETPATHandMETAPATHand we have come up with: #1668 (comment)PATHPATTERN only checked to be strings
Again with @jku we had discussions for
PATHPATTERNas well here: #1461 (comment)@lukpueh what do you think about those arguments?
It's important to consider the different opinions as it's possible we have made a mistake.Reacted by Lukas PühringerThanks for sharing those resources, @MVrachev. Do I understand correctly that they argue that we don't need the check because the client will fail gracefully for any string even if it does not have the recommended format? That would be a fair argument. Although it could also be used against the
_validate_length-check.I suppose for TARGETPATH my logic was this:
- handling things like "/ cannot be a starting character" or "\ cannot be a directory separator" are workarounds
- the issue they worked around is that TARGETPATH was being used as a local file path. This is impossible to implement comprehensively (filesystems have different requirements and TARGETPATH must still be a URL fragment) and very hard to implement safely -- leading to workarounds
- the proper solution is to not use TARGETPATH as a local file path, and treat it as just an identifier that is a URL path fragment (a specific client could define rules that make it possible to use TARGETPATH as a filepath (like define only [a-z] as valid chars) but a library probably should not)
PATHPATTERN just follows from there.
Reacted by Lukas Pühringer and Martin VrachevThanks for the follow-up, @jku! I added a note about this workaround to the repository tool feature request ticket for a convenience function that builds TARGETPATHs from local files.
I agree that the Metadata API should not provide that feature (workaround) and as a consequence does not have to vet these parameters as strictly. Closing here..
Reacted by Jussi Kukkonen
Description of issue or feature request:
#1008 removes ambiguity in interfaces that take TARGETPATH and/or PATHPATTERN arguments, by only accepting paths and path patterns that use "/" as directory separator and don't start with "/".
To make the interface even stronger, we should only accept paths and path patterns that completely adhere to the recommended format.
Note that, although the specification only recommends a format, we agreed that an implementation can be more assertive than the spec (see spec#63 and spec#67).
Current behavior:
Expected behavior:
Functions that take TARGETPATH arguments only accept path-relative-scheme-less-URL strings.
Functions that take PATHPATTERN arguments only accept path-relative-scheme-less-URL strings that may include shell-style glob characters. I suggest to align this with whatever the client updater supports.