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

opamfile: parse error on escapable paths #5562

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented May 26, 2023

@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label May 26, 2023
@rjbou rjbou force-pushed the error-on-escapable-paths branch from 3045578 to ef84cd2 Compare May 30, 2023 08:29
Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A parse error on files is actually a pretty good idea, and a likely security improvement.

But enforcing the invariant at the OpamFilename level seems pretty strict to me, I am afraid that it could break a few things. Couldn't that affect, for example, commands like opam pin ../foo/foo.opam ? Or is the path always resolved and made absolute soon enough ?

@hannesm
Copy link
Member

hannesm commented Nov 7, 2023

I looked at the last commit, and am convinced that it avoids .. being in a path. From a security point of view, I do appreciate this PR. But of course @AltGr question is valid - maybe the result is too strict?

@rjbou rjbou added this to the 2.3 milestone Jun 27, 2024
@rjbou rjbou marked this pull request as draft July 10, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: QUEUED Pending pull request, waiting for other work to be merged or closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants