Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions internal/usecase/devices/interceptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,13 @@ func (uc *UseCase) ListenToDevice(deviceConnection *DeviceConnection) {
conn := deviceConnection.Conn

defer func() {
// Clean up on exit
// Notify the browser immediately so the UI updates without waiting for
// ListenToBrowser to unblock on its ReadMessage call.
_ = deviceConnection.Conn.WriteMessage(
websocket.CloseMessage,
websocket.FormatCloseMessage(websocket.CloseNormalClosure, "AMT session ended"),
)
_ = deviceConnection.Conn.Close()
Comment on lines +220 to +224
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ListenToDevice, the function writes device data using the locally captured conn := deviceConnection.Conn, but the new deferred cleanup closes deviceConnection.Conn directly. Since deviceConnection.Conn can be updated elsewhere (e.g., reconnect path), this can lead to closing a different WebSocket than the one used for writes (and potentially leaving the original connection open). Consider using a single, consistent reference for both writing and cleanup (e.g., close conn, or access/update Conn under a lock and always use the current value).

Suggested change
_ = deviceConnection.Conn.WriteMessage(
websocket.CloseMessage,
websocket.FormatCloseMessage(websocket.CloseNormalClosure, "AMT session ended"),
)
_ = deviceConnection.Conn.Close()
if conn != nil {
_ = conn.WriteMessage(
websocket.CloseMessage,
websocket.FormatCloseMessage(websocket.CloseNormalClosure, "AMT session ended"),
)
_ = conn.Close()
}

Copilot uses AI. Check for mistakes.
deviceConnection.cancel()
}()

Expand Down Expand Up @@ -650,7 +656,7 @@ func writeLength(buf *bytes.Buffer, challenge *client.AuthChallenge, response st
return ErrLengthLimit // If total length is too large, throws an error and stops here
}

length := uint32(totalLength) //nolint:gosec // Ignore potential integer overflow here as overflow is validated earlier in code
length := uint32(totalLength) // overflow validated above
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

length := uint32(totalLength) will likely be flagged by the enabled gosec linter (integer conversion) even though there's a bounds check above. Since this file already uses //nolint:gosec for similar validated conversions, consider restoring a //nolint:gosec on this line or refactoring the conversion in a way that satisfies gosec so CI doesn't fail.

Suggested change
length := uint32(totalLength) // overflow validated above
length := uint32(totalLength) //nolint:gosec // overflow validated above via explicit bounds check

Copilot uses AI. Check for mistakes.

return binary.Write(buf, binary.LittleEndian, length)
}
Expand Down
88 changes: 88 additions & 0 deletions internal/usecase/devices/interceptor_private_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,22 @@ package devices

import (
"bytes"
"context"
"errors"
"io"
"math"
"strings"
"testing"
"time"

"github.com/gorilla/websocket"
"github.com/stretchr/testify/require"

"github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman"
"github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman/client"

"github.com/device-management-toolkit/console/internal/entity"
"github.com/device-management-toolkit/console/pkg/logger"
)

func TestProcessBrowserData(t *testing.T) {
Expand Down Expand Up @@ -834,3 +841,84 @@ func TestRandomValueHexErrorCase(t *testing.T) {
require.NoError(t, err)
require.Empty(t, result)
}

// spyRedirection is a minimal Redirection stub whose RedirectListen returns
// an error to simulate AMT dropping the TCP connection.
type spyRedirection struct {
listenErr error
}

func (s *spyRedirection) SetupWsmanClient(_ entity.Device, _, _ bool) (wsman.Messages, error) {
return wsman.Messages{}, nil
}

func (s *spyRedirection) RedirectConnect(_ context.Context, _ *DeviceConnection) error { return nil }
func (s *spyRedirection) RedirectClose(_ context.Context, _ *DeviceConnection) error { return nil }
func (s *spyRedirection) RedirectSend(_ context.Context, _ *DeviceConnection, _ []byte) error {
return nil
}

func (s *spyRedirection) RedirectListen(_ context.Context, _ *DeviceConnection) ([]byte, error) {
return nil, s.listenErr
}

// spyWebSocketConn is a minimal spy that records WriteMessage and Close calls
// without importing the mocks package (which would create an import cycle for
// the internal test package).
type spyWebSocketConn struct {
writeMessageCalled bool
writeMessageType int
closeCalled bool
}

func (s *spyWebSocketConn) WriteMessage(messageType int, _ []byte) error {
s.writeMessageCalled = true
s.writeMessageType = messageType

return nil
}

func (s *spyWebSocketConn) ReadMessage() (messageType int, p []byte, err error) {
return 0, nil, errors.New("spy: not reading")
}

func (s *spyWebSocketConn) Close() error {
s.closeCalled = true

return nil
}

func TestListenToDeviceClosesWebSocketOnAMTDisconnect(t *testing.T) {
t.Parallel()

spy := &spyWebSocketConn{}
ctx, cancel := context.WithCancel(context.Background())

deviceConnection := &DeviceConnection{
Conn: spy,
Mode: "kvm",
Device: entityDevice(),
ctx: ctx,
cancel: cancel,
healthTicker: time.NewTicker(HeartbeatInterval),
}

Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test allocates a ticker and a cancellable context but never stops/cancels them. Even though it’s a unit test, it’s better to avoid leaking timers/resources (and it helps with leak-detecting tooling). Consider using t.Cleanup to stop the ticker and call cancel(), or omit healthTicker entirely if it isn’t needed for this test.

Suggested change
t.Cleanup(func() {
cancel()
deviceConnection.healthTicker.Stop()
})

Copilot uses AI. Check for mistakes.
uc := &UseCase{
redirection: &spyRedirection{listenErr: errors.New("connection reset by peer")},
redirConnections: make(map[string]*DeviceConnection),
log: logger.New("silent"),
}

uc.ListenToDevice(deviceConnection)

require.True(t, spy.writeMessageCalled, "expected WriteMessage(CloseMessage) to be called on browser WebSocket")
require.Equal(t, websocket.CloseMessage, spy.writeMessageType)
require.True(t, spy.closeCalled, "expected Close() to be called on browser WebSocket")
}

func entityDevice() entity.Device {
return entity.Device{
GUID: "test-guid",
Username: "admin",
}
}
Loading