-
Notifications
You must be signed in to change notification settings - Fork 5
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
Ensure special characters in manifest filepaths are urlencoded #35
Conversation
@pwinckles not sure if you had a specific test that you had been running that you'd like to retry with this PR. I added some tests but always appreciate more eyes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have any specific tests. I was just eyeballing the libraries to see what they were doing.
Your changes seem good to me.
src/BagUtils.php
Outdated
*/ | ||
public static function checkUnencodedFilepath(string $filepath): bool | ||
{ | ||
return (strpos($filepath, "\n") > -1 || strpos($filepath, "\r") > -1 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me what fgets
considers a newline, but if you're reading the file line by line as defined by the spec where CR, LF, or CRLF are line endings, then it shouldn't be possible for the filepath to contain a CR or LF at this point.
The encoding check is technically correct. However, it does make it so the code would reject bags created by libraries that haven't be updated to encode paths correctly. I don't know if you're interested in accepting those bags or not though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I struggled with this, mostly because I don't use PHP on Windows and so I am biased towards LF as newline characters. Looking at it now, I'm not handling CR line endings properly. I'll make a new ticket for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolves #33
This change: