-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix external library path validation #8319 #8366
Merged
jrasm91
merged 8 commits into
immich-app:main
from
pablodre:fix/#8319-external-library-path-validation
Mar 31, 2024
Merged
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
6f92c95
Fix isImmichPath
pablodre 2121234
prettier write
pablodre 162eda2
Fis isImmichPath code comment
pablodre cdff02a
Refactor isImmichPath function based on team suggestions
pablodre 19d1af9
Test isImmichPath
pablodre ac1b645
fix: clean comments
pablodre 014ef1d
Refactor isImmichPath test based on team suggestions
pablodre 0d550c9
Clean code with lintern suggestions
pablodre File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 am very certain
resolve()
will always return a path without a trailing slashThere 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'm also not sure if it's necessary to add a slash to that path anyways?
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.
At least the official docs https://nodejs.org/api/path.html#pathresolvepaths seem to imply this. Or are you aware of a scenario where that isn't the case?
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 think
/
is the exception where we wouldn't want to add a forward slash.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.
If we don't we run into the issue we're currently experiencing;
/foo-bar
starts with/foo
, even though both are different folders and/foo
is the media locationThere 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.
But we need to resolve the media location as that can also be a relative path, right? @bo0tzz
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.
Making sure both paths are in the same format before you compare them is a smart move.
What is the final decision?
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.
Probably yes. I think the best way forward would be to create some unit tests for this that spell out all the different paths.
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.
After all this discussion I think what you currently have is the best (as in most resilient). However, I would like to hear the others chime in as well :)
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.
this is a long discussion about how it might or might not work, but at the end of the day we should definitely just add a few unit tests for the situations we're worried about and make sure it is handling them correctly. The easiest one would just be
/photos_old
and/photos