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

feat: upgrade cookie dependency and cleanup imports #77

Merged
merged 9 commits into from
Nov 11, 2024
Merged

Conversation

siimsams
Copy link
Contributor

@siimsams siimsams commented Oct 23, 2024

What kind of change does this PR introduce?

  • Bump cookie package to the latest version.
  • Use correct types from cookie package.
  • Fix typescript warning.
  • Cleanup unused imports.

What is the current behavior?

It currently shows up as unfixable security issue in my project.
GHSA-pxg6-pf52-xh8x

Related issues:
#73

What is the new behavior?

The new version of this package does not have this security issue.

Additional context

Screenshot 2024-10-23 at 19 56 31

@siimsams siimsams changed the title chore: bump cookie to the latest version chore: fix npm audit issues Oct 24, 2024
@siimsams
Copy link
Contributor Author

siimsams commented Oct 25, 2024

@alaister @dshukertjr

Wdyt? Could this be merged? Then I could start to look at another issue with the latest next.js.

EDIT: I fixed the lint. Forgot to run it.

@siimsams
Copy link
Contributor Author

@alaister @dshukertjr

Fixed the lint problems.

@retr0cube
Copy link

@alaister @dshukertjr

Wdyt? Could this be merged? Then I could start to look at another issue with the latest next.js.

EDIT: I fixed the lint. Forgot to run it.

This issue is also happening to me with SvelteKit:

cookie  <0.7.0
cookie accepts cookie name, path, and domain with out of bounds characters - https://github.com/advisories/GHSA-pxg6-pf52-xh8x
fix available via `npm audit fix --force`
Will install @sveltejs/[email protected], which is a breaking change
node_modules/cookie
  @supabase/ssr  *
  Depends on vulnerable versions of cookie
  node_modules/@supabase/ssr
  @sveltejs/kit  >=1.0.0-next.0
  Depends on vulnerable versions of cookie
  node_modules/@sveltejs/kit
    @sveltejs/adapter-auto  >=1.0.0-next.0
    Depends on vulnerable versions of @sveltejs/kit
    node_modules/@sveltejs/adapter-auto
    @sveltejs/adapter-vercel  >=1.0.0-next.0
    Depends on vulnerable versions of @sveltejs/kit
    node_modules/@sveltejs/adapter-vercel

5 low severity vulnerabilities

To address all issues possible (including breaking changes), run:
  npm audit fix --force

Some issues need review, and may require choosing
a different dependency.

@J0 J0 changed the title chore: fix npm audit issues fix: upgrade cookie dependency and cleanup imports Oct 28, 2024
@J0 J0 changed the title fix: upgrade cookie dependency and cleanup imports feat: upgrade cookie dependency and cleanup imports Oct 28, 2024
@siimsams siimsams requested a review from J0 October 28, 2024 07:55
@siimsams
Copy link
Contributor Author

@J0 Thank you for the review. Do I need to do anything additional for this to be merged and released?

@J0
Copy link
Contributor

J0 commented Oct 28, 2024

Could you regenerate the package-lock.json and package.json? Apologies I was slightly hesitant to bump the version to v1.0.1 as that's a jump in major version. I went ahead and bumped the minor version which resulted in some conflicts. The minor version bump to v0.7.0 should also resolve the warning for now I believe

We'll still consider the v1.0.1 upgrade and changes but I need to check in with the team before I go ahead and merge.

@siimsams
Copy link
Contributor Author

siimsams commented Oct 28, 2024

@J0 Thank you for the feedback. I regenerated the package-lock.json.

The breaking changes are described here:
https://github.com/jshttp/cookie/releases

@siimsams
Copy link
Contributor Author

@J0 The security fix has not been released as of now. It's just a RC version not a published version.

I get that this needs some more validation but can you guys release the current RC version as V5.0.2?
Then we could release this as 6.0.0 and list the minimum node version as breaking change.

@lorikku
Copy link

lorikku commented Nov 10, 2024

When will this be merged? Want to fix my audits :)

@J0
Copy link
Contributor

J0 commented Nov 11, 2024

Hey apologies for missing this.

We are releasing v0.5.2 now.

@J0
Copy link
Contributor

J0 commented Nov 11, 2024

Thanks for your patience @siimsams

AFAICT this shouldn't affect our API beyond the requirement for an increment in node version to v18 (current LTS is v20)

I think it should be fine to merge this as a minor version bump so going to merge. Welcome dissenting opinions though.

This should live in rc for a while, which will give us time to test.

@J0 J0 merged commit 9524528 into supabase:main Nov 11, 2024
2 checks passed
@siimsams
Copy link
Contributor Author

Thank you for releasing the fix. Not in a hurry with this PR.

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.

4 participants