Changeset View
Standalone View
source/ps/ModIo.cpp
- This file was added.
/* Copyright (C) 2018 Wildfire Games. | |||||
* | |||||
* Permission is hereby granted, free of charge, to any person obtaining | |||||
* a copy of this software and associated documentation files (the | |||||
* "Software"), to deal in the Software without restriction, including | |||||
* without limitation the rights to use, copy, modify, merge, publish, | |||||
* distribute, sublicense, and/or sell copies of the Software, and to | |||||
* permit persons to whom the Software is furnished to do so, subject to | |||||
* the following conditions: | |||||
* | |||||
* The above copyright notice and this permission notice shall be included | |||||
* in all copies or substantial portions of the Software. | |||||
* | |||||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, | |||||
* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF | |||||
* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. | |||||
* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY | |||||
* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, | |||||
* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE | |||||
wraitii: I feel like, as it stands now, it's short enough that people would just rewrite it, and it's… | |||||
Done Inline ActionsAlso I'm generally in favour of keeping licensing simple. wraitii: Also I'm generally in favour of keeping licensing simple. | |||||
Done Inline ActionsWell, assuming someone just wants to fix up a few things (the JSON part) and include it elsewhere having this as MIT seems nicer. Also given that the full MIT license is shorter than that blurp about this being GPL and where to get the full license if it wasn't included, that seems more like keeping it actually simple. leper: Well, assuming someone just wants to fix up a few things (the JSON part) and include it… | |||||
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | |||||
*/ | |||||
// TODO: Comment on why we handle things this way (not relying on user input, not trusting the remote too much, etc.) | |||||
// TODO: Better error messages. Also ask upstream about some special queries with partially malformed replies | |||||
#include "precompiled.h" | |||||
#include "ModIo.h" | |||||
#include "lib/file/file_system.h" | |||||
Not Done Inline ActionsWhat the heck... leper: What the heck... | |||||
#include "lib/sysdep/sysdep.h" | |||||
#include "lib/sysdep/filesystem.h" | |||||
Done Inline ActionsWhy is there a blank line between full path includes, and why are those not sorted properly? leper: Why is there a blank line between full path includes, and why are those not sorted properly? | |||||
Not Done Inline ActionsWhy is there a blank line? leper: Why is there a blank line? | |||||
Done Inline ActionsI suppose you consider that a stylistic issue, but the way includes are handled here and elsewhere seems both ridiculous and maintenance-unfriendly. What if you move one file in that same directory elsewhere? Right you need to update not only that file, but all other files that include it. As opposed to only one of those with proper full paths. leper: I suppose you consider that a stylistic issue, but the way includes are handled here and… | |||||
Not Done Inline ActionsYou are right, it is maintenance-unfriendly, I hadn't thought of this. I put that back in order and I'll change that part of the conventions. Itms: You are right, it is maintenance-unfriendly, I hadn't thought of this. I put that back in order… | |||||
Done Inline ActionsHaving the ps/**.h separated from the others still keeps this slightly unfriendly. That said, moving towards proper full paths seems a lot nicer than moving to partial paths. leper: Having the `ps/**.h` separated from the others still keeps this slightly unfriendly.
That said… | |||||
#include "maths/MD5.h" | |||||
#include "ps/CLogger.h" | |||||
#include "ps/ConfigDB.h" | |||||
#include "ps/GameSetup/Paths.h" | |||||
#include "ps/Mod.h" | |||||
#include "scriptinterface/ScriptConversions.h" | |||||
#include <iomanip> | |||||
Done Inline ActionsIs this one used for anything? Itms: Is this one used for anything? | |||||
Done Inline Actionssetfill and setw for getting the md5 digest. leper: `setfill` and `setw` for getting the md5 digest. | |||||
Done Inline ActionsAh it builds without so I was confused. Itms: Ah it builds without so I was confused. | |||||
ModIo* g_ModIo = nullptr; | |||||
struct DownloadCallbackData { | |||||
FILE* fp; | |||||
MD5 md5; | |||||
}; | |||||
static bool VerifyDownload(const OsPath& filePath, DownloadCallbackData& callbackData, const std::map<std::string, std::string>& modData); | |||||
ModIo::ModIo() | |||||
Done Inline ActionsGiven the view in phabricator, this comment (and possibly others) might need some fixing so those can be nicely read on some standard size. leper: Given the view in phabricator, this comment (and possibly others) might need some fixing so… | |||||
: m_GamesRequest("/games") | |||||
{ | |||||
// Get config values from the sytem namespace, or below (default); this can be overridden on the command line. | |||||
// We do this so a malicious mod cannot change the base url and get the user to make connections | |||||
// to someone else's endpoint. | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.baseurl", m_BaseUrl); | |||||
// TODO: Should we allow mods to actually change the two settings below? (Might be nice for total conversions.) | |||||
{ | |||||
Done Inline ActionsI would see this in ModIo.h. Itms: I would see this in `ModIo.h`. | |||||
std::string api_key; | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.api_key", api_key); | |||||
m_ApiKey = "api_key=" + api_key; | |||||
} | |||||
{ | |||||
Not Done Inline Actionsagree elexis: agree | |||||
Not Done Inline ActionsThanks, I was worried that you didn't. leper: Thanks, I was worried that you didn't. | |||||
std::string nameid; | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.name_id", nameid); | |||||
m_IdQuery = "name_id="+nameid; | |||||
} | |||||
// Initialise everything except Win32 sockets (because our networking | |||||
// system already inits those) | |||||
curl_global_init(CURL_GLOBAL_ALL & ~CURL_GLOBAL_WIN32); | |||||
// TODO we should only do this in one single place (same for shutdown, currently we do it here and in the userreporter) | |||||
m_Curl = curl_easy_init(); | |||||
ENSURE(m_Curl); | |||||
// TODO: Do we actually care? | |||||
// Capture error messages | |||||
curl_easy_setopt(m_Curl, CURLOPT_ERRORBUFFER, m_ErrorBuffer); | |||||
// Disable signal handlers (required for multithreaded applications) | |||||
Not Done Inline ActionsThe URL must be separated from the logic elexis: The URL must be separated from the logic | |||||
Not Done Inline ActionsWhy? leper: Why? | |||||
Not Done Inline ActionsHardcoding: Reducing the effort to change URLs in case of changing the home URL. A sufficiently popular mod reusing the pyrogenesis engine or a mod modifying the mod downloader might want to change or extend the mod API key and agent. elexis: Hardcoding: Reducing the effort to change URLs in case of changing the home URL. A sufficiently… | |||||
Not Done Inline Actionsman 1 sed Given that a sufficiently popular mod will still be using the same engine the agent should still be the same. If it shouldn't be then it should also not be the same engine. Unless you didn't notice something the API key is in the configuration. leper: `man 1 sed`
Given that a sufficiently popular mod will still be using the same engine the… | |||||
curl_easy_setopt(m_Curl, CURLOPT_NOSIGNAL, 1L); | |||||
// To minimise security risks, don't support redirects | |||||
curl_easy_setopt(m_Curl, CURLOPT_FOLLOWLOCATION, 0L); | |||||
m_Headers = NULL; | |||||
Done Inline ActionsComment should explain the clueless reader how using this identifier rather than the common identifier adds security. elexis: Comment should explain the clueless reader how using this identifier rather than the common… | |||||
Not Done Inline ActionsStrange how that one TODO comment above does hint that this still needs to be done. One could also note the WIP in the title, or the PoC there. Onions... leper: Strange how that one TODO comment above does hint that this still needs to be done. One could… | |||||
std::string ua = "User-Agent: pyrogenesis "; | |||||
ua += curl_version(); | |||||
ua += " (https://play0ad.com/)"; | |||||
m_Headers = curl_slist_append(m_Headers, ua.c_str()); | |||||
// TODO more? | |||||
curl_easy_setopt(m_Curl, CURLOPT_HTTPHEADER, m_Headers); | |||||
Done Inline ActionsI think that way is good. Full total conversions are likely to be still mods of 0 A.D. on mod.io, no? Itms: I think that way is good. Full total conversions are likely to be still mods of 0 A.D. on mod. | |||||
Not Done Inline ActionsThey would most likely be, however as roughly explained in the comment (and assuming they aren't installed like mods for some strange reason) they can still change this if they really want to. I guess I'll just change this to some rationale instead of a TODO. leper: They would most likely be, however as roughly explained in the comment (and assuming they… | |||||
// we only accept indices from JS when downloading something later on (security) | |||||
Done Inline ActionsThat comment, like most others I've had in other patches, does not start with TODO: which means it should be fixed before this is merged, or in case it isn't valid anymore removed. leper: That comment, like most others I've had in other patches, does not start with `TODO: ` which… | |||||
Not Done Inline ActionsAh thanks for giving the key ^^ Itms: Ah thanks for giving the key ^^ | |||||
Not Done Inline ActionsThings not properly indented are also things that should be fixed. leper: Things not properly indented are also things that should be fixed. | |||||
} | |||||
ModIo::~ModIo() | |||||
{ | |||||
curl_slist_free_all(m_Headers); | |||||
Done Inline ActionsNow compare this error to the one below, which states "invalid public key". So in one of these ENSUREs you have what should have happened, but didn't, and in another you have what happened, but shouldn't have. At least being consistent would be nice. Though I do prefer asserts that tell one what should have happened (and are explicit in doing so). leper: Now compare this error to the one below, which states "invalid public key". So in one of these… | |||||
curl_easy_cleanup(m_Curl); | |||||
curl_global_cleanup(); // TODO | |||||
} | |||||
size_t ModIo::ReceiveCallback(void* buffer, size_t size, size_t nmemb, void* userp) | |||||
{ | |||||
ModIo* self = static_cast<ModIo*>(userp); | |||||
self->m_ResponseData += std::string((char*)buffer, (char*)buffer+size*nmemb); | |||||
return size*nmemb; | |||||
} | |||||
size_t DownloadCallback(void* buffer, size_t size, size_t nmemb, void* userp) | |||||
{ | |||||
DownloadCallbackData* data = static_cast<DownloadCallbackData*>(userp); | |||||
Done Inline ActionsI still sort of disagree with having ENSUREs (or similar) have code that should always be called in them. Not that I expect someone to just #define ENSURE(p) them away, but given a few recent developments I want to be able to say "told you so" once that happens. leper: I still sort of disagree with having `ENSURE`s (or similar) have code that should always be… | |||||
Not Done Inline ActionsSo what was wrong about the if (!check) ENSURE(0 && "message") that I had in a previous iteration? (I did consider it was just a remark, not a change request, but you explicitly said I had ignored the comment so here I am ? ) Itms: So what was wrong about the `if (!check) ENSURE(0 && "message")` that I had in a previous… | |||||
Not Done Inline ActionsIt ended up inconsistent with the other one that got changed to ENSURE(foo && "bar"); above. leper: It ended up inconsistent with the other one that got changed to `ENSURE(foo && "bar");` above. | |||||
Not Done Inline ActionsAhhhhh. I would have preferred a comment on the other ENSURE then, but whatever. Itms: Ahhhhh. I would have preferred a comment on the other `ENSURE` then, but whatever. | |||||
size_t len = fwrite(buffer, size, nmemb, data->fp); | |||||
// Only update the hash with data we actually managed to write. | |||||
// In case we did not write all of it we will fail the download, | |||||
Not Done Inline ActionsGiven that the above ENSURE got changed to have the call inside it this should be made consistent. Not that I really consider having calls in something that could not be present with some options a good idea, since having code within ASSERT or ENSURE (or for the really ambitious UNUSED) that has any non-verifying effects is likely going to cause bugs down the road. leper: Given that the above `ENSURE` got changed to have the call inside it this should be made… | |||||
Not Done Inline ActionsThis was not fixed, see the comment a bit above. The latter part of the comment was also not really fixed or considered... leper: This was not fixed, see the comment a bit above.
The latter part of the comment was also not… | |||||
Not Done Inline ActionsNot having this fail, and fail hard at that possibly opens an issue in that the resulting state of those values allows someone to sign things and get them accepted. I repeat, this has to fail hard. leper: Not having this fail, and fail hard at that possibly opens an issue in that the resulting state… | |||||
Not Done Inline ActionsNow fails hard again and prevents the key to stay set to something that could create security concerns (the value reset prevents me from being consistent with the other ENSURE but if I understood correctly (maybe I didn't) this wasn't your actual issue). Itms: Now fails hard again and prevents the key to stay set to something that could create security… | |||||
Not Done Inline ActionsToJSVal would possibly be nicer, not that the whole thing seems that important. leper: `ToJSVal` would possibly be nicer, not that the whole thing seems that important. | |||||
// but we do not want to have a possibly valid hash in that case. | |||||
data->md5.Update((const u8*)buffer, len*size); | |||||
return len*size; | |||||
} | |||||
const std::vector<std::map<std::string, std::string>>& ModIo::GetMods(const ScriptInterface& scriptInterface) | |||||
{ | |||||
m_ModData.clear(); | |||||
Not Done Inline ActionsMaybe the date, unless that is sent by curl by default. And I'd love to add a X-Clacks-Overhead but maybe as a config option... Itms: Maybe the date, unless that is sent by curl by default. And I'd love to add a `X-Clacks… | |||||
Not Done Inline ActionsIs the date needed? And no it does not. I guess the user agent construction could also be factored out since we do that in about 2 places right now. I guess adding a header there should be easy. leper: Is the date needed? And no it does not.
I guess the user agent construction could also be… | |||||
Not Done Inline ActionsSounds good! Itms: Sounds good! | |||||
Not Done Inline ActionsThat's not really a shutdown hook, but whatever. leper: That's not really a shutdown hook, but whatever. | |||||
// Set IO callbacks | |||||
curl_easy_setopt(m_Curl, CURLOPT_WRITEFUNCTION, ReceiveCallback); | |||||
curl_easy_setopt(m_Curl, CURLOPT_WRITEDATA, this); | |||||
// Get the game id if we didn't fetch it already | |||||
if (m_GameId.empty()) | |||||
{ | |||||
Done Inline ActionsEven though every self-respecting compiler should ensure that len*size does not change, it might help the reader if that was computed once, and would fit the comment above. leper: Even though every self-respecting compiler should ensure that `len*size` does not change, it… | |||||
std::string url = m_BaseUrl+m_GamesRequest+"?"+m_ApiKey+"&"+m_IdQuery; | |||||
curl_easy_setopt(m_Curl, CURLOPT_URL, url.c_str()); | |||||
CURLcode err = curl_easy_perform(m_Curl); | |||||
if (err != CURLE_OK) | |||||
{ | |||||
LOGERROR("Server said no: %d", err); | |||||
return m_ModData; | |||||
} | |||||
Done Inline Actions(bit meh, but code appears indeed better with it) elexis: (bit meh, but code appears indeed better with it) | |||||
if (!ParseGameIdResponse(scriptInterface)) | |||||
return m_ModData; | |||||
} | |||||
Done Inline ActionsThose C++ style casts seem slightly unwieldy, and this function does not even attempt to handle dlnow > dltotal. Also this could be handled by a ternary, not that it really matters. leper: Those C++ style casts seem slightly unwieldy, and this function does not even attempt to handle… | |||||
Not Done Inline ActionsI think @vladislavbelov and the doxa discourage the use of C-style casts. Do we really care if curl passes something stupid to this callback? Added the ternary. Itms: I think @vladislavbelov and the doxa discourage the use of C-style casts. Do we really care if… | |||||
Not Done Inline ActionsI'm not sure I agree with any of those, given that some of those also misuse defines meant for parameters only and break code if those aren't defined. leper: I'm not sure I agree with any of those, given that some of those also misuse defines meant for… | |||||
// Get the actual mods | |||||
std::string url = m_BaseUrl+m_GamesRequest+m_GameId+"/mods?"+m_ApiKey; | |||||
curl_easy_setopt(m_Curl, CURLOPT_URL, url.c_str()); | |||||
CURLcode err = curl_easy_perform(m_Curl); | |||||
if (err != CURLE_OK) | |||||
{ | |||||
LOGERROR("Server said no to mods %d", err); | |||||
return m_ModData; | |||||
} | |||||
if (!ParseModsResponse(scriptInterface)) | |||||
Done Inline ActionsThis is never reset, at least when reading this diff top-down, including the rest of this function. leper: This is never reset, at least when reading this diff top-down, including the rest of this… | |||||
Not Done Inline ActionsThis was done when tearing requests down. I agree it would be clearer here so I moved it. Itms: This was done when tearing requests down. I agree it would be clearer here so I moved it. | |||||
m_ModData.clear(); // Failed during parsing, make sure we don't provide partial data | |||||
PrintMods(); | |||||
return m_ModData; | |||||
} | |||||
#define FAIL(...) STMT(LOGERROR(__VA_ARGS__); return false;) | |||||
/** | |||||
* Parses the current content of m_ResponseData to extract m_GameId. | |||||
Not Done Inline ActionsShouldn't the whole member be initialized here? leper: Shouldn't the whole member be initialized here? | |||||
Not Done Inline ActionsI could set the status here but it is alredy done elsewhere, and for non-files I can't set it here. Itms: I could set the status here but it is alredy done elsewhere, and for non-files I can't set it… | |||||
* | |||||
* The JSON data is expected to look like | |||||
* { "data": [{"id": 42, ...}, ...], ... } | |||||
* where we are only interested in the value of the id property. | |||||
* | |||||
* @returns true iff it successfully parsed the id. | |||||
*/ | |||||
Not Done Inline ActionsEven though this request is likely to be fast, I'd make the GUI behave like during the mod download and display a message box with something spinning. Itms: Even though this request is likely to be fast, I'd make the GUI behave like during the mod… | |||||
Not Done Inline ActionsCurrently it displays (unless you switch windows) the warning box, not too nice, but I did mention that someone else can do the GUI. leper: Currently it displays (unless you switch windows) the warning box, not too nice, but I did… | |||||
bool ModIo::ParseGameIdResponse(const ScriptInterface& scriptInterface) | |||||
{ | |||||
JSContext* cx = scriptInterface.GetContext(); | |||||
JS::RootedValue gameResponse(cx); | |||||
// What the data is expected to look like, and what we need. | |||||
// There is also a result_count property at the same level as data, which should always be 1 following | |||||
// our query, but maybe we should check that (nah) | |||||
if (!scriptInterface.ParseJSON(m_ResponseData, &gameResponse)) | |||||
Not Done Inline ActionsNo, well, better than before. Doing this after the failure is ok, but seems bad, the original code would likely leave the state in some somewhat random state at least, this leaves it in a predictable state, which, while it might be hard to generate the corresponding key, is quite bad. leper: No, well, better than before.
Doing this after the failure is ok, but seems bad, the original… | |||||
Not Done Inline ActionsNah, not really, well still not, see the main comment IIRC. leper: Nah, not really, well still not, see the main comment IIRC. | |||||
FAIL("Failed to parse response as JSON."); | |||||
m_ResponseData.clear(); | |||||
JS::RootedObject gameResponseObj(cx, gameResponse.toObjectOrNull()); | |||||
// TODO handle it being null, or does JS_GetProperty do that for us? | |||||
JS::RootedValue dataVal(cx); | |||||
if (!JS_GetProperty(cx, gameResponseObj, "data", &dataVal)) | |||||
FAIL("data property not in response."); | |||||
// [{"id": 42, ...}, ...] | |||||
if (!dataVal.isObject()) | |||||
Done Inline Actionswondering if the data quantity mandates to parse the response in C++ rather than benefiting from using JS to parse JS (not only ES6 but also many more basic functions, no spidermonkey terms, more people who can write JS than C++, if modio updates their protocol, we only need to update JS, could write protocol tests in JS for instance) elexis: wondering if the data quantity mandates to parse the response in C++ rather than benefiting… | |||||
Done Inline ActionsNo, they don't. The reason we don't do something stupid like that has been explained elsewhere. But maybe one should first try to figure out why I'm doing this the hard way instead of making this a nice attack vector. leper: No, they don't. The reason we don't do something stupid like that has been explained elsewhere. | |||||
FAIL("data property not an object"); | |||||
JS::RootedObject data(cx, dataVal.toObjectOrNull()); | |||||
u32 length; | |||||
if (!JS_IsArrayObject(cx, data) || !JS_GetArrayLength(cx, data, &length) || !length) | |||||
FAIL("data property not an array with at least one element"); | |||||
// {"id": 42, ...} | |||||
JS::RootedValue first(cx); | |||||
if (!JS_GetElement(cx, data, 0, &first)) | |||||
FAIL("couldn't get first element."); | |||||
int id = -1; | |||||
if (!ScriptInterface::FromJSProperty(cx, first, "id", id)) | |||||
FAIL("couldn't get id"); | |||||
Done Inline ActionsI'd have gone with if (error) { ... return; } /* expected code path */. leper: I'd have gone with `if (error) { ... return; } /* expected code path */`. | |||||
m_GameId = "/" + std::to_string(id); | |||||
Done Inline Actions(didn't that already throw an error if it failed to parse?) elexis: (didn't that already throw an error if it failed to parse?) | |||||
Not Done Inline ActionsAh, right because only a single error should ever be shown. Also note that there is no TODO comment regarding error handling anywhere. I guess I should open tickets for all of those. leper: Ah, right because only a single error should ever be shown. Also note that there is no TODO… | |||||
return true; | |||||
} | |||||
/** | |||||
* Parses the current content of m_ResponseData into m_ModData. | |||||
* | |||||
* The JSON data is expected to look like | |||||
* { data: [modobj1, modobj2, ...], ... (including result_count) } | |||||
* where modobjN has the following structure | |||||
* { homepage: "url", name: "displayname", nameid: "short-non-whitespace-name", | |||||
* summary: "short desc.", modfile: { version: "1.2.4", filename: "asdf.zip", | |||||
* filehash: { md5: "deadbeef" }, filesize: 1234, download_url: "someurl" }, ... }. | |||||
Not Done Inline ActionsI should have mentioned it earlier, but having that first parameter with a name to explain why we construct it and what is is going to be used for might be nice. Also with my C hat on not using that err variable anywhere but in the condition of the if on the following line, you could just inline that... leper: I should have mentioned it earlier, but having that first parameter with a name to explain why… | |||||
Not Done Inline ActionsThe different parts of the address are named so this looks pretty clear to me already. And no err is used to format the error message. Itms: The different parts of the address are named so this looks pretty clear to me already. And no… | |||||
* Only the listed properties are of interest to consumers, and we flatten | |||||
Not Done Inline ActionsI always make mistakes with this, but don't we usually omit the semicolon and call CLEANUP();? Wouldn't break the FAIL macro. Itms: I always make mistakes with this, but don't we usually omit the semicolon and call `CLEANUP();`? | |||||
Not Done Inline ActionsCould be ommitted, but doesn't really change anything. leper: Could be ommitted, but doesn't really change anything. | |||||
Not Done Inline ActionsOne should wrap it in a STMT (aka do { ... } while(0)) if that isn't considered localized enough to not really matter, might do that at some point. leper: One should wrap it in a `STMT` (aka `do { ... } while(0)`) if that isn't considered localized… | |||||
Done Inline ActionsSame as above for the if (err) { /* ... */ return; }. leper: Same as above for the `if (err) { /* ... */ return; }`. | |||||
* the modfile structure as that simplifies handling and there are no conflicts. | |||||
*/ | |||||
Not Done Inline Actions(wasnt there a way to merge those 2 lines?) elexis: (wasnt there a way to merge those 2 lines?) | |||||
Not Done Inline ActionsNo, there is none. The other case a few lines above is completely different. leper: No, there is none. The other case a few lines above is completely different. | |||||
bool ModIo::ParseModsResponse(const ScriptInterface& scriptInterface) | |||||
{ | |||||
JSContext* cx = scriptInterface.GetContext(); | |||||
JS::RootedValue modResponse(cx); | |||||
if (!scriptInterface.ParseJSON(m_ResponseData, &modResponse)) | |||||
FAIL("Failed to parse response as JSON."); | |||||
m_ResponseData.clear(); | |||||
JS::RootedObject modResponseObj(cx, modResponse.toObjectOrNull()); | |||||
// TODO handle it being null, or does JS_GetProperty do that for us? | |||||
JS::RootedValue dataVal(cx); | |||||
if (!JS_GetProperty(cx, modResponseObj, "data", &dataVal)) | |||||
FAIL("data property not in response."); | |||||
// [modobj1, modobj2, ... ] | |||||
if (!dataVal.isObject()) | |||||
FAIL("data property not an object"); | |||||
JS::RootedObject data(cx, dataVal.toObjectOrNull()); | |||||
u32 length; | |||||
if (!JS_IsArrayObject(cx, data) || !JS_GetArrayLength(cx, data, &length) || !length) | |||||
FAIL("data property not an array with at least one element"); | |||||
m_ModData.clear(); | |||||
m_ModData.reserve(length); | |||||
Done Inline ActionsCLEANUP()(;) Itms: `CLEANUP()`(`;`) | |||||
Not Done Inline ActionsNo, because this does not any cleanup. This just initializes something so the below works regardless of what the user passed us. leper: No, because this does not any cleanup. This just initializes something so the below works… | |||||
Not Done Inline ActionsAh right sorry! Itms: Ah right sorry! | |||||
for (u32 i = 0; i < length; ++i) | |||||
{ | |||||
JS::RootedValue el(cx); | |||||
if (!JS_GetElement(cx, data, i, &el) || !el.isObject()) | |||||
FAIL("Failed to get array element object"); | |||||
m_ModData.emplace_back(); | |||||
#define COPY_STRINGS(prefix, obj, ...) \ | |||||
for (const std::string& prop : { __VA_ARGS__ }) \ | |||||
Done Inline ActionsI seem to recall that this comment told about a hook at one point in time. That hook should clean up the temporary file if a download was started before the program termination. leper: I seem to recall that this comment told about a hook at one point in time. That hook should… | |||||
Not Done Inline ActionsAdded the cleanup code in the dtor. Itms: Added the cleanup code in the dtor. | |||||
{ \ | |||||
std::string val; \ | |||||
ScriptInterface::FromJSProperty(cx, obj, prop.c_str(), val); \ | |||||
m_ModData.back().emplace(prefix+prop, val); \ | |||||
} | |||||
Not Done Inline ActionsThat status seems like a lie. leper: That status seems like a lie. | |||||
Not Done Inline ActionsTrue, it is more a FAILED_DOWNLOAD_START, but there won't be any difference of behavior, and the error message makes explicit that a file I/O was the thing that failed. Itms: True, it is more a `FAILED_DOWNLOAD_START`, but there won't be any difference of behavior, and… | |||||
// TODO: Currently the homepage field does not contain a non-null value for any entry. | |||||
// TODO: look into storing the remaining mod.json things we need in metatdata_blob (deps | |||||
COPY_STRINGS("", el, "name", "name_id", "summary"); | |||||
// Now copy over the modfile part, but without the pointless substructure | |||||
JS::RootedObject elObj(cx, el.toObjectOrNull()); | |||||
Done Inline Actionsif (error) { /* ... */ return; } leper: `if (error) { /* ... */ return; }` | |||||
JS::RootedValue modFile(cx); | |||||
if (!JS_GetProperty(cx, elObj, "modfile", &modFile)) | |||||
FAIL("Failed to get modfile data"); | |||||
Not Done Inline ActionsThough I guess it is just not a nice match for the issues that were deemed important for downloading mods. leper: Though I guess it is just not a nice match for the issues that were deemed important for… | |||||
COPY_STRINGS("", modFile, "version", "filename", "filesize", "download_url"); | |||||
JS::RootedObject modFileObj(cx, modFile.toObjectOrNull()); | |||||
Done Inline ActionsShouldn't there be a check if the mod was already downloaded? elexis: Shouldn't there be a check if the mod was already downloaded? | |||||
Not Done Inline ActionsYes, because the function is really named DownloadModOnlyIfWeDidNotDoSoAlready. leper: Yes, because the function is really named `DownloadModOnlyIfWeDidNotDoSoAlready`. | |||||
JS::RootedValue filehash(cx); | |||||
if (!JS_GetProperty(cx, modFileObj, "filehash", &filehash)) | |||||
FAIL("Failed to get filehash data"); | |||||
// TODO: Actually copy all elements of this over? (So we get them automatically instead of having to do this manually) | |||||
COPY_STRINGS("filehash_", filehash, "md5"); | |||||
#undef COPY_STRINGS | |||||
} | |||||
return true; | |||||
} | |||||
Done Inline Actionsremove questionmark if these calls dont already complain (no remaining diskspace is one possibility) elexis: remove questionmark if these calls dont already complain (no remaining diskspace is one… | |||||
#undef FAIL | |||||
void ModIo::DownloadMod(size_t idx) | |||||
{ | |||||
if (idx >= m_ModData.size()) | |||||
return; | |||||
Done Inline ActionsWhen do you imagine this ENSURE to not be satisfied? leper: When do you imagine this `ENSURE` to not be satisfied? | |||||
// TODO: do this asynchronously? or at least tell the gui that we are doing something -> progress bar (or something spinning) | |||||
Done Inline Actionscould expose some of curls options as user config options elexis: could expose some of curls options as user config options | |||||
Done Inline ActionsTo mess things up for the sake of messing things up? No. leper: To mess things up for the sake of messing things up? No. | |||||
Done Inline ActionsRate limiting, disable cert check, proxies, retry times might not be useless. When considdering patches, we should explore things and decide things to be useless or say it can be done later. But declaring every curl option as messing things up preemptively seems wrong. elexis: Rate limiting, disable cert check, proxies, retry times might not be useless. When considdering… | |||||
Not Done Inline ActionsDisabling cert checks for something that installs something? Should I just open a reverse shell to make things even easier? leper: Disabling cert checks for something that installs something? Should I just open a reverse shell… | |||||
std::string url = m_ModData[idx]["download_url"]+"?shhh=secret"; // TODO remove param again once upstream fixes this | |||||
Not Done Inline ActionsDoes this handle all cases, if not why not? If it does, why is the default here? Nice compilers complain about missing cases already. leper: Does this handle all cases, if not why not?
If it does, why is the `default` here? Nice… | |||||
Not Done Inline ActionsYes it handles all cases where a status change is needed. I fail again to understand the default remark: in other cases (that might happen) we should do nothing here. Itms: Yes it handles all cases where a status change is needed. I fail again to understand the… | |||||
Not Done Inline ActionsA switch over an enum where all entries of the enum are handled does not need a default case. If it does that might hide compiler warnings when someone changes the entries of the enum. leper: A switch over an enum where all entries of the enum are handled does not need a default case. | |||||
Not Done Inline ActionsNot all the entries are handled, as you can see. And if the status is none of those three, we shouldn't do anything here, hence default: break;. What is wrong? Itms: Not all the entries are handled, as you can see. And if the status is none of those three, we… | |||||
const std::string& filename = m_ModData[idx]["filename"]; | |||||
const Paths paths(g_args); | |||||
OsPath modUserPath = paths.UserData()/"mods"; | |||||
OsPath modPath = modUserPath/m_ModData[idx]["name_id"]; | |||||
// TODO: Always call create? | |||||
if (!DirectoryExists(modPath) && INFO::OK != CreateDirectories(modPath, 0700, false)) | |||||
return; // TODO complain? | |||||
// TODO: ignore filename, treat as pyromod | |||||
OsPath filePath = modPath/filename; // TODO ignore filename, we don't care what people name it. | |||||
DownloadCallbackData callbackData; | |||||
callbackData.fp = sys_OpenFile(filePath, "w"); | |||||
if (!callbackData.fp) | |||||
return; // TODO error handling | |||||
// Set IO callbacks | |||||
curl_easy_setopt(m_Curl, CURLOPT_WRITEFUNCTION, DownloadCallback); | |||||
curl_easy_setopt(m_Curl, CURLOPT_WRITEDATA, (void*)&callbackData); | |||||
LOGERROR("Trying to download %s", url.c_str()); | |||||
Done Inline ActionsIf there is a return here, why is there an else? leper: If there is a `return` here, why is there an `else`? | |||||
// The download link will most likely redirect elsewhere, so allow that. | |||||
// We verify the validity of the file below. | |||||
curl_easy_setopt(m_Curl, CURLOPT_FOLLOWLOCATION, 1L); | |||||
// TODO: Restrict how often we can be redirected? | |||||
// Download the file | |||||
curl_easy_setopt(m_Curl, CURLOPT_URL, url.c_str()); | |||||
CURLcode err = curl_easy_perform(m_Curl); | |||||
fclose(callbackData.fp); | |||||
Not Done Inline ActionsOne could do something like int in_queue; while((message = curl_multi_info_read(m_CurlMulti, &in_queue)) && message->msg == CURLMSG_CONE && message->easy_handle == m_Curl Also should it really be easy_handle? leper: One could do something like
```
int in_queue;
while((message = curl_multi_info_read(m_CurlMulti… | |||||
Not Done Inline ActionsThat looks a bit cumbersome to me, I like the do...while. Personal taste I guess. Is this a VLC joke? ? Yes, multi_handle groups several easy ones, one have to check the message comes from the one we care about. This check is not necessary as we only have one handle in the multi, but better safe than worry. Itms: That looks a bit cumbersome to me, I like the `do...while`. Personal taste I guess.
Is this a… | |||||
Not Done Inline ActionsThe message that satisfies message->msg == CURLMSG_DONE and message->easy_handle == m_Curl may not be skipped, because it is the only one that triggers the error code handling below. elexis: The message that satisfies `message->msg == CURLMSG_DONE` and `message->easy_handle == m_Curl`… | |||||
Not Done Inline ActionsCondition comes from https://gitlab.com/na-Itms/0ad/commit/cdc324f7f57bf3098c196c59a92718b1ab4d1e87
elexis: Condition comes from https://gitlab.com/na-Itms/0ad/commit/cdc324f7f57bf3098c196c59a92718b1ab4d… | |||||
callbackData.fp = NULL; | |||||
// To minimise security risks, don't support redirects | |||||
curl_easy_setopt(m_Curl, CURLOPT_FOLLOWLOCATION, 0L); | |||||
if (err != CURLE_OK) | |||||
{ | |||||
LOGERROR("Server said no: %d", err); | |||||
if (wunlink(filePath) != 0) | |||||
LOGERROR("Failed to delete file."); | |||||
return; | |||||
} | |||||
// TODO: verify that the file is what we expect it to be (hash, filesize, signature) | |||||
// XXX | |||||
if (!VerifyDownload(filePath, callbackData, m_ModData[idx])) | |||||
{ | |||||
Not Done Inline ActionsWhy isn't this done immediately after the if above? As far as I can see stillRunning is never set after the curl_multi_perform call. leper: Why isn't this done immediately after the `if` above? As far as I can see `stillRunning` is… | |||||
Not Done Inline ActionsI'm not sure whether all sources of curl easy errors also decrement the multi's number of running handles. If there is an error that doesn't bring stillRunning to 0 for some reason, the error will never be processed/displayed and the function will keep returning false. Itms: I'm not sure whether **all** sources of curl easy errors also decrement the multi's number of… | |||||
LOGERROR("File %s failed to verify.", filePath.string8()); | |||||
// delete the file again as it does not match | |||||
if (wunlink(filePath) != 0) | |||||
LOGERROR("Failed to delete file."); | |||||
return; | |||||
} | |||||
LOGERROR("downloaded something."); | |||||
// TODO: hook into .pyromod code to do the win32 specific thing because win32... | |||||
// everything else should already work, given that we placed the file in the correct location already. | |||||
} | |||||
static bool VerifyDownload(const OsPath& UNUSED(filePath), DownloadCallbackData& callbackData, const std::map<std::string, std::string>& modData) | |||||
{ | |||||
// TODO: Should we also check that the filesize is what was returned by the API? | |||||
// This seems slightly useless, given that the filesize is already contained in the hash. | |||||
// However we currently only have md5 which is easy to collide... | |||||
u8 digest[MD5::DIGESTSIZE]; | |||||
callbackData.md5.Final(digest); | |||||
std::stringstream md5digest; | |||||
md5digest << std::hex << std::setfill('0'); | |||||
for (size_t i = 0; i < MD5::DIGESTSIZE; ++i) | |||||
md5digest << std::setw(2) << (int)digest[i]; | |||||
if (modData.at("filehash_md5") != md5digest.str()) | |||||
{ | |||||
LOGERROR("Invalid file. Expected md5 %s, got %s.", modData.at("filehash_md5").c_str(), md5digest.str()); | |||||
return false; | |||||
} | |||||
Done Inline ActionsWhy would it? To reduce confusion about this question: What is one of the very few atomic operations on a file system? leper: Why would it?
To reduce confusion about this question: What is one of the very few atomic… | |||||
Not Done Inline ActionsSorry it used to fail before the pyromod patch was correctly hooked. Deleted the comment. Itms: Sorry it used to fail before the pyromod patch was correctly hooked. Deleted the comment. | |||||
// TODO XXX: Get upstream to actually have proper hashes that are collision-resistant. | |||||
Done Inline ActionsMoving. Sorry about the suspense, but well, you seemed like you needed some. leper: Moving.
Sorry about the suspense, but well, you seemed like you needed some. | |||||
// (someone might want to fix the hashes we provide for our downloads) | |||||
// (that someone might also want to add those hashes in other places than right next to the downloads) | |||||
// XXX check signature, this requires upstream to allow adding that (unless we shove it into the metadata blob) (which would be ugly but doable) | |||||
// TODO decide on what to use. | |||||
// Options: | |||||
Done Inline ActionsAnd this right here seems like a spot where one could wonder why there is no default here. leper: And this right here seems like a spot where one could wonder why there is no `default` here. | |||||
// * gpgme | |||||
// - seems quite unwieldy to use | |||||
// - would allow key revocation | |||||
// - could use the different trust levels to allow some trusted people to sign things | |||||
// * signify (possibly asignify, since that can be used as a library) | |||||
// - seems quite simple to use | |||||
// - shorter keys | |||||
// - simple command line tool (harder to mess things up) | |||||
Done Inline ActionsThis might be shorter if written as auto bar = foo(baz); baz.clear(); if (bar) return; leper: This might be shorter if written as
```auto bar = foo(baz); baz.clear(); if (bar) return;``` | |||||
// Having signatures that _we_ create for the files one can retrieve from the API is | |||||
// a requirement (XXX). Else the API could just feed the users whatever in case of a compromise. | |||||
// We need signatures so we do not have to trust the API to provide files we actually want | |||||
// to be distributed. This means mods checked by us to be non-malicious only. | |||||
// If things are signed a compromised API can just continue to feed old files that might have | |||||
// issues that nobody noticed when we signed the files. | |||||
Done Inline ActionsThe original patch had a nice sequence of checks (if !foo fail; if !bar fail; if !valid fail; everything correct, do baz), that seems completely lost here. leper: The original patch had a nice sequence of checks (if !foo fail; if !bar fail; if !valid fail… | |||||
// Note that this does not prevent all possible attacks a package manager/update system should | |||||
// defend against. This is not an update system, however other possible attack vectors should | |||||
Not Done Inline ActionsThe progress bar would be great for a file download. If asynchronously is a PITA, a GUI popup would work, with a mandatory abort button. Itms: The progress bar would be great for a file download. If asynchronously is a PITA, a GUI popup… | |||||
// be evalutated if they apply and how to fix those. (See https://github.com/theupdateframework/specification/blob/master/tuf-spec.md for some ideas.) | |||||
return true; | |||||
Done Inline ActionsSo now we need double the free space, because placing the file in nearly the right place would be too simple? The original code meant to use the special OS handling code only. leper: So now we need double the free space, because placing the file in nearly the right place would… | |||||
} | |||||
// TODO: nuke this at some point | |||||
void ModIo::PrintMods() | |||||
{ | |||||
printf("[\n"); | |||||
for (const auto& a : m_ModData) | |||||
{ | |||||
printf(" {\n"); | |||||
for (const auto& b : a) | |||||
{ | |||||
printf(" \"%s\": \"%s\",\n", b.first.c_str(), b.second.c_str()); | |||||
} | |||||
printf(" },\n"); | |||||
} | |||||
Done Inline ActionsThis would do well with a comment. leper: This would do well with a comment. | |||||
printf("]\n"); | |||||
} | |||||
Not Done Inline ActionsMaybe it should go to the same _modcache folder used in the pyromod patch. Itms: Maybe it should go to the same `_modcache` folder used in the pyromod patch. | |||||
Not Done Inline ActionsMaybe, though I'm not that certain where that one is, or if that location makes a lot of sense. Part of the reason for placing it nearly where it should be is that it is most likely going to be on the right filesystem already, so a move to the right name is going to be fast. (Apart from being atomic.) leper: Maybe, though I'm not that certain where that one is, or if that location makes a lot of sense. | |||||
Done Inline ActionsEverything else does work :) Itms: Everything else does work :) | |||||
Done Inline ActionsThis document is fascinating, thanks for the link. My suggestion for the better location is with the top-level comment, which might be moved to the header file. Itms: This document is fascinating, thanks for the link. My suggestion for the better location is… | |||||
Done Inline ActionsI think it could be moved there, but this specific function should retain at least some explanation of what is done and why we do it. leper: I think it could be moved there, but this specific function should retain at least some… | |||||
Done Inline ActionsDo you really think that this will be fixed at some point in time? leper: Do you really think that this will be fixed at some point in time? | |||||
Not Done Inline ActionsI created #5128. Itms: I created #5128. | |||||
Not Done Inline ActionsMaybe just resizeing modData would be a bit nicer. leper: Maybe just `resize`ing `modData` would be a bit nicer. | |||||
Not Done Inline ActionsIt might be possible to enforce that this field is set, or if it isn't have it default to the mod.io url of that mod. The latter might require upstream to help out with this. leper: It might be possible to enforce that this field is set, or if it isn't have it default to the… | |||||
Done Inline Actionsmetadata_blob would possibly be a bit more explicit. leper: `metadata_blob` would possibly be a bit more explicit. | |||||
Done Inline ActionsSame as above. leper: Same as above. | |||||
Done Inline ActionsThe resize thing above would allow using FAIL here, which might make this easier to understand. leper: The `resize` thing above would allow using `FAIL` here, which might make this easier to… | |||||
Done Inline ActionsWe shouldn't FAIL here, since we need to check the other mods, right? If I am I don't see the difference between emplace_back and resize. Itms: We shouldn't `FAIL` here, since we need to check the other mods, right? If I am I don't see the… | |||||
Done Inline ActionsRight, ignore that one. leper: Right, ignore that one. | |||||
Done Inline ActionsWhile this is indeed a TODO that starts with TODO: it should really be fixed before this is comitted. Checking a few chars against something known shouldn't be that complicated. leper: While this is indeed a `TODO` that starts with `TODO:` it should really be fixed before this is… | |||||
Not Done Inline ActionsThis could possibly be changed to if (ret) m_GameId = ...; return ret;, but I have changed that code a few times already, and I'm still not quite happy with it. leper: This could possibly be changed to `if (ret) m_GameId = ...; return ret;`, but I have changed… | |||||
Done Inline ActionsAn invalid filesize is most likely only a negative value. Incorrect or wrong seem to be better choices. leper: An invalid filesize is most likely only a negative value. Incorrect or wrong seem to be better… | |||||
Done Inline ActionsHm, maybe mismatch would be another option. leper: Hm, maybe mismatch would be another option. | |||||
Done Inline ActionsFor this I suppose not indenting it is nicer so it is obvious it is related to the define. leper: For this I suppose not indenting it is nicer so it is obvious it is related to the define. | |||||
Not Done Inline ActionsI'd have used starts_with here, which isn't in a standard that can be relied on yet, but we do use the boost version of that somewhere already. leper: I'd have used `starts_with` here, which isn't in a standard that can be relied on yet, but we… | |||||
Done Inline ActionsDoing this in that order (that is before the continue below) could possibly cause issues if another signing algorithm is supported by some future version, and old clients could thus error out. Swapping this with the keynum check would allow working around that issue by having a new key (which new releases should do anyway) for the new version that would also have a different keynum. leper: Doing this in that order (that is before the continue below) could possibly cause issues if… | |||||
Not Done Inline ActionsThis err is not supposed to be the same as the ParseSignature one, since FAILing in ParseSignature is not supposed to make ParseModsResponse fail. So I removed propagation (the signature parsing error is actually only used in tests). Itms: This `err` is not supposed to be the same as the `ParseSignature` one, since `FAIL`ing in… |
I feel like, as it stands now, it's short enough that people would just rewrite it, and it's also somewhat coupled with 0 A.D.'s engine so MIT-ing this might be a "lie" to an extent.
But that's not a very strong feeling.