From e69a55b5322cecf276ec84f70ae01281264e4624 Mon Sep 17 00:00:00 2001 From: Gwynne Raskind Date: Thu, 9 May 2024 21:35:27 -0500 Subject: [PATCH] Log source file and line info for errors in ErrorMiddleware when possible (#3187) * Make ErrorMiddleware report source location (file, function, line) if available from the given error (will always be available on Abort errors). Don't hold a lock while doing a JSON encode. * Improve the correctness of the error messages provided for DecodingErrors. Update tests accordingly. * Don't use hardcoded strings where not necessary --- Sources/Vapor/Error/AbortError.swift | 27 ++++---- .../Vapor/Middleware/ErrorMiddleware.swift | 62 +++++++++---------- Tests/VaporTests/ContentTests.swift | 8 +-- 3 files changed, 44 insertions(+), 53 deletions(-) diff --git a/Sources/Vapor/Error/AbortError.swift b/Sources/Vapor/Error/AbortError.swift index 205a4a51ce..ba8beade88 100644 --- a/Sources/Vapor/Error/AbortError.swift +++ b/Sources/Vapor/Error/AbortError.swift @@ -68,18 +68,17 @@ extension DecodingError: AbortError { /// See `AbortError.reason` public var reason: String { switch self { - case .dataCorrupted(let ctx): - return "Data corrupted at path '\(ctx.codingPath.dotPath)'\(ctx.debugDescriptionAndUnderlyingError)" - case .keyNotFound(let key, let ctx): - let path = ctx.codingPath + [key] - return "Value required for key at path '\(path.dotPath)'\(ctx.debugDescriptionAndUnderlyingError)" - case .typeMismatch(let type, let ctx): - return "Value at path '\(ctx.codingPath.dotPath)' was not of type '\(type)'\(ctx.debugDescriptionAndUnderlyingError)" - case .valueNotFound(let type, let ctx): - return "Value of type '\(type)' was not found at path '\(ctx.codingPath.dotPath)'\(ctx.debugDescriptionAndUnderlyingError)" - @unknown default: return "Unknown error." + case let .dataCorrupted(ctx): return "Data corrupted \(self.contextReason(ctx))" + case let .keyNotFound(key, ctx): return "No such key '\(key.stringValue)' \(self.contextReason(ctx))" + case let .typeMismatch(type, ctx): return "Value was not of type '\(type)' \(self.contextReason(ctx))" + case let .valueNotFound(type, ctx): return "No value found (expected type '\(type)') \(self.contextReason(ctx))" + @unknown default: return "Unknown error" } } + + private func contextReason(_ context: Context) -> String { + "at path '\(context.codingPath.dotPath)'\(context.debugDescriptionAndUnderlyingError)" + } } private extension DecodingError.Context { @@ -92,17 +91,13 @@ private extension DecodingError.Context { if self.debugDescription.isEmpty { return "" } else if self.debugDescription.last == "." { - return ". \(String(self.debugDescription.dropLast()))" + return ". \(self.debugDescription.dropLast())" } else { return ". \(self.debugDescription)" } } private var underlyingErrorDescription: String { - if let underlyingError = self.underlyingError { - return ". Underlying error: \(underlyingError)" - } else { - return "" - } + self.underlyingError.map { ". Underlying error: \(String(describing: $0))" } ?? "" } } diff --git a/Sources/Vapor/Middleware/ErrorMiddleware.swift b/Sources/Vapor/Middleware/ErrorMiddleware.swift index 97211dde24..c367079bcd 100644 --- a/Sources/Vapor/Middleware/ErrorMiddleware.swift +++ b/Sources/Vapor/Middleware/ErrorMiddleware.swift @@ -20,48 +20,44 @@ public final class ErrorMiddleware: Middleware { /// - environment: The environment to respect when presenting errors. public static func `default`(environment: Environment) -> ErrorMiddleware { return .init { req, error in - // variables to determine - let status: HTTPResponseStatus - let reason: String - let headers: HTTPHeaders + let status: HTTPResponseStatus, reason: String, source: ErrorSource + var headers: HTTPHeaders - // inspect the error type + // Inspect the error type and extract what data we can. switch error { + case let debugAbort as (DebuggableError & AbortError): + (reason, status, headers, source) = (debugAbort.reason, debugAbort.status, debugAbort.headers, debugAbort.source ?? .capture()) + case let abort as AbortError: - // this is an abort error, we should use its status, reason, and headers - reason = abort.reason - status = abort.status - headers = abort.headers + (reason, status, headers, source) = (abort.reason, abort.status, abort.headers, .capture()) + + case let debugErr as DebuggableError: + (reason, status, headers, source) = (debugErr.reason, .internalServerError, [:], debugErr.source ?? .capture()) + default: - // if not release mode, and error is debuggable, provide debug info - // otherwise, deliver a generic 500 to avoid exposing any sensitive error info - reason = environment.isRelease - ? "Something went wrong." - : String(describing: error) - status = .internalServerError - headers = [:] + // In debug mode, provide the error description; otherwise hide it to avoid sensitive data disclosure. + reason = environment.isRelease ? "Something went wrong." : String(describing: error) + (status, headers, source) = (.internalServerError, [:], .capture()) } - // Report the error to logger. - req.logger.report(error: error) - - // create a Response with appropriate status - let response = Response(status: status, headers: headers) + // Report the error + req.logger.report(error: error, file: source.file, function: source.function, line: source.line) // attempt to serialize the error to json + let body: Response.Body do { - let errorResponse = ErrorResponse(error: true, reason: reason) - try response.responseBox.withLockedValue { box in - box.body = try .init(data: JSONEncoder().encode(errorResponse), byteBufferAllocator: req.byteBufferAllocator) - box.headers.replaceOrAdd(name: .contentType, value: "application/json; charset=utf-8") - } + body = .init( + buffer: try JSONEncoder().encodeAsByteBuffer(ErrorResponse(error: true, reason: reason), allocator: req.byteBufferAllocator), + byteBufferAllocator: req.byteBufferAllocator + ) + headers.contentType = .json } catch { - response.responseBox.withLockedValue { box in - box.body = .init(string: "Oops: \(error)", byteBufferAllocator: req.byteBufferAllocator) - box.headers.replaceOrAdd(name: .contentType, value: "text/plain; charset=utf-8") - } + body = .init(string: "Oops: \(String(describing: error))\nWhile encoding error: \(reason)", byteBufferAllocator: req.byteBufferAllocator) + headers.contentType = .plainText } - return response + + // create a Response with appropriate status + return Response(status: status, headers: headers, body: body) } } @@ -77,8 +73,8 @@ public final class ErrorMiddleware: Middleware { } public func respond(to request: Request, chainingTo next: Responder) -> EventLoopFuture { - return next.respond(to: request).flatMapErrorThrowing { error in - return self.closure(request, error) + next.respond(to: request).flatMapErrorThrowing { error in + self.closure(request, error) } } } diff --git a/Tests/VaporTests/ContentTests.swift b/Tests/VaporTests/ContentTests.swift index 5015a60152..a529549b6e 100644 --- a/Tests/VaporTests/ContentTests.swift +++ b/Tests/VaporTests/ContentTests.swift @@ -80,7 +80,7 @@ final class ContentTests: XCTestCase { try app.testable().test(.GET, "/decode_error") { res in XCTAssertEqual(res.status, .badRequest) - XCTAssertContains(res.body.string, #"Value at path 'bar' was not of type 'Int'. Expected to decode Int but found a string"#) + XCTAssertContains(res.body.string, #"Value was not of type 'Int' at path 'bar'. Expected to decode Int but found a string"#) } } @@ -564,7 +564,7 @@ final class ContentTests: XCTestCase { XCTAssertThrowsError(try req.content.decode(PostInput.self)) { error in XCTAssertEqual( (error as? AbortError)?.reason, - #"Value required for key at path 'is_free'. No value associated with key CodingKeys(stringValue: "is_free", intValue: nil) ("is_free")."# + #"No such key 'is_free' at path ''. No value associated with key CodingKeys(stringValue: "is_free", intValue: nil) ("is_free")."# ) } } @@ -617,7 +617,7 @@ final class ContentTests: XCTestCase { XCTAssertThrowsError(try req.content.decode(DecodeModel.self)) { error in XCTAssertEqual( (error as? AbortError)?.reason, - #"Value of type 'String' was not found at path 'items.Index 1'. Unkeyed container is at end."# + #"No value found (expected type 'String') at path 'items.Index 1'. Unkeyed container is at end."# ) } } @@ -642,7 +642,7 @@ final class ContentTests: XCTestCase { XCTAssertThrowsError(try req.content.decode(DecodeModel.self)) { error in XCTAssertContains( (error as? AbortError)?.reason, - #"Value at path 'item.title' was not of type 'Int'. Expected to decode Int but found a string"# + #"Value was not of type 'Int' at path 'item.title'. Expected to decode Int but found a string"# ) } }