Return correct response for unknown messages, add handler for unknown messages to Session (extends #112) - #114
Return correct response for unknown messages, add handler for unknown messages to Session (extends #112)#114jcspencer wants to merge 3 commits into
Session (extends #112)#114Conversation
Signed-off-by: Carlos Quintana <carlos@cquintana.dev>
Signed-off-by: James Spencer <github@jcspencer.net>
d10d6f0 to
b445171
Compare
|
|
||
| /// Handle the case where an unknown message is received from the client. | ||
| /// | ||
| /// By default - per [RFC9987 § 5.1](https://www.rfc-editor.org/rfc/rfc9987.html#section-5.1) - |
There was a problem hiding this comment.
Note to self: we should probably update the references to the draft RFC now that it's been accepted as a proposed standard 🤔
There was a problem hiding this comment.
Good idea, I think we can create an issue and then tackle that separately 👌
| /// However, if the message type _is_ known, a custom response can be returned to the client. | ||
| async fn unknown_message( | ||
| &mut self, | ||
| message_id: u8, |
There was a problem hiding this comment.
We interchage message_id and command in a few places; the RFC refers to them as type. Happy to take suggestions!
There was a problem hiding this comment.
Having one unified name that is aligned with the RFC would be ideal. Sadly type is a keyword in Rust 😥 we need something else hmm... type_id? 🤔 I'm not sure...
The timing to adjust all these places is good though since this would change the API anyway 😅
|
|
||
| /// From [PR#112](https://github.com/wiktor-k/ssh-agent-lib/pull/112), this | ||
| /// payload is an: | ||
| /// > `SSH2_AGENT_REQUEST_VERSION` (message type 1) [request] with a "2.0" payload, |
There was a problem hiding this comment.
Note that although the new Session::unknown_message handler allows responding to messages, it doesn't support custom response messages (yet). Perhaps we could add a Response variant to handle that in a follow-up PR 🤔
Add `Session::unknown_message` variant to allow processing messages not known to the library Add roundtrip test case for `Unknown` request type Remove `tests/unknown_request.rs` test now that the message is being roundtripped. Signed-off-by: James Spencer <github@jcspencer.net>
c411825 to
a09599d
Compare
wiktor-k
left a comment
wiktor-k
left a comment
There was a problem hiding this comment.
Looks great IMO. We need a rebase on top of main for the CI to be green again 🍃
|
|
||
| /// Handle the case where an unknown message is received from the client. | ||
| /// | ||
| /// By default - per [RFC9987 § 5.1](https://www.rfc-editor.org/rfc/rfc9987.html#section-5.1) - |
There was a problem hiding this comment.
Good idea, I think we can create an issue and then tackle that separately 👌
| /// However, if the message type _is_ known, a custom response can be returned to the client. | ||
| async fn unknown_message( | ||
| &mut self, | ||
| message_id: u8, |
There was a problem hiding this comment.
Having one unified name that is aligned with the RFC would be ideal. Sadly type is a keyword in Rust 😥 we need something else hmm... type_id? 🤔 I'm not sure...
The timing to adjust all these places is good though since this would change the API anyway 😅
| return match self.unknown_message(message_id, payload).await? { | ||
| Some(response) => Ok(response), | ||
| None => Ok(Response::Failure), | ||
| } |
There was a problem hiding this comment.
This is totally my preference but for Options I like using if let instead:
| return match self.unknown_message(message_id, payload).await? { | |
| Some(response) => Ok(response), | |
| None => Ok(Response::Failure), | |
| } | |
| return if let Some(response) self.unknown_message(message_id, payload).await? { | |
| Ok(response) | |
| } else { | |
| Ok(Response::Failure), | |
| } |
(with the formatting adjusted 😅 )
3 participants
This PR builds on the changes proposed by @cquintana92 in #112 (
fix: unknown request types should return failure)This:
Request::Unknownto include an payload of typeUnparsed.Session::unknown_message()method toSessiontrait to allow processing messages not known to the librarySee #112 for more information on the original issue.