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

RefreshPorts code path doesn't really make sense #509

Open
williammartin opened this issue Jan 21, 2025 · 1 comment
Open

RefreshPorts code path doesn't really make sense #509

williammartin opened this issue Jan 21, 2025 · 1 comment

Comments

@williammartin
Copy link

williammartin commented Jan 21, 2025

Description

Someone shared this error log for gh cs ssh (used to connect to GitHub codespaces over ssh):

kex_exchange_identification: read: Connection reset by peer
Connection reset by 127.0.0.1 port 65225
tunnel closed: error forwarding port: refresh ports failed: failed to refresh ports: %!w(<nil>)

Looking at the code where this error is returned, there doesn't appear to be any case in which the error will be non-nil:

res, _, err := c.ssh.SendSessionRequest("RefreshPorts", true, make([]byte, 0))
if err != nil {
return fmt.Errorf("failed to send port refresh message: %w", err)
}
if !res {
return fmt.Errorf("failed to refresh ports: %w", err)
}
return err
}

I have no opinion on what the message should say. Happy to open a PR with your recommendation.


Separately, as a minor nit reading this code, it seems like it would be better for the final return on this method to be return nil, since again, no other value is possible.

@williammartin
Copy link
Author

In looking at the SSH spec, a false boolean return seems to be an occurrence of the server sending SSH_MSG_REQUEST_FAILURE, which happens when "If the recipient does not recognize or support the request"

https://datatracker.ietf.org/doc/html/rfc4254#section-9

So I suspect this should really say something like:

 	if !res { 
 		return errors.New("failed to refresh ports: server did not recognise or support the request") 
 	} 

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

1 participant