-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix SetCookieHeaderValue.TryParse throwing ArgumentOutOfRangeException for Max-Age overflow #63323
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
Conversation
Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com>
… validation Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com>
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.
Pull Request Overview
This PR fixes a bug where SetCookieHeaderValue.TryParse
would throw ArgumentOutOfRangeException
instead of returning false
when parsing Max-Age values that exceed TimeSpan
's maximum representable seconds (922337203685). The fix ensures proper TryParse
behavior by handling overflow conditions gracefully.
Key changes:
- Added exception handling in
GetSetCookieLength
to catchArgumentOutOfRangeException
fromTimeSpan.FromSeconds
- Added comprehensive test coverage for both overflow cases and valid edge cases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/Http/Headers/src/SetCookieHeaderValue.cs |
Added try-catch block to handle TimeSpan overflow in Max-Age parsing |
src/Http/Headers/test/SetCookieHeaderValueTest.cs |
Added test methods covering Max-Age overflow scenarios and valid edge cases |
@BrennanConroy any concerns |
Also, is this worth porting into RC2? |
@copilot please go back to original approach without try catch |
…n overflow handling Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com>
Done! Reverted to the manual validation approach without try-catch as requested. The code now uses explicit bounds checking instead of exception handling. See commit 74c8f83. |
Co-authored-by: Günther Foidl <gue@korporal.at>
Sure. It's a very safe improvement, but I don't think anything in the framework calls SetCookieHeaderValue.TryParse internally, so it might not be worth it to backport further. |
/ba-g Test failure is quarantined |
The
SetCookieHeaderValue.TryParse
method was throwingArgumentOutOfRangeException
when the Max-Age value exceededTimeSpan
's maximum representable seconds (922337203685), violating the expected behavior ofTryParse
methods which should returnfalse
for invalid input rather than throwing exceptions.Problem
The issue occurred because
TimeSpan.FromSeconds(maxAge)
throws whenmaxAge
exceeds the valid range, but the calling code didn't validate the input before the conversion.Solution
Added overflow validation in
GetSetCookieLength
before callingTimeSpan.FromSeconds
:Now overflow cases return
false
as expected:Added comprehensive tests covering both overflow cases (which should fail parsing) and valid edge cases (which should succeed).
Fixes #61735.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.