Skip to content

feat: added require to load scripts using CommonJs #2272

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JulienLeal
Copy link

@JulienLeal JulienLeal commented Jan 26, 2025

Proposed changes

Enabled CommonJS require support in GraalJS engine for Maestro flows

  • Added GraalJS context configuration with js.commonjs-require and js.commonjs-require-cwd options
  • Implemented module resolution relative to the executed YAML file's directory
  • Set MAESTRO_YAML_DIR environment variable automatically for script context
  • Added validation for module directory existence
  • Modified RunScriptCommand to propagate YAML file location context

Testing

Added integration test:

  • Case 121 - CommonJS require functionality
    • Verifies successful module resolution from YAML file directory
    • Tests cross-file require() with JSON serialization
    • Validates console output logging

Manual verification:

  1. Tested multi-level directory structures
  2. Verified relative path resolution (./lib/utils.js)
  3. Checked error handling for missing modules
  4. Validated Windows/Linux path compatibility

Issues fixed

@Fishbowler Fishbowler added the v1.40.0 Release 1.40.0 label Feb 5, 2025
@Fishbowler
Copy link
Contributor

I'm trying to work out how we can support this both in Maestro CLI, but also then in the Maestro hosted infrastructure, but without introducing a security risk through path traversal. I can't see that js.commonjs-require-cwd and related options give any security guarantees.

@JulienLeal
Copy link
Author

Hi @Fishbowler I'll take a look on it on the next weekend. I was on vacations, sorry my delay

@Angelk90
Copy link
Contributor

@JulienLeal : Is there anything new?

@JulienLeal
Copy link
Author

@Angelk90 yes, I'll give update this week

@JulienLeal
Copy link
Author

Hi @Fishbowler, how are you?

I believe the best way to address this security concern is to implement the Polyglot Sandbox policies described here:
https://docs.oracle.com/en/graalvm/jdk/22/docs/security-guide/polyglot-sandbox/#sandbox-policies

Unfortunately, sandbox policies require GraalVM 23.0 (see https://www.graalvm.org/sdk/javadoc/org/graalvm/polyglot/SandboxPolicy.html), so upgrading will likely take extra time to address any new compatibility concerns.

For now, I’ve provided a custom FileSystem implementation that effectively guards against path traversal.

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

Successfully merging this pull request may close these issues.

3 participants