-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Changes from 7 commits
350a08b
4ec39c3
2220c26
10755a9
326e1c2
b81020a
c26840c
af40b43
2c4db02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
import 'dart:collection'; | ||
import 'dart:convert'; | ||
import 'dart:io'; | ||
import 'dart:typed_data'; | ||
|
||
import 'package:cookie_jar/cookie_jar.dart'; | ||
import 'package:dio/dio.dart'; | ||
import 'package:dio/io.dart'; | ||
import 'package:dio_cookie_manager/dio_cookie_manager.dart'; | ||
import 'package:test/fake.dart'; | ||
import 'package:test/test.dart'; | ||
|
||
typedef _FetchCallback = Future<ResponseBody> Function( | ||
RequestOptions options, | ||
Stream<Uint8List>? requestStream, | ||
Future<void>? cancelFuture, | ||
); | ||
|
||
class _TestAdapter implements HttpClientAdapter { | ||
_TestAdapter({required _FetchCallback fetch}) : _fetch = fetch; | ||
|
||
final _FetchCallback _fetch; | ||
final HttpClientAdapter _adapter = IOHttpClientAdapter(); | ||
|
||
@override | ||
Future<ResponseBody> fetch( | ||
RequestOptions options, | ||
Stream<Uint8List>? requestStream, | ||
Future<void>? cancelFuture, | ||
) => | ||
_fetch(options, requestStream, cancelFuture); | ||
|
||
@override | ||
void close({bool force = false}) { | ||
_adapter.close(force: force); | ||
} | ||
} | ||
|
||
class _SaveCall { | ||
_SaveCall(this.uri, this.cookies); | ||
|
||
final String uri; | ||
final String cookies; | ||
|
||
@override | ||
bool operator ==(Object other) => | ||
identical(this, other) || | ||
other is _SaveCall && | ||
runtimeType == other.runtimeType && | ||
uri == other.uri && | ||
cookies == other.cookies; | ||
|
||
@override | ||
int get hashCode => uri.hashCode ^ cookies.hashCode; | ||
|
||
@override | ||
String toString() { | ||
return '_SaveCall{uri: $uri, cookies: $cookies}'; | ||
} | ||
} | ||
|
||
class _FakeCookieJar extends Fake implements CookieJar { | ||
final _saveCalls = <_SaveCall>[]; | ||
List<_SaveCall> get saveCalls => UnmodifiableListView(_saveCalls); | ||
|
||
@override | ||
Future<List<Cookie>> loadForRequest(Uri uri) async { | ||
return const []; | ||
} | ||
|
||
@override | ||
Future<void> saveFromResponse(Uri uri, List<Cookie> cookies) async { | ||
_saveCalls.add(_SaveCall( | ||
uri.toString(), | ||
cookies.join('; '), | ||
)); | ||
} | ||
} | ||
|
||
void main() { | ||
test( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? @kuhnroyal or @AlexV525 do you happen to know whether this is or can be a problem? I'm not really knowledgable in this area. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
'CookieJar.saveFromResponse() is called with a full Uri for requests that had relative redirects', | ||
() async { | ||
final cookieJar = _FakeCookieJar(); | ||
final dio = Dio() | ||
..httpClientAdapter = _TestAdapter( | ||
fetch: (options, requestStream, cancelFuture) async => ResponseBody( | ||
Stream.value(Uint8List.fromList(utf8.encode(''))), | ||
HttpStatus.ok, | ||
redirects: [ | ||
RedirectRecord( | ||
HttpStatus.found, | ||
'GET', | ||
Uri(path: 'redirect'), | ||
), | ||
], | ||
headers: { | ||
HttpHeaders.setCookieHeader: ['Cookie1=value1; Path=/'], | ||
}, | ||
), | ||
) | ||
..interceptors.add(CookieManager(cookieJar)) | ||
..options.validateStatus = | ||
(status) => status != null && status >= 200 && status < 400; | ||
|
||
await dio.get('https://test.com'); | ||
|
||
expect(cookieJar.saveCalls, [ | ||
_SaveCall( | ||
'https://test.com/redirect', | ||
'Cookie1=value1; Path=/', | ||
), | ||
]); | ||
}); | ||
test( | ||
'CookieJar.saveFromResponse() saves cookies only for final destination upon non-relative redirects', | ||
() async { | ||
final cookieJar = _FakeCookieJar(); | ||
final dio = Dio() | ||
..httpClientAdapter = _TestAdapter( | ||
fetch: (options, requestStream, cancelFuture) async => ResponseBody( | ||
Stream.value(Uint8List.fromList(utf8.encode(''))), | ||
HttpStatus.ok, | ||
redirects: [ | ||
RedirectRecord( | ||
HttpStatus.found, | ||
'GET', | ||
Uri.parse('https://example.com/redirect'), | ||
), | ||
], | ||
headers: { | ||
HttpHeaders.setCookieHeader: ['Cookie1=value1; Path=/'], | ||
}, | ||
), | ||
) | ||
..interceptors.add(CookieManager(cookieJar)) | ||
..options.validateStatus = | ||
(status) => status != null && status >= 200 && status < 400; | ||
|
||
await dio.get('https://test.com'); | ||
|
||
expect(cookieJar.saveCalls, [ | ||
_SaveCall( | ||
'https://example.com/redirect', | ||
'Cookie1=value1; Path=/', | ||
), | ||
]); | ||
}); | ||
} |
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.
Do we have any standards that indicate we should resolve the real dest based on the original one?
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.
https://www.rfc-editor.org/rfc/rfc7231#section-7.1.2
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.
Great. Could you add it as a comment, so we can track it back somedays if we forgot it.
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.
@s0nerik