Update relevant channel request callbacks to return a bool #348
+77
−40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a breaking change, but it tweaks channel request callbacks to return a bool rather than requiring the user to manually call `session.channel_success` or `session.channel_failure`. This has the added advantage of changing the defaults of a number of request callbacks to more-secure defaults (deny), and makes it impossible for a user to miss responding to callbacks which require responses. Note that this does *not* handle sending responses for all requests, only channel requests listed in RFC4254 as having a "want reply" param rather than just "false", even though it may be more correct to respond to malformed requests which have improperly set that byte to "true" even though the RFC specifies "false".
belak
force-pushed
the
belak/simpler-channel-replies
branch
from
4242b34
to
144f58a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a breaking change (as it changes a trait), but it tweaks channel request callbacks to return a bool rather than requiring the user to manually call
session.channel_successorsession.channel_failure. Also, IIRC, the protocol docs specify that the request channel should continue to be serviced even when a session is started, so it makes sense to require users to spin off a background task and return the status.Alternatively this could be done as a non-breaking change by making the server implementation call
session.channel_failureafter a channel request is handled.I do understand there are valid reasons to deny this, but it seemed like an easy place for a user to make a mistake, and I wanted to see how hard this would be to change.
This has the added advantage of changing the defaults of a number of request callbacks to more-secure defaults (deny), and makes it impossible for a user to miss responding to callbacks which require responses. Even if this PR is not accepted, that change should probably be implemented - I would be happy to submit that separately if you'd prefer.
Note that this does not handle sending responses for all requests, only channel requests listed in RFC4254 as having a "want reply" param rather than just "false", even though it may be more correct to respond to malformed requests which have improperly set that byte to "true" even though the RFC specifies "false".
EDIT: with the combination of the channel message stream and the handlers, this should continue to work as expected, at least with sftp, but that's only because it uses
.into_stream()which only handlesdataand doesn't require a reply. I'm not certain the "correct" way to handle this - the channel is definitely useful because it allows you to get animpl AsyncRead / impl AsyncWrite, but it definitely complicates this.If you have any advice I'd love to hear it.