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.

Sign up for GitHub

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

Jump to bottom

feat: Add feature to parse multiple set-cookies #688

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: Add feature to parse multiple set-cookies #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0826580
feat: Add feature to parse multiple set-cookies
myConsciousness Apr 19, 2022
e5de3ed
feat: fix typo
myConsciousness Apr 19, 2022
d0c13e1
feat: Add test for a single set-cookie
myConsciousness Apr 19, 2022
db07459
fix: Refactor for pull request #688
myConsciousness Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 28 additions & 2 deletions lib/src/base_response.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:io';
Copy link
Member

Choose a reason for hiding this comment

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

We can't import dart:io from this library. universal_io is intending to get around that problem, but we don't want to take a dependency on that library either. If we wanted to have this functionality in this library we'd have to reimplement an interface for Cookie.

This feels like the type of thing that should be added on in a side package (which could more easily have a dependency on universal_io)

extension Cookies on Response {
  List<Cookie> get cookies {
    // ...
  }
}

I suppose the nice thing about implementing it here is being able to cache it, but using an expando is probably a fine alternative if caching is important. Probably you'll only read .cookies once anyway.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your review @natebosch !

I found several issues have been raised regarding this case of multiple set-cookies, and I feel that a fundamental solution is needed. At the same time, I also understood the difficulties related to dart:io. It's quite a deep-rooted problem.

It's possible to add a Cookie interface and implementation, but is there a reluctance to add a Cookie object as a development policy for the http library?

If we can implement Cookie interface in http library, I thought the following example was a very good solution.

extension Cookies on Response {
  List<Cookie> get cookies {
    // ...
  }
}


import 'base_client.dart';
import 'base_request.dart';

Expand All @@ -24,16 +26,20 @@ abstract class BaseResponse {
/// If the size of the request is not known in advance, this is `null`.
final int? contentLength;

// TODO(nweiz): automatically parse cookies from headers

// TODO(nweiz): make this a HttpHeaders object.
final Map<String, String> headers;

/// The cookies parsed from [headers].
final List<Cookie> cookies = [];

final bool isRedirect;

/// Whether the server requested that a persistent connection be maintained.
final bool persistentConnection;

/// The regex pattern to split the cookies in `set-cookie`.
static final _regexSplitSetCookies = RegExp(',(?=[^ ])');

BaseResponse(this.statusCode,
{this.contentLength,
this.request,
Expand All @@ -46,5 +52,25 @@ abstract class BaseResponse {
} else if (contentLength != null && contentLength! < 0) {
throw ArgumentError('Invalid content length $contentLength.');
}

final setCookie = _getSetCookie(headers);
if (setCookie.isNotEmpty) {
for (final cookie in setCookie.split(_regexSplitSetCookies)) {
cookies.add(Cookie.fromSetCookieValue(cookie));
}
}
}

/// Returns the value of the `set-cookie` if the [headers] has,
/// otherwise empty.
static String _getSetCookie(final Map<String, dynamic> headers) {
for (final key in headers.keys) {
// Some systems return "set-cookie" for various cases.
if (key.toLowerCase() == 'set-cookie') {
return headers[key] as String;
}
}

return '';
}
}
46 changes: 46 additions & 0 deletions test/response_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,50 @@ void main() {
expect(response.bodyBytes, equals([104, 101, 108, 108, 111]));
});
});

// If multiple set-cookies are included in the response header,
// the http library will merge them into a comma-separated list
// and set them in the Response object.
test('checks multiple set-cookies', () {
final response = http.Response('', 200, headers: {
'set-cookie':
// This response header contains 6 set-cookies
'AWSALB=AWSALB_TEST; Expires=Tue, 26 Apr 2022 00:26:55 GMT; Path=/,AWSALBCORS=AWSALBCORS_TEST; Expires=Tue, 26 Apr 2022 00:26:55 GMT; Path=/; SameSite=None; Secure,jwt_token=JWT_TEST; Domain=.test.com; Max-Age=31536000; Path=/; expires=Wed, 19-Apr-2023 00:26:55 GMT; SameSite=lax; Secure,csrf_token=CSRF_TOKEN_TEST_1; Domain=.test.com; Max-Age=31536000; Path=/; expires=Wed, 19-Apr-2023 00:26:55 GMT,csrf_token=CSRF_TOKEN_TEST_2; Domain=.test.com; Max-Age=31536000; Path=/; expires=Wed, 19-Apr-2023 00:26:55 GMT,wuuid=WUUID_TEST'
});

expect(response.cookies.length, 6);
for (final cookie in response.cookies) {
expect(
cookie.name,
anyOf([
Copy link

Choose a reason for hiding this comment

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

this makes the test pass if response.cookies is always just a single cookie, e.g. the first one, repeated 6 times. This isn't so unimaginable in case of an error in the parsing code.

'AWSALB',
'AWSALBCORS',
'jwt_token',
'csrf_token',
'wuuid',
'csrf_token'
]));
expect(
cookie.value,
anyOf([
'AWSALB_TEST',
'AWSALBCORS_TEST',
'JWT_TEST',
'CSRF_TOKEN_TEST_1',
'CSRF_TOKEN_TEST_2',
'WUUID_TEST'
]));
}
});

test('checks a set-cookie', () {
final response = http.Response('', 200, headers: {
'set-cookie':
'AWSALB=AWSALB_TEST; Expires=Tue, 26 Apr 2022 00:26:55 GMT; Path=/'
});

expect(response.cookies.length, 1);
expect(response.cookies.first.name, 'AWSALB');
expect(response.cookies.first.value, 'AWSALB_TEST');
});
}