Skip to content

Conversation

KoloInDaCrib
Copy link

@KoloInDaCrib KoloInDaCrib commented May 29, 2025

Would close issues such as FunkinCrew/Funkin#4995

Description

This PR adds two new functions to Polymod, blacklistStaticFields and blacklistInstanceFields. These functions allow for specific variables and functions of the parent class to be blacklisted from usage in scripts. Blacklisted fields cannot be gotten (and thus called) or set.

This PR would also remove the need to fully blacklist classes if they have a few potentially malicious fields.

Examples

Static Field Blacklisting

If in your project you were to use:

Polymod.blacklistStaticFields(flixel.util.FlxSave, ["resolveFlixelClasses"]);

The function resolveFlixelClasses would be blacklisted from being used in scripts to access malicious packages.

Instance Field Blacklisting

If in your project you were to use:

Polymod.blacklistInstanceFields(flixel.util.FlxSave, ["data"]);

The field data would be blacklisted from bein used. This means that using FlxG.save.data or new FlxSave().data would result in an error.

@EliteMasterEric
Copy link
Collaborator

There's a reason I didn't add this earlier. You just added a bunch of checks to EVERY instance field query, which is done DOZENS of times on the average script. We will have to get good data on the potential performance impacts of this change especially on scripts that run in onUpdate.

@KoloInDaCrib
Copy link
Author

There's a reason I didn't add this earlier. You just added a bunch of checks to EVERY instance field query, which is done DOZENS of times on the average script. We will have to get good data on the potential performance impacts of this change especially on scripts that run in onUpdate.

I have modified the check to only use Type.typeof when checking for instance fields, hopefully it doesn't impact performance as much as Type.getClassName and Type.getClass. If you have any other complications or have found a solution internally, feel free to close this PR.

Copy link
Collaborator

@EliteMasterEric EliteMasterEric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I'm still a little worried about the performance impact but I don't think it'll be that big in the greater scheme of things. Maybe we can find and implement a fast map algorithm (or switch to checking for values in a flat array like ['FlxSave.resolveFlixelClasses', 'FlxSave.data'])

I did have one note for an obvious perf improvement.

@EliteMasterEric EliteMasterEric self-requested a review June 30, 2025 20:17
@KoloInDaCrib KoloInDaCrib force-pushed the tasteful-blacklist-mmm-yum branch from 2335e49 to 9aed087 Compare June 30, 2025 20:25
Copy link

@NotHyper-474 NotHyper-474 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the same be done to these too?

Comment on lines 581 to 878
for (cls => flds in PolymodScriptClass.blacklistedInstanceFields)
{
if (oCls == Std.string(cls) && flds.contains(f))
{
errorEx(EInvalidAccess(f));
return null;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here

Comment on lines 672 to 977
for (cls => flds in PolymodScriptClass.blacklistedInstanceFields)
{
if (oCls == Std.string(cls) && flds.contains(f))
{
errorEx(EInvalidAccess(f));
return null;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

@KoloInDaCrib
Copy link
Author

KoloInDaCrib commented Jun 30, 2025

Shouldn't the same be done to these too?

I don't think so because I'm changing the keys from Dynamic (Classes) to String so that they could compare with oCls (String).

@EliteMasterEric EliteMasterEric changed the base branch from develop to experimental July 2, 2025 00:23
@EliteMasterEric
Copy link
Collaborator

Git says there's still merge conflicts but it doesn't say what they are?

@EliteMasterEric EliteMasterEric force-pushed the tasteful-blacklist-mmm-yum branch from bca8f1b to 100508c Compare July 3, 2025 00:45
@EliteMasterEric
Copy link
Collaborator

Shouldn't the same be done to these too?

I don't think so because I'm changing the keys from Dynamic (Classes) to String so that they could compare with oCls (String).

The field keys should probably be Strings to begin with for the purposes of optimization, but we can change that later.

@EliteMasterEric
Copy link
Collaborator

EliteMasterEric commented Jul 3, 2025

Just pushed a commit which improves the error handling (calling errorEx() allows errors to be caught and handled, while Polymod.error() prevents that, and also allows specifying that the error was specifically a blacklist violation).

However, I'm currently experiencing an issue where Type.typeof(FlxColor) is TObject and I can't get the proper string name for it for the error message. It currently says "TObject.resolveFlixelClasses" is not valid rather than "FlxSave.resolveFlixelClasses".

Copy link
Collaborator

@EliteMasterEric EliteMasterEric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either you or I should figure out how to properly resolve oCls for error reporting before merging.

@EliteMasterEric
Copy link
Collaborator

As a reminder, the only issue with this PR is that the error message reads Class field TObject.resolveFlixelClasses is blacklisted and cannot be used in scripts rather than stating the classname that caused the problem.

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.

3 participants