-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Remote ISO: Allow sharing a full folder instead of Recent #18632
Conversation
@@ -49,6 +51,16 @@ static ServerStatus serverStatus; | |||
static std::mutex serverStatusLock; | |||
static int serverFlags; | |||
|
|||
// NOTE: These *only* encode spaces, which is really enough. |
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.
Well, you also want to ecnode at least %. A filename with "%20" in it (such as a downloaded file) will not work.
-[Unknown]
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, true. Just doing what the old Recent code does.
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.
Oh yeah, and I did first use UriDecode, but some logic broke because it encoded slashes. Should maybe fix the root cause of that instead maybe, but could also simply not encode slashes to keep compatibilty with older versions.
case RemoteISOShareType::LOCAL_FOLDER: | ||
{ | ||
std::string decoded = ServerUriDecode(path); | ||
return Path(g_Config.sRemoteISOSharedDir) / decoded; |
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 probably technically a security gap. The Recent path is intentionally a bit careful to only take the filename-part, and not take for example "../../../passwd". I guess this is probably fine as hopefully you're only on a network with devices you trust...
-[Unknown]
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... Probably worth adding checks against though indeed.
std::vector<File::FileInfo> entries; | ||
File::GetFilesInDir(Path(g_Config.sRemoteISOSharedDir), &entries); | ||
for (const auto &entry : entries) { | ||
// TODO: Support browsing into subdirs. How are folders marked? |
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.
Folders have a trailing slash. But I can't remember if browsing supports them... I kinda think it does? It wouldn't be hard to anyway.
People will probably expect this to be recursive or else to support folders (my personal preference would be folders to work.)
-[Unknown]
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 will add folder support later. I didn't think about the trailing slash - that'll be enough, and I'll make sure it works if it doesn't already.
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.
Hm, let's hope the user doesn't have a subfolder called "debugger". Since /debugger is handled separately :)
We're gonna have to use FallbackHandler to handle subfolder requests.
Can be convenient. This is backwards-compatible with old builds (old builds can stream these).
Will rework the UI a bit soon, so not adding translation strings properly yet.