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

Ignore expires and maxAge in res.clearCookie() #5792

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

jonchurch
Copy link
Member

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 relative expires value if it sees a maxAge 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 an expires into the future relative to the maxAge

@jonchurch
Copy link
Member Author

eslint is choking on the spread syntax

@jonchurch
Copy link
Member Author

jonchurch commented Jul 29, 2024

It's a lack of ecmaVersion in the .eslintrc.yml

For spread, we'd need ecmaVersion: 2015

For v5 though, with support targetting >= Node 18, we'd want ecmaVersion: 2022

@wesleytodd
Copy link
Member

I was thinking after we land 5 we should work out moving to neostandard (or standard as it seems the fork could get resolved). I wonder if maybe it is just landing this without the lint offending things and just move forward?

@jonchurch
Copy link
Member Author

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
@jonchurch
Copy link
Member Author

updated the PR to use Object.assign so we don't have to update the linting rn

@wesleytodd
Copy link
Member

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?

@jonchurch
Copy link
Member Author

jonchurch commented Aug 2, 2024

@wesleytodd it's misocnfiguration weirdness with ci

5.0 branch protection requires the node 22 job from master's CI to pass
But master's CI doesn't trigger for PRs to 5.0 (that workflow triggers on 5.x, 4.x, master) after my updates we landed a few weeks ago.

5.0 has it's own version of CI which is outdated and doesn't care about branches. 5.0 branche's CI are the status jobs running here. That version of CI doesn't include 22 in it's matrix, but the job that is pending but required is the master CI

History.md Outdated Show resolved Hide resolved
lib/response.js Outdated Show resolved Hide resolved
@ctcpip
Copy link
Member

ctcpip commented Aug 2, 2024

it's not really a lint failure per se -- it's a parsing error, which means we can't even add an eslint-disable-line directive

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

@wesleytodd
Copy link
Member

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?

@ctcpip ctcpip force-pushed the jonchurch/clear-cookie-take-2 branch from 80a3d68 to 4feefb9 Compare August 2, 2024 20:24
@ctcpip ctcpip merged commit 82fc12a into expressjs:5.0 Aug 2, 2024
12 of 20 checks passed
@ctcpip
Copy link
Member

ctcpip commented Aug 2, 2024

@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

Bbjj88h

This comment was marked as spam.

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.

4 participants