Page MenuHomeWildfire Games
Differential D11 Diff 14538 source/ps/scripting/JSInterface_VFS.cpp

Changeset View

Standalone View

View Options

source/ps/scripting/JSInterface_VFS.cpp

/* Copyright (C) 2020 Wildfire Games. /* Copyright (C) 2020 Wildfire Games.
elexisUnsubmitted
Not Done
Inline Actions

9 and so on

elexis: 9 and so on
* This file is part of 0 A.D. * This file is part of 0 A.D.
* *
* 0 A.D. is free software: you can redistribute it and/or modify * 0 A.D. is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by * it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or * the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version. * (at your option) any later version.
* *
* 0 A.D. is distributed in the hope that it will be useful, * 0 A.D. is distributed in the hope that it will be useful,
▲ Show 20 Lines • Show All 194 Lines • ▼ Show 20 Linesvoid JSI_VFS::WriteJSONFile(ScriptInterface::CmptPrivate* pCmptPrivate, const std::wstring& filePath, JS::HandleValue val1)
std::string str(scriptInterface.StringifyJSON(&val, false)); std::string str(scriptInterface.StringifyJSON(&val, false));
VfsPath path(filePath); VfsPath path(filePath);
WriteBuffer buf; WriteBuffer buf;
buf.Append(str.c_str(), str.length()); buf.Append(str.c_str(), str.length());
g_VFS->CreateFile(path, buf.Data(), buf.Size()); g_VFS->CreateFile(path, buf.Data(), buf.Size());
} }
bool JSI_VFS::DeleteCampaignSave(ScriptInterface::CmptPrivate* UNUSED(pCmptPrivate), const CStrW& filePath)
{
OsPath realPath;
if (filePath.Left(16) != L"saves/campaigns/" || filePath.Right(12) != L".0adcampaign")
return false;
return VfsFileExists(filePath) &&
g_VFS->GetRealPath(filePath, realPath) == INFO::OK &&
elexisUnsubmitted
Not Done
Inline Actions

If I recall correctly, we were already discussing the possibility of abuse of this function, a possible restriction to the two directories where we allow deletion (so that a malicious mod can only delete all savegames and campaign saves).
The delete-savegame function can be superseded by this one.

elexis: If I recall correctly, we were already discussing the possibility of abuse of this function, a…
wraitiiAuthorUnsubmitted
Not Done
Inline Actions

We did. Haven't implemented said restriction when rebasing since it's more work and honestly this patch is already almost too big to grok.

wraitii: We did. Haven't implemented said restriction when rebasing since it's more work and honestly…
elexisUnsubmitted
Not Done
Inline Actions

Apparently JS is agnostic of the .0adsave suffix, so SavedGames::DeleteSavedGame can't be replaced without hardcoding the suffic in JS.

One could also examine the position that everything that this function could delete could already be deleted otherwise (config system) or be a valid user-triggered deletion (screenshots? but that would require displaying them in 0ad. So perhaps make it safe until then. Should be two lines, one of them defining a path, the other calling PathRestrictionMet)

elexis: Apparently JS is agnostic of the `.0adsave` suffix, so `SavedGames::DeleteSavedGame` can't be…
g_VFS->RemoveFile(filePath) == INFO::OK &&
wunlink(realPath) == 0;
}
bool JSI_VFS::PathRestrictionMet(ScriptInterface::CmptPrivate* pCmptPrivate, const std::vector<CStrW>& validPaths, const CStrW& filePath) bool JSI_VFS::PathRestrictionMet(ScriptInterface::CmptPrivate* pCmptPrivate, const std::vector<CStrW>& validPaths, const CStrW& filePath)
{ {
elexisUnsubmitted
Done
Inline Actions

Looks like it might become one statement with newlines between the &&. (It still aborts on first unsatisfied condition)

elexis: Looks like it might become one statement with newlines between the &&. (It still aborts on…
for (const CStrW& validPath : validPaths) for (const CStrW& validPath : validPaths)
if (filePath.find(validPath) == 0) if (filePath.find(validPath) == 0)
return true; return true;
CStrW allowedPaths; CStrW allowedPaths;
for (std::size_t i = 0; i < validPaths.size(); ++i) for (std::size_t i = 0; i < validPaths.size(); ++i)
{ {
if (i != 0) if (i != 0)
Show All 32 Linesvoid JSI_VFS::RegisterScriptFunctions_GUI(const ScriptInterface& scriptInterface)
scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &Script_ListDirectoryFiles_GUI>("ListDirectoryFiles"); scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &Script_ListDirectoryFiles_GUI>("ListDirectoryFiles");
scriptInterface.RegisterFunction<bool, std::wstring, Script_FileExists_GUI>("FileExists"); scriptInterface.RegisterFunction<bool, std::wstring, Script_FileExists_GUI>("FileExists");
scriptInterface.RegisterFunction<double, std::wstring, &JSI_VFS::GetFileMTime>("GetFileMTime"); scriptInterface.RegisterFunction<double, std::wstring, &JSI_VFS::GetFileMTime>("GetFileMTime");
scriptInterface.RegisterFunction<unsigned int, std::wstring, &JSI_VFS::GetFileSize>("GetFileSize"); scriptInterface.RegisterFunction<unsigned int, std::wstring, &JSI_VFS::GetFileSize>("GetFileSize");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFile>("ReadFile"); scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFile>("ReadFile");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFileLines>("ReadFileLines"); scriptInterface.RegisterFunction<JS::Value, std::wstring, &JSI_VFS::ReadFileLines>("ReadFileLines");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_GUI>("ReadJSONFile"); scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_GUI>("ReadJSONFile");
scriptInterface.RegisterFunction<void, std::wstring, JS::HandleValue, &WriteJSONFile>("WriteJSONFile"); scriptInterface.RegisterFunction<void, std::wstring, JS::HandleValue, &WriteJSONFile>("WriteJSONFile");
scriptInterface.RegisterFunction<bool, CStrW, &DeleteCampaignSave>("DeleteCampaignSave");
elexisUnsubmitted
Not Done
Inline Actions

(OT: Because we already own FromJSVal<Path>, all JSInterface files should be registered with VfsPath directly instead of receiving a string and converting that (possibly incorrectly).)

elexis: (OT: Because we already own `FromJSVal<Path>`, all JSInterface files should be registered with…
} }
void JSI_VFS::RegisterScriptFunctions_Simulation(const ScriptInterface& scriptInterface) void JSI_VFS::RegisterScriptFunctions_Simulation(const ScriptInterface& scriptInterface)
{ {
scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &Script_ListDirectoryFiles_Simulation>("ListDirectoryFiles"); scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &Script_ListDirectoryFiles_Simulation>("ListDirectoryFiles");
scriptInterface.RegisterFunction<bool, std::wstring, Script_FileExists_Simulation>("FileExists"); scriptInterface.RegisterFunction<bool, std::wstring, Script_FileExists_Simulation>("FileExists");
elexisUnsubmitted
Not Done
Inline Actions

Wrong name since its not restricted to JSON.

Yay more VFS functions to unify. SavedGame::DeleteSavedGame does the same but with a directory restriction.
We could handle any unification in an independent patch.

Since the function has no restrictoin, it could also be used to delete screenshots and whatnot. I'm not fully sure that this is a disadvantage however.
If some malicious GUI script starts deleting things, it can already delete all savegames.
Deleting the user.cfg sounds more problematic, so perhaps we should keep the directory restriction in C++ one way or another.

elexis: Wrong name since its not restricted to JSON. Yay more VFS functions to unify. `SavedGame…
wraitiiAuthorUnsubmitted
Not Done
Inline Actions

Right.

If some malicious GUI script starts deleting things, it can already delete all savegames.

Well actually writeJSONFile takes an arbitrary path, so you could overwrite any existing file too I think.

I do agree that both of those could use stricter checks though, as malicious mods could be annoying.

wraitii: Right. > If some malicious GUI script starts deleting things, it can already delete all…
elexisUnsubmitted
Not Done
Inline Actions

The SavedGame one already is strict because it only allows deletion in the savegame directory. This one isn't. Perhaps both of the savegame variants could use the path restriction mechanism of the VFS in D1085 and only one function.

elexis: The SavedGame one already is strict because it only allows deletion in the savegame directory.
elexisUnsubmitted
Not Done
Inline Actions
# can be extended later for skirmish savegames
#define PathRestriction_Deletion {L"campaignsaves/"}

bool JSI_VFS::DeleteFile(ScriptInterface::CxPrivate* pCxPrivate, const std::wstring& filePath)
	if (!PathRestrictionMet(pCxPrivate, PathRestriction_Deletion, filePath))
		return false;
elexis: ``` # can be extended later for skirmish savegames #define PathRestriction_Deletion…
scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_Simulation>("ReadJSONFile"); scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_Simulation>("ReadJSONFile");
} }
void JSI_VFS::RegisterScriptFunctions_Maps(const ScriptInterface& scriptInterface) void JSI_VFS::RegisterScriptFunctions_Maps(const ScriptInterface& scriptInterface)
{ {
scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &Script_ListDirectoryFiles_Maps>("ListDirectoryFiles"); scriptInterface.RegisterFunction<JS::Value, std::wstring, std::wstring, bool, &Script_ListDirectoryFiles_Maps>("ListDirectoryFiles");
scriptInterface.RegisterFunction<bool, std::wstring, Script_FileExists_Maps>("FileExists"); scriptInterface.RegisterFunction<bool, std::wstring, Script_FileExists_Maps>("FileExists");
scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_Maps>("ReadJSONFile"); scriptInterface.RegisterFunction<JS::Value, std::wstring, &Script_ReadJSONFile_Maps>("ReadJSONFile");
} }
elexisUnsubmitted
Not Done
Inline Actions

??

elexis: ??
wraitiiAuthorUnsubmitted
Not Done
Inline Actions

Same, not sure when that got here.

wraitii: Same, not sure when that got here.
StanUnsubmitted
Not Done
Inline Actions

Still there ?

Stan: Still there ?
FreagarachUnsubmitted
Not Done
Inline Actions

^

Freagarach: ^
Wildfire Games · Phabricator