Skip to content
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

Enable passing environment variables to the node process #240

Merged
merged 3 commits into from
Jun 23, 2024

Conversation

jukra
Copy link
Contributor

@jukra jukra commented Jun 13, 2024

We noticed that Chrome does not behave well with jemalloc enabled, and we prefer to use that in general.

Instead of disabling jemalloc in the whole environment, we opted to just disable it for the node process used. Based on quick search, others have had the same issue as well (#80)

We disabled it by passing a new environment variable for the node process like this

Copy link
Contributor

@abrom abrom left a comment

Choose a reason for hiding this comment

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

At face value this certainly seems reasonable, unfortunately it is also possible to use this change to allow a pretty broad set of attacks on the host via the middleware. Options can be parsed from the body of the page being converted, and if the system supports file uploads and has the middleware enabled to allow conversion of responses it would be possible to have the end user alter these settings per request. This might include things like including extra files (--require ..) or changing the default CA, or enabling process inspection, changing the node path. This is a surefire way to allow potential back-dooring or file exfiltration.

https://nodejs.org/api/cli.html#environment-variables_1

This would either require an absolutely bullet proof set of guards in Grover::OptionsBuilder#meta_options or configuring this outside the bounds of the options passed to the Grover initialiser.

lib/grover/processor.rb Outdated Show resolved Hide resolved
@jukra
Copy link
Contributor Author

jukra commented Jun 14, 2024

Ah yes, you are right, I did not consider the middleware usage as we are not using it. Could this be under the config like this? So not using the options but adding one more flag here lib/grover/configuration.rb

# config/initializers/grover.rb
Grover.configure do |config|
  config.node_env_vars = { 'LD_PRELOAD': '' }
end

Then passing it along for the lib/grover/processor.rb?

@abrom
Copy link
Contributor

abrom commented Jun 15, 2024

Yes, exactly.. much safer

@jukra jukra force-pushed the disable_jemalloc branch from 8e42200 to 201815f Compare June 17, 2024 06:06
@jukra jukra requested a review from abrom June 17, 2024 06:08
@jukra
Copy link
Contributor Author

jukra commented Jun 17, 2024

Changed now so that node_env_vars is part of the config instead of options

@abrom abrom merged commit 29d1794 into Studiosity:main Jun 23, 2024
11 checks passed
@abrom
Copy link
Contributor

abrom commented Jun 23, 2024

Released in v1.1.8

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