Skip to content

integrate cookie-signature#197

Merged
Uzlopak merged 26 commits intofastify:masterfrom
Uzlopak:integrate-cookie-signer
Aug 11, 2022
Merged

integrate cookie-signature#197
Uzlopak merged 26 commits intofastify:masterfrom
Uzlopak:integrate-cookie-signer

Conversation

@Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jul 17, 2022 •
edited
Loading

I know you maybe dont like to integrate the cookie-signature package, but I think it has some benefits.

  • We can specify which algorithm we want to use for the signing. So you can specify e.g. sha512 or sha1 instead of sha256
  • standalone unsign-function will be able to handle multiple secrets and will return an UnsignResult Object, basically unifying the behaviour of unsign from the signerFactory with the standalone unsign-Function
  • because the return value of the unsign function is changed, it probably means a semver major

Checklist

jsumners
jsumners requested changes Jul 17, 2022
View reviewed changes
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Have we tried to get the feature added upstream instead of copying their code here?

mcollina
mcollina reviewed Jul 17, 2022
View reviewed changes
mcollina
mcollina requested changes Jul 17, 2022
View reviewed changes
mcollina
mcollina reviewed Jul 18, 2022
View reviewed changes
jsumners
jsumners previously requested changes Jul 18, 2022
View reviewed changes
Uzlopak
Uzlopak commented Jul 18, 2022
View reviewed changes
jsumners
jsumners reviewed Jul 18, 2022
View reviewed changes
@jsumners jsumners dismissed their stale review July 18, 2022 16:29

I am not blocking at this point.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 18, 2022

Is it an issue that it is a breaking change? I could still export Signer also as signerFactory, keeping it backwards compatible...

@climba03003
Copy link
Member

climba03003 commented Jul 18, 2022 •
edited
Loading

I could still export Signer also as signerFactory, keeping it backwards compatible...

Yes, please. Reduce breaking changes when possible.
Please keep the name signerFactory as it already exposed in 7.2.0.

mcollina
mcollina approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@budarin
Copy link
Contributor

budarin commented Jul 21, 2022 •
edited
Loading

I would like any description in the documentation on the proposed features

Uzlopak and others added 6 commits July 25, 2022 15:12
@Uzlopak
add secure: 'auto' Option (fastify#199)
87aaf65 
* add secure: auto Option

* document deviation from cookie serialize

* describe better

* lowercase Lax
@mcollina @Uzlopak
Bumped v7.3.0
f7a9148 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@wight554 @Uzlopak
make type definitons "module": "nodenext" compatible (fastify#184)
3e619be 
* make type definitons `"module": "nodenext"` compatible

* see microsoft/TypeScript#48845

* fix test

* fix default export type

* make typing extra safe and small refactoring

* restore brackets

* add additional properties to named export and fix test suite

* remove formatting

* revert formatting

* reuse type
@mcollina @Uzlopak
Bumped v7.3.1
7f6a157 
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@Uzlopak
Merge branch 'master' into integrate-cookie-signer
36d3b72
@Uzlopak
modify types
63fcd47
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 25, 2022

Ok so far?

@budarin
Copy link
Contributor

budarin commented Jul 25, 2022

Ok so far?

any docs?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 25, 2022

Well i am actually more interested about the integration with e.g. @fastify/session. Should we e.g. use then the shorthandle on fastify.signCookie and fastify.unsignCookie instead of using sign and unsign.

Other than that i think documentation is only needed for the algorithm option.

mcollina
mcollina requested changes Jul 26, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Explicitly marking it as changes needed: drop the dummy secret.

@Uzlopak Uzlopak requested a review from mcollina July 26, 2022 09:29
mcollina
mcollina approved these changes Jul 27, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Uzlopak Uzlopak requested a review from jsumners August 4, 2022 13:33
@Uzlopak Uzlopak merged commit f7e5d27 into fastify:master Aug 11, 2022
@Uzlopak Uzlopak deleted the integrate-cookie-signer branch August 11, 2022 08:22
@U-4-E-A
Copy link

U-4-E-A commented Oct 4, 2024

cookie-signature still exists in dependencies but does not appear to be required in either the library code or the testing. Should this be removed?

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

Reviewers

@mcollina mcollina mcollina approved these changes

@jsumners jsumners Awaiting requested review from jsumners

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Uzlopak @climba03003 @budarin @U-4-E-A @mcollina @jsumners @wight554