Page MenuHomeWildfire Games

Do not fail if some mod is broken [Mod Io]
ClosedPublic

Authored by Silier on Jul 23 2019, 3:51 PM.

Details

Summary

If one mod is not signed or broken in any other way, mod.io downloader fails and does not display any mod.
The problem with unsigned mod is that its metadata are empty.

That means one cannot break mod io downloader with mods having invalid data.

mark mod as invalid and display in list as disabled and display reason instead description to not spam ugly error messages on screen as this is not error by the game but of the moder
report failed signatures back to list of mods
fail if property is not set using strict mode when getting from js
check in js for undefined values

Allow to filter only valid mods.

Test Plan

Mark unsigned mod as primary release at mod io page and try to download mods in game.
You should see other signed mods, but not the one which is not signed.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8553
Build 13997: Vulcan BuildJenkins
Build 13996: arc lint + arc unit

Event Timeline

Silier created this revision.Jul 23 2019, 3:51 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/204/display/redirect

Silier updated this revision to Diff 9082.Jul 23 2019, 4:01 PM

also remove test assert as we ignore mod silently

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/tests/test_ModIo.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/tests/test_ModIo.h
|  31| class·TestModIo·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestModIo:' is invalid C code. Use --std or --language to configure the language.

source/ps/ModIo.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/205/display/redirect

elexis added a subscriber: elexis.Jul 23 2019, 4:29 PM

Would it be cleaner to only push valid elements rather than adding all elements and removing the invalid?

Are there reasons to inform the users that a mod was not signed?
Is it considered a mistake if such an element appears, and if it is, would it be reasonable to inform the user so as to have it reported to the mod signers?

Silier updated this revision to Diff 9083.Jul 23 2019, 4:34 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/tests/test_ModIo.h
|  31| class·TestModIo·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestModIo:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/206/display/redirect

In D2114#88408, @elexis wrote:

Would it be cleaner to only push valid elements rather than adding all elements and removing the invalid?

I think it would.

In D2114#88408, @elexis wrote:

Are there reasons to inform the users that a mod was not signed?

At first: Users should not be able to download not signed mods or mods with outdated signature.
And they cannot do much about it, so spamming them with the messages about all unsigned mods would not be kind.

In D2114#88408, @elexis wrote:

Is it considered a mistake if such an element appears, and if it is, would it be reasonable to inform the user so as to have it reported to the mod signers?

If you mean that there is no information about signature, what means there is no object, it is error, because that metadata contains information also about dependencies.
But in the first place ingame downloander permits users to downloand not - signed mods, because that means that mod is not trusted.

From ModIo.h

We do not want a malicious mod to use this to download arbitrary files, nor do we want the API to make us download something we have not verified.

Reasonable would be to inform mod creator, that he provided unsigned version of that mod.

elexis added a comment.EditedJul 23 2019, 5:24 PM

I didn't propose to allow download of malicious mods but I asked whether there should be some feedback channel from the user to the mod signer or mod author through the UI of everyone using the service without impacting the service negatively (spam / noise) (for example a LOGERROR or unusable mock item)

Silier planned changes to this revision.Jul 23 2019, 6:33 PM

I think we should never fail in case some mod has wrong data anyway, unless it is fail of whole mod io.
After that, I try to come with some not downlandable mod line for mods with wrong data.

Silier updated this revision to Diff 9085.Jul 23 2019, 8:28 PM
Silier retitled this revision from Do not fail if one mod is not signed to Do not fail if some mod is broken [Mod Io].
Silier edited the summary of this revision. (Show Details)
Silier planned changes to this revision.Jul 23 2019, 8:32 PM
Silier added inline comments.
source/ps/ModIo.cpp
446

ehm, this now displays error if we call FAIL2, but does not display errors if some mods are wrong

561

collect errors of all broken mods.
TODO: make it useful, not just tests

source/ps/tests/test_ModIo.h
152

return this assert

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/ModIo.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/ModIo.h
|  49| »   std::map<std::string,·std::string>·properties;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'std::map' is invalid C code. Use --std or --language to configure the language.

source/ps/tests/test_ModIo.h
|  31| class·TestModIo·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestModIo:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/208/display/redirect

Itms edited reviewers, added: Itms; removed: Restricted Owners Package.Aug 30 2019, 5:01 PM
Itms added a subscriber: Itms.

Hello! I had not noticed the patch when you created it ? I think it's a good idea.

Some background: on the mod.io interface, modders cannot fill the metadata_blob entry, it has to be provided by me. I asked the mod.io devs to prevent the user to enable the mod and make it public in responses to requests as long as the metadata is not provided, but their checks are not that hard and modders always end up finding loopholes by mistake.

I think it should be enough to stop assuming that metadata_blob is valid, and just skip the mod if that blob is not what we expect (just like we skip the mod if the signature is wrong).

As far as I know, this is the only thing that breaks the mod.io client. I'm open to the other improvements that you propose but if you want to make it simple, you can reduce the patch to just a better metadata handling.

Silier updated this revision to Diff 12442.Jun 23 2020, 10:42 PM

rebase
mark mod as invalid and display in list as disabled and display reason instead description to not spam ugly error messages on screen as this is not error by the game but of the moder
report failed signatures back to list of mods
fail if property is not set using strict mode when getting from js
check in js for undefined values

Todo (?):
Filter in/valid mods

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1982/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2515/display/redirect

Silier added inline comments.Jun 24 2020, 7:42 AM
source/ps/tests/test_ModIo.h
128

So it looks like it is ok to get conversion warnings instead failing ?
@Itms

Silier planned changes to this revision.Jun 24 2020, 7:42 AM

fix tests one or another way

Silier added inline comments.Jun 24 2020, 7:44 AM
source/ps/tests/test_ModIo.h
128

now it would be:
Failed to get name from el.

Error: Expected (std::string(err[0]) == std::string("Failed to get name_id from el.")), found ("Failed to get name from el." != "Failed to get name_id from el.")

Silier added inline comments.Jun 24 2020, 7:51 AM
source/ps/ModIo.cpp
675

Design question: now we do not really need this one if we do not want to log that errors in mainlog as message what looks redundant when it is displayed in mod io downloander now.
What means it can be returned to one string as parameter.

source/ps/tests/test_ModIo.h
110

Note to myself: now we return all mods but marked as invalid

131

Note to myself: now we return all mods but marked as invalid

Itms added inline comments.Jun 26 2020, 12:34 PM
source/ps/ModIo.cpp
560

Maybe call them FAIL_MULTI and WARN_MULTI, or at least WARN2 instead of WARN for grouping them.

689

The new macros don't need a semicolon

Silier updated this revision to Diff 12463.Jun 26 2020, 9:18 PM
Silier edited the summary of this revision. (Show Details)

filter
fix tests
fix naming
remove redundant macros

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/tests/test_ModIo.h
|  31| class·TestModIo·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestModIo:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/mod/gui/modio/modio.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/gui/modio/modio.js
| 206| 206| 
| 207| 207| function isSelectedModInvalid(selected)
| 208| 208| {
| 209|    |-	return selected !== undefined && !!g_ModsAvailableOnline[selected].invalid && g_ModsAvailableOnline[selected].invalid == "true"
|    | 209|+	return selected !== undefined && !!g_ModsAvailableOnline[selected].invalid && g_ModsAvailableOnline[selected].invalid == "true";
| 210| 210| }
| 211| 211| 
| 212| 212| function showModDescription()

binaries/data/mods/mod/gui/modio/modio.js
| 209| »   return·selected·!==·undefined·&&·!!g_ModsAvailableOnline[selected].invalid·&&·g_ModsAvailableOnline[selected].invalid·==·"true"
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2530/display/redirect

Silier planned changes to this revision.Jun 26 2020, 10:14 PM
Silier added inline comments.
binaries/data/mods/mod/gui/modio/modio.js
209 ↗(On Diff #12463)

;

Itms requested changes to this revision.Jul 4 2020, 7:09 PM

Really like the patch and the new UX with broken mods, it's a big improvement ??

One big bug, though, is that the Download button doesn't work when it's enabled... And I think the scroll bars of the mod list are also broken.

binaries/data/mods/mod/gui/modio/modio.js
219 ↗(On Diff #12463)

I suggest displaying something like "Invalid mod: "+error, with "Invalid mod" marked for translation.

Silier updated this revision to Diff 12560.Jul 5 2020, 9:47 AM

blame copy paste :)

Silier updated this revision to Diff 12561.Jul 5 2020, 10:26 AM

fix translation

Itms accepted this revision.Jul 5 2020, 10:38 AM

Looks good to me! I will look into fixing the issues you pointed out in your audit of the original commit.

This revision is now accepted and ready to land.Jul 5 2020, 10:38 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/tests/test_ModIo.h
|  31| class·TestModIo·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestModIo:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2585/display/redirect

Stan added a subscriber: Stan.Jul 5 2020, 11:03 AM
Stan added inline comments.
binaries/data/mods/mod/gui/modio/modio.js
209 ↗(On Diff #12463)

There is a lot of negation here maybe you could invert the whole thing instead of checking if the mod is valid check if it is invalid?

binaries/data/mods/mod/gui/modio/modio.xml
68 ↗(On Diff #12561)

Nuke?

source/ps/ModIo.cpp
452–453

Nuke here and below?

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/tests/test_ModIo.h
|  31| class·TestModIo·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestModIo:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /zpool0/trunk/binaries/data/mods/mod/gui/modio/modio.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/gui/modio/modio.js
| 216| 216| 	let isInvalid = isSelectedModInvalid(selected);
| 217| 217| 	Engine.GetGUIObjectByName("downloadButton").enabled = isSelected && !isInvalid;
| 218| 218| 	Engine.GetGUIObjectByName("modDescription").caption = isSelected && !isInvalid ? g_ModsAvailableOnline[selected].summary : "";
| 219|    |-	Engine.GetGUIObjectByName("modError").caption = isSelected && isInvalid ? sprintf(translate("Invalid mod: %(error)s"), {"error": g_ModsAvailableOnline[selected].error }) : "";
|    | 219|+	Engine.GetGUIObjectByName("modError").caption = isSelected && isInvalid ? sprintf(translate("Invalid mod: %(error)s"), { "error": g_ModsAvailableOnline[selected].error }) : "";
| 220| 220| }
| 221| 221| 
| 222| 222| function cancelModListUpdate()
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2586/display/redirect

Silier added inline comments.Jul 5 2020, 2:19 PM
binaries/data/mods/mod/gui/modio/modio.js
209 ↗(On Diff #12463)

? I check if mod is invalid
there is question by design if no selected mod is valid or not

Stan added inline comments.Jul 5 2020, 3:47 PM
binaries/data/mods/mod/gui/modio/modio.js
209 ↗(On Diff #12463)

Could be

selected === undefined || !g_ModsAvailableOnline[selected].invalid || g_ModsAvailableOnline[selected].invalid == "false";
Silier added inline comments.Jul 10 2020, 3:24 PM
binaries/data/mods/mod/gui/modio/modio.js
209 ↗(On Diff #12463)

maybe, but then I would need to negate it at L257.
and invalid is set to "true" or not set so checking against "false" is doing nothing.