Page MenuHomeWildfire Games

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

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

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
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.Tue, Jul 23, 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.Tue, Jul 23, 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.Tue, Jul 23, 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.Tue, Jul 23, 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.Tue, Jul 23, 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.EditedTue, Jul 23, 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.Tue, Jul 23, 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.Tue, Jul 23, 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.Tue, Jul 23, 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