Skip to content

Conversation

@marcbachmann
Copy link
Owner

@marcbachmann marcbachmann commented Apr 20, 2021

Changelog

  • πŸ›‘οΈ Prevent local file access by default using the localUrlAccess: false option
  • πŸ’₯ Drop node versions older than v12

πŸ’₯ Breaking Change

Prevent local file access by default to fix a security issue.
Please provide the localUrlAccess: true option if you want to keep the old behavior
but keep your system vulnerable to local file access.

Not sure this module is even usable without installing phantomjs manually.
On linux you might need to download the executable.

The tests are running locally on macos. I'll publish this as version 3.

…false` option

BREAKING CHANGE: Prevent local file access by default to fix a security issue.
Please provide the `localUrlAccess: true` option if you want to keep the old behavior
but keep your system vulnerable to local file access.
@marcbachmann marcbachmann force-pushed the prevent-local-file-access-by-default branch 4 times, most recently from a29aa70 to d2ad4e8 Compare April 20, 2021 19:48
@marcbachmann marcbachmann force-pushed the prevent-local-file-access-by-default branch from d2ad4e8 to 9d90212 Compare April 20, 2021 19:55
@marcbachmann marcbachmann merged commit 296313e into master Apr 20, 2021
if (!this.options.phantomPath) this.options.phantomPath = phantomjs && phantomjs.path
this.options.phantomArgs = this.options.phantomArgs || []

if (this.options.localUrlAccess) this.options.phantomArgs.push('--local-url-access=false')
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I crazy or is this inverted? if localUrlAccess is false (which it is by default, for security reasons), this option never gets set for phantomjs to disallow local url access? Are the tests inverted as well?

Copy link
Owner Author

@marcbachmann marcbachmann May 6, 2021

Choose a reason for hiding this comment

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

hah. oops.
should we push it as fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the case you didn't already go ahead and fix it, I created a PR here to address this #623

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.

3 participants