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

Allow relative URLs in the redirect location header. #229

Merged
merged 5 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions safe_url_check/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## v.1.1.2
* Allow relative URLs in the redirect `location` header.

## v1.1.1
* Added `topics` to `pubspec.yaml`.

Expand Down
120 changes: 10 additions & 110 deletions safe_url_check/lib/safe_url_check.dart
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,7 @@ import 'dart:async';

import 'package:retry/retry.dart';

import 'src/private_ip.dart';
import 'src/unique_local_ip.dart';
import 'src/version.dart';

const _defaultUserAgent = 'package:safe_url_check/$packageVersion '
'(+https://github.com/google/dart-neats/tree/master/safe_url_check)';
import 'src/safe_url_check.dart';

/// Check if [url] is available, without allowing access to private networks.
///
Expand All @@ -63,112 +58,17 @@ const _defaultUserAgent = 'package:safe_url_check/$packageVersion '
Future<bool> safeUrlCheck(
Uri url, {
int maxRedirects = 8,
String userAgent = _defaultUserAgent,
String userAgent = defaultUserAgent,
HttpClient? client,
RetryOptions retryOptions = const RetryOptions(maxAttempts: 3),
Duration timeout = const Duration(seconds: 90),
}) async {
ArgumentError.checkNotNull(url, 'url');
ArgumentError.checkNotNull(maxRedirects, 'maxRedirects');
ArgumentError.checkNotNull(userAgent, 'userAgent');
ArgumentError.checkNotNull(retryOptions, 'retryOptions');
if (maxRedirects < 0) {
throw ArgumentError.value(
maxRedirects,
'maxRedirects',
'must be a positive integer',
);
}

try {
// Create client if one wasn't given.
var c = client;
c ??= HttpClient();
try {
return await _safeUrlCheck(
url,
maxRedirects,
c,
userAgent,
retryOptions,
timeout,
);
} finally {
// Close client, if it was created here.
if (client == null) {
c.close(force: true);
}
}
} on Exception {
return false;
}
}

Future<bool> _safeUrlCheck(
Uri url,
int maxRedirects,
HttpClient client,
String userAgent,
RetryOptions retryOptions,
Duration timeout,
) async {
assert(maxRedirects >= 0);

// If no scheme or not http or https, we fail.
if (!url.hasScheme || (!url.isScheme('http') && !url.isScheme('https'))) {
return false;
}

final ips = await retryOptions.retry(() async {
final ips = await InternetAddress.lookup(url.host).timeout(timeout);
if (ips.isEmpty) {
throw Exception('DNS resolution failed');
}
return ips;
});
for (final ip in ips) {
// If given a loopback, linklocal or multicast IP, return false
if (ip.isLoopback ||
ip.isLinkLocal ||
ip.isMulticast ||
isPrivateIpV4(ip) ||
isUniqueLocalIpV6(ip)) {
return false;
}
}

final response = await retryOptions.retry(() async {
// We can't use the HttpClient from dart:io with a custom socket, so instead
// of making a connection to one of the IPs resolved above, and specifying
// the host header, we rely on the OS caching DNS queries and not returning
// different IPs for a second lookup.
final request = await client.headUrl(url).timeout(timeout);
request.followRedirects = false;
request.headers.set(HttpHeaders.userAgentHeader, userAgent);
final response = await request.close().timeout(timeout);
await response.drain().catchError((e) => null).timeout(timeout);
if (500 <= response.statusCode && response.statusCode < 600) {
// retry again, when we hit a 5xx response
throw Exception('internal server error');
}
return response;
});
if (200 <= response.statusCode && response.statusCode < 300) {
return true;
}
if (response.isRedirect &&
response.headers[HttpHeaders.locationHeader]!.isNotEmpty &&
maxRedirects > 0) {
return _safeUrlCheck(
Uri.parse(response.headers[HttpHeaders.locationHeader]![0]),
maxRedirects - 1,
client,
userAgent,
retryOptions,
timeout,
);
}

// Response is 4xx, or some other unsupported code.
return false;
return doSafeUrlCheck(
url,
maxRedirects: maxRedirects,
userAgent: userAgent,
client: client,
retryOptions: retryOptions,
timeout: timeout,
);
}
144 changes: 144 additions & 0 deletions safe_url_check/lib/src/safe_url_check.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
// Copyright 2023 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import 'dart:io';

import 'package:retry/retry.dart';

import 'private_ip.dart';
import 'unique_local_ip.dart';
import 'version.dart';

const defaultUserAgent = 'package:safe_url_check/$packageVersion '
'(+https://github.com/google/dart-neats/tree/master/safe_url_check)';

Future<bool> doSafeUrlCheck(
Uri url, {
int maxRedirects = 8,
String userAgent = defaultUserAgent,
HttpClient? client,
RetryOptions retryOptions = const RetryOptions(maxAttempts: 3),
Duration timeout = const Duration(seconds: 90),
}) async {
ArgumentError.checkNotNull(url, 'url');
ArgumentError.checkNotNull(maxRedirects, 'maxRedirects');
ArgumentError.checkNotNull(userAgent, 'userAgent');
ArgumentError.checkNotNull(retryOptions, 'retryOptions');
if (maxRedirects < 0) {
throw ArgumentError.value(
maxRedirects,
'maxRedirects',
'must be a positive integer',
);
}

try {
// Create client if one wasn't given.
var c = client;
c ??= HttpClient();
try {
return await SafeUrlChecker(
client: c,
userAgent: userAgent,
retryOptions: retryOptions,
timeout: timeout,
).checkUrl(url, maxRedirects);
} finally {
// Close client, if it was created here.
if (client == null) {
c.close(force: true);
}
}
} on Exception {
return false;
}
}

class SafeUrlChecker {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, PTAL.

final HttpClient client;
final String userAgent;
final RetryOptions retryOptions;
final Duration timeout;
final bool skipLocalNetworkCheck;

SafeUrlChecker({
required this.client,
required this.userAgent,
required this.retryOptions,
required this.timeout,
this.skipLocalNetworkCheck = false,
isoos marked this conversation as resolved.
Show resolved Hide resolved
});

Future<bool> checkUrl(
Uri url,
int maxRedirects,
) async {
assert(maxRedirects >= 0);

// If no scheme or not http or https, we fail.
if (!url.hasScheme || (!url.isScheme('http') && !url.isScheme('https'))) {
return false;
}

final ips = await retryOptions.retry(() async {
final ips = await InternetAddress.lookup(url.host).timeout(timeout);
if (ips.isEmpty) {
throw Exception('DNS resolution failed');
}
return ips;
});
if (!skipLocalNetworkCheck) {
for (final ip in ips) {
// If given a loopback, linklocal or multicast IP, return false
if (ip.isLoopback ||
ip.isLinkLocal ||
ip.isMulticast ||
isPrivateIpV4(ip) ||
isUniqueLocalIpV6(ip)) {
return false;
}
}
}

final response = await retryOptions.retry(() async {
// We can't use the HttpClient from dart:io with a custom socket, so instead
// of making a connection to one of the IPs resolved above, and specifying
// the host header, we rely on the OS caching DNS queries and not returning
// different IPs for a second lookup.
final request = await client.headUrl(url).timeout(timeout);
request.followRedirects = false;
request.headers.set(HttpHeaders.userAgentHeader, userAgent);
final response = await request.close().timeout(timeout);
await response.drain().catchError((e) => null).timeout(timeout);
if (500 <= response.statusCode && response.statusCode < 600) {
// retry again, when we hit a 5xx response
throw Exception('internal server error');
}
return response;
});
if (200 <= response.statusCode && response.statusCode < 300) {
return true;
}
if (response.isRedirect &&
response.headers[HttpHeaders.locationHeader]!.isNotEmpty &&
maxRedirects > 0) {
final loc = Uri.parse(response.headers[HttpHeaders.locationHeader]![0]);
final nextUri = url.resolveUri(loc);
return checkUrl(nextUri, maxRedirects - 1);
}

// Response is 4xx, or some other unsupported code.
return false;
}
}
2 changes: 1 addition & 1 deletion safe_url_check/lib/src/version.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions safe_url_check/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: safe_url_check
version: 1.1.1
version: 1.1.2
description: >-
Check if an untrusted URL is broken, without allowing connections to a private
IP address.
Expand All @@ -18,6 +18,6 @@ dependencies:

dev_dependencies:
test: ^1.5.1
lints: ^2.0.1
lints: ^3.0.0
build_runner: ^2.3.3
build_version: ^2.0.1
61 changes: 61 additions & 0 deletions safe_url_check/test/relative_redirect_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright 2023 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import 'dart:io';

import 'package:retry/retry.dart';
import 'package:safe_url_check/src/safe_url_check.dart';
import 'package:test/test.dart';

void main() {
late HttpServer server;
setUpAll(() async {
server = await HttpServer.bind(InternetAddress.anyIPv4, 0);
server.listen((e) async {
switch (e.requestedUri.path) {
case '/redirect/local':
e.response.statusCode = 303;
e.response.headers.set('location', 'target');
await e.response.close();
return;
case '/redirect/target':
e.response.write('OK');
await e.response.close();
return;
default:
e.response.statusCode = 404;
await e.response.close();
}
});
});

tearDownAll(() async {
await server.close();
});

test('relative redirect', () async {
final client = HttpClient();
final checker = SafeUrlChecker(
client: client,
userAgent: defaultUserAgent,
retryOptions: RetryOptions(),
timeout: Duration(seconds: 2),
skipLocalNetworkCheck: true,
);
expect(
await checker.checkUrl(
Uri.parse('http://localhost:${server.port}/redirect/local'), 2),
isTrue);
});
}
1 change: 1 addition & 0 deletions safe_url_check/test/safe_url_check_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ void main() {
testValidUrl('https://google.com');
testValidUrl('https://github.com');
testValidUrl('https://github.com/google/dart-neats.git');
testValidUrl('https://httpbin.org/redirect-to?url=status%2F200');
testInvalidUrl('https://github.com/google/dart-neats.git/bad-url');
}