Fix external library path validation #8319 #8366
Conversation
|
I think the right change here would be to still use |
|
@danieldietzler @mertalev Thank you very much, both of you are right and I was wrong, fixed. |
server/src/cores/storage.core.ts
Outdated
| @@ -115,7 +115,11 @@ export class StorageCore { | |||
| } | |||
|
|
|||
| static isImmichPath(path: string) { | |||
| return resolve(path) === resolve(APP_MEDIA_LOCATION); | |||
| const normalizedPath = resolve(path).endsWith('/') ? resolve(path) : `${resolve(path)}/`; | |||
There was a problem hiding this comment.
I am very certain resolve() will always return a path without a trailing slash
There was a problem hiding this comment.
I'm also not sure if it's necessary to add a slash to that path anyways?
There was a problem hiding this comment.
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.
I think / is the exception where we wouldn't want to add a forward slash.
There was a problem hiding this comment.
I'm also not sure if it's necessary to add a slash to that path anyways?
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 location
There was a problem hiding this comment.
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.
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.
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.
Making sure both paths are in the same format before you compare them is a smart move. What is the final decision?
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.
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
|
I think this approach is good. Could you move the resolved paths to separate variables before doing the string concatenation? It would be cleaner. It would also be more expressive with |
|
Can we please add some tests for this method? |
|
Awesome! Thanks a lot for the fast fixing |
Hi its my first PR here, i chose a easy one
This is the fix for this the issue #8319