Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
85 changes: 85 additions & 0 deletions server/cmd/api/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ package api

import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"os"
"strconv"
"sync"
"time"

Expand All @@ -28,8 +31,90 @@ type ApiService struct {
procs map[string]*processHandle
}

// We're extending the StrictServerInterface to include our new endpoint
var _ oapi.StrictServerInterface = (*ApiService)(nil)

// SetScreenResolution endpoint
// (GET /screen/resolution)
Comment on lines +41 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible temporary note or outdated comment.

func (s *ApiService) SetScreenResolutionHandler(w http.ResponseWriter, r *http.Request) {
// Parse query parameters
width := 0
height := 0
var rate *int

// Parse width
widthStr := r.URL.Query().Get("width")
if widthStr == "" {
http.Error(w, "missing required query parameter: width", http.StatusBadRequest)
return
}
var err error
width, err = strconv.Atoi(widthStr)
if err != nil {
http.Error(w, "invalid width parameter: must be an integer", http.StatusBadRequest)
return
}

// Parse height
heightStr := r.URL.Query().Get("height")
if heightStr == "" {
http.Error(w, "missing required query parameter: height", http.StatusBadRequest)
return
}
height, err = strconv.Atoi(heightStr)
if err != nil {
http.Error(w, "invalid height parameter: must be an integer", http.StatusBadRequest)
return
}

// Parse optional rate parameter
rateStr := r.URL.Query().Get("rate")
if rateStr != "" {
rateVal, err := strconv.Atoi(rateStr)
if err != nil {
http.Error(w, "invalid rate parameter: must be an integer", http.StatusBadRequest)
return
}
rate = &rateVal
}

// Create request object
reqObj := SetScreenResolutionRequestObject{
Width: width,
Height: height,
Rate: rate,
}

// Call the actual implementation
resp, err := s.SetScreenResolution(r.Context(), reqObj)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}

// Handle different response types
switch r := resp.(type) {
case SetScreenResolution200JSONResponse:
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
json.NewEncoder(w).Encode(r)
case SetScreenResolution400JSONResponse:
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusBadRequest)
json.NewEncoder(w).Encode(r)
case SetScreenResolution409JSONResponse:
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusConflict)
json.NewEncoder(w).Encode(r)
case SetScreenResolution500JSONResponse:
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusInternalServerError)
json.NewEncoder(w).Encode(r)
default:
http.Error(w, "unexpected response type", http.StatusInternalServerError)
}
}
Copy link

Choose a reason for hiding this comment

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

Bug: Manual Implementation Causes JSON Encoding Errors

The SetScreenResolutionHandler is implemented manually, bypassing the oapi-codegen strict server interface. This duplicates request parsing and response serialization logic, and it omits error handling for JSON encoding, which can result in incomplete responses without client notification.

Fix in Cursor Fix in Web

Copy link
Contributor

Choose a reason for hiding this comment

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

@raiden-staging cursor is correct here. The rough process to add a new endpoint:

  • add it to openapi.yaml
  • run make oapi-generate
  • implement the new method generated on StrictServerInterface


func New(recordManager recorder.RecordManager, factory recorder.FFmpegRecorderFactory) (*ApiService, error) {
switch {
case recordManager == nil:
Expand Down
147 changes: 147 additions & 0 deletions server/cmd/api/api/computer.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@ package api

import (
"context"
"encoding/json"
"fmt"
"os/exec"
"strconv"
"time"

"github.com/gorilla/websocket"
"github.com/onkernel/kernel-images/server/lib/logger"
oapi "github.com/onkernel/kernel-images/server/lib/oapi"
)
Expand Down Expand Up @@ -54,6 +58,149 @@ func (s *ApiService) MoveMouse(ctx context.Context, request oapi.MoveMouseReques
return oapi.MoveMouse200Response{}, nil
}

// Define interface types for our new endpoint
// These should match the structure expected by the generated code

// SetScreenResolutionParams represents query parameters for our endpoint
type SetScreenResolutionParams struct {
Width int
Height int
Rate *int
}

// For testing
type SetScreenResolutionFunc func(ctx context.Context, req SetScreenResolutionRequestObject) (SetScreenResolutionResponseObject, error)

// This would be auto-generated by oapi-codegen, but we're defining it manually
type SetScreenResolutionRequestObject struct {
Width int // Required query parameter
Height int // Required query parameter
Rate *int // Optional query parameter
}

// Response types for different status codes
type SetScreenResolution200JSONResponse struct {
Ok bool `json:"ok"`
}

type SetScreenResolution400JSONResponse struct {
Message string `json:"message"`
}

type SetScreenResolution409JSONResponse struct {
Message string `json:"message"`
}

type SetScreenResolution500JSONResponse struct {
Message string `json:"message"`
}

// Union type for all possible responses
type SetScreenResolutionResponseObject interface {
SetScreenResolutionResponse()
}

// Implement response interface for each response type
func (SetScreenResolution200JSONResponse) SetScreenResolutionResponse() {}
func (SetScreenResolution400JSONResponse) SetScreenResolutionResponse() {}
func (SetScreenResolution409JSONResponse) SetScreenResolutionResponse() {}
func (SetScreenResolution500JSONResponse) SetScreenResolutionResponse() {}

Copy link

Choose a reason for hiding this comment

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

Bug: Inconsistent API Endpoint Registration

The new /screen/resolution endpoint bypasses the oapi-codegen workflow. It manually defines its types and is registered directly, which is inconsistent with other API endpoints and will cause compilation errors when oapi-codegen generates the same types.

Additional Locations (1)

Fix in Cursor Fix in Web

func (s *ApiService) SetScreenResolution(ctx context.Context, request SetScreenResolutionRequestObject) (SetScreenResolutionResponseObject, error) {
log := logger.FromContext(ctx)

// Validate parameters
width := request.Width
height := request.Height
rate := request.Rate

// Parameters were already validated in OpenAPI spec, but we'll do a sanity check here
if width < 200 || width > 8000 {
return SetScreenResolution400JSONResponse{
Message: fmt.Sprintf("width must be between 200 and 8000, got %d", width),
}, nil
}

if height < 200 || height > 8000 {
return SetScreenResolution400JSONResponse{
Message: fmt.Sprintf("height must be between 200 and 8000, got %d", height),
}, nil
}

if rate != nil && (*rate < 24 || *rate > 240) {
return SetScreenResolution400JSONResponse{
Message: fmt.Sprintf("rate must be between 24 and 240, got %d", *rate),
}, nil
}

// Check if ffmpeg is running (indicating an active recording)
cmd := exec.Command("pgrep", "ffmpeg")
if err := cmd.Run(); err == nil {
// ffmpeg is running
return SetScreenResolution409JSONResponse{
Message: "detected ongoing replay recording process, close the recording first before switching resolution",
}, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @Sayan- on ideas for how this should interact with replays

Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of a pgrep you could fetch the list of recorders and determine if we're actively recording via:

	recs := s.recordManager.ListActiveRecorders(ctx)
	for _, r := range recs {
		if r.IsRecording(ctx) {
			return true
		}
	}
	return false

I'd also recommend grabbing a lock and blocking StartRecording until we've completed the screen resizing


// Connect to websocket
wsURL := "ws://localhost:8080/ws?password=admin&username=kernel"
Copy link

Choose a reason for hiding this comment

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

Bug: WebSocket Credentials Leak and ffmpeg Check Flaw

The SetScreenResolution function hard-codes WebSocket connection credentials, exposing sensitive information. It also uses an unreliable pgrep check for ffmpeg, which may incorrectly allow resolution changes while a recording is active.

Fix in Cursor Fix in Web

dialer := websocket.Dialer{
HandshakeTimeout: 5 * time.Second,
}

conn, _, err := dialer.Dial(wsURL, nil)
if err != nil {
log.Error("failed to connect to websocket", "err", err)
return SetScreenResolution500JSONResponse{
Message: "failed to connect to websocket server",
}, nil
}
defer conn.Close()

// Prepare message
message := map[string]interface{}{
"event": "screen/set",
"width": width,
"height": height,
}

// Add rate if provided
if rate != nil {
message["rate"] = *rate
}

// Serialize message to JSON
messageJSON, err := json.Marshal(message)
if err != nil {
log.Error("failed to marshal JSON message", "err", err)
return SetScreenResolution500JSONResponse{
Message: "failed to prepare websocket message",
}, nil
}

// Send message
if err := conn.WriteMessage(websocket.TextMessage, messageJSON); err != nil {
log.Error("failed to send websocket message", "err", err)
return SetScreenResolution500JSONResponse{
Message: "failed to send command to websocket server",
}, nil
}

// Wait for response (optional, but might be good to ensure it worked)
conn.SetReadDeadline(time.Now().Add(5 * time.Second))
_, response, err := conn.ReadMessage()
if err != nil {
log.Warn("did not receive websocket response, but proceeding", "err", err)
// Continue anyway since we don't know if the server responds
} else {
log.Info("received websocket response", "response", string(response))
}

return SetScreenResolution200JSONResponse{
Ok: true,
}, nil
}

func (s *ApiService) ClickMouse(ctx context.Context, request oapi.ClickMouseRequestObject) (oapi.ClickMouseResponseObject, error) {
log := logger.FromContext(ctx)

Expand Down
111 changes: 111 additions & 0 deletions server/cmd/api/api/computer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package api

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestScreenResolutionParameterValidation(t *testing.T) {
// Test parameter validation in the SetScreenResolution function
testCases := []struct {
name string
width int
height int
rate *int
expectError bool
errorMsg string
}{
{
name: "valid parameters",
width: 1920,
height: 1080,
rate: intPtr(60),
expectError: false,
},
{
name: "valid without rate",
width: 1280,
height: 720,
rate: nil,
expectError: false,
},
{
name: "width too small",
width: 100,
height: 1080,
rate: nil,
expectError: true,
errorMsg: "width must be between 200 and 8000",
},
{
name: "width too large",
width: 9000,
height: 1080,
rate: nil,
expectError: true,
errorMsg: "width must be between 200 and 8000",
},
{
name: "height too small",
width: 1920,
height: 100,
rate: nil,
expectError: true,
errorMsg: "height must be between 200 and 8000",
},
{
name: "height too large",
width: 1920,
height: 9000,
rate: nil,
expectError: true,
errorMsg: "height must be between 200 and 8000",
},
{
name: "rate too small",
width: 1920,
height: 1080,
rate: intPtr(10),
expectError: true,
errorMsg: "rate must be between 24 and 240",
},
{
name: "rate too large",
width: 1920,
height: 1080,
rate: intPtr(300),
expectError: true,
errorMsg: "rate must be between 24 and 240",
},
}

// Create stub request object
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
req := SetScreenResolutionRequestObject{
Width: tc.width,
Height: tc.height,
Rate: tc.rate,
}

// Just test the validation part
if req.Width < 200 || req.Width > 8000 {
assert.True(t, tc.expectError, "Expected validation error for width")
}

if req.Height < 200 || req.Height > 8000 {
assert.True(t, tc.expectError, "Expected validation error for height")
}

if req.Rate != nil && (*req.Rate < 24 || *req.Rate > 240) {
assert.True(t, tc.expectError, "Expected validation error for rate")
}
})
}
}

// Helper function to create int pointer
func intPtr(i int) *int {
return &i
}
3 changes: 3 additions & 0 deletions server/cmd/api/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ func main() {
strictHandler := oapi.NewStrictHandler(apiService, nil)
oapi.HandlerFromMux(strictHandler, r)

// Register our custom endpoint handler
r.Get("/screen/resolution", apiService.SetScreenResolutionHandler)

// endpoints to expose the spec
r.Get("/spec.yaml", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/vnd.oai.openapi")
Expand Down
Loading
Loading