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

Fixes #3133 by upgrading serialize-to-js from 1.1.1 to 3.0.0 #3451

Merged
merged 2 commits into from
Aug 23, 2019
Merged

Fixes #3133 by upgrading serialize-to-js from 1.1.1 to 3.0.0 #3451

merged 2 commits into from
Aug 23, 2019

Conversation

jonsth131
Copy link

@jonsth131 jonsth131 commented Aug 21, 2019

↪️ Pull Request

Fixes issue #3133 by upgrading serialize-to-js from 1.1.1 to 3.0.0

Changed import in serializeObject.js to work with 3.0.0.

🚨 Test instructions

Create YAML import by following https://parceljs.org/yaml.html and run.

Create TOML import by following https://parceljs.org/toml.html and run.

✔️ PR Todo

  • Filled out test instructions
  • Included links to related issues/PRs

@suchig
Copy link

suchig commented Aug 23, 2019

Is there a reason this change is not merged? This shows as a high vulnerability and needs to be fixed.

@DeMoorJasper DeMoorJasper merged commit dc393bf into parcel-bundler:master Aug 23, 2019
@DeMoorJasper
Copy link
Member

@suchig merged it. The npm audit thing is always pretty extreme, it gives high risk to something that's barely medium, don't blindly trust these warnings.

A lot of people make a fuss about these warnings in tools and most of these vulnerabilities are about user input which we don't really have except for people using them and why would someone dos their own computer...

There's probably not a single npm module that abuses this vulnerability and it seems unlikely that someone would trigger this DoS vulnerability in their own code...

Haven't read through the entire doc about the vulnerability but I doubt it even affected Parcel.

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Aug 23, 2019

@suchig looked further into this to validate my thoughts on why this probably didn't affect Parcel.

According to the issue linked in the original issue here. The vulnerability does not apply to Parcel in any way.

According to the disclosed audit report I'd even say this is just how JavaScript's eval works and isn't even a vulnerability more as common sense by a dev of any program to just not allow users to write code that goes straight into eval...

It seems unlikely that any project was affected by this vulnerability, see commenthol/serialize-to-js#7 (comment)

@moeffju
Copy link

moeffju commented Aug 23, 2019

Hi all, I would like to install the fixed version but due to the monorepo I cannot find a way to properly reference the commit id from package.json. Adding the dependency "parcel-bundler/parcel#dc393bf" tries to install the entire monorepo, which is obviously not what I want.

Before I build this version locally and commit it to the “downstream” repo for the time being, is there a plan to publish a new version ASAP, or do you know of a way to make npm install the specific commit but build the “sub-package”?

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Aug 23, 2019

You can use https://yarnpkg.com/lang/en/docs/selective-version-resolutions/ to fix a version although it contains a breaking change so it will break Parcel.

Locally linking from monorepo is pretty hard unless you clone the entire repo somewhere and link it that way.

Anyway I'm not gonna go in depth here as this isn't even a security issue and nobody should worry about updating asap. (As I clearly explained above...)

@devongovett
Copy link
Member

We will not be releasing further updates to Parcel 1. As Jasper said, this is not a real issue. Please just ignore the warning and keep using the existing published version.

@moeffju
Copy link

moeffju commented Aug 23, 2019

I don’t disagree that it is exceedingly unlikely for the reported vulnerability to be an issue in parcel, however as long as 1.12.3 is the latest version on the npm registry, every project using parcel in their devDependencies will get this very serious-sounding warning. It might be easiest to just release a new minor version. If parcel requires the deserialize function and you believe there is no security risk in doing so, you could just copy it into parcel from https://github.com/commenthol/serialize-to-js/blob/a73605a0098233f12794a2b4ee2c8d7e2594a332/lib/deserialize.js#L31.

I totally understand how annoying this must feel, but it seems that the alternatives are more people asking about this and/or desensitizing developers to npm audit warnings, both of which seem undesirable.

@parcel-bundler parcel-bundler locked as resolved and limited conversation to collaborators Aug 25, 2019
@devongovett
Copy link
Member

Just published v1.12.4 with the update.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants