Skip to content

Improve error message when length exceeds receive limit #1247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 26, 2021
Merged

Improve error message when length exceeds receive limit #1247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Sources/GRPC/GRPCClientChannelHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,8 @@ extension GRPCClientChannelHandler: ChannelInboundHandler {
case let .decompressionLimitExceeded(compressedSize):
return GRPCError.DecompressionLimitExceeded(compressedSize: compressedSize)
.captureContext()
case let .lengthExceedsLimit(underlyingError):
return underlyingError.captureContext()
Copy link
Collaborator

@fabianfett fabianfett Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we create the error in LengthPrefixedMessageReader.swift we already capture the context. Is it right, that we capture the context here again, even though we pass the original error onwards?

Copy link
Collaborator Author

@glbrntt glbrntt Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context just holds the error (and location info). Here we just have the wrapped error and no context. It's intentional that we capture it again, although the reason we have to do this is a bit gross: the error we're switching over is Equatable but the error wrapper providing context is not Equatable (because not all wrapped errors are Equatable). The server doesn't wrap/unwrap the error like this so we still need to capture the context in LengthPrefixedMessageReader.swift.

case .invalidState:
return GRPCError.InvalidState("parsing data as a response message").captureContext()
}
Expand Down
13 changes: 13 additions & 0 deletions Sources/GRPC/GRPCError.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,19 @@ public enum GRPCError {
}
}

/// The length of the received payload was longer than is permitted.
Copy link
Collaborator

@fabianfett fabianfett Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be really nice to the user, we could point them to the config struct that they need to tune.

public struct PayloadLengthLimitExceeded: GRPCErrorProtocol {
public let description: String

public init(actualLength length: Int, limit: Int) {
self.description = "Payload length exceeds limit (\(length) > \(limit))"
}

public func makeGRPCStatus() -> GRPCStatus {
return GRPCStatus(code: .resourceExhausted, message: self.description)
}
}

/// It was not possible to compress or decompress a message with zlib.
public struct ZlibCompressionFailure: GRPCErrorProtocol {
var code: Int32
Expand Down
5 changes: 4 additions & 1 deletion Sources/GRPC/LengthPrefixedMessageReader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,10 @@ internal struct LengthPrefixedMessageReader {
}

if messageLength > maxLength {
throw GRPCError.DeserializationFailure().captureContext()
throw GRPCError.PayloadLengthLimitExceeded(
actualLength: messageLength,
limit: maxLength
).captureContext()
}

self.state = .expectingMessage(messageLength, compressed: compressed)
Expand Down
16 changes: 11 additions & 5 deletions Sources/GRPC/ReadWriteStates.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,15 @@ enum ReadState {
}
} catch {
self = .notReading
if let grpcError = error as? GRPCError.WithContext,
let limitExceeded = grpcError.error as? GRPCError.DecompressionLimitExceeded {
return .failure(.decompressionLimitExceeded(limitExceeded.compressedSize))
} else {
return .failure(.deserializationFailed)
if let grpcError = error as? GRPCError.WithContext {
if let compressionLimit = grpcError.error as? GRPCError.DecompressionLimitExceeded {
return .failure(.decompressionLimitExceeded(compressionLimit.compressedSize))
} else if let lengthLimit = grpcError.error as? GRPCError.PayloadLengthLimitExceeded {
return .failure(.lengthExceedsLimit(lengthLimit))
}
}

return .failure(.deserializationFailed)
}

// We need to validate the number of messages we decoded. Zero is fine because the payload may
Expand Down Expand Up @@ -209,6 +212,9 @@ enum MessageReadError: Error, Equatable {
/// The limit for decompression was exceeded.
case decompressionLimitExceeded(Int)

/// The length of the message exceeded the permitted maximum length.
case lengthExceedsLimit(GRPCError.PayloadLengthLimitExceeded)

/// An invalid state was encountered. This is a serious implementation error.
case invalidState
}
16 changes: 8 additions & 8 deletions Tests/GRPCTests/GRPCMessageLengthLimitTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ final class GRPCMessageLengthLimitTests: GRPCTestCase {

let get = self.echo.get(self.makeRequest(minimumLength: 1024))
XCTAssertThrowsError(try get.response.wait())
XCTAssertEqual(try get.status.map { $0.code }.wait(), .internalError)
XCTAssertEqual(try get.status.map { $0.code }.wait(), .resourceExhausted)
}

func testServerRejectsLongClientStreamingRequest() throws {
Expand All @@ -83,7 +83,7 @@ final class GRPCMessageLengthLimitTests: GRPCTestCase {
// (No need to send end, the server is going to close the RPC because the message was too long.)

XCTAssertThrowsError(try collect.response.wait())
XCTAssertEqual(try collect.status.map { $0.code }.wait(), .internalError)
XCTAssertEqual(try collect.status.map { $0.code }.wait(), .resourceExhausted)
}

func testServerRejectsLongServerStreamingRequest() throws {
Expand All @@ -94,7 +94,7 @@ final class GRPCMessageLengthLimitTests: GRPCTestCase {
XCTFail("Unexpected response")
}

XCTAssertEqual(try expand.status.map { $0.code }.wait(), .internalError)
XCTAssertEqual(try expand.status.map { $0.code }.wait(), .resourceExhausted)
}

func testServerRejectsLongBidirectionalStreamingRequest() throws {
Expand All @@ -107,7 +107,7 @@ final class GRPCMessageLengthLimitTests: GRPCTestCase {
XCTAssertNoThrow(try update.sendMessage(self.makeRequest(minimumLength: 1024)).wait())
// (No need to send end, the server is going to close the RPC because the message was too long.)

XCTAssertEqual(try update.status.map { $0.code }.wait(), .internalError)
XCTAssertEqual(try update.status.map { $0.code }.wait(), .resourceExhausted)
}

func testClientRejectsLongUnaryResponse() throws {
Expand All @@ -117,7 +117,7 @@ final class GRPCMessageLengthLimitTests: GRPCTestCase {

let get = self.echo.get(.with { $0.text = String(repeating: "x", count: 1024) })
XCTAssertThrowsError(try get.response.wait())
XCTAssertEqual(try get.status.map { $0.code }.wait(), .internalError)
XCTAssertEqual(try get.status.map { $0.code }.wait(), .resourceExhausted)
}

func testClientRejectsLongClientStreamingResponse() throws {
Expand All @@ -130,7 +130,7 @@ final class GRPCMessageLengthLimitTests: GRPCTestCase {
XCTAssertNoThrow(try collect.sendEnd().wait())

XCTAssertThrowsError(try collect.response.wait())
XCTAssertEqual(try collect.status.map { $0.code }.wait(), .internalError)
XCTAssertEqual(try collect.status.map { $0.code }.wait(), .resourceExhausted)
}

func testClientRejectsLongServerStreamingRequest() throws {
Expand All @@ -143,7 +143,7 @@ final class GRPCMessageLengthLimitTests: GRPCTestCase {
XCTFail("Unexpected response")
}

XCTAssertEqual(try expand.status.map { $0.code }.wait(), .internalError)
XCTAssertEqual(try expand.status.map { $0.code }.wait(), .resourceExhausted)
}

func testClientRejectsLongServerBidirectionalStreamingResponse() throws {
Expand All @@ -157,6 +157,6 @@ final class GRPCMessageLengthLimitTests: GRPCTestCase {
// (No need to send end, the client will close the RPC when it receives a response which is too
// long.

XCTAssertEqual(try update.status.map { $0.code }.wait(), .internalError)
XCTAssertEqual(try update.status.map { $0.code }.wait(), .resourceExhausted)
}
}