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.

Sign up for GitHub

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

Jump to bottom

fix: backport regex change from 8.0.1 #20

Closed
wants to merge 1 commit into from
Closed

fix: backport regex change from 8.0.1 #20

wants to merge 1 commit into from

Conversation

G-Rath
Copy link

@G-Rath G-Rath commented Apr 14, 2021 •
edited
Loading

Backport of #17 so it's easier for people to patch.

Once/if this is merged and released, the advisory will need to be updated to reflect the new vulnerability/fixed range to allow > 7.0.2

Relates to #19.

References

@G-Rath G-Rath requested a review from a team as a code owner April 14, 2021 20:38
This was referenced Apr 14, 2021
Closed
Merged
@jbo023 jbo023 mentioned this pull request Apr 16, 2021
Closed
@undergroundwires undergroundwires mentioned this pull request Apr 16, 2021
Closed
@G-Rath G-Rath mentioned this pull request Apr 18, 2021
Closed
@darcyclarke darcyclarke self-assigned this Apr 19, 2021
@darcyclarke darcyclarke added this to the OSS - Sprint 29 milestone Apr 19, 2021
@nikoladev
Copy link

nikoladev commented Apr 21, 2021 •
edited
Loading

This would be nice to get backported to version 7 of ssri for packages which depend on ssri@7.

For example, gatsby@2 depends on terser-webpack-plugin@^2.3.8 which depends on cacache@^13.0.1 which then depends on ssri@^7.0.0:

└─┬ gatsby@2.32.12
  └─┬ terser-webpack-plugin@2.3.8
    └─┬ cacache@13.0.1
      └── ssri@7.1.0

And I'm sure there's more packages like this.

@georgeconstantinou
Copy link

georgeconstantinou commented Apr 23, 2021 •
edited
Loading

We are facing the same issue with @vue/cli-service direct and nested dependency:

$ npm ls ssri
└─┬ @vue/cli-service@4.5.12
  ├── ssri@7.1.0 
  └─┬ terser-webpack-plugin@2.3.8
    └─┬ cacache@13.0.1
      └── ssri@7.1.0

@nikoladev
Copy link

Friendly ping, @nlf

You released the previous updates for ssri. Could you look at this backport?

@darcyclarke darcyclarke removed their assignment May 3, 2021
ghost
ghost reviewed May 5, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I think I need this fix

@ghost
Copy link

ghost commented May 5, 2021

I need write access

@ghost
Copy link

ghost commented May 6, 2021

I'm just learning this platform so I'll look for other instances of this software on my local directories--is that part of the pipeline so to say? Thanks.

ghost
ghost approved these changes May 7, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Approve changes

ghost
ghost reviewed May 8, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Reviewed changes

ghost
ghost approved these changes May 8, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks

ghost
ghost approved these changes May 8, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks

@G-Rath
Copy link
Author

G-Rath commented May 11, 2021

@nlf @darcyclarke @wraithgar @isaacs would it be possible to get this merged and released?

We've coming up to this being open for a month now with the community being very vocal on this being annoying and showing a clear desire for the patch, which was promptly landed for the v6 & v8 lines in a desire to make it easy for devs to update which this version would also do.

If there is anything I can do on my end to help speed up getting this landed please let me know, but so far I've not had any review from the npm team :(

@ghost
Copy link

ghost commented May 11, 2021

Am I holding up this project

ghost
ghost approved these changes May 11, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Reviewed changes

ghost
ghost reviewed May 15, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Approve changes

ghost
ghost reviewed May 15, 2021
View reviewed changes
index.js
@@ -8,7 +8,7 @@ const SPEC_ALGORITHMS = ['sha256', 'sha384', 'sha512']

const BASE64_REGEX = /^[a-z0-9+/]+(?:=?=?)$/i
const SRI_REGEX = /^([^-]+)-([^?]+)([?\S*]*)$/
const STRICT_SRI_REGEX = /^([^-]+)-([A-Za-z0-9+/=]{44,88})(\?[\x21-\x7E]*)*$/
const STRICT_SRI_REGEX = /^([^-]+)-([A-Za-z0-9+/=]{44,88})(\?[\x21-\x7E]*)?$/
const VCHAR_REGEX = /^[\x21-\x7E]+$/

const SsriOpts = figgyPudding({
Copy link

Choose a reason for hiding this comment

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

Approve remove const SsriOpts = figgyPudding({

ghost
ghost reviewed May 15, 2021
View reviewed changes
index.js
@@ -8,7 +8,7 @@ const SPEC_ALGORITHMS = ['sha256', 'sha384', 'sha512']

const BASE64_REGEX = /^[a-z0-9+/]+(?:=?=?)$/i
const SRI_REGEX = /^([^-]+)-([^?]+)([?\S*]*)$/
const STRICT_SRI_REGEX = /^([^-]+)-([A-Za-z0-9+/=]{44,88})(\?[\x21-\x7E]*)*$/
Copy link

Choose a reason for hiding this comment

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

Approve remove constricting change

nlf
nlf approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

thank you for doing this work! sorry for the delay, but i'm about to publish it right now

nlf pushed a commit that referenced this pull request May 17, 2021
@G-Rath @nlf
fix: backport regex change from 8.0.1
809c84d 
PR-URL: #20
Credit: @G-Rath
Close: #20
Reviewed-by: @nlf
@nlf
Copy link
Contributor

nlf commented May 17, 2021

this was merged via commit 809c84d and published as ssri@7.1.1

@nlf nlf closed this May 17, 2021
@G-Rath G-Rath deleted the backport-regex-fix branch May 17, 2021 22:16
@G-Rath
Copy link
Author

G-Rath commented May 17, 2021

@nlf I'd say no problem but... it was a long wait 😅

Are you able to fast-track getting the npm advisory updated to include 7.1.1 as being non-vulnerable so we could start making our tooling happy? 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scastaldi scastaldi scastaldi approved these changes

@georgeconstantinou georgeconstantinou georgeconstantinou approved these changes

@nlf nlf nlf approved these changes

Assignees

@nlf nlf

Labels
None yet
Projects
None yet
Milestone
OSS - Sprint 30
Development

Successfully merging this pull request may close these issues.
6 participants
@G-Rath @nikoladev @georgeconstantinou @nlf @scastaldi @darcyclarke