Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-gcj9-jj38-hwmc
* Start backfilling some tests for metrics

* Add failing test for not found route

* Rewrite 404 path to avoid DOS attack

* Failing test to distinguish between undefined routes and routes that return a 404

* Finalise path expressions

* Don't crash the tests by bootstrapping multiple times

* Tests passing

* Make tests non-throwing

* Check request.route instead of adding new bool

* Update Tests/VaporTests/Utilities/CapturingMetricsSystem.swift

Co-authored-by: Gwynne Raskind <gwynne@darkrainfall.org>

* Add attribution for CapturingMetricsSystem

* Update Tests/VaporTests/Utilities/CapturingMetricsSystem.swift

Co-authored-by: Gwynne Raskind <gwynne@darkrainfall.org>

* Remove last uses of NSUUID

* Revert change to stop the tests crashing

* Backfill more tests

* Migrate CapturingMetricsSystem to NIOConcurrencyHelpers

* Ensure undefined route methods get mapped to undefined

* Add tests for the timers as well

* Reuse dimensions for the timer as well

* Rewrite undefined routes' method to undefined as well as the path

* Avoid duplicating the method in the path

* Add comment for rewriting path in metrics

* Add test to ensure we don't pass through the ID for a dynamic path

* Fix typo in comment

Co-authored-by: Gwynne Raskind <gwynne@darkrainfall.org>
  • Loading branch information
0xTim and gwynne committed Feb 22, 2021
1 parent b23d83a commit e3aa712
Show file tree
Hide file tree
Showing 4 changed files with 349 additions and 14 deletions.
21 changes: 21 additions & 0 deletions NOTICES.txt
@@ -0,0 +1,21 @@

//===----------------------------------------------------------------------===//
//
// This source file is part of the Vapor open source project
//
// Copyright (c) 2017-2021 Vapor project authors
// Licensed under MIT
//
// See LICENSE for license information
//
// SPDX-License-Identifier: MIT
//
//===----------------------------------------------------------------------===//

This product contains a derivation of the TestMetrics test implementation
from Swift Metrics.

* LICENSE (Apache License 2.0):
* https://www.apache.org/licenses/LICENSE-2.0
* HOMEPAGE:
* https://github.com/apple/swift-metrics
38 changes: 24 additions & 14 deletions Sources/Vapor/Responder/DefaultResponder.swift
Expand Up @@ -41,13 +41,10 @@ internal struct DefaultResponder: Responder {
public func respond(to request: Request) -> EventLoopFuture<Response> {
let startTime = DispatchTime.now().uptimeNanoseconds
let response: EventLoopFuture<Response>
let path: String
if let cachedRoute = self.getRoute(for: request) {
path = cachedRoute.route.description
request.route = cachedRoute.route
response = cachedRoute.responder.respond(to: request)
} else {
path = request.url.path
response = self.notFoundResponder.respond(to: request)
}
return response.always { result in
Expand All @@ -60,7 +57,6 @@ internal struct DefaultResponder: Responder {
}
self.updateMetrics(
for: request,
path: path,
startTime: startTime,
statusCode: status.code
)
Expand All @@ -83,25 +79,39 @@ internal struct DefaultResponder: Responder {
/// Records the requests metrics.
private func updateMetrics(
for request: Request,
path: String,
startTime: UInt64,
statusCode: UInt
) {
let counterDimensions = [
("method", request.method.string),
("path", path),
let pathForMetrics: String
if let route = request.route {
// We don't use route.description here to avoid duplicating the method in the path
pathForMetrics = "/\(route.path.map { "\($0)" }.joined(separator: "/"))"
} else {
// If the route is undefined (i.e. a 404 and not something like /users/:userID
// We rewrite the path and the method to undefined to avoid DOSing the
// application and any downstream metrics systems. Otherwise an attacker
// could spam the service with unlimited requests and exhaust the system
// with unlimited timers/counters
pathForMetrics = "vapor_route_undefined"
}
let methodForMetrics: String
if request.route == nil {
methodForMetrics = "undefined"
} else {
methodForMetrics = request.method.string
}
let dimensions = [
("method", methodForMetrics),
("path", pathForMetrics),
("status", statusCode.description),
]
Counter(label: "http_requests_total", dimensions: counterDimensions).increment()
Counter(label: "http_requests_total", dimensions: dimensions).increment()
if statusCode >= 500 {
Counter(label: "http_request_errors_total", dimensions: counterDimensions).increment()
Counter(label: "http_request_errors_total", dimensions: dimensions).increment()
}
Timer(
label: "http_request_duration_seconds",
dimensions: [
("method", request.method.string),
("path", path)
],
dimensions: dimensions,
preferredDisplayUnit: .seconds
).recordNanoseconds(DispatchTime.now().uptimeNanoseconds - startTime)
}
Expand Down
126 changes: 126 additions & 0 deletions Tests/VaporTests/MetricsTests.swift
@@ -0,0 +1,126 @@
import XCTVapor
import Vapor
import Metrics
@testable import CoreMetrics

class MetricsTests: XCTestCase {
func testMetricsIncreasesCounter() {
let metrics = CapturingMetricsSystem()
MetricsSystem.bootstrapInternal(metrics)

let app = Application(.testing)
defer { app.shutdown() }

struct User: Content {
let id: Int
let name: String
}

app.routes.get("users", ":userID") { req -> User in
let userID = try req.parameters.require("userID", as: Int.self)
if userID == 1 {
return User(id: 1, name: "Tim")
} else {
throw Abort(.notFound)
}
}

XCTAssertNoThrow(try app.testable().test(.GET, "/users/1") { res in
XCTAssertEqual(res.status, .ok)
let resData = try res.content.decode(User.self)
XCTAssertEqual(resData.id, 1)
XCTAssertEqual(metrics.counters.count, 1)
let counter = metrics.counters["http_requests_total"] as! TestCounter
print(counter.dimensions)
let pathDimension = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "path"}))
XCTAssertEqual(pathDimension.1, "/users/:userID")
XCTAssertNil(counter.dimensions.first(where: { $0.0 == "path" && $0.1 == "/users/1" }))
let methodDimension = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "method"}))
XCTAssertEqual(methodDimension.1, "GET")
let status = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "status"}))
XCTAssertEqual(status.1, "200")

let timer = metrics.timers["http_request_duration_seconds"] as! TestTimer
let timerPathDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "path"}))
XCTAssertEqual(timerPathDimension.1, "/users/:userID")
let timerMethodDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "method"}))
XCTAssertEqual(timerMethodDimension.1, "GET")
let timerStatusDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "status"}))
XCTAssertEqual(timerStatusDimension.1, "200")
})
}

func testID404DoesntSpamMetrics() {
let metrics = CapturingMetricsSystem()
MetricsSystem.bootstrapInternal(metrics)

let app = Application(.testing)
defer { app.shutdown() }

struct User: Content {
let id: Int
let name: String
}

app.routes.get("users", ":userID") { req -> User in
let userID = try req.parameters.require("userID", as: Int.self)
if userID == 1 {
return User(id: 1, name: "Tim")
} else {
throw Abort(.notFound)
}
}

XCTAssertNoThrow(try app.testable().test(.GET, "/users/2") { res in
XCTAssertEqual(res.status, .notFound)
let counter = metrics.counters["http_requests_total"] as! TestCounter
let pathDimension = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "path"}))
XCTAssertEqual(pathDimension.1, "/users/:userID")
let methodDimension = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "method"}))
XCTAssertEqual(methodDimension.1, "GET")
let status = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "status"}))
XCTAssertEqual(status.1, "404")
XCTAssertNil(counter.dimensions.first(where: { $0.1 == "200" }))
XCTAssertNil(counter.dimensions.first(where: { $0.0 == "path" && $0.1 == "/users/1" }))

let timer = metrics.timers["http_request_duration_seconds"] as! TestTimer
let timerPathDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "path"}))
XCTAssertEqual(timerPathDimension.1, "/users/:userID")
let timerMethodDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "method"}))
XCTAssertEqual(timerMethodDimension.1, "GET")
let timerStatusDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "status"}))
XCTAssertEqual(timerStatusDimension.1, "404")
XCTAssertNil(timer.dimensions.first(where: { $0.1 == "200" }))
})
}

func test404RewritesPathForMetricsToAvoidDOSAttack() {
let metrics = CapturingMetricsSystem()
MetricsSystem.bootstrapInternal(metrics)

let app = Application(.testing)
defer { app.shutdown() }

XCTAssertNoThrow(try app.testable().test(.GET, "/not/found") { res in
XCTAssertEqual(res.status, .notFound)
XCTAssertEqual(metrics.counters.count, 1)
let counter = metrics.counters["http_requests_total"] as! TestCounter
let pathDimension = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "path"}))
XCTAssertEqual(pathDimension.1, "vapor_route_undefined")
let methodDimension = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "method"}))
XCTAssertEqual(methodDimension.1, "undefined")
let status = try XCTUnwrap(counter.dimensions.first(where: { $0.0 == "status"}))
XCTAssertEqual(status.1, "404")

let timer = metrics.timers["http_request_duration_seconds"] as! TestTimer
let timerPathDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "path"}))
XCTAssertEqual(timerPathDimension.1, "vapor_route_undefined")
let timerMethodDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "method"}))
XCTAssertEqual(timerMethodDimension.1, "undefined")
let timerStatusDimension = try XCTUnwrap(timer.dimensions.first(where: { $0.0 == "status"}))
XCTAssertEqual(timerStatusDimension.1, "404")
XCTAssertNil(timer.dimensions.first(where: { $0.1 == "200" }))
})
}
}

178 changes: 178 additions & 0 deletions Tests/VaporTests/Utilities/CapturingMetricsSystem.swift
@@ -0,0 +1,178 @@
// ===----------------------------------------------------------------------===##
//
// This source file is part of the Vapor open source project
//
// Copyright (c) 2017-2021 Vapor project authors
// Licensed under MIT
//
// See LICENSE for license information
//
// SPDX-License-Identifier: MIT
//
// ===----------------------------------------------------------------------===##
// This was adapted from Swift Metrics's TestMetrics.swift code.
// The license for the original work is reproduced below. See NOTICES.txt for
// more.
import Metrics
import Foundation
import NIOConcurrencyHelpers

/// Metrics factory which allows inspecting recorded metrics programmatically.
/// Only intended for tests of the Metrics API itself.
internal final class CapturingMetricsSystem: MetricsFactory {
private let lock = Lock()
var counters = [String: CounterHandler]()
var recorders = [String: RecorderHandler]()
var timers = [String: TimerHandler]()

public func makeCounter(label: String, dimensions: [(String, String)]) -> CounterHandler {
return self.make(label: label, dimensions: dimensions, registry: &self.counters, maker: TestCounter.init)
}

public func makeRecorder(label: String, dimensions: [(String, String)], aggregate: Bool) -> RecorderHandler {
let maker = { (label: String, dimensions: [(String, String)]) -> RecorderHandler in
TestRecorder(label: label, dimensions: dimensions, aggregate: aggregate)
}
return self.make(label: label, dimensions: dimensions, registry: &self.recorders, maker: maker)
}

public func makeTimer(label: String, dimensions: [(String, String)]) -> TimerHandler {
return self.make(label: label, dimensions: dimensions, registry: &self.timers, maker: TestTimer.init)
}

private func make<Item>(label: String, dimensions: [(String, String)], registry: inout [String: Item], maker: (String, [(String, String)]) -> Item) -> Item {
return self.lock.withLock {
let item = maker(label, dimensions)
registry[label] = item
return item
}
}

func destroyCounter(_ handler: CounterHandler) {
if let testCounter = handler as? TestCounter {
self.counters.removeValue(forKey: testCounter.label)
}
}

func destroyRecorder(_ handler: RecorderHandler) {
if let testRecorder = handler as? TestRecorder {
self.recorders.removeValue(forKey: testRecorder.label)
}
}

func destroyTimer(_ handler: TimerHandler) {
if let testTimer = handler as? TestTimer {
self.timers.removeValue(forKey: testTimer.label)
}
}
}

internal class TestCounter: CounterHandler, Equatable {
let id: String
let label: String
let dimensions: [(String, String)]

let lock = Lock()
var values = [(Date, Int64)]()

init(label: String, dimensions: [(String, String)]) {
self.id = UUID().uuidString
self.label = label
self.dimensions = dimensions
}

func increment(by amount: Int64) {
self.lock.withLock {
self.values.append((Date(), amount))
}
print("adding \(amount) to \(self.label)")
}

func reset() {
self.lock.withLock {
self.values = []
}
print("resetting \(self.label)")
}

public static func == (lhs: TestCounter, rhs: TestCounter) -> Bool {
return lhs.id == rhs.id
}
}

internal class TestRecorder: RecorderHandler, Equatable {
let id: String
let label: String
let dimensions: [(String, String)]
let aggregate: Bool

let lock = Lock()
var values = [(Date, Double)]()

init(label: String, dimensions: [(String, String)], aggregate: Bool) {
self.id = UUID().uuidString
self.label = label
self.dimensions = dimensions
self.aggregate = aggregate
}

func record(_ value: Int64) {
self.record(Double(value))
}

func record(_ value: Double) {
self.lock.withLock {
values.append((Date(), value))
}
print("recording \(value) in \(self.label)")
}

public static func == (lhs: TestRecorder, rhs: TestRecorder) -> Bool {
return lhs.id == rhs.id
}
}

internal class TestTimer: TimerHandler, Equatable {
let id: String
let label: String
var displayUnit: TimeUnit?
let dimensions: [(String, String)]

let lock = Lock()
var values = [(Date, Int64)]()

init(label: String, dimensions: [(String, String)]) {
self.id = UUID().uuidString
self.label = label
self.displayUnit = nil
self.dimensions = dimensions
}

func preferDisplayUnit(_ unit: TimeUnit) {
self.lock.withLock {
self.displayUnit = unit
}
}

func retriveValueInPreferredUnit(atIndex i: Int) -> Double {
return self.lock.withLock {
let value = values[i].1
guard let displayUnit = self.displayUnit else {
return Double(value)
}
return Double(value) / Double(displayUnit.scaleFromNanoseconds)
}
}

func recordNanoseconds(_ duration: Int64) {
self.lock.withLock {
values.append((Date(), duration))
}
print("recording \(duration) \(self.label)")
}

public static func == (lhs: TestTimer, rhs: TestTimer) -> Bool {
return lhs.id == rhs.id
}
}

0 comments on commit e3aa712

Please sign in to comment.