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

[ENHANCEMENT] FileUtil additions + sandboxing #3032

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

cyn0x8
Copy link

@cyn0x8 cyn0x8 commented Jul 15, 2024

Briefly describe the issue(s) fixed.

im currently working on a mod launcher/manager/updater mod, and require certain file manipulation functions that are blacklisted and also not currently present in FileUtil, so this pr aims to expand FileUtil functionality by adding the following:

moveFile(path:String, destination:String):Void

deleteFile(path:String):Void

getFileSize(path:String):Int

isDirectory(path:String):Bool

readDir(path:String):Array<String>

moveDir(path:String, destination:String, ?ignore:Array<String>):Void

deleteDir(path:String, recursive:Bool = false, ?ignore:Array<String>):Void

getDirSize(path:String):Int

rename(path:String, newName:String, keepExtension:Bool = true):Void

this pr also sandboxes all the functions!! the core functionality is now housed in FileUtilBase (used only internally, blacklisted in scripts), which FileUtil uses, sanitizing the paths first and preventing them from leaving the game folder

functions that have the capability to modify/delete are prevented from messing with assets, manifest and everything in it, plugins and everything in it, and the dlls and game executable


all functions above have been tested; if anyone finds any faults, please make them known

@AbnormalPoof
Copy link

Wouldn't this theoretically allow mods to do malicious things? Unless somehow directories that are not Funkin's are restricted.

@cyn0x8
Copy link
Author

cyn0x8 commented Jul 15, 2024

Wouldn't this theoretically allow mods to do malicious things? Unless somehow directories that are not Funkin's are restricted.

mods can already do that with the functions already in FileUtil like going ../ repeatedly upwards out of the game root directory and then back down to some important file and overwriting it or something

var root:String = "./";
while (FileUtil.doesFileExist(root)) {root += "../";}
FileUtil.writeStringToPath(root.substring(0, root.length - 6) + "some important path here", "", FileWriteMode.Force);

though sandboxing to only the game root directory is definitely needed i think thats an issue for another pr

@AbnormalPoof
Copy link

Fair enough! Sandboxing would be very nice.

@cyn0x8
Copy link
Author

cyn0x8 commented Jul 15, 2024

Fair enough! Sandboxing would be very nice.

itd be an easy addition like using this in all the functions that have a path input

/**
 * Prevent paths from exiting the root.
 *
 * @param path The path to sanitize.
 * @return The sanitized path.
 */
public static function sanitizePath(path:String):String
{
  path = path.trim().replace('\\', '/');

  if (path.contains(':'))
  {
    path = path.substring(path.lastIndexOf(':') + 1);
  }

  while (path.charAt(0) == '/')
  {
    path = path.substring(1);
  }

  var parts:Array<String> = path.split('/');
  var sanitized:Array<String> = [];
  for (part in parts)
  {
    switch (part)
    {
      case '.':
      case '':
        continue;
      case '..':
        if (sanitized.length > 0) sanitized.pop();
      default:
        sanitized.push(part);
    }
  }

  return sanitized.join('/');
}

however the problem is that theres a few places internally where they use the functions to do stuff outside of the root folder (unit tests/polymod asset redirect/etc)

@cyn0x8 cyn0x8 marked this pull request as draft July 16, 2024 21:09
@cyn0x8 cyn0x8 changed the title [ENHANCEMENT] FileUtil additions [ENHANCEMENT] FileUtil additions + sandboxing Jul 17, 2024
@cyn0x8 cyn0x8 marked this pull request as ready for review July 17, 2024 05:27
@cyn0x8
Copy link
Author

cyn0x8 commented Jul 17, 2024

image
image

it works

@tposejank
Copy link
Contributor

Shouldn't it be better to extend FileUtilBase and override all functions including those that cause trouble or am i dumb ?

@cyn0x8
Copy link
Author

cyn0x8 commented Jul 17, 2024

Shouldn't it be better to extend FileUtilBase and override all functions including those that cause trouble or am i dumb ?

image

will do that rn lol

@cyn0x8
Copy link
Author

cyn0x8 commented Jul 17, 2024

wait nvm static doesnt have inheritance

pr is fine so far

@EliteMasterEric EliteMasterEric added the status: pending triage The bug or PR has not been reviewed yet. label Jul 18, 2024
@EliteMasterEric EliteMasterEric added type: enhancement Provides an enhancement or new feature. mods Issue is related to the use of mods. status: reviewing internally This PR is under internal review and quality assurance testing large A large pull request with more than 100 changes and removed status: pending triage The bug or PR has not been reviewed yet. labels Jul 25, 2024
@EliteMasterEric EliteMasterEric self-assigned this Jul 25, 2024
@EliteMasterEric
Copy link
Member

Pretty interesting!

Of note, you could change the names so that the one that gets used by the core game is still FileUtil, and the one that the scripts uses is called something like FileUtilSandboxed, and then you can add a Polymod import alias which replaces imports of FileUtil with FileUtilSandboxed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large A large pull request with more than 100 changes mods Issue is related to the use of mods. status: reviewing internally This PR is under internal review and quality assurance testing type: enhancement Provides an enhancement or new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants