Skip to content
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.

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

Server crashes when accessing body in middleware #362

Closed
ronaldmannak opened this issue Jan 25, 2024 · 5 comments
Closed

Server crashes when accessing body in middleware #362

ronaldmannak opened this issue Jan 25, 2024 · 5 comments

Comments

@ronaldmannak
Copy link

I am not sure if this issue is because I don't use Hummingbird correctly or not, but the server app crashes when accessing request.body.buffer in case the request is streamed (e.g. when a proxy server like Ngrok is in between the client and the server. This issue was brought to my attention by @upeugene, I'm tagging him here.

To replicate the issue, add a middleware to config, like MyAuthenticator in the example below.

  • When the server is called by a client directly, the middleware works as expected.
  • When the server is called via a proxy server like ngrog, the server app crashes.

Again, the issue might be a misunderstanding how to access the body of the request, but my assumption is the app shouldn't crash.

I will ask Eugene to copy and paste the call stack in a comment.

struct MyAuthenticator: HBAsyncAuthenticator {
    func authenticate(request: HBRequest) async throws -> User? {
        
        // Fetch the body of the request
        // request.body.buffer crashes the app if the request is streaming
        guard let buffer = request.body.buffer, let body = buffer.getString(at: buffer.readerIndex, length: buffer.readableBytes) else {
            throw HBHTTPError(.badRequest)
        }
        ...
@upeugene
Copy link

image

@adam-fowler
Copy link
Member

Sorry my error message isn't clear enough.
Before you access the body in middleware you need to collate the stream of buffers into one buffer. The most likely reason you are seeing this when running behind a proxy is because the proxy is breaking up the body into a lot smaller sections.

var request = request
request.collateBody().flatMap {
    /// You can now access 
    authenticate(request)
}

You also you have the issue where you are processing a stream of buffers in two places (the middleware and the route). If you collate the request body stream you need to edit the request to give it the buffer back, otherwise you are sending a request with an empty body to you route. Unfortunately an authenticator doesn't give you a chance to pass the edited request on. So either you'll have to write a Middleware that acts like an Authenticator but passes on the collated request body, or you could stick another middleware in front of your authenticator that does the request body collation

struct CollateBodyMiddleware: HBMiddleware {
    public func apply(to request: HBRequest, next: HBResponder) -> EventLoopFuture<HBResponse> {
        return request.collateBody().flatMap { request in
            return next.respond(to: request)
        }
    }
}

@ronaldmannak
Copy link
Author

Thanks the quick reply and examples. This is a clearly an implementation issue on my side and not a bug so I'll close the this issue.

I do have a question regarding some potential complexities related to asynchronous operations and event loops. For instance, I noticed that collateBody is based on the event loop, and the method apply(to request: HBRequest, next: HBResponder) async throws is not accessible within HBAsyncMiddleware because HBAsyncMiddleware extends HBMiddleware rather than HBAsyncMiddleware. I'm wondering if the upcoming HummingBird 2.0 release might address these issues and simplify the implementation. If that's the case, I might wait for the 2.0 release to resolve this

@adam-fowler
Copy link
Member

I'm not sure what you mean about the async apply method being unavailable in HBAsyncMiddleware.

collateBody needs to run on the EventLoop the request was created on but that is all managed internally. I thought I added an async version of it, but if not you can call

let request = try await request.collateBody().get()

V2 of Hummingbird simplifies this by removing all the EventLoop internals. It is a pure Swift Concurrency based solution.

@ronaldmannak
Copy link
Author

Thanks Adam, appreciate it.

Re: apply, I meant the async version of apply isn't available in HBAsyncAuthenticator since HBAsyncAuthenticator extends HBAuthenticator and HBAuthenticator extends HBMiddleware, which means HBAsyncAuthenticator does not extend HBAsyncMiddleware where the async version of apply is declared. It would indeed be great to have a single concurrency based version on HB, it will make it a lot easier to develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants