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

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 added 28 commits May 17, 2022 08:15
@jaredhanson
Regenerate session on login.
7e9b9cf
@jaredhanson
Fix tests.
a7513c4
@jaredhanson
Regenerate session in logout.
a77271f
@jaredhanson
Save session on login before invoking callback.
9cde808
@jaredhanson
Clear user from old session on logout.
c018dea
@jaredhanson
Fix tests.
fa80b20
@jaredhanson
Error if session isn't available.
fa70e2f
@jaredhanson
Handle logout without session manager.
88c1f1b
@jaredhanson
Add test.
71c54f6
@jaredhanson
Add tests.
cc7606c
@jaredhanson
Add tests.
ee0bf81
@jaredhanson
Add tests.
cfa8259
@jaredhanson
Clean up tests.
b395106
@jaredhanson
Add tests.
3001654
@jaredhanson
Add tests.
80cc4e3
@jaredhanson
Better session detection and exceptions.
294f22c
@jaredhanson
Add tests.
c1991cf
@jaredhanson
Add tests.
8825a9a
@jaredhanson
Add optional options to login and logout.
e69834e
@jaredhanson
Add option to keep session data.
a349c2b
@jaredhanson
Add option to keep session data on logout.
17111d7
@jaredhanson
Add tests.
bfba8a1
@jaredhanson
No need to guard callback existence.
29a90d6
@jaredhanson
Add tests.
f8a175f
@jaredhanson
Add tests.
987b191
@jaredhanson
Silence verbose logging.
46756e5
@jaredhanson
Change keepSessionData to keepSessionData.
4f6bd5b
@jaredhanson
Use utils-merge rather than Object.assign for compatibility.
8dd79fe
@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-gt sushiljainam-gt mentioned this pull request Aug 30, 2022
Open
@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.

@sync-by-unito sync-by-unito bot mentioned this pull request Sep 15, 2022
Open
@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 DevWithTheHair mentioned this pull request Nov 22, 2022
Merged
DevWithTheHair added a commit to Banno/consumer-api-openid-connect-example that referenced this pull request Nov 22, 2022
@DevWithTheHair
fix: use keepSessionInfo to maintain session
984eddb 
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
@dependabot @DevWithTheHair
chore(deps): bump passport from 0.4.0 to 0.6.0 (#31)
7a2e5bd 
* 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] <[email protected]>

* 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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jaime Lopez <[email protected]>
@ethanherbertson ethanherbertson mentioned this pull request Jun 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jaredhanson @sushiljainam @tgroutars @wmurphyrd