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

Address Session Fixation Concerns #900

Merged
merged 28 commits into from May 20, 2022
Merged

Address Session Fixation Concerns #900

merged 28 commits into from May 20, 2022

Conversation

jaredhanson
Copy link
Owner

This PR addresses session fixation concerns. Anytime a user logs in or logs out, the session is regenerated (resulting in a new session ID).

@jaredhanson jaredhanson merged commit 42630cb into master May 20, 2022
@sushiljainam
Copy link

No reviews, No assignees.
This PR includes breaking changes - #900
This error crashed our PRODUCTION.

No mention of BREAKING CHANGE
in changelog, in commit messages, in release guide, in migration guide
well there is no migration guide at all.
and there are no release docs - https://github.com/jaredhanson/passport/releases/tag/v0.6.0 mentions nothing of what includes

This is a REPO having 20k+ stars, 1.2k forks, consistent 1M+ weekly downloads on npm

Please make BREAKING CHANGES more visible and informed.

@jaredhanson

@jaredhanson
Copy link
Owner Author

No reviews, No assignees.

I'm the only person who maintains this project.

in changelog

It's listed here: https://github.com/jaredhanson/passport/blob/master/CHANGELOG.md#060---2022-05-20

in migration guide

and here: https://medium.com/passportjs/fixing-session-fixation-b2b68619c51d

@sushiljainam
Copy link

changelog was updated after the version was released.
and finding that medium blog link from repo or npm module is not possible or easy.

I appreciate your efforts and response, but for such a popular module
making it difficult for people to find changes and breaking changes feels reckless.
You can also add some contributors from community, I think many people are ready to help in this.
I am also ready to help if possible.

@tgroutars
Copy link

@sushiljainam honestly it's on you for upgrading a non stable package without checking what changed first, don't come at the maintainer like that when you're clearly the one in the wrong

https://semver.org/#spec-item-4

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

@sushiljainam
Copy link

I agree to you @tgroutars,
but this module has gained so much popularity in its life of 11 years to still be in 0.y.z format.
And you can see in Issues section of this repo, that many people are concerned about this same breaking change.

Also my suggestion, maintainer should add few more maintainers who can welcome community contributions faster as he gets less time, which is completely okay.

Anyway, I learned this new thing about version starting with 0 (0.y.z), So yeah, I'll take care in future.
Thanks.

And Respect for @jaredhanson for creating and maintaining such a popular OSS.

@wmurphyrd
Copy link

No reviews, No assignees. This PR includes breaking changes - #900 This error crashed our PRODUCTION.

No mention of BREAKING CHANGE in changelog, in commit messages, in release guide, in migration guide well there is no migration guide at all. and there are no release docs - https://github.com/jaredhanson/passport/releases/tag/v0.6.0 mentions nothing of what includes

This is a REPO having 20k+ stars, 1.2k forks, consistent 1M+ weekly downloads on npm

Please make BREAKING CHANGES more visible and informed.

@sushiljainam
this comment could have been kinder, but I'm glad it's here because I also did not catch that there were breaking changes from the changelog entry and was about to merge the dependabot PR 🙏

DevWithTheHair added a commit to Banno/consumer-api-openid-connect-example that referenced this pull request Nov 22, 2022
The passport.js changes in `0.6.0` have breaking changes related to protecting against "Session Fixation".
- jaredhanson/passport#900
- https://medium.com/passportjs/fixing-session-fixation-b2b68619c51d

The assumption for the fix in this commit is that our example project here only has the session storage as its storage mechanism, so we're not quite vulnerable to the same thing since the storage goes away when the local project is stopped.
DevWithTheHair added a commit to Banno/consumer-api-openid-connect-example that referenced this pull request Nov 22, 2022
* chore(deps): bump passport from 0.4.0 to 0.6.0

Bumps [passport](https://github.com/jaredhanson/passport) from 0.4.0 to 0.6.0.
- [Release notes](https://github.com/jaredhanson/passport/releases)
- [Changelog](https://github.com/jaredhanson/passport/blob/master/CHANGELOG.md)
- [Commits](jaredhanson/passport@v0.4.0...v0.6.0)

---
updated-dependencies:
- dependency-name: passport
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

* fix: use `keepSessionInfo` to maintain session

The passport.js changes in `0.6.0` have breaking changes related to protecting against "Session Fixation".
- jaredhanson/passport#900
- https://medium.com/passportjs/fixing-session-fixation-b2b68619c51d

The assumption for the fix in this commit is that our example project here only has the session storage as its storage mechanism, so we're not quite vulnerable to the same thing since the storage goes away when the local project is stopped.

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jaime Lopez <31429468+DevWithTheHair@users.noreply.github.com>
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.

None yet

4 participants