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

cookie: cap expire times to 400 days #15937

Closed
wants to merge 2 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Jan 8, 2025

The pending cookie RFC update (currently known as 6265bis draft-19) says

Let cookie-age-limit be the maximum age of the cookie (which name of
Max-Age and an attribute-value of expiry-time. SHOULD be 400 days or
less.

This change makes received cookies over the wire get capped to 400 days.

It does not cap the expiry date of cookies loaded from file.

It does this by aligning the expire time to an even minute. This, to allow the test suite to do the same and have a chance to get the same number for stable testing without requiring a debug build.

@bagder bagder added the cookies label Jan 8, 2025
@github-actions github-actions bot added the tests label Jan 8, 2025
@bagder bagder force-pushed the bagder/cookie-cap-again branch from d6992f6 to 01a9dc2 Compare January 8, 2025 09:39
lib/cookie.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member Author

bagder commented Jan 8, 2025

@danielgustafsson I'm much more pleased with this approach as now we can still run all the cookie tests, including the capping of the expire times, without requiring debug builds. This version I believe we can merge.

Copy link
Member

@danielgustafsson danielgustafsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach, it's a clever way to avoid test-specific code.

tests/FILEFORMAT.md Outdated Show resolved Hide resolved
@testclutch
Copy link

Analysis of PR #15937 at 343f740e:

Test 61 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Test 483 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 1415 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Generated by Testclutch

The pending cookie RFC update (currently known as 6265bis draft-19) says

  Let cookie-age-limit be the maximum age of the cookie (which name of
  Max-Age and an attribute-value of expiry-time. SHOULD be 400 days or
  less.

This change makes received cookies over the wire get capped to 400 days.

It does not cap the expiry date of cookies loaded from file.

It does this by rounding the expire time to a even minute. This, to
allow the test suite to do the same and have a chance to get the same
number for stable testing without requiring a debug build.

Closes #15937
@bagder bagder force-pushed the bagder/cookie-cap-again branch from 343f740 to 5572c09 Compare January 9, 2025 08:34
@bagder
Copy link
Member Author

bagder commented Jan 9, 2025

Clearly not good enough yet. It fails in CI intermittently.

@danielgustafsson
Copy link
Member

While not the cause of CI failures, we only align the expiration date on a 60 min boundary if the expiration is above the maxage, but in the test harness we always align for %days[] (all cases are 400 right now though). Shouldn't the test harness do the same logic?

@bagder
Copy link
Member Author

bagder commented Jan 9, 2025

We actually don't need the test scripts to do it automatically as the content is delivered strictly as provided in the test* files. So we only use %days[] in test data for fields we know are going to be further away in the future than 400 days.

The remaining problem with this take is that if the test scripts generate the test file and aligns the max time to to ***60, there is still a small risk that when curl is invoked immediately afterwards, time() has increased another integer second and then the expiry data aligns to ***120. This makes the cookie tests flaky. 😞

The test script now generates TWO numbers for each %days[], and the
function that compares and verifies output is fine with either of the
two numbers.

This, so if the test case is generated the second before curl runs, that
updated expiry number is also deemed okay. It still checks for an exact
match of either number.
@bagder
Copy link
Member Author

bagder commented Jan 9, 2025

The test script now generates TWO numbers for each %days[], and the function that compares and verifies output is fine with either of the two numbers.

This, so if the test case is generated the second before curl runs, that updated expiry number is also deemed okay. It still checks for an exact match of either number.

@bagder
Copy link
Member Author

bagder commented Jan 9, 2025

This is a decent approach I think. We might be able to use this extended feature in the test suite for other things as well going forward.

@bagder bagder closed this in 386f570 Jan 10, 2025
@bagder bagder deleted the bagder/cookie-cap-again branch January 10, 2025 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants