Allow relative URLs in the redirect location header. #229
Conversation
| @@ -159,8 +159,10 @@ Future<bool> _safeUrlCheck( | |||
| if (response.isRedirect && | |||
| response.headers[HttpHeaders.locationHeader]!.isNotEmpty && | |||
| maxRedirects > 0) { | |||
| final loc = Uri.parse(response.headers[HttpHeaders.locationHeader]![0]); | |||
| final nextUri = loc.isAbsolute ? loc : url.resolveUri(loc); | |||
There was a problem hiding this comment.
Won't this always work?
| final nextUri = loc.isAbsolute ? loc : url.resolveUri(loc); | |
| final nextUri = url.resolveUri(loc); |
There was a problem hiding this comment.
Also how about adding a test case.
If we're really lazy we can probably use https://httpbin.org/redirect-to?url=status%2F200 to test it?
There was a problem hiding this comment.
I've prepared a server + redirect logic for a test, but the test logic checks for local / private IPs and it always fails for such tests. Any preference how to suppress the IP checks for tests?
There was a problem hiding this comment.
Honestly, isn't it tempting to be lazy and just use: https://httpbin.org/redirect-to?url=status%2F200
Ignoring such local / private IP tests is kind of a bad idea, since the whole point is to avoid hitting private networks :D
I suppose one could do a one-off using a constant like bool.fromEnvironment() but that doesn't make testing easier, since you'll have to do -D .. something.
Let's either not test it, or use httpbin to not make our problems bigger than they have to be.
There was a problem hiding this comment.
Honestly, isn't it tempting to be lazy and just use: https://httpbin.org/redirect-to?url=status%2F200
Already added that: https://github.com/google/dart-neats/pull/229/files#diff-d7c4d80be7934367cf516c2c7d1c65dd444fc2a10786ea20790381d3c361dea1R30
Ignoring such local / private IP tests is kind of a bad idea, since the whole point is to avoid hitting private networks :D
I think it is not unreasonable to turn off some parts of the library for local testing - it feels that more coverage is better in this case. I'll check if I can implement something that is not exposed to the end user in any other way.
There was a problem hiding this comment.
Updated the PR with refactor + override for local testing. I can remove the commit if needed, but I think it is better to have it this way.
isoos
force-pushed
the
relative-redirect
branch
from
af56554 to
1a87ca1
Compare
| } | ||
| } | ||
|
|
||
| class SafeUrlChecker { |
There was a problem hiding this comment.
Why are we creating a class that has no purpose?
This is just a convoluted way to make function, right?
Why not just keep the old _safeUrlCheck make it safeUrlCheck in the safe_url_check/lib/src/safe_url_check.dart file and add a parameter skipLocalNetworkCheck which defaults to false and has @visibleForTestingOnly annotation.
Or we could just accept that we don't have testing, or use httpbin.org for testing.
There was a problem hiding this comment.
Or put the parameter on doSafeUrlCheck.
This introduces a class SafeUrlChecker which we only every construct inorder to call a single method.
It's just a confusing way to have a function that you can pass arguments to in two different ways.
Another, somewhat simpler and ugly option is to make a global:
@visibleForTestingOnly
bool skipLocalNetworkCheck = false;Put this in an lib/src/... file and check it from within safeUrlCheck.
If no code actively sets this property, hopefully the compiler will optimize it away.
There was a problem hiding this comment.
This is just a convoluted way to make function, right?
I didn't like the recursive function with 6+ parameters. The class keeps the non-changing parts as fields.
Fixes #228.