Page MenuHomeWildfire Games

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

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

Details

Reviewers
Itms
Trac Tickets
#5459
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.

This patch is silently ignoring broken mods, it does not display them, but keeps displaying signed mods.
That means one cannot break mod io downloader with mods having invalid data.

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
SeverityLocationCodeMessage
Auto-Fixsource\ps\tests\test_ModIo.h:112TXT6Trailing Whitespace
Unit
No Unit Test Coverage
Build Status
Buildable 8557
Build 14005: Vulcan BuildJenkins
Build 14004: arc lint + arc unit

Event Timeline

Angen 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

Angen 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?

Angen 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

Angen added a comment.Jul 23 2019, 5:19 PM
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)

Angen 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.

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

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

562

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.