-
-
Notifications
You must be signed in to change notification settings - Fork 16.8k
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
Ignore expires
and maxAge
in res.clearCookie()
#5792
Conversation
e6035e3
to
4668a6e
Compare
eslint is choking on the spread syntax |
It's a lack of For spread, we'd need For v5 though, with support targetting >= Node 18, we'd want |
I was thinking after we land 5 we should work out moving to |
That's where Im at rn too, will update this to not break lint |
this is to take into account the built-in relative expires when passing a maxAge to res.cookie I realized that using maxAge to invalidate cookies inherrently hit this relativee expires behavior, and the goal of this PR is not to rework that relative expires behavior w/ maxAge, but to prevent users from overwriting these values by accident when clearing cookies
d878ab2
to
1abae7a
Compare
updated the PR to use |
I am unsure why 22 is blocking this and don't have time to dig in, but since there is both engines and new CI for 5.0 I wonder if we can just ignore this for now and merge? |
@wesleytodd it's misocnfiguration weirdness with ci 5.0 branch protection requires the node 22 job from master's CI to pass
|
Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
it's not really a lint failure per se -- it's a parsing error, which means we can't even add an the question about whether and what linter and rules to use is a different matter than language support, and we should not arbitrarily subject our selves to a syntactical penitentiary. we are going to run into this all the time. and here, it's in a critical path and where we take a perf hit by using object assign |
Sounds to me with both of these comments that these are entirely unrelated issues to this PR so we could move forward with just merging and dealing with fixing CI in the 5.0 branch in one of the PRs specifically already working on that? |
80a3d68
to
4feefb9
Compare
@wesleytodd there's a bit of a circular dependency with the merge PR/branch needing the 5.0 changes due to both merge conflicts and test failure reconciliation, but @jonchurch and I just spent some time getting things cleaned up. at this point the merge branch/PR is looking good and all we are waiting for is the next release of v4, then to do one last sync and then merge to 5.0. (we could also merge to 5.0 sooner than that, but the typical process had been to merge only after a release). either way wfm |
Opened a new PR for this, supersedes #4852
closes #4851
The PR overrides any expires value set, and explicitly deletes maxAge from the options object.
The reason this is needed is that
res.cookie()
will set a relativeexpires
value if it sees amaxAge
value in the options.clearCookie is meant to delete a cookie, but that deletion can be thwarted if you pass a
maxAge
value, by setting anexpires
into the future relative to themaxAge