Fix truncated cookie #5215
Merged
Fix truncated cookie #5215
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5215 +/- ##
==========================================
+ Coverage 72.96% 72.98% +0.02%
==========================================
Files 69 69
Lines 14855 14855
==========================================
+ Hits 10839 10842 +3
+ Misses 4016 4013 -3 ☔ View full report in Codecov by Sentry. |
ext-src/swoole_http_response.cc
Outdated
Comment on lines
1045
to
1049
| cookie_size += sizeof(max_age); | ||
| char *new_cookie = (char *) emalloc(cookie_size); | ||
| strlcpy(new_cookie, cookie, cookie_size); | ||
| efree(cookie); | ||
| cookie = new_cookie; |
There was a problem hiding this comment.
size_t cookie_size = name_len /* + value_len */ + path_len + domain_len + 200 + samesite_len + priority_len;Attempting to modify the allocated memory size here.
There was a problem hiding this comment.
We don't know the exact length of max_age until expires is processed, though. 🧐
There was a problem hiding this comment.
@NathanFreeman, I've updated the code to try and calculate the cookie_size. How is it now?
There was a problem hiding this comment.
It would be better to use sizeof("") - 1 to represent the length
stnguyen90
force-pushed
the
fix-truncated-cookie
branch
from
9380b3b
to
01faf30
Compare
Previously, Max-Age was added to the cookie, but cookie_size wasn't updated to account for the additional string size. This lead to the data being truncated in cases where everything was set. This change updates the cookie_size based on the length of max_age and then increases the size of cookie to the increased cookie_size.
stnguyen90
force-pushed
the
fix-truncated-cookie
branch
from
01faf30
to
af1a66a
Compare
stnguyen90
added a commit
to appwrite/docker-base
that referenced
this pull request
Feb 9, 2024
This new swoole version is important because it includes a patch for cookies (swoole/swoole-src#5215) that caused a problem in Appwrite (appwrite/appwrite#7253) where cookies were not being saved.
stnguyen90
added a commit
to appwrite/docker-base
that referenced
this pull request
Feb 9, 2024
This new swoole version is important because it includes a patch for cookies (swoole/swoole-src#5215) that caused a problem in Appwrite (appwrite/appwrite#7253) where cookies were not being saved.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, Max-Age was added to the cookie, but cookie_size wasn't updated to account for the additional string size. This lead to the data being truncated in cases where everything was set.
This change updates the cookie_size based on the length of max_age and then increases the size of cookie to the increased cookie_size.
Fixes #5214