Skip to content

Use Map to maintain delegator environment overrides#33

Merged
jeffsmale90 merged 2 commits into
mainfrom
fix/overridesMap
Jul 15, 2025
Merged

Use Map to maintain delegator environment overrides#33
jeffsmale90 merged 2 commits into
mainfrom
fix/overridesMap

Conversation

@jeffsmale90

Copy link
Copy Markdown
Collaborator

📝 Description

Previously the delegation overrides were maintained on a simple object, mapping version, and chainId to the DeleGatorEnvironment. With this change, a Map<string, DeleGatorEnvironment> is used to maintain this information.

🔄 What Changed?

When overriding a delegator deployment, a key is generated from the version and chainId, which is used in the key of a Map to set the value to the overiden environment. When fetching the same util is used to generate the key, if the key exists in the map, then the value is returned.

🚀 Why?

This resolves the following vulnerability, where malicious input could potentially pollute the global object prototype https://github.com/MetaMask/delegation-toolkit/security/code-scanning/1

🧪 How to Test?

No functional changes - overriding deployed environments is covered in unit tests.

⚠️ Breaking Changes

List any breaking changes:

  • No breaking changes
  • Breaking changes (describe below):

📋 Checklist

Check off completed items:

  • Code follows the project's coding standards
  • Self-review completed
  • Documentation updated (if needed)
  • Tests added/updated
  • Changelog updated (if needed)
  • All CI checks pass

🔗 Related Issues

Link to related issues:
Closes # https://github.com/MetaMask/delegation-toolkit/security/code-scanning/1

@jeffsmale90 jeffsmale90 requested a review from a team as a code owner July 11, 2025 04:15
): DeleGatorEnvironment {
const overrideKey = getContractOverrideKey(chainId, version);

if (contractOverrideMap.has(overrideKey)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the has check necessary? get will return undefined if there is no key found so the if (overridenContracts) should take care of the check.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, good call! I've fixed this :)

@jeffsmale90 jeffsmale90 requested a review from MoMannn July 14, 2025 22:43
@jeffsmale90 jeffsmale90 merged commit fe9f8f1 into main Jul 15, 2025
16 checks passed
@jeffsmale90 jeffsmale90 deleted the fix/overridesMap branch July 15, 2025 18:41

@Rahul-cyber26 Rahul-cyber26 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

#37

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