Fix X-Forwarded-For and cleanup old code (#369)
diff --git a/pkgs/appengine/CHANGELOG.md b/pkgs/appengine/CHANGELOG.md
index baf76cf..1772f43 100644
--- a/pkgs/appengine/CHANGELOG.md
+++ b/pkgs/appengine/CHANGELOG.md
@@ -1,11 +1,13 @@
-## 0.13.13-wip
+## 0.13.13
* Rename error classes to use the `Exception` suffix
(`AppEngineException`, `NetworkException`, `ProtocolException`,
`ServiceException`, `ApplicationException`) to follow Dart conventions for
types implementing `Exception`. The old `*Error` names remain available as
deprecated type aliases, so this change is non-breaking.
-- Automated formatting and fixes.
+* Automated formatting and fixes.
+* Cleanup legacy code.
+* Fix `X-Forwarded-For` handling.
## 0.13.12
diff --git a/pkgs/appengine/lib/src/server/context_registry.dart b/pkgs/appengine/lib/src/server/context_registry.dart
index b9dfeba..e97ccb0 100644
--- a/pkgs/appengine/lib/src/server/context_registry.dart
+++ b/pkgs/appengine/lib/src/server/context_registry.dart
@@ -25,6 +25,9 @@
Logging newBackgroundLogger();
}
+/// Sanity check for traceId (must be 32 char hex)
+final _traceIdFormat = RegExp(r'^[0-9a-f]{32}$');
+
class ContextRegistry {
final LoggerFactory _loggingFactory;
final db.DatastoreDB _db;
@@ -42,13 +45,16 @@
ClientContext add(HttpRequest request) {
String? traceId;
- // See https://cloud.google.com/trace/docs/support
+ // See https://docs.cloud.google.com/trace/docs/trace-context#legacy-http-header
final traceHeader = _headerOrEmptyString(
request.headers,
'X-Cloud-Trace-Context',
);
if (traceHeader != '') {
- traceId = traceHeader.split('/')[0];
+ final traceIdFromHeader = traceHeader.split('/').first;
+ if (_traceIdFormat.hasMatch(traceIdFromHeader)) {
+ traceId = traceIdFromHeader;
+ }
}
final services = _getServices(request, traceId);
@@ -84,12 +90,19 @@
String ip;
if (forwardedFor != null && forwardedFor.isNotEmpty) {
- // It seems that, in general, if `x-forwarded-for` has multiple values
- // it is sent as a single header value separated by commas.
- // To ensure only one value for IP is provided, we join all of the
- // `x-forwarded-for` headers into a single string, split on comma,
- // then use the first value.
- ip = forwardedFor.join(',').split(',').first.trim();
+ // Google Cloud Load Balancers append the connecting client IP and the
+ // load balancer IP to any existing `X-Forwarded-For` values.
+ // Client-supplied (possibly spoofed) IPs appear first, so we use
+ // the second-to-last IP as the client IP that arrived at GCLB.
+ // See: https://cloud.google.com/load-balancing/docs/https#x-forwarded-for_header
+ final parts = forwardedFor
+ .expand((header) => header.split(','))
+ .map((ip) => ip.trim())
+ .where((ip) => ip.isNotEmpty)
+ .toList();
+ ip = parts.length >= 2
+ ? parts[parts.length - 2]
+ : request.connectionInfo!.remoteAddress.host;
} else {
ip = request.connectionInfo!.remoteAddress.host;
}
diff --git a/pkgs/appengine/lib/src/server/server.dart b/pkgs/appengine/lib/src/server/server.dart
index 5525ba1..5d3a699 100644
--- a/pkgs/appengine/lib/src/server/server.dart
+++ b/pkgs/appengine/lib/src/server/server.dart
@@ -3,7 +3,6 @@
// BSD-style license that can be found in the LICENSE file.
import 'dart:async';
-import 'dart:convert' show utf8;
import 'dart:io';
import '../client_context.dart';
@@ -22,9 +21,6 @@
final bool _shared;
final Completer _shutdownCompleter = Completer();
- int _pendingRequests = 0;
-
- HttpServer? _httpServer;
AppEngineHttpServer(this._contextRegistry,
{String hostname = '0.0.0.0', int port = 8080, bool shared = false})
@@ -38,34 +34,13 @@
Function(HttpRequest request, ClientContext context) applicationHandler, {
void Function(InternetAddress address, int port)? onAcceptingConnections,
}) {
- final serviceHandlers = {
- '/_ah/start': _start,
- '/_ah/health': _health,
- '/_ah/stop': _stop
- };
-
HttpServer.bind(_hostname, _port, shared: _shared)
.then((HttpServer server) {
- _httpServer = server;
if (onAcceptingConnections != null) {
onAcceptingConnections(server.address, server.port);
}
server.listen((HttpRequest request) {
- // Default handling is sending the request to the application.
- dynamic Function(HttpRequest, ClientContext)? handler =
- applicationHandler;
-
- // Check if the request path is one of the service handlers.
- final String path = request.uri.path;
- for (final pattern in serviceHandlers.keys) {
- if (path.startsWith(pattern)) {
- handler = serviceHandlers[pattern];
- break;
- }
- }
-
- _pendingRequests++;
final context = _contextRegistry.add(request);
request.response.done.whenComplete(() {
_contextRegistry.remove(request);
@@ -75,56 +50,10 @@
if (!_contextRegistry.isDevelopmentEnvironment) {
_info('Error while handling response: $error');
}
- _pendingRequests--;
- _checkShutdown();
});
- handler!(request, context);
+ applicationHandler(request, context);
});
});
}
-
- void _start(HttpRequest request, _) {
- request.drain().then((_) {
- _sendResponse(request.response, HttpStatus.ok, 'ok');
- });
- }
-
- void _health(HttpRequest request, _) {
- request.drain().then((_) {
- _sendResponse(request.response, HttpStatus.ok, 'ok');
- });
- }
-
- void _stop(HttpRequest request, _) {
- request.drain().then((_) {
- if (_httpServer != null) {
- _httpServer!.close().then((_) {
- _httpServer = null;
- _sendResponse(request.response, HttpStatus.ok, 'ok');
- });
- } else {
- _sendResponse(request.response, HttpStatus.conflict, 'fail');
- }
- });
- }
-
- void _checkShutdown() {
- if (_pendingRequests == 0 && _httpServer == null) {
- _shutdownCompleter.complete();
- }
- }
-
- void _sendResponse(HttpResponse response, int statusCode, String message) {
- final data = utf8.encode(message);
- response.headers.contentType =
- ContentType('text', 'plain', charset: 'utf-8');
- response.headers.set('Cache-Control', 'no-cache');
- response.headers.set('Server', _hostname);
- response
- ..contentLength = data.length
- ..statusCode = statusCode
- ..add(data)
- ..close();
- }
}
diff --git a/pkgs/appengine/pubspec.yaml b/pkgs/appengine/pubspec.yaml
index 7ad3525..b9ddf7e 100644
--- a/pkgs/appengine/pubspec.yaml
+++ b/pkgs/appengine/pubspec.yaml
@@ -1,5 +1,5 @@
name: appengine
-version: 0.13.13-wip
+version: 0.13.13
description: >-
Support for using Dart as a custom runtime on Google App Engine Flexible
Environment.
