Fix supporting space in cookie storage cookie values#4548
Conversation
d8d8585 to
9713a3a
Compare
9713a3a to
31bf0a6
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes support for spaces in cookie storage cookie values by changing the cookie parsing logic from split_ascii_whitespace() to splitn(7, ...) to properly handle the 7-field Netscape cookie format where the last field (value) can contain spaces. The PR also refactors cookie handling to use a new CookieStore abstraction instead of Vec<Cookie>, and renames functions for clarity.
Key changes:
- Cookie parsing now uses
splitn(7, ...)to preserve spaces in cookie values - Introduced
CookieStoretype to encapsulate cookie storage operations - Renamed
cookie_storage_set/cookie_storage_cleartoget_cmd_cookie_storage_set/get_cmd_cookie_storage_clear
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/hurl/src/http/cookie_store.rs | Core parsing fix: changed from split_ascii_whitespace() to splitn(7, ...) to support spaces in cookie values; added CookieStore abstraction and is_expired()/match_domain() methods |
| packages/hurl/src/http/client.rs | Refactored to return CookieStore instead of Vec<Cookie>; moved domain matching logic into Cookie type |
| packages/hurl/src/http/curl_cmd.rs | Updated to use CookieStore parameter; refactored cookies_params to directly use cookie store methods |
| packages/hurl/src/runner/request.rs | Renamed functions with clearer names and improved documentation |
| packages/hurl/src/runner/entry.rs | Updated function calls to use renamed functions and new cookie_store() API |
| packages/hurl/src/runner/hurl_file.rs | Changed to use cookie_store() API |
| packages/hurl/src/main.rs | Updated to call to_netscape_str() before redact() |
| packages/hurl/src/http/mod.rs | Updated exports to use cookie_store module |
| integration/hurl/tests_ok/cookie/cookie_jar.* | Added test coverage for cookie with spaces in value |
Comments suppressed due to low confidence (2)
packages/hurl/src/http/cookie_store.rs:174
- The comment 'cookie expired when libcurl set value to 1?' expresses uncertainty about the expiration logic. Either verify this behavior and remove the question mark, or provide a reference to the libcurl documentation. The comment should clearly document the intended behavior rather than questioning it.
packages/hurl/src/http/curl_cmd.rs:659 - The test adds a second cookie ('cookie2') with expires='1' but only asserts that 'cookie1' appears in the output. This test doesn't verify that expired cookies are correctly filtered out. Consider adding an explicit assertion or comment explaining why only cookie1 is expected in the output (presumably because cookie2 is expired).
assert_eq!(
cmd.to_string(),
"curl \
--header 'User-Agent: iPhone' \
--header 'Foo: Bar' \
--cookie 'cookie1=valueA' \
--output foo.out \
'http://localhost:8000/hello'"
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bb00d3a to
27be092
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
27be092 to
b898c8a
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/accept |
|
🕗 /accept is running, please wait for completion. |
|
✅ Pull request merged with fast forward by |
No description provided.