Skip to content

Commit 268c4cf

Browse files
isoosisoos
authored
Strengthen the input validation for /documentation/* requests. (dart-lang#3630)
* Strengthen the input validation for /documentation/* requests. * More escaping in URLs
1 parent ea59606 commit 268c4cf

File tree

4 files changed

+82
-50
lines changed

4 files changed

+82
-50
lines changed

‎app/lib/frontend/handlers/documentation.dart

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ import 'dart:async';
66
import 'dart:io';
77

88
import 'package:http_parser/http_parser.dart';
9+
import 'package:path/path.dart' as p;
10+
// ignore: implementation_imports
11+
import 'package:pub_package_reader/src/names.dart';
12+
import 'package:pub_semver/pub_semver.dart';
913
import 'package:shelf/shelf.dart' as shelf;
1014

1115
import '../../dartdoc/backend.dart';
@@ -26,7 +30,7 @@ Future<shelf.Response> documentationHandler(shelf.Request request) async {
2630
if (redirectDartdocPages.containsKey(docFilePath.package)) {
2731
return redirectResponse(redirectDartdocPages[docFilePath.package]);
2832
}
29-
if (docFilePath.package == null || docFilePath.version == null) {
33+
if (docFilePath.package != null && docFilePath.version == null) {
3034
return redirectResponse(pkgDocUrl(docFilePath.package, isLatest: true));
3135
}
3236
if (docFilePath.path == null) {
@@ -108,13 +112,17 @@ DocFilePath parseRequestUri(Uri uri) {
108112
if (uri.pathSegments[0] != 'documentation') return null;
109113

110114
final String package = uri.pathSegments[1];
111-
if (package.isEmpty) return null;
115+
// reject empty or invalid package names
116+
if (package.isEmpty || !identifierExpr.hasMatch(package)) {
117+
return null;
118+
}
112119
if (segmentCount == 2) {
113120
return DocFilePath(package, null, null);
114121
}
115122

116123
final String version = uri.pathSegments[2];
117-
if (version.isEmpty) {
124+
// reject empty or invalid version names
125+
if (!_isValidVersion(version)) {
118126
return DocFilePath(package, null, null);
119127
}
120128
if (segmentCount == 3) {
@@ -123,11 +131,21 @@ DocFilePath parseRequestUri(Uri uri) {
123131

124132
final relativeSegments =
125133
uri.pathSegments.skip(3).where((s) => s.isNotEmpty).toList();
126-
String path = relativeSegments.join('/');
127-
if (path.isEmpty) {
128-
path = 'index.html';
129-
} else if (path.isNotEmpty && !relativeSegments.last.contains('.')) {
130-
path = '$path/index.html';
134+
var pathSegments = relativeSegments;
135+
if (relativeSegments.isEmpty || !relativeSegments.last.contains('.')) {
136+
pathSegments = [...relativeSegments, 'index.html'];
131137
}
138+
final path = p.normalize(p.joinAll(pathSegments));
132139
return DocFilePath(package, version, path);
133140
}
141+
142+
bool _isValidVersion(String version) {
143+
if (version.trim().isEmpty) return false;
144+
if (version == 'latest') return true;
145+
try {
146+
Version.parse(version);
147+
} on FormatException catch (_) {
148+
return false;
149+
}
150+
return true;
151+
}

‎app/lib/shared/urls.dart

Lines changed: 22 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import 'package:path/path.dart' as p;
77
import '../frontend/request_context.dart' show requestContext;
88
import '../package/overrides.dart';
99
import '../search/search_service.dart' show SearchOrder, SearchQuery;
10-
import 'versions.dart';
1110

1211
export '../search/search_service.dart' show SearchOrder;
1312

@@ -18,21 +17,25 @@ const siteRoot = 'https://$primaryHost';
1817
const dartSiteRoot = 'https://dart.dev';
1918
const httpsApiDartDev = 'https://api.dart.dev/';
2019

20+
final _siteRootUri = Uri.parse('$siteRoot/');
21+
final _pathRootUri = Uri(path: '/');
22+
2123
String pkgPageUrl(
2224
String package, {
2325
String version,
2426
bool includeHost = false,
2527
String fragment,
2628
}) {
27-
String url = includeHost ? siteRoot : '';
28-
url += '/packages/$package';
29-
if (version != null) {
30-
url += '/versions/$version';
31-
}
32-
if (fragment != null) {
33-
url += '#$fragment';
34-
}
35-
return url;
29+
final segments = <String>[
30+
'packages',
31+
package,
32+
if (version != null) 'versions',
33+
if (version != null) version,
34+
];
35+
final baseUri = includeHost ? _siteRootUri : _pathRootUri;
36+
return baseUri
37+
.resolveUri(Uri(pathSegments: segments, fragment: fragment))
38+
.toString();
3639
}
3740

3841
String pkgReadmeUrl(String package, {String version}) =>
@@ -80,14 +83,17 @@ String pkgDocUrl(
8083
bool omitTrailingSlash = false,
8184
bool isLatest = false,
8285
}) {
83-
String url = includeHost ? siteRoot : '';
84-
url += '/documentation/$package';
85-
if (isLatest) {
86+
if (isLatest || version == null) {
8687
version = 'latest';
8788
}
88-
if (version != null) {
89-
url += '/$version';
90-
}
89+
final segments = <String>[
90+
'documentation',
91+
package,
92+
version,
93+
];
94+
final baseUri = includeHost ? _siteRootUri : _pathRootUri;
95+
String url = baseUri.resolveUri(Uri(pathSegments: segments)).toString();
96+
9197
if (relativePath != null) {
9298
url = p.join(url, relativePath);
9399
} else if (!omitTrailingSlash) {
@@ -240,15 +246,6 @@ String inferServiceProviderName(String url) {
240246
return null;
241247
}
242248

243-
String panaUrl() {
244-
return '$siteRoot/packages/pana';
245-
}
246-
247-
/// Returns the versioned documentation URL for pana's maintenance suggestions.
248-
String panaMaintenanceUrl() {
249-
return '$siteRoot/documentation/pana/$panaVersion/#maintenance-score';
250-
}
251-
252249
/// Returns the consent URL that will be sent to the invited user.
253250
String consentUrl(String consentId) => '$siteRoot/consent?id=$consentId';
254251

‎app/test/frontend/handlers/documentation_test.dart

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,34 +25,43 @@ void main() {
2525
}
2626
}
2727

28-
test('/documentation', () {
29-
testUri('/documentation', null);
28+
test('bad prefix', () {
29+
testUri('/doc/pkg/latest/', null);
3030
});
31-
test('/documentation/', () {
31+
32+
test('insufficient prefix', () {
33+
testUri('/documentation', null);
3234
testUri('/documentation/', null);
3335
});
34-
test('/documentation/angular', () {
35-
testUri('/documentation/angular', 'angular');
36+
37+
test('bad package name', () {
38+
testUri('/documentation//latest/', null);
39+
testUri('/documentation/<pkg>/latest/', null);
40+
testUri('/documentation/pkg with space/latest/', null);
3641
});
37-
test('/documentation/angular/', () {
42+
43+
test('no version specified', () {
44+
testUri('/documentation/angular', 'angular');
3845
testUri('/documentation/angular/', 'angular');
3946
});
40-
test('/documentation/angular/4.0.0+2', () {
41-
testUri('/documentation/angular/4.0.0+2', 'angular', '4.0.0+2');
47+
48+
test('bad version', () {
49+
testUri('/documentation/pkg//', 'pkg');
50+
testUri('/documentation/pkg/first-release/', 'pkg');
51+
testUri('/documentation/pkg/1.2.3.4.5.6/', 'pkg');
4252
});
43-
test('/documentation/angular/4.0.0+2/', () {
53+
54+
test('version without path', () {
55+
testUri('/documentation/angular/4.0.0+2', 'angular', '4.0.0+2');
4456
testUri('/documentation/angular/4.0.0+2/', 'angular', '4.0.0+2',
4557
'index.html');
4658
});
47-
test('/documentation/angular/4.0.0+2/subdir/', () {
59+
60+
test('version with a path', () {
4861
testUri('/documentation/angular/4.0.0+2/subdir/', 'angular', '4.0.0+2',
4962
'subdir/index.html');
50-
});
51-
test('/documentation/angular/4.0.0+2/file.html', () {
5263
testUri('/documentation/angular/4.0.0+2/file.html', 'angular', '4.0.0+2',
5364
'file.html');
54-
});
55-
test('/documentation/angular/4.0.0+2/file.html', () {
5665
testUri('/documentation/angular/4.0.0+2/file.html', 'angular', '4.0.0+2',
5766
'file.html');
5867
});
@@ -76,7 +85,7 @@ void main() {
7685
testWithServices('/documentation/foor/bar redirect', () async {
7786
await expectRedirectResponse(
7887
await issueGet('/documentation/foor/bar'),
79-
'https://pub.dev/documentation/foor/bar/',
88+
'/documentation/foor/latest/',
8089
);
8190
});
8291

‎app/test/shared/urls_test.dart

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ void main() {
1212
expect(pkgPageUrl('foo_bar'), '/packages/foo_bar');
1313
expect(pkgPageUrl('foo_bar', version: '1.0.0'),
1414
'/packages/foo_bar/versions/1.0.0');
15+
expect(pkgPageUrl('foo_bar', version: '1.0.0', fragment: 'hash'),
16+
'/packages/foo_bar/versions/1.0.0#hash');
1517
});
1618

1719
test('with host', () {
@@ -24,7 +26,7 @@ void main() {
2426

2527
group('documentation page', () {
2628
test('without host', () {
27-
expect(pkgDocUrl('foo_bar'), '/documentation/foo_bar/');
29+
expect(pkgDocUrl('foo_bar'), '/documentation/foo_bar/latest/');
2830
expect(pkgDocUrl('foo_bar', version: '1.0.0'),
2931
'/documentation/foo_bar/1.0.0/');
3032
expect(pkgDocUrl('foo_bar', version: '1.0.0', omitTrailingSlash: true),
@@ -33,7 +35,7 @@ void main() {
3335

3436
test('with host', () {
3537
expect(pkgDocUrl('foo_bar', includeHost: true),
36-
'https://pub.dev/documentation/foo_bar/');
38+
'https://pub.dev/documentation/foo_bar/latest/');
3739
expect(pkgDocUrl('foo_bar', version: '1.0.0', includeHost: true),
3840
'https://pub.dev/documentation/foo_bar/1.0.0/');
3941
expect(
@@ -180,6 +182,12 @@ void main() {
180182
});
181183

182184
group('search urls', () {
185+
test('non-identifier characters', () {
186+
expect(searchUrl(q: 'a b'), '/packages?q=a+b');
187+
expect(searchUrl(q: '<a>'), '/packages?q=%3Ca%3E');
188+
expect(searchUrl(q: 'ó'), '/packages?q=%C3%B3');
189+
});
190+
183191
test('sdk:*', () {
184192
expect(searchUrl(), '/packages');
185193
expect(searchUrl(q: 'abc'), '/packages?q=abc');

0 commit comments

Comments
 (0)