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. | |||||
*/ | |||||
#include "precompiled.h" | |||||
#include "ModIo.h" | |||||
#include "lib/file/file_system.h" | |||||
#include "lib/sysdep/sysdep.h" | |||||
#include "lib/sysdep/filesystem.h" | |||||
#include "maths/MD5.h" | |||||
#include "ps/CLogger.h" | |||||
Not Done Inline ActionsWhat the heck... leper: What the heck... | |||||
#include "ps/ConfigDB.h" | |||||
#include "ps/GameSetup/Paths.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 "ps/Mod.h" | |||||
#include "scriptinterface/ScriptConversions.h" | |||||
#include "scriptinterface/ScriptInterface.h" | |||||
#include <sodium.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. | |||||
#include <boost/algorithm/string/classification.hpp> | |||||
#include <boost/algorithm/string/split.hpp> | |||||
ModIo* g_ModIo = nullptr; | |||||
struct DownloadCallbackData { | |||||
DownloadCallbackData(FILE* _fp) | |||||
: fp(_fp), md5() | |||||
{ | |||||
crypto_generichash_init(&hash_state, NULL, 0U, crypto_generichash_BYTES_MAX); | |||||
} | |||||
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… | |||||
FILE* fp; | |||||
MD5 md5; | |||||
crypto_generichash_state hash_state; | |||||
}; | |||||
ModIo::ModIo() | |||||
: m_GamesRequest("/games") | |||||
{ | |||||
Done Inline ActionsI would see this in ModIo.h. Itms: I would see this in `ModIo.h`. | |||||
// 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. | |||||
// If another user of the engine wants to provide different values here, | |||||
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. | |||||
// while still using the same engine version, they can just provide some shortcut/script | |||||
// that sets these using command line parameters. | |||||
std::string pk_str; | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.public_key", pk_str); | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.baseurl", m_BaseUrl); | |||||
{ | |||||
std::string api_key; | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.api_key", api_key); | |||||
m_ApiKey = "api_key=" + api_key; | |||||
} | |||||
{ | |||||
std::string nameid; | |||||
g_ConfigDB.GetValue(CFG_SYSTEM, "modio.v1.name_id", nameid); | |||||
m_IdQuery = "name_id="+nameid; | |||||
} | |||||
// TODO we should only do this in one single place (same for shutdown, currently we do it here and in the userreporter) | |||||
{ | |||||
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… | |||||
// Initialise everything except Win32 sockets (because our networking | |||||
// system already inits those) | |||||
curl_global_init(CURL_GLOBAL_ALL & ~CURL_GLOBAL_WIN32); | |||||
} | |||||
m_Curl = curl_easy_init(); | |||||
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… | |||||
ENSURE(m_Curl); | |||||
// Capture error messages | |||||
curl_easy_setopt(m_Curl, CURLOPT_ERRORBUFFER, m_ErrorBuffer); | |||||
// Fail if the server did | |||||
curl_easy_setopt(m_Curl, CURLOPT_FAILONERROR, 1L); | |||||
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… | |||||
// Disable signal handlers (required for multithreaded applications) | |||||
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. | |||||
curl_easy_setopt(m_Curl, CURLOPT_NOSIGNAL, 1L); | |||||
// To minimise security risks, don't support redirects | |||||
curl_easy_setopt(m_Curl, CURLOPT_FOLLOWLOCATION, 0L); | |||||
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… | |||||
m_Headers = NULL; | |||||
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); | |||||
if (sodium_init() < 0) { | |||||
LOGERROR("Failed to initialize libsodium"); | |||||
ENSURE(0 && "sodium_init returned success."); | |||||
} | |||||
size_t bin_len = 0; | |||||
if (sodium_base642bin((unsigned char*)&m_pk, sizeof m_pk, pk_str.c_str(), pk_str.size(), NULL, &bin_len, NULL, sodium_base64_VARIANT_ORIGINAL) != 0 || bin_len != sizeof m_pk) | |||||
{ | |||||
LOGERROR("failed to decode base64 public key"); | |||||
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. | |||||
ENSURE(0 && "invalid public key"); | |||||
} | |||||
} | |||||
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. | |||||
ModIo::~ModIo() | |||||
{ | |||||
curl_slist_free_all(m_Headers); | |||||
curl_easy_cleanup(m_Curl); | |||||
curl_global_cleanup(); // TODO | |||||
} | |||||
size_t ModIo::ReceiveCallback(void* buffer, size_t size, size_t nmemb, void* userp) | |||||
{ | |||||
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. | |||||
ModIo* self = static_cast<ModIo*>(userp); | |||||
self->m_ResponseData += std::string((char*)buffer, (char*)buffer+size*nmemb); | |||||
return size*nmemb; | |||||
} | |||||
size_t ModIo::DownloadCallback(void* buffer, size_t size, size_t nmemb, void* userp) | |||||
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… | |||||
{ | |||||
DownloadCallbackData* data = static_cast<DownloadCallbackData*>(userp); | |||||
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, | |||||
// but we do not want to have a possibly valid hash in that case. | |||||
data->md5.Update((const u8*)buffer, len*size); | |||||
Done Inline Actions(bit meh, but code appears indeed better with it) elexis: (bit meh, but code appears indeed better with it) | |||||
crypto_generichash_update(&data->hash_state, (const u8*)buffer, len*size); | |||||
return len*size; | |||||
} | |||||
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… | |||||
CURLcode ModIo::PerformRequest(const std::string& url) | |||||
{ | |||||
m_ErrorBuffer[0] = '\0'; | |||||
curl_easy_setopt(m_Curl, CURLOPT_URL, url.c_str()); | |||||
return curl_easy_perform(m_Curl); | |||||
} | |||||
const std::vector<ModIoModData>& ModIo::GetMods(const ScriptInterface& scriptInterface) | |||||
{ | |||||
m_ModData.clear(); | |||||
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. | |||||
// 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()) | |||||
{ | |||||
CURLcode err = PerformRequest(m_BaseUrl+m_GamesRequest+"?"+m_ApiKey+"&"+m_IdQuery); | |||||
if (err != CURLE_OK) | |||||
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… | |||||
{ | |||||
LOGERROR("Failure while querying for game id. Server response: %s; %s", curl_easy_strerror(err), m_ErrorBuffer); | |||||
return m_ModData; | |||||
} | |||||
if (!ParseGameId(scriptInterface)) | |||||
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… | |||||
return m_ModData; | |||||
} | |||||
// Get the actual mods | |||||
CURLcode err = PerformRequest(m_BaseUrl+m_GamesRequest+m_GameId+"/mods?"+m_ApiKey); | |||||
if (err != CURLE_OK) | |||||
{ | |||||
LOGERROR("Failure while querying for mods. Server response: %s; %s", curl_easy_strerror(err), m_ErrorBuffer); | |||||
return m_ModData; | |||||
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. | |||||
} | |||||
if (!ParseMods(scriptInterface)) | |||||
m_ModData.clear(); // Failed during parsing, make sure we don't provide partial data | |||||
return m_ModData; | |||||
} | |||||
#define FAIL(...) STMT(LOGERROR(__VA_ARGS__); CLEANUP(); return false;) | |||||
/** | |||||
* Parses the current content of m_ResponseData to extract m_GameId. | |||||
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. | |||||
* | |||||
* 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. | |||||
*/ | |||||
bool ModIo::ParseGameIdResponse(const ScriptInterface& scriptInterface, const std::string& responseData, int& id) | |||||
{ | |||||
#define CLEANUP() id = -1; | |||||
JSContext* cx = scriptInterface.GetContext(); | |||||
JS::RootedValue gameResponse(cx); | |||||
if (!scriptInterface.ParseJSON(responseData, &gameResponse)) | |||||
FAIL("Failed to parse response as JSON."); | |||||
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 */`. | |||||
if (!gameResponse.isObject()) | |||||
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… | |||||
FAIL("response not an object"); | |||||
JS::RootedObject gameResponseObj(cx, gameResponse.toObjectOrNull()); | |||||
JS::RootedValue dataVal(cx); | |||||
if (!JS_GetProperty(cx, gameResponseObj, "data", &dataVal)) | |||||
FAIL("data property not in response."); | |||||
// [{"id": 42, ...}, ...] | |||||
if (!dataVal.isObject()) | |||||
FAIL("data property not an object"); | |||||
JS::RootedObject data(cx, dataVal.toObjectOrNull()); | |||||
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… | |||||
u32 length; | |||||
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; }`. | |||||
if (!JS_IsArrayObject(cx, data) || !JS_GetArrayLength(cx, data, &length) || !length) | |||||
FAIL("data property not an array with at least one element"); | |||||
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. | |||||
// {"id": 42, ...} | |||||
JS::RootedValue first(cx); | |||||
if (!JS_GetElement(cx, data, 0, &first)) | |||||
FAIL("couldn't get first element."); | |||||
id = -1; | |||||
// TODO: This check currently does not really do anything if "id" is present... | |||||
// meaning we can set id to be null and we get id=0. | |||||
// This should check for id != -1, but the conversion code is broken. | |||||
// There is a script value conversion check failed warning, but that does not change anything. | |||||
// TODO: We should probably make those fail (hard), if that isn't done by default (which it probably should) | |||||
// then we should add a templated variant that does. | |||||
// TODO check if id < 0 (<=?) and if yes also fail here | |||||
// NOTE: FromJSProperty does set things to probably 0 even if there is some stupid type conversion | |||||
// So we check for <= 0, so we actually get proper results here... | |||||
// Valid ids are always > 0. | |||||
if (!ScriptInterface::FromJSProperty(cx, first, "id", id) || id <= 0) | |||||
FAIL("couldn't get id"); | |||||
return true; | |||||
#undef CLEANUP | |||||
} | |||||
bool ModIo::ParseGameId(const ScriptInterface& scriptInterface) | |||||
{ | |||||
int id = -1; | |||||
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! | |||||
if (!ParseGameIdResponse(scriptInterface, m_ResponseData, id)) | |||||
{ | |||||
m_ResponseData.clear(); | |||||
return false; | |||||
} | |||||
m_ResponseData.clear(); | |||||
m_GameId = "/" + std::to_string(id); | |||||
return true; | |||||
} | |||||
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. | |||||
/** | |||||
* Parses the current content of m_ResponseData into m_ModData. | |||||
* | |||||
* The JSON data is expected to look like | |||||
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… | |||||
* { 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: { binary_url: "someurl", ... } }, ... }. | |||||
* Only the listed properties are of interest to consumers, and we flatten | |||||
* the modfile structure as that simplifies handling and there are no conflicts. | |||||
Done Inline Actionsif (error) { /* ... */ return; } leper: `if (error) { /* ... */ return; }` | |||||
*/ | |||||
bool ModIo::ParseModsResponse(const ScriptInterface& scriptInterface, const std::string& responseData, std::vector<ModIoModData>& modData, const PKStruct& pk) | |||||
{ | |||||
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… | |||||
// Make sure we don't end up passing partial results back | |||||
#define CLEANUP() modData.clear(); | |||||
JSContext* cx = scriptInterface.GetContext(); | |||||
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 modResponse(cx); | |||||
if (!scriptInterface.ParseJSON(responseData, &modResponse)) | |||||
FAIL("Failed to parse response as JSON."); | |||||
if (!modResponse.isObject()) | |||||
FAIL("response not an object"); | |||||
JS::RootedObject modResponseObj(cx, modResponse.toObjectOrNull()); | |||||
JS::RootedValue dataVal(cx); | |||||
if (!JS_GetProperty(cx, modResponseObj, "data", &dataVal)) | |||||
FAIL("data property not in response."); | |||||
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… | |||||
// [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) | |||||
Done Inline ActionsWhen do you imagine this ENSURE to not be satisfied? leper: When do you imagine this `ENSURE` to not be satisfied? | |||||
FAIL("data property not an array with at least one element"); | |||||
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… | |||||
modData.clear(); | |||||
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… | |||||
modData.reserve(length); | |||||
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"); | |||||
modData.emplace_back(); | |||||
#define COPY_STRINGS(prefix, obj, ...) \ | |||||
for (const std::string& prop : { __VA_ARGS__ }) \ | |||||
{ \ | |||||
std::string val; \ | |||||
if (!ScriptInterface::FromJSProperty(cx, obj, prop.c_str(), val)) \ | |||||
FAIL("failed to get %s from %s", prop, #obj);\ | |||||
modData.back().properties.emplace(prefix+prop, val); \ | |||||
} | |||||
// TODO: Currently the homepage field does not contain a non-null value for any entry. | |||||
COPY_STRINGS("", el, "name", "name_id", "summary"); | |||||
// Now copy over the modfile part, but without the pointless substructure | |||||
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`? | |||||
JS::RootedObject elObj(cx, el.toObjectOrNull()); | |||||
JS::RootedValue modFile(cx); | |||||
if (!JS_GetProperty(cx, elObj, "modfile", &modFile)) | |||||
FAIL("Failed to get modfile data"); | |||||
if (!modFile.isObject()) | |||||
FAIL("modfile not an object"); | |||||
COPY_STRINGS("", modFile, "version", "filesize"); | |||||
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… | |||||
JS::RootedObject modFileObj(cx, modFile.toObjectOrNull()); | |||||
JS::RootedValue filehash(cx); | |||||
if (!JS_GetProperty(cx, modFileObj, "filehash", &filehash)) | |||||
FAIL("Failed to get filehash data"); | |||||
COPY_STRINGS("filehash_", filehash, "md5"); | |||||
JS::RootedValue download(cx); | |||||
if (!JS_GetProperty(cx, modFileObj, "download", &download)) | |||||
FAIL("Failed to get download data"); | |||||
COPY_STRINGS("", download, "binary_url"); | |||||
// Parse metadata_blob (sig+deps) | |||||
std::string metadata_blob; | |||||
if (!ScriptInterface::FromJSProperty(cx, modFile, "metadata_blob", metadata_blob)) | |||||
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… | |||||
FAIL("failed to get metadata_blob from modFile"); | |||||
JS::RootedValue metadata(cx); | |||||
if (!scriptInterface.ParseJSON(metadata_blob, &metadata)) | |||||
FAIL("Failed to parse metadata_blob as JSON."); | |||||
if (!metadata.isObject()) | |||||
FAIL("metadata_blob not decoded as an object"); | |||||
if (!ScriptInterface::FromJSProperty(cx, metadata, "dependencies", modData.back().dependencies)) | |||||
FAIL("failed to get dependencies from metadata"); | |||||
std::vector<std::string> minisigs; | |||||
if (!ScriptInterface::FromJSProperty(cx, metadata, "minisigs", minisigs)) | |||||
FAIL("failed to get minisigs from metadata"); | |||||
// Remove this entry if we did not find a valid matching signature | |||||
if (!ParseSignature(minisigs, modData.back().sig, pk)) | |||||
modData.pop_back(); | |||||
#undef COPY_STRINGS | |||||
} | |||||
return true; | |||||
#undef CLEANUP | |||||
} | |||||
/** | |||||
* Parse signatures to find one that matches the public key, and has a valid global signature. | |||||
* Returns true and sets @param sig to the valid matching signature. | |||||
*/ | |||||
bool ModIo::ParseSignature(const std::vector<std::string>& minisigs, SigStruct& sig, const PKStruct& pk) | |||||
{ | |||||
#define CLEANUP() sig = {}; | |||||
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. | |||||
for (const std::string& file_sig : minisigs) | |||||
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. | |||||
{ | |||||
// Format of a .minisig file (created using minisign(1) with -SHm file.zip) | |||||
// untrusted comment: .*\nb64sign_of_file\ntrusted comment: .*\nb64sign_of_sign_of_file_and_trusted_comment | |||||
// TODO: Verify that both the untrusted comment and the trusted comment start with the correct prefix | |||||
std::vector<std::string> sig_lines; | |||||
boost::split(sig_lines, file_sig, boost::is_any_of("\n")); | |||||
if (sig_lines.size() < 4) | |||||
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. | |||||
FAIL("invalid (too short) sig"); | |||||
// We only _really_ care about the second line which is the signature of the file (b64-encoded) | |||||
// Also handling the other signature is nice, but not really required. | |||||
const std::string& msg_sig = sig_lines[1]; | |||||
size_t bin_len = 0; | |||||
if (sodium_base642bin((unsigned char*)&sig, sizeof sig, msg_sig.c_str(), msg_sig.size(), NULL, &bin_len, NULL, sodium_base64_VARIANT_ORIGINAL) != 0 || bin_len != sizeof sig) | |||||
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;``` | |||||
FAIL("failed to decode base64 sig"); | |||||
cassert(sizeof pk.keynum == sizeof sig.keynum); | |||||
if (memcmp(&sig.sig_alg, "ED", 2) != 0) | |||||
FAIL("only hashed minisign signatures are supported"); | |||||
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… | |||||
if (memcmp(&pk.keynum, &sig.keynum, sizeof sig.keynum) != 0) | |||||
continue; // mismatched key, try another one | |||||
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… | |||||
// Signature matches our public key | |||||
// Now verify the global signature (sig || trusted_comment) | |||||
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… | |||||
unsigned char global_sig[crypto_sign_BYTES]; | |||||
if (sodium_base642bin(global_sig, sizeof global_sig, sig_lines[3].c_str(), sig_lines[3].size(), NULL, &bin_len, NULL, sodium_base64_VARIANT_ORIGINAL) != 0 || bin_len != sizeof global_sig) | |||||
FAIL("failed to decode base64 global_sig"); | |||||
const std::string trusted_comment_prefix = "trusted comment: "; | |||||
if (sig_lines[2].size() < trusted_comment_prefix.size()) | |||||
FAIL("malformed trusted comment"); | |||||
const std::string trusted_comment = sig_lines[2].substr(trusted_comment_prefix.size()); | |||||
unsigned char* sig_and_trusted_comment = (unsigned char*)sodium_malloc((sizeof sig.sig) + trusted_comment.size()); | |||||
if (!sig_and_trusted_comment) | |||||
FAIL("sodium_malloc failed"); | |||||
Done Inline ActionsThis would do well with a comment. leper: This would do well with a comment. | |||||
memcpy(sig_and_trusted_comment, sig.sig, sizeof sig.sig); | |||||
memcpy(sig_and_trusted_comment + sizeof sig.sig, trusted_comment.data(), trusted_comment.size()); | |||||
if (crypto_sign_verify_detached(global_sig, sig_and_trusted_comment, (sizeof sig.sig) + trusted_comment.size(), pk.pk) != 0) | |||||
{ | |||||
LOGERROR("failed to verify global signature"); | |||||
sodium_free(sig_and_trusted_comment); | |||||
return false; | |||||
} | |||||
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… | |||||
sodium_free(sig_and_trusted_comment); | |||||
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. | |||||
// Valid global sig, and the keynum matches the real one | |||||
return true; | |||||
} | |||||
return false; | |||||
#undef CLEANUP | |||||
} | |||||
#undef FAIL | |||||
bool ModIo::ParseMods(const ScriptInterface& scriptInterface) | |||||
{ | |||||
bool ret = ParseModsResponse(scriptInterface, m_ResponseData, m_ModData, m_pk); | |||||
m_ResponseData.clear(); | |||||
return ret; | |||||
} | |||||
void ModIo::DownloadMod(size_t idx) | |||||
{ | |||||
if (idx >= m_ModData.size()) | |||||
return; | |||||
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. | |||||
// TODO: do this asynchronously? or at least tell the gui that we are doing something -> progress bar (or something spinning) | |||||
const Paths paths(g_args); | |||||
const OsPath modUserPath = paths.UserData()/"mods"; | |||||
const OsPath modPath = modUserPath/m_ModData[idx].properties["name_id"]; | |||||
if (!DirectoryExists(modPath) && INFO::OK != CreateDirectories(modPath, 0700, false)) | |||||
{ | |||||
LOGERROR("Could not create mod directory: %s", modPath.string8()); | |||||
return; | |||||
} | |||||
// Name the file after the name_id, since using the filename would mean that | |||||
// we could end up with multiple zip files in the folder that might not work | |||||
// as expected for a user (since a later version might remove some files | |||||
// that aren't compatible anymore with the engine version). | |||||
// So we ignore the filename provided by the API and assume that we do not | |||||
// care about handling update.zip files. If that is the case we would need | |||||
// a way to find out what files are required by the current one and which | |||||
// should be removed for everything to work. This seems to be too complicated | |||||
// so we just do not support that usage. | |||||
// NOTE: We do save the file under a slightly different name from the final | |||||
// one, to ensure that in case a download aborts and the file stays | |||||
// around, the game will not attempt to open the file which has not | |||||
// been verified. | |||||
// TODO: Add a shutdown hook to remove that file if a user requests shutdown. | |||||
// Which should remove this temporary file if the user terminates the game. | |||||
// This would require us to actually listen to anything during the download... | |||||
const OsPath filePath = modPath/(m_ModData[idx].properties["name_id"]+".zip.temp"); | |||||
DownloadCallbackData callbackData(sys_OpenFile(filePath, "wb")); | |||||
if (!callbackData.fp) | |||||
{ | |||||
LOGERROR("Could not open temporary file for mod download: %s", filePath.string8()); | |||||
return; | |||||
} | |||||
Done Inline ActionsEverything else does work :) Itms: Everything else does work :) | |||||
// 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", m_ModData[idx].properties["binary_url"].c_str()); | |||||
// 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); | |||||
// One redirect seems plenty for a CDN servint the files. | |||||
curl_easy_setopt(m_Curl, CURLOPT_MAXREDIRS, 1L); | |||||
// Download the file | |||||
CURLcode err = PerformRequest(m_ModData[idx].properties["binary_url"]); | |||||
fclose(callbackData.fp); | |||||
callbackData.fp = NULL; | |||||
// To minimise security risks, don't support redirects for queries | |||||
curl_easy_setopt(m_Curl, CURLOPT_FOLLOWLOCATION, 0L); | |||||
if (err != CURLE_OK) | |||||
{ | |||||
LOGERROR("Failure while downloading mod file. Server response: %s; %s", curl_easy_strerror(err), m_ErrorBuffer); | |||||
if (wunlink(filePath) != 0) | |||||
LOGERROR("Failed to delete file."); | |||||
return; | |||||
} | |||||
if (!VerifyDownload(filePath, callbackData, m_ModData[idx])) | |||||
{ | |||||
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."); | |||||
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. | |||||
return; | |||||
} | |||||
const OsPath finalFilePath = modPath/(m_ModData[idx].properties["name_id"]+".zip"); | |||||
if (wrename(filePath, finalFilePath) != 0) | |||||
{ | |||||
LOGERROR("failed to rename file."); | |||||
wunlink(filePath); | |||||
return; | |||||
} | |||||
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… | |||||
LOGERROR("downloaded something and verified it successfully."); | |||||
// TODO: hook into .pyromod code to do the win32 specific thing because win32... | |||||
} | |||||
bool ModIo::VerifyDownload(const OsPath& filePath, DownloadCallbackData& callbackData, const ModIoModData& modData) const | |||||
{ | |||||
{ | |||||
u64 filesize = std::stoull(modData.properties.at("filesize")); | |||||
if (filesize != FileSize(filePath)) | |||||
{ | |||||
LOGERROR("Invalid filesize."); | |||||
return false; | |||||
} | |||||
} | |||||
// MD5 (because upstream provides it) | |||||
// Just used to make sure there was no obvious corruption during transfer. | |||||
{ | |||||
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.properties.at("filehash_md5") != md5digest.str()) | |||||
{ | |||||
LOGERROR("Invalid file. Expected md5 %s, got %s.", modData.properties.at("filehash_md5").c_str(), md5digest.str()); | |||||
return false; | |||||
} | |||||
} | |||||
// Verify file signature. | |||||
// Used to make sure that the downloaded file was actually checked and signed | |||||
// by Wildfire Games. And has not been tampered with by the API provider, or the CDN. | |||||
unsigned char hash_fin[crypto_generichash_BYTES_MAX] = {}; | |||||
if (crypto_generichash_final(&callbackData.hash_state, hash_fin, sizeof hash_fin) != 0) | |||||
{ | |||||
LOGERROR("failed to compute final hash"); | |||||
return false; | |||||
} | |||||
if (crypto_sign_verify_detached(modData.sig.sig, hash_fin, sizeof hash_fin, m_pk.pk) != 0) | |||||
{ | |||||
LOGERROR("failed to verify signature"); | |||||
return false; | |||||
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. | |||||
} | |||||
return true; | |||||
} | |||||
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 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.