jflick58/wasm_exec

WASM-powered sandbox implementation of exec() for safely running dynamic Python code

wasm_exec's Issues

allow namespaces

the original exec comes with a handy namespace. it allows for the addition of trusted extra code

     # Dictionary to store definitions from the script
    script_namespace = {'myFunction': myFunction}

    # Execute the script within the module's namespace
    exec(script_content, script_namespace)

it would be handy to allow for extra functions to be imported within the execution context

Confusion between dependency groups and extras

Thank you for making this library!

I have noticed that, in the quick start installation instructions, the poetry install -E all command is not correct for this project, and always fails when run, with the error message:

Extra [all] is not specified.

This points to what I think is a deeper issue in the way the project's dependencies are specified. I've proposed a fix in #18.

poetry does not treat all specially. Passing -E all only works in a project that explicitly defines an extra whose name is literally all. The --all-extras flag installs all extras, and replacing -E all with --all-extras makes the command succeed. But that change would not, by itself, be sufficient, because pyproject.toml does not currently define any extras. It defines dependency groups, some of which are marked optional. Dependency groups and extras are separate, independent features.

There is more than one way to fix this. The approach I recommend (and have opened #18 for), which I believe achieves all the intended benefits while avoiding excess complexity, is:

• Change the command given in the quick start instructions to poetry install --all-extras.
• Convert the install_wasm dependency group into an install-wasm extra, since it is intended to be installable with pip. The dependencies it specifies--currently just requests--are intended to be usable even outside a development setting, so it should be an extra rather than a dependency group.
• Make the remaining optional dependency group, test, non-optional. Dependency groups, other than the implicit main dependency group, are only installable by poetry. They are never made available when installing the package with pip. The use case where the test group would not be wanted in local development is marginal, and the instructions for interactive local development should be kept as simple as possible.
• In the GitHub Actions workflows, use --only=main,X instead of --with X (where X is a dependency group). This avoids installing extra dependency groups on CI, while keeping them non-optional so poetry installs them automatically when simple commands like poetry install or poetry install --all-extras are used.
• Possibly expand README.md to note that the WASM runtime downloading functionality requires the install-wasm extra, which is included if the package is installed by pip install 'wasm_exec[install-wasm]'. But maybe that should not be done yet, because the code in wasm_runtime.py is not yet under test, and most users would only want to use the vendored runtime.

PR closure attempts release in more cases than intended

This if-condition on the if_release job in release.yml is intended to ensure that a release is attempted due to a closed PR only when the PR was closed by merging and the PR had the release label applied to it:

wasm_exec/.github/workflows/release.yml

Lines 17 to 19 in 7e1fcbd

	if: |
	${{ github.event.pull_request.merged == true }}
	&& ${{ contains(github.event.pull_request.labels.*.name, 'release') }}

Unfortunately, that if-condition always evaluates as true. While testing CI for #17 in a separate PR internal to my fork, EliahKagan#1, I noticed that the release job ran even though it shouldn't. (This caused no harm: it was on my fork, which is not set up to authenticate with PyPI, so it failed.) This was even though EliahKagan#1 was closed without merging, and didn't have any labels applied to it.

I've submitted a fix in #20. See below (and in #20) for details.

Analysis

The reason that condition always evaluates to true is that an expression with ${{ }} is always treated as a string:

When you use expressions in an if conditional, you may omit the ${{ }} expression syntax because GitHub Actions automatically evaluates the if conditional as an expression. Using the ${{ }} expression syntax turns the contents into a string, and strings are truthy. For example, if: true && ${{ false }} will evaluate to true. For more information about if conditionals, see "Workflow syntax for GitHub Actions."

(emphasis mine)

Fix

In further tests internal to my fork, I have verified that removing the ${{ }} fixes the problem. I've opened #20 to propose this fix. It includes links to the results of my further testing.

Impact

I believe this is actually not a security bug. Because the pull_request trigger is used, I don't think CI running on a pull request from a fork ever has access to repository secrets. So I don't think this bug allows anyone without write access to this upstream repository to exfiltrate its secrets, nor to cause those secrets to be used to publish to PyPI.

The main impact is confusing CI results, though unintended publishing could occur from PRs internal to this upstream repository. (In most cases, I expect publishing would fail due to the same version already existing on PyPI, but if the version number in pyproject.toml were bumped, then unwanted publishing would succeed.)

Impact is further limited by the workflow's paths limitation. Many PRs don't modify pyproject.toml, and those don't run the release.yml workflow. so publishing is skipped without ever having to check the if-condition on the if_release job. That is also probably why this issue has not been discovered before.