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_manager] Fix FileSystemException when saving redirect cookies without a proper host #1948

Merged
merged 9 commits into from
Oct 1, 2023

Conversation

s0nerik
Copy link
Contributor

@s0nerik s0nerik commented Aug 31, 2023

Upon relative redirect, cookies were not saved because the host was missing. resolveUri provides the proper host info for a redirect Uri in such cases and doesn't affect others.

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

@s0nerik s0nerik requested a review from a team as a code owner August 31, 2023 21:39
Copy link
Contributor

@ueman ueman left a comment

Choose a reason for hiding this comment

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

Thanks for fixing and figuring this out. Please add a test for this change, though. Otherwise, it might get broken in the future again.

…th a full Uri for requests that had relative redirects
@s0nerik
Copy link
Contributor Author

s0nerik commented Sep 1, 2023

@ueman I've added a test.

@s0nerik s0nerik requested a review from ueman September 1, 2023 12:28
}

void main() {
test(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test that makes sure non relative redirects aren't modified?
I.e.: a redirect from example.org to sample.com should not write a cookie to example.org.

@kuhnroyal or @AlexV525 do you happen to know whether this is or can be a problem? I'm not really knowledgable in this area.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@AlexV525
Copy link
Member

I was wondering if this should be resolved from the cookie_jar. @s0nerik

@s0nerik
Copy link
Contributor Author

s0nerik commented Sep 11, 2023

I was wondering if this should be resolved from the cookie_jar. @s0nerik

It doesn't look like a problem with the cookie_jar itself to me. cookie_jar lays responsibility of providing a proper host for saving cookies on a caller. The error it gives upon providing it with an incorrect host could've been better, though.

The CookieManager just wasn't providing it with a proper host info upon a relative redirect.

Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

Sry for the late review

@@ -123,7 +123,9 @@ class CookieManager extends Interceptor {
// users will be available to handle cookies themselves.
final isRedirectRequest = statusCode >= 300 && statusCode < 400;
// Saving cookies for the original site.
await cookieJar.saveFromResponse(response.realUri, cookies);
final originalUri = response.requestOptions.uri;
final realUri = originalUri.resolveUri(response.realUri);
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any standards that indicate we should resolve the real dest based on the original one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://www.rfc-editor.org/rfc/rfc7231#section-7.1.2

The field value consists of a single URI-reference. When it has the
form of a relative reference ([RFC3986], Section 4.2), the final
value is computed by resolving it against the effective request URI
([RFC3986], Section 5).

Copy link
Member

Choose a reason for hiding this comment

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

Great. Could you add it as a comment, so we can track it back somedays if we forgot it.

Copy link
Member

Choose a reason for hiding this comment

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

Co-authored-by: Alex Li <[email protected]>
Signed-off-by: Alex Isaienko <[email protected]>
Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

LGTM after the above comments are resolved

@AlexV525 AlexV525 changed the title Fixes FileSystemException: Cannot open file ... (OS Error: Is a directory, errno = 21) [cookie_manager] Fix FileSystemException when saving redirect cookies without a proper host Oct 1, 2023
@AlexV525 AlexV525 enabled auto-merge October 1, 2023 06:12
@AlexV525 AlexV525 disabled auto-merge October 1, 2023 06:19
@AlexV525 AlexV525 merged commit 57a6577 into cfug:main Oct 1, 2023
34 checks passed
@AlexV525 AlexV525 linked an issue Oct 1, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on redirect when PersistCookieJar is used
3 participants