Falcon and protocol body rewind

[tleish]

We are currently using an older version of Falcon 0.42.3 (working on upgrading code to Ruby 3 soon) and using the ruby Grape API library. Grape calls input.read and then input.rewind, however if attempting to read again then the body is empty

see: https://github.com/ruby-grape/grape/blob/master/lib/grape/middleware/formatter.rb#L67-L79

I noticed that the body is Protocol::HTTP1::Body::Fixed which does not support rewind, but the Protocol::HTTP does support rewind. Debugging this a little, it appears falcon chooses the protocol on startup. It's not clear to me how falcon decided which protocol to use (at startup) and if it's possible to configure to use a different one that supports rewind.

[ioquatix]

You might be able to use https://github.com/socketry/protocol-rack/blob/main/lib/protocol/rack/rewindable.rb or something like that. You can configure that as part of your application stack using:

rack ... do
  middleware do
    Protocol::Rack::Rewindable.new(super())
  end
end

or something to that effect.

[tleish]

What's the overall logic behind choosing Protocol::HTTP vs Protocol::HTTP1?

[ioquatix]

Protocol::HTTP is HTTP semantics, Protocol::HTTP1 is the HTTP/1.x protocol parser/semantics specifically.

[tleish]

Ok, one last question. I'm curious as to why .read method does not throw an error, but also does not rewind the body. I'm more interested in the question of why not support rewind as expected. Is there something I should be aware of before I implement a middleware that supports it?

[ioquatix]

read just means read everything available. If there is nothing available, nothing is read. Does that answer your question?

[tleish]

Sorry, that was a typo. It meant to put .rewind (not.read). Why is .rewind essentially a no-op in falcon? Is there something I should be aware of before I implement a middleware that supports.rewind?

[joeljunstrom]

👋 Recently got bit by what I assume is the same behaviour as discussed here.

We have a path where we need to verify the integrity of an incoming post from Slack's api. For one reason or another they use application/x-www-form-urlencoded for ”interactions” and application/json for other events. The verification is the normal compare ”hmac of a shared secret, some data and the full post body” with an incoming header.

What bit us was that rails request.raw_post checks if the body is rewindable and if not just yields (https://github.com/rails/rails/blob/main/actionpack/lib/action_dispatch/http/request.rb#L531-L555) which I assume is pretty reasonable.

However under puma the request.body was just a StringIO or similar this ”just worked” while under falcon application/x-www-form-urlencoded is not rewindable which led to silent failures where we reject the post.

This might be absolutely something to discuss in Rails rather than here (the name raw_post kinda indicates that it will just give it to me in full) but feels like it would spill over here regardless. Would need some supported way of converting it to a rewindable?

While I get that Rack 3 does not require us to be rewindable, there are certainly cases where one would like to be 😅

We solved for now with a pretty nasty middleware

class SlackRawBodyBuffer
  PATH_PREFIX = "/callbacks/slack"

  def initialize(app)
    @app = app
  end

  def call(env)
    if env["PATH_INFO"]&.start_with?(PATH_PREFIX) && (input = env["rack.input"])
      body = input.read || ""
      input.close if input.respond_to?(:close)
      env["rack.input"] = StringIO.new(body)
      env["RAW_POST_DATA"] = body
    end

    @app.call(env)
  end
end

[ioquatix]

You can use https://github.com/socketry/protocol-rack/blob/main/lib/protocol/rack/rewindable.rb if you want rewindable inputs. However, that will apply to all requests, which introduces additional overhead. I think this is probably more of an application level concern, since that's the only place you'd really know "should I buffer this input". Since Request objects may consume the post body, it's tricky... the logic of the above middleware is to always buffer application/x-www-form-urlencoded.

Raw post data access via raw_post might be a bit of an anti-pattern. It probably makes sense to have a before action to read the post data, validate the checksum, reject the request before doing further processing (e.g. creating the Request object), etc.

cc @matthewd FYI.

[joeljunstrom]

I think this is probably more of an application level concern, since that's the only place you'd really know "should I buffer this input"

Yep! Just ”impossible” to do now without doing it early on the rack layer (as I understood the code).

Raw post data access via raw_post might be a bit of an anti-pattern. It probably makes sense to have a before action to read the post data, validate the checksum, reject the request before doing further processing (e.g. creating the Request object), etc.

Yep this is what we are doing but at that time the body as already been read and will return an empty string. Maybe one could somehow change rails raw_post to re-wrap the input and force the issue.

[matthewd]

Yeah, I don't think it's reasonable / net-valuable for Rails to try to counteract Rack's design decision by making all bodies rewindable by default.

The best result that comes to mind would be for read_body_stream to raise if the body is not rewindable, is at EOF, and was not empty. Off the top of my head I'm not sure whether we have the means of determining those on various servers' actual input streams though. (It's certainly not visible in the spec-mandated interface, but then neither is rewind 😄)

[ioquatix]

Yeah, I don't think it's reasonable / net-valuable for Rails to try to counteract Rack's design decision by making all bodies rewindable by default.

Just to be clear on this one point, the default for a network stream is "not rewindable" and the only way to make something rewindable is to buffer it into a file/memory. So Rack's design decision here is mostly just "what the network does".

The best result that comes to mind would be for read_body_stream to raise if the body is not rewindable, is at EOF, and was not empty.

Maybe it's possible that after reading env["rack.input"], if it was closed, further reads would raise IOError. So maybe on the normal code path, after a complete read is done, for whatever reason, you should also close it. Then, subsequent reads would fail.

[joeljunstrom]

It's all very reasonable with both the design of rack and how falcon handles it. But I guess the question is what the consumer of it (Rails) should do.

Given both backwards backward compatibility (request.raw_post returning what you expect) and app specific needs (like ours above) of looking at the raw request body it does feel like one should be able to opt in to buffer this and retrieve it even after rails has done an internal read of it.

But it feels really hard to find a good spot for that knowledge (before action needs params already parsed etc, routing layer does not really make sense).

[ioquatix]

But I guess the question is what the consumer of it (Rails) should do.

This is a totally reasonable question.

Probably the most sane fix here is the routing layer of Rails should know whether the input body should be cached or not, e.g. per-route middleware, and explicit failure trying to rewind/reread an input body.