Skip to content

Remove unnecessary eval from WebUI settings cleanup - #611

Closed
maghuro wants to merge 1 commit into
ExtremeFiretop:devfrom
maghuro:security/remove-settings-eval
Closed

maghuro wants to merge 1 commit into
ExtremeFiretop:devfrom
maghuro:security/remove-settings-eval

Conversation

@maghuro

@maghuro maghuro commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Remove an unnecessary use of eval from the WebUI custom-settings cleanup.

Problem

LoadCustomSettings() iterates over the properties of shared_custom_settings and removes old MerlinAU entries using dynamically generated JavaScript:

eval('delete shared_custom_settings.' + prop);

The property name is already available directly as prop, so dynamic code evaluation is unnecessary.

Using eval here also makes property handling less robust because dot notation requires the generated property name to be valid JavaScript identifier syntax.

Fix

Use normal bracket property access:

delete shared_custom_settings[prop];

This deletes exactly the property currently being processed by the loop without constructing or evaluating JavaScript source code.

Behavior

The existing filtering logic remains unchanged.

Properties are removed when:

  • their name contains MerlinAU
  • their name does not contain MerlinAU_version_

Version properties continue to be preserved.

Benefits

  • removes unnecessary dynamic code execution
  • handles arbitrary property names safely
  • preserves the existing cleanup behavior
  • simplifies the code

Scope

This PR changes only one statement in LoadCustomSettings().

No settings loading, saving, filtering, version handling, or firmware-update behavior is otherwise changed.

@maghuro
maghuro force-pushed the security/remove-settings-eval branch from b45667d to 6e529bf Compare September 24, 2026 00:49
@ExtremeFiretop

ExtremeFiretop commented Sep 24, 2026 •

Copy link
Copy Markdown
Owner

Hey @maghuro

Thanks for the PR. While I can probably agree that the bracket notation is cleaner, after reviewing the code a bit closer, I don't think this is actually fixing any issue in MerlinAU.

prop is coming from the router-generated shared_custom_settings object, and the properties this cleanup is intended to process are MerlinAU settings such as MerlinAU_*.

Those setting names are controlled by MerlinAU and use valid JavaScript identifier syntax, so the existing dot-notation expression works for the properties we actually create and support.

I also couldn't identify a realistic path where an attacker-controlled property name reaches this code and causes the current eval() to behave incorrectly.

So while:

delete shared_custom_settings[prop];

Might be arguably cleaner looking, this looks mostly like a code-style/refactoring change rather than a bug fix or security fix.

I'd prefer not to change working code purely for cleanup though. Unless there's a error/failure/edge case that the current MerlinAU code cannot handle around it. The project is so mature at this point changing things just to change them is generally "unnecessary" itself ;)

Thanks again for taking the time to look through the code though!
Moving on to the next PR, I am starting with the low hanging fruit, but I do think you might be on to something with other PRs. Will review more after my dinner.

@maghuro
maghuro deleted the security/remove-settings-eval branch September 27, 2026 00:35
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