-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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 <[email protected]>
… validation Co-authored-by: danmoseley <[email protected]>
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
GetSetCookieLengthto catchArgumentOutOfRangeExceptionfromTimeSpan.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 <[email protected]>
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 <[email protected]>
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.TryParsemethod was throwingArgumentOutOfRangeExceptionwhen the Max-Age value exceededTimeSpan's maximum representable seconds (922337203685), violating the expected behavior ofTryParsemethods which should returnfalsefor invalid input rather than throwing exceptions.Problem
The issue occurred because
TimeSpan.FromSeconds(maxAge)throws whenmaxAgeexceeds the valid range, but the calling code didn't validate the input before the conversion.Solution
Added overflow validation in
GetSetCookieLengthbefore callingTimeSpan.FromSeconds:Now overflow cases return
falseas 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.