Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(scanallclasses): create a scan all classes boolean #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ThatGamerBlue
Copy link
Contributor

Not perf tested, but (empirically) doing something similar to this in another project isn't noticably slower.
By default only allows classes globally allowed in the config. Not functional under modlauncher, couldn't see a way to get the transformer to indiscriminately apply to every class loaded.

Wasn't sure if I should've added filtering to not process java.* classes, but theoretically these are exploitable so I decided to leave them filtered, might be a bad choice.

@ThatGamerBlue ThatGamerBlue marked this pull request as draft July 29, 2023 23:40
Not perf tested, shouldn't be too slow though.
By default only allows classes globally allowed in the config.
Not functional under modlauncher, couldn't see a way to get the transformer to indiscriminately apply to every class loaded.

Wasn't sure if I should've added filtering to not process java.* classes, but theoretically these are exploitable so I decided to leave them filtered, might be a bad choice.
@ThatGamerBlue
Copy link
Contributor Author

Not sure if this might be worthy of a rewrite w.r.t #10 to allow multiple PatchModules to target the same class, so we could have multiple patchmodules patching the SerializationUtils, grouped in the file by what mod adds the requirement for organization reasons eg:

        {
            "modName": "CustomOreGen",
            "classesToPatch": [
                "org.apache.commons.lang3.SerializationUtils"
            ],
            "classAllowlist": [
                "CustomOreGen.GeometryData"
            ],
            "packageAllowlist": []
        },
        {
            "modName": "SomeOtherModThatUsesTheSameClass",
            "classesToPatch": [
                "org.apache.commons.lang3.SerializationUtils"
            ],
            "classAllowlist": [
                "com.othermod.packet.something.UnsafelyDeserializedType"
            ],
            "packageAllowlist": []
        }

@dogboy21
Copy link
Owner

I would like to keep the config stable enough so that we don't break any older version of the patcher or that we need to maintain new versions of the config in this repo.

But I think one approach to that problem could be the merging of PatchModules once one is requested for a specific class. So basically searching all PatchModules that have the class to patch in classesToPatch and then just merging classAllowList and packageAllowList across all of them.

@ThatGamerBlue
Copy link
Contributor Author

I think no matter what we do we're gonna be struggling with backwards compatibility, unless we just have one big entry in the config that contains every mod that uses the SerializationUtils, given older versions will just find the first one in the list and only allow those classes through, ignoring the rest.

@dogboy21
Copy link
Owner

Hmm yeah true, totally forgot about that part :/

@ThatGamerBlue
Copy link
Contributor Author

I think for now this PR should be fine as a stop-gap until we find most of the mods with issues, I don't think there's gonna be a way to have both the nicely formatted config file and maintain full backwards-compatibility, and the backwards-compat is definitely the most important of the two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants