Opened 5 years ago

Last modified 5 years ago

#3995 enhancement new

need unit test that ssh_CHANNEL_OPEN sends a packet rather than raising a TypeError

Reported by: jesstess Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, thijs, exarkun Branch:
Author: Launchpad Bug:

Description

Prior to the changes in #3657, ConchUser.lookupChannel raised a ConchError with the arguments reversed. twisted.conch.ssh.connection.ssh_CHANNEL_OPEN uses the attributes of ConchError to send an SSH message so this reversal caused the traceback described at the top of #3657.

Once #3657 is closed, we need a unit test that the behavior is now correct and ssh_CHANNEL_OPEN sends a packet rather than raising a TypeError.

Attachments (1)

conch-SSH_CHANNEL_OPEN-test.patch (1.7 KB) - added by jesstess 5 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

The patch tests that a ConchError coming from lookupChannel is logged and doesn't cause an exception when trying to stuff a packet with the textual information in the ConchError's 'data' field.

comment:2 Changed 5 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to jesstess

The patch should also update the copyright date.

Changed 5 years ago by jesstess

comment:3 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

Copyright updated.

comment:4 Changed 5 years ago by exarkun

  • Cc exarkun added
  • Keywords review removed
  • Owner set to jesstess

The test looks good, as far as it goes. It needs to go a bit futher, though (and its docstring suggests this): ssh_CHANNEL_OPEN ends with a call to sendPacket; the test should verify that that call is doing the right thing. I'm not quite sure what the right thing is, or whether the implementation is currently correct or not, partly because I'm not sure how this patch is intended to interact with the patch from #3657 (and partly because this is a pretty low-level part of Conch which I haven't looked at before :). Can you elaborate on that, or re-submit this after #3657 is resolved?

comment:5 Changed 3 years ago by <automation>

  • Owner jesstess deleted
Note: See TracTickets for help on using tickets.