-
Notifications
You must be signed in to change notification settings - Fork 15.4k
New lesson: Session Management in Practice #29814
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
base: main
Are you sure you want to change the base?
Conversation
To lead into addressing two main issues
Not meant as a 'stateless/JWT is always bad in every context' thing
"or anyone" bit ends up reading awkwardly, and that example is focused on the impact of stale data. The impact of other users using copied tokens is already addressed in the next section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🚀
Loki is not related to Odin. That's a Marvel thing... oops
Fixes codespell to ignore URLs
Holding off on addressing review suggestions while https://discord.com/channels/505093832157691914/1181210153358471168/1390089775846785175 is still up for discussion. Will address accordingly one we know how we wish to proceed. |
Updated scoped and intent
Co-authored-by: Asartea <76259120+Asartea@users.noreply.github.com>
As discussed on Discord, the lesson has now had wording amendments as per review applied, as well as additional content on third-party cookies and reverse proxies, kept within only the scope needed for TOP projects. If you want to see the code in action, check out https://github.com/mao-sz/test-proxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor grammar check. But where you said "(though this is of course sometimes an desirable security feature, like with many banking websites)." should say "sometimes a desirable security feature"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Though you're looking at an outdated commit - the latest version has that fixed (fixed in 3e0b841).
There will have been quite a few changes since the commit you had looked at.
Looks good to me |
One comment: how necessary is http://cryto.net/~joepie91/blog/2016/06/13/stop-using-jwt-for-sessions/ ? It's currently giving security warnings because it its lacking a valid certificate; I would prefer to avoid sites which work only over HTTP, especially as HTTPS only mode is becoming more and more common |
Edit: One another review, I think it's fine to drop the assignment
Hmmm. I really like that article and how it explains things, but I agree that lacking https sucks. From an outside perspective, how do you feel about the assignment section as a whole? Do you feel it's "complete" without the cryto resource? Or do you feel the cryto resource fills what would otherwise be a gap, and so would benefit from finding an equivalent https resource if possible? |
Website served with HTTP (no HTTPS), and not strictly necessary given the other assigned resources.
Because
As part of the Node revamp's 2nd milestone
This PR
Issue
Closes #29735
Pull Request Requirements
location of change: brief description of change
format, e.g.Intro to HTML and CSS lesson: Fix link text
Because
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section