Skip to content

Replace non-essential eval usage with explicit registries for static lookups (palettes + JS export)#5863

Closed
Chaitu7032 wants to merge 2 commits intosugarlabs:masterfrom
Chaitu7032:eval
Closed

Replace non-essential eval usage with explicit registries for static lookups (palettes + JS export)#5863
Chaitu7032 wants to merge 2 commits intosugarlabs:masterfrom
Chaitu7032:eval

Conversation

@Chaitu7032
Copy link
Contributor

@Chaitu7032 Chaitu7032 commented Feb 22, 2026

Why

This PR removes eval from a small number of locations where it was used only for static, name-based reflection (palette image constants and API action class enumeration/dispatch).

The change improves auditability, maintainability, and CSP readiness, while preserving existing runtime behavior and public APIs.

No dynamic or user-controlled code execution paths are modified.

Scope / Non-goals (explicit)

In scope

  • Removal of non-dynamic, non-user-controlled eval used for static lookups
  • Palette built-in image resolution
  • JS export API enumeration and dispatch

Out of scope / unchanged

  • Plugin execution or loading
  • User-authored code execution
  • Dynamic expressions
  • Third-party libraries (e.g., RequireJS / reqwest)
  • Other dynamic eval sites in the core runtime (e.g., Logo flow dictionaries)

What changed

1) Palette built-in image selection (remove static eval)

File: js/palette.js

Before

Built-in palette icons for media, camera, and video were resolved using:
eval(b.blkname + "PALETTE")

After

Introduces an explicit, lazy registry:

const BUILTIN_IMAGE_PALETTE_REGISTRY = { media: () => mediaPALETTE, camera: () => cameraPALETTE, video: () => videoPALETTE };

Palette rendering now:

  • Looks up BUILTIN_IMAGE_PALETTE_REGISTRY[b.blkname]
  • If present, calls the resolver and passes the result to makePaletteIcons(...)
  • Plugin image handling remains unchanged

Why this is safe

  • b.blkname is already constrained to ["media", "camera", "video"]
  • Lazy resolvers preserve script load-order assumptions (same reason eval worked previously)
  • No changes to plugin image paths or palette structure

2) JS export API enumeration & dispatch (replace reflective eval)

File: js/js-export/export.js

Before

  • API enumeration used:
    - eval(className + ".prototype") (Painter)
    - eval(className) (Singer.*Actions, Turtle.DictActions)

  • runCommand() used eval(cname) to resolve the action object

After

  • Introduces a hoisted, explicit registry:
    STATIC_API_CLASS_REGISTRY: string → resolver function

  • Enumeration logic:

    • Painter uses Painter.prototype directly
    • Known action objects are resolved via the registry
  • Dispatch (runCommand):

    • "Painter" continues routing to this.turtle.painter (unchanged)
    • Other classes resolve via:
      1] the registry (preferred)
      2] a backward-compatible fallback resolveGlobalByPath(cname) (no eval)
  • Hardening:

    • resolveGlobalByPath rejects non-strings and values not matching ^[A-Za-z0-9_.]+$
    • Validates the resolved value is an object or function
    • Throws a clear ReferenceError otherwise

Why this is safe

  • Registry keys are explicit and reviewable
  • Lazy resolvers avoid early ReferenceError in environments where globals may load later or be absent in tests
  • Fallback preserves previous name-based resolution used by test harnesses, but without eval and with input guards.

Problem observed

When loading an HTML-exported project, the embedded JSON inside <div class="code">...</div> is HTML-escaped (e.g., quotes become &quot;).
The loader path attempted [JSON.parse()] directly on that escaped string, causing:

[SyntaxError: Unexpected token '&', "&quot;..." is not valid JSON] ------------ ### fixed in #5808

Note - please check #5808 before this merge .. for 👆 fix

tests

image image image

Summary

This PR removes non-essential eval usage in static lookup paths, improves clarity and security posture, and preserves existing behavior by design. It is intentionally scoped and avoids changes to dynamic execution or plugins.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@Chaitu7032
Copy link
Contributor Author

@walterbender sir , @omsuneri
Whenever you have some time, could you please take a look at this PR?

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

Copy link

@shashank03-dev shashank03-dev left a comment

Choose a reason for hiding this comment

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

dude this is good PR i think we should add a final check to ensure that apiclass is actually a object before calling Object.getOwnPropertyNames this might prevent type_error

@Chaitu7032
Copy link
Contributor Author

Sure will take a look on that

@Chaitu7032 Chaitu7032 closed this by deleting the head repository Feb 27, 2026
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