[L] Change in dart/sdk[main]: Reland "[security] Harden Host and Origin checks across VM Service, D...
1 view
Skip to first unread message
Ben Konyi (Gerrit)
Aug 4, 2026, 10:44:38 AM (yesterday) Aug 4
to Mark Zhou, Nicholas Shahan, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org
Attention needed from Mark Zhou and Nicholas Shahan
Ben Konyi voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Mark Zhou
- Nicholas Shahan
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Aug 4, 2026, 1:29:39 PM (yesterday) Aug 4
to Ben Konyi, Nicholas Shahan, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org
Attention needed from Ben Konyi and Nicholas Shahan
Mark Zhou added 2 comments![]( "Open in Gerrit")
File pkg/dart_runtime_service/lib/src/handlers.dart
Line 127, Patchset 4 (Latest):
bool _isAllowedHost(String hostHeader, DartRuntimeService frontend) {Mark Zhou . unresolved
nit: Could some of this be made shareable with `isAllowedOrigin`?
File pkg/dtd_impl/lib/src/dart_tooling_daemon.dart
Line 373, Patchset 4 (Latest):
uri.host == '::1') {Mark Zhou . unresolved
Is this vs the other impl of `_isAllowedOrigin` accepted - or are they equivalent? I'm not too familiar with the '::1' shorthand syntax.
Related details
Attention is currently required from:
- Ben Konyi
- Nicholas Shahan
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Aug 4, 2026, 2:24:26 PM (yesterday) Aug 4
to Mark Zhou, Nicholas Shahan, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org
Attention needed from Mark Zhou and Nicholas Shahan
Ben Konyi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ben Konyi
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
File pkg/dart_runtime_service/lib/src/handlers.dart
Line 127, Patchset 4:
bool _isAllowedHost(String hostHeader, DartRuntimeService frontend) {Mark Zhou . resolved
nit: Could some of this be made shareable with `isAllowedOrigin`?
Ben Konyi
Done
File pkg/dtd_impl/lib/src/dart_tooling_daemon.dart
Is this vs the other impl of `_isAllowedOrigin` accepted - or are they equivalent? I'm not too familiar with the '::1' shorthand syntax.
Ben Konyi
These are equivalent. `::1` is the IPv6 equivalent of `127.0.0.1` 😊
Related details
Attention is currently required from:
- Mark Zhou
- Nicholas Shahan
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Aug 4, 2026, 4:20:28 PM (yesterday) Aug 4
to Ben Konyi, Mark Zhou, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org
Attention needed from Ben Konyi and Mark Zhou
Nicholas Shahan added 13 comments![]( "Open in Gerrit")
File pkg/dart_runtime_service/lib/src/handlers.dart
Line 127, Patchset 4:
bool _isAllowedHost(String hostHeader, DartRuntimeService frontend) {Nicholas Shahan . unresolved
Needs a doc comment. What formatting is expected in this String?
Line 130, Patchset 4:
uri = Uri.parse('http://$hostHeader');Nicholas Shahan . unresolved
Is adding the scheme here needed? It isn't used in any way right?
File pkg/dds/lib/src/dds_impl.dart
Line 349, Patchset 5 (Latest):
/// Shelf middleware to validate Host and Origin headers to prevent
/// DNS-rebinding and CSRF attacks.Nicholas Shahan . unresolved
This isn't a well formed doc comment. It should read more like a description of what it does with the inputs and what is returned.
Line 368, Patchset 5 (Latest):
// No origin sent. This is a non-browser client or a same-origin request.
// Since we already validated the Host header, we know it's a legitimate
// local same-origin request (or a local non-browser tool).Nicholas Shahan . unresolved
nit: line length
File pkg/dds/lib/src/utils/validation.dart
Line 27, Patchset 5 (Latest):
// scheme (http:// or https://), and we prepend one when validating Host headers.Nicholas Shahan . unresolved
nit: line length
Line 65, Patchset 5 (Latest):
// Host header might not have a scheme, so prepend http:// to parse it as an origin.Nicholas Shahan . unresolved
nit: line length
File pkg/dds/test/devtools_server/security_test.dart
Line 26, Patchset 5 (Latest):
{
final request = await client.getUrl(serverUri.resolve('api/ping'));
final response = await request.close();
expect(response.statusCode, HttpStatus.ok);
await response.drain();
}Nicholas Shahan . unresolved
Is this block pattern common? I haven't really seen it much. Is it just ensuring various bits don't leak between expectations? Should these actually be seperate test cases?
Line 71, Patchset 5 (Latest):
// Bad Host Header (should be ALLOWED -> 200 OK because checks are disabled)Nicholas Shahan . unresolved
nit: line length
Line 79, Patchset 5 (Latest):
// Bad Origin Header to /api/sse (should be ALLOWED -> not 403, though it might fail with 400/404 because of invalid SSE handshake)Nicholas Shahan . unresolved
nit: line length
File pkg/dds/test/security_test.dart
Line 34, Patchset 5 (Latest):
// 1. Legitimate GET request to DDS-handled path (should return 404 Not Found, but NOT 403)Nicholas Shahan . unresolved
nit: line length
Line 35, Patchset 5 (Latest):
{
final request = await client.getUrl(dds.uri!.resolve('devtools'));
final response = await request.close();
expect(response.statusCode, HttpStatus.notFound);
await response.drain();
}Nicholas Shahan . unresolved
ditto for these blocks too.
Line 93, Patchset 5 (Latest):
// Bad Host Header (should be ALLOWED -> 404 because DevTools not served, but not 403)Nicholas Shahan . unresolved
nit: line length
File pkg/vm_service/test/http_invocations/http_security_test.dart
Line 11, Patchset 5 (Latest):
final securityTests = <IsolateTest>[Nicholas Shahan . unresolved
Is this a list of one test? Should we separate them too?
Related details
Attention is currently required from:
- Ben Konyi
- Mark Zhou
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
12:04 PM (7 hours ago) 12:04 PM
to Mark Zhou, Nicholas Shahan, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org
Attention needed from Mark Zhou and Nicholas Shahan
Ben Konyi voted and added 13 comments![]( "Open in Gerrit")
Votes added by Ben Konyi
| Auto-Submit | +1 |
| Commit-Queue | +1 |
13 comments
File pkg/dart_runtime_service/lib/src/handlers.dart
Line 127, Patchset 4:
bool _isAllowedHost(String hostHeader, DartRuntimeService frontend) {Nicholas Shahan . resolved
Needs a doc comment. What formatting is expected in this String?
Ben Konyi
Done
Line 130, Patchset 4:
uri = Uri.parse('http://$hostHeader');Nicholas Shahan . resolved
Is adding the scheme here needed? It isn't used in any way right?
Ben Konyi
It's needed so `localhost:8080` doesn't result in `localhost` being interpreted as the scheme for the URI.
File pkg/dds/lib/src/dds_impl.dart
Line 349, Patchset 5:
/// Shelf middleware to validate Host and Origin headers to prevent
/// DNS-rebinding and CSRF attacks.Nicholas Shahan . resolved
This isn't a well formed doc comment. It should read more like a description of what it does with the inputs and what is returned.
Ben Konyi
Done
Line 368, Patchset 5:
// No origin sent. This is a non-browser client or a same-origin request.
// Since we already validated the Host header, we know it's a legitimate
// local same-origin request (or a local non-browser tool).Nicholas Shahan . resolved
Ben Konyinit: line length
Done
File pkg/dds/lib/src/utils/validation.dart
Line 27, Patchset 5:
// scheme (http:// or https://), and we prepend one when validating Host headers.Nicholas Shahan . resolved
Ben Konyinit: line length
Done
Line 65, Patchset 5:
// Host header might not have a scheme, so prepend http:// to parse it as an origin.Nicholas Shahan . resolved
Ben Konyinit: line length
Done
File pkg/dds/test/devtools_server/security_test.dart
final request = await client.getUrl(serverUri.resolve('api/ping'));
final response = await request.close();
expect(response.statusCode, HttpStatus.ok);
await response.drain();
}Nicholas Shahan . resolved
Is this block pattern common? I haven't really seen it much. Is it just ensuring various bits don't leak between expectations? Should these actually be seperate test cases?
Ben Konyi
Yeah, these should be separate test cases. Done.
Line 71, Patchset 5:
// Bad Host Header (should be ALLOWED -> 200 OK because checks are disabled)Nicholas Shahan . resolved
Ben Konyinit: line length
Done
// Bad Origin Header to /api/sse (should be ALLOWED -> not 403, though it might fail with 400/404 because of invalid SSE handshake)Nicholas Shahan . resolved
Ben Konyinit: line length
Done
File pkg/dds/test/security_test.dart
Line 34, Patchset 5:
// 1. Legitimate GET request to DDS-handled path (should return 404 Not Found, but NOT 403)Nicholas Shahan . resolved
Ben Konyinit: line length
Done
final request = await client.getUrl(dds.uri!.resolve('devtools'));
final response = await request.close();
expect(response.statusCode, HttpStatus.notFound);
await response.drain();
}Nicholas Shahan . resolved
ditto for these blocks too.
Ben Konyi
Done
Line 93, Patchset 5:
// Bad Host Header (should be ALLOWED -> 404 because DevTools not served, but not 403)Nicholas Shahan . resolved
Ben Konyinit: line length
Done
File pkg/vm_service/test/http_invocations/http_security_test.dart
Is this a list of one test? Should we separate them too?
Attention is currently required from:
- Mark Zhou
- Nicholas Shahan
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages