ngclient: Don't use target path as local path#1592
ngclient: Don't use target path as local path#1592jku merged 1 commit intotheupdateframework:developfrom
Conversation
Doing so is not always safe and has various other issues (like target paths "a/../b" and "b" ending up as the same local path). Instead URL-encode the target path to make it a plain filename. This removes any opportunity for path trickery and removes the need to create the required sub directories (which we were not doing currently, leading to failed downloads). URL-encoding encodes much more than we really need but doing so should not hurt: the important thing is that it encodes all path separators. Return the actual filepath as return value. I would like to modify the arguments so caller could decide the filename if they want to. But I won't do it now because updated_targets() (the caching mechanism) relies on filenames being chosen by TUF. The plan is to make it possible for caller to choose the filename though. This is clearly a "filesystem API break" for anyone depending on the actual target file names, and does not make sense if we do not plan to go forward with other updated_targets()/download_target() changes listed in theupdateframework#1580. This is part of bigger plan in theupdateframework#1580 Fixes theupdateframework#1571 Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
Pull Request Test Coverage Report for Build 1277507758
💛 - Coveralls |
MVrachev
left a comment
MVrachev
left a comment
There was a problem hiding this comment.
Looks good to me!
I tried to think if there are problem corner cases, but everything seems fine.
yes and I should have pointed that out in the PR message, sorry about that... we can either merge this with the pinky promise that the real tests with "directories" are going to be added, or alternatively we combine this PR and 1756 and merge the result to develop. @MVrachev 1756 is yours, any opinions? I'd maybe lean towards merging this as is, and then you rebasing your work -- is that ok? |
I don't have any strong opinions, but if you prefer merging this pr as it is I am okay. |
|
ok, let's try this: if it doesn't end up working we can always admit defeat and bring back the old API -- we have not promised ngclient is stable yet |
4 participants
Doing so is not always safe and has various other issues
(like target paths "a/../b" and "b" ending up as the same
local path).
Instead URL-encode the target path to make it a plain filename. This
removes any opportunity for path trickery and removes the need to create
the required sub directories (which we were not doing currently, leading
to failed downloads). URL-encoding encodes much more than we really need
but doing so should not hurt: the important thing is that it encodes
all path separators.
Return the actual filepath as return value. I would like to modify the
arguments so caller could decide the filename if they want to. But I
won't do it now because updated_targets() (the caching mechanism)
relies on filenames being chosen by TUF. The plan is to make it
possible for caller to choose the filename though.
This is clearly a "filesystem API break" for anyone depending on the
actual target file names, and does not make sense if we do not plan to
go forward with other updated_targets()/download_target() changes
listed in #1580.
This is part of bigger plan in #1580
Fixes #1571
Signed-off-by: Jussi Kukkonen jkukkonen@vmware.com