KubeJS-Mods / KubeJS

https://kubejs.com
GNU Lesser General Public License v3.0
307 stars 90 forks source link

Cannot access files in the kubejs/config directory #829

Closed PrincessRTFM closed 5 months ago

PrincessRTFM commented 5 months ago

Minecraft Version

1.20.1

KubeJS Version

2001.6.5-build.7

Rhino Version

2001.2.2-build.18

Architectury Version

9.2.14

Forge/Fabric Version

Forge 47.2.21

Describe your issue

The kubejs.bindings.UtilsWrapper class does not contain any bindings for the kubejs.utils.UtilJS.getPath() method, which makes it impossible to access any files whatsoever within the game directory. This makes it impossible to provide a configuration file for scripts, or to store persistent state information via JsonIO.readJson() and JsonIO.write(), as those methods take full paths and do not themselves resolve the given path to the game directory. Unless the full instance folder path is known in advance and hard-coded into the script, no configuration files can be read, and while it is technically possible to hardcode paths to store persistent data, doing so would be both platform-dependent and heavily inadvisable.

As a result, the JsonIO class is of very limited use, since relative paths depend entirely on the working directory of the game process itself, which may not be guaranteed to be within the instance folder.

Crash report/logs

No response

pietro-lopes commented 5 months ago

I think this is pretty much intended for security reasons.

PrincessRTFM commented 5 months ago

That makes no sense, because file access is available, with arbitrary paths. The functionality that's missing is resolving paths to within the game directory specifically. I could access files under AppData if I could guess the user's name, and while I can't read arbitrary file content (JSON only), I could write some random JSON junk over existing files.

ChiefArug commented 5 months ago

Having the working folder of the game be the instance folder is a basic assumption modders make in modded minecraft. The last time the vanilla launcher broke that, almost every single modpack broke and it was quickly reverted.

PrincessRTFM commented 5 months ago

That makes sense, and definitely helps. This is still a security flaw though, because you can still overwrite paths outside the instance folder.

pietro-lopes commented 5 months ago

I think I'm not understanding the issue here, I tried this and works just fine

StartupEvents.init(_ => {
  let config = JsonIO.readJson("kubejs/config/my_modpack_configs.json")
  if (config == null) {
    config = { someKey: "someValue" }
    JsonIO.write("kubejs/config/my_modpack_configs.json", config)
  }
  // do something with config
})
MundM2007 commented 5 months ago

Yeah this posses security risks, and is completely unneccary anyway, cause there's No way to garantee a specific file existing and as such can only be really used tonsteal Data or overwrite json Files that Break other programms. Config files shouldn't be located Outside of the modpack instance directory

MundM2007 commented 5 months ago

Ah i now realised what you mean. You think that you need the full Disk path to a specific file to read Files. This is incorrect though, you only need to specify the path from the modpack instance directory

ChiefArug commented 5 months ago

because you can still overwrite paths outside the instance folder.

Have you actually attempted that? Last I checked, KubeJS verifies that paths you write or read to with JsonIO or NBTIO are within the instance folder.

PrincessRTFM commented 5 months ago

I didn't see that in the source, but if so, then my apologies for the false alarm. I was trying to figure out how to handle persistent data storage, and the documentation I found said scripts could only read/write files in the kubejs/config directory but never explained how, so I started digging through the mod source. The kubejs.util.JsonIO.write() method has no checks, so if the path is checked, then I can only assume there's an interface layer I didn't see that handles that.

ChiefArug commented 5 months ago

The typewrapper for Path checks if it is allowed or not, so it checks everywhere you pass an object to something that requires Path (even if you pass Path itself). https://github.com/KubeJS-Mods/KubeJS/blob/2001/common%2Fsrc%2Fmain%2Fjava%2Fdev%2Flatvian%2Fmods%2Fkubejs%2Futil%2FUtilsJS.java#L223

PrincessRTFM commented 5 months ago

Aha, that's what I'd missed!