Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ngclient: avoid lstrip(os.sep) on target paths #1506

Closed
sechkova opened this issue Jul 23, 2021 · 4 comments
Closed

ngclient: avoid lstrip(os.sep) on target paths #1506

sechkova opened this issue Jul 23, 2021 · 4 comments
Assignees
Labels
backlog Issues to address with priority for current development goals ngclient

Comments

@sechkova
Copy link
Contributor

Description of issue or feature request:

During matching of target file path and pathpattern in ngclient/updater.py we normalize the paths by removing the leading os separator.

 # Make sure to strip any leading
 # path separators so that a match is made.
 # Example: "foo.tgz" should match with "/*.tgz".
 if fnmatch.fnmatch(
     target_filepath.lstrip(os.sep), pathpattern.lstrip(os.sep)
 ):

Current behavior:
Normalize file path and pathpattern in case they start with os.sep

Expected behavior:
Define a valid target file path and pathpattern strings and reject everything else.
Don't support corner cases.

@jku
Copy link
Member

jku commented Aug 9, 2021

some additional thoughts here, first for pathpattern:

  • pathpattern comes from the server: the spec "recommends" that '/' is used as the separator. I believe it is impossible for a client implementation to support arbitrary separators -- every implementation needs to choose one and stick with it. I also believe it makes no sense for even the reference implementation to even optionally use/accept anything else than '/'. We should just document that we are stricter than the spec here
  • spec "recommends" that pathpattern not start with a separator: I guess we can leave the pathpattern stripping in there, but for '/' specifically, not for os.sep -- that part is clearly a bug, the client os separator has nothing to do with pathpattern separator which is decided by the server

For target_filepath:

  • this comes from API user so using os.sep may make sense in some client API -- however I think there is no reason to accept file paths starting with a separator? It's just complexity for no benefit, I would just remove this lstrip().

@joshuagl
Copy link
Member

RFC2119 states:

SHOULD This word, or the adjective "RECOMMENDED", mean that there
may exist valid reasons in particular circumstances to ignore a
particular item, but the full implications must be understood and
carefully weighed before choosing a different course.

IMNSHO the reference implementation does not have a valid reason to ignore any RECOMMENDED/SHOULD entry in the specification and doing so does not make us stricter than the spec.

@jku jku added the backlog Issues to address with priority for current development goals label Aug 18, 2021
@MVrachev
Copy link
Collaborator

I am going to address this issue in my pr #1512.

@MVrachev MVrachev self-assigned this Aug 18, 2021
@MVrachev
Copy link
Collaborator

Fixed by #1512 with commit 34e7546

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals ngclient
Projects
None yet
Development

No branches or pull requests

4 participants