Configurable viewport #73

hiroTamada · 2025-09-30T00:17:08Z

Initially

After running

➜  ~ curl -X PATCH http://localhost:444/display \
  -H "Content-Type: application/json" \
  -d '{"width": 1920, "height": 1080}'
{"height":1080,"width":1920}

For headless, I exec-ed into the running docker, and I confirmed the xvfb config by

root@18f971b11d5d:/# ps aux | grep Xvfb
root      1675  0.0  0.8 203536 69648 ?        S    18:31   0:00 Xvfb :1 -ac -screen 0 1920x1080x24 -retro -dpi 96 -nolisten tcp -nolisten unix
root      2195  0.0  0.0   2904  1532 pts/1    S+   18:34   0:00 grep --color=auto Xvfb

TL;DR

This PR introduces a PATCH /display API endpoint to dynamically change the browser's viewport resolution without restarting the container.

Why we made these changes

This allows users to programmatically control the browser's viewport size, which is crucial for testing responsive designs, simulating different device screens, and tailoring the resolution for various automation tasks.

What changed?

API: Added a PATCH /display endpoint in server/cmd/api/api/display.go to allow dynamic resolution changes for both headful (Xorg) and headless (Xvfb) modes.
Resolution Logic: The new handler uses xrandr for Xorg and supervisor restarts for Xvfb. It includes a require_idle safety check and restarts Chromium to apply the new viewport.
Neko Integration: Added an authenticated Neko client (server/lib/nekoclient/client.go) to manage headful sessions.
Dependencies: Added the x11-xserver-utils package to the images/chromium-headful/Dockerfile to provide xrandr.
Configuration: Updated images/chromium-headful/xorg.conf to include additional display modes.
API Spec: Updated openapi.yaml with the new display endpoint and regenerated the Go client.
Scripts: Modified run-docker.sh and run-unikernel.sh to pass Chromium flags as a JSON array instead of a space-separated string.
Testing: Added a new e2e test, TestDisplayResolutionChange, to validate the functionality.

Validation

Manually tested resolution change with the curl command.
Verified Chromium restarts and fits the new resolution.
Ensured require_idle flag prevents changes during an active session.

^{Description generated by Mesa. Update settings}

^{Description generated by Mesa. Update settings}

server/openapi.yaml

server/docs/resolution-change-alternatives.md

server/cmd/api/api/display.go

mesa-dot-dev

Performed full review of 2d73dc8...fc57289

Analysis

Session Disruption Risk: The requirement to restart Chromium after resolution changes could negatively impact user experience and session continuity.
Deployment Infrastructure Coupling: Direct modification of supervisor configuration files for Xvfb creates tight coupling between the application and deployment infrastructure.
System Component Coordination Complexity: The implementation requires orchestrating multiple system components (X server, supervisor, Chromium), increasing the number of potential failure points and making the system more brittle.
Runtime Flexibility vs. Stability Tradeoff: While the configurable viewport adds runtime flexibility, it also introduces additional complexity that could impact system stability.

Tip

⚡ Quick Actions

This review was generated by Mesa.

Actions:

Configure your agents

Slash Commands:

/review - Request a full code review
/review latest - Review only changes since the last review
/describe - Generate PR description. This will update the PR body or issue comment depending on your configuration
/help - Get help with Mesa commands and configuration options

^{4 files reviewed | 0 comments | Review on Mesa | Edit Reviewer Settings}

hiroTamada · 2025-10-07T18:13:32Z

images/chromium-headful/Dockerfile

    wget ca-certificates python2 supervisor xclip xdotool \
    pulseaudio dbus-x11 xserver-xorg-video-dummy \
    libcairo2 libxcb1 libxrandr2 libxv1 libopus0 libvpx7 \
+    x11-xserver-utils \


need this for xrandr

server/cmd/api/api/display.go

rgarcia · 2025-10-08T14:54:22Z

server/cmd/api/api/display.go

+	s.ProcessExec(ctx, oapi.ProcessExecRequestObject{Body: &removeEnvReq})
+
+	// Add the environment line with WIDTH and HEIGHT
+	addEnvCmd := []string{"-lc", fmt.Sprintf(`sed -i '/\[program:xvfb\]/a environment=WIDTH="%d",HEIGHT="%d",DPI="96",DISPLAY=":1"' /etc/supervisor/conf.d/services/xvfb.conf`, width, height)}


this logic is sufficiently intricate where an e2e test might be beneficial. See https://github.com/onkernel/kernel-images/blob/main/server/e2e/e2e_chromium_test.go#L102-L219

Sayan-

great stuff!

two general thoughts for neko:

instead of raw http requests I wonder if it would be easier to generate an oapi client of their spec (e.g. https://github.com/onkernel/neko/blob/aa0487f68ebbff1056d7355ec2da127986e5be5f/server/openapi.yaml#L146-L170 +
It might be helpful to pull out the neko bits into their own client that internally manages the token rather than that being a responsibility of the ApiService

Sayan- · 2025-10-08T16:20:46Z

server/openapi.yaml

+        refresh_rate:
+          type: integer
+          enum: [60, 30, 25]
+          description: Display refresh rate in Hz. If omitted, uses the highest available rate for the resolution.


double checking - did we also run numbers on impact of refresh rate in benchmarking? I don't see anything in https://docs.google.com/spreadsheets/d/1VBhIEoNTJoD95gKsalM3XUH2lDnz2XwFlyVwECV-45I/edit?gid=0#gid=0

namely, I'd think higher refresh rates would consume more resources for the gstreamer pipeline backing live views, so I'm curious on the numbers there

on the api side, we are only going to allow these following configurations

1024x768x60 1920x1080x60 2560x1440x30 1920 x 1200x60 1440 x 900x30

On the images side, I think we can leave it flexible?

did we also want 10 in here?

Sayan- · 2025-10-08T16:21:23Z

server/openapi.yaml

+        live_view_sessions:
+          type: integer
+          description: Number of active Neko viewer sessions.
+        is_recording:
+          type: boolean
+          description: Whether recording is currently active
+        is_replaying:
+          type: boolean
+          description: Whether replay is currently active
+        resizable_now:
+          type: boolean
+          description: True when no blockers are present for resizing


do we still need all these bits?

I guess not... I can just return the current width, height and refresh_rate

server/cmd/api/api/display.go

Sayan- · 2025-10-08T16:26:34Z

server/cmd/api/api/display.go

+	if requireIdle {
+		live := s.getActiveNekoSessions(ctx)
+		isRecording := s.anyRecordingActive(ctx)
+		isReplaying := false // replay not currently implemented


is this droppable?

This was my thought process.

We can make the images api to be flexible to accommodate both cases where it requires no active sessions or not.

If the request says requireIdle, then we reject the request if there are active sessions

ah sorry this was in reference to isReplaying. I think it's just a dupe of isRecording and it's not implemented

ah I see. Yeah, I will do that. Good thanks!!

Sayan- · 2025-10-08T16:27:06Z

server/cmd/api/api/display.go

+	displayMode := s.detectDisplayMode(ctx)
+
+	// Parse restartChromium flag (default depends on mode)
+	restartChrome := (displayMode == "xvfb") // default true for xvfb, false for xorg


checking my understanding - why don't we restart for headful?

we use this flag to --start-maximized to start chromium.

So when x.org expands, the chromium expands. But, we can optionally override this through the request. chromium restarts take time, so we want to avoid it as much as possible

Sayan- · 2025-10-08T16:28:28Z

server/cmd/api/api/display.go

+// detectDisplayMode detects whether we're running Xorg (headful) or Xvfb (headless)
+func (s *ApiService) detectDisplayMode(ctx context.Context) string {
+	log := logger.FromContext(ctx)
+	checkCmd := []string{"-lc", "supervisorctl status xvfb >/dev/null 2>&1 && echo 'xvfb' || echo 'xorg'"}
+	checkReq := oapi.ProcessExecRequest{Command: "bash", Args: &checkCmd}
+	checkResp, _ := s.ProcessExec(ctx, oapi.ProcessExecRequestObject{Body: &checkReq})
+
+	if execResp, ok := checkResp.(oapi.ProcessExec200JSONResponse); ok {
+		if execResp.StdoutB64 != nil {
+			if output, err := base64.StdEncoding.DecodeString(*execResp.StdoutB64); err == nil {
+				outputStr := strings.TrimSpace(string(output))
+				if outputStr == "xvfb" {
+					log.Info("detected Xvfb display (headless mode)")
+					return "xvfb"
+				}
+			}
+		}
+	}
+	log.Info("detected Xorg display (headful mode)")
+	return "xorg"
+}


honestly wonder if this is easier as an env var

Yeah.... good point. Let me research this. I talked about this with Raf as well. He was more comfortable with this approach

server/cmd/api/api/display.go

Sayan- · 2025-10-08T16:29:33Z

server/cmd/api/api/display.go

+// setResolutionXorgViaNeko changes resolution for Xorg using Neko API
+func (s *ApiService) setResolutionXorgViaNeko(ctx context.Context, width, height, refreshRate int, restartChrome bool) error {
+	if err := s.setResolutionViaNeko(ctx, width, height, refreshRate); err != nil {
+		return fmt.Errorf("failed to change resolution via Neko API: %w", err)
+	}
+
+	if restartChrome {
+		s.restartChromium(ctx)
+	}
+
+	return nil
+}


do we need this wrapper?

Sayan- · 2025-10-08T16:35:13Z

server/cmd/api/api/display.go

+// getNekoToken obtains a bearer token from Neko API for authentication.
+// It caches the token and reuses it for subsequent requests.
+func (s *ApiService) getNekoToken(ctx context.Context) (string, error) {


nit - we should pull the neko port / url from the env instead of hardcoding

how long do these tokens last for? do they survive neko restarts? mainly wondering if we could just do it at API start up time so it's always ready but can see there could be problems with that approach

I think the token lifetime is technically infinite. But you can manually expire it or rotate it. Maybe it is safer to maintain the current implementation?

server/cmd/api/api/display.go

cursor · 2025-10-09T03:39:26Z

images/chromium-headful/run-unikernel.sh

+  eval "FLAGS_ARRAY=($CHROMIUM_FLAGS)"
+else
+  FLAGS_ARRAY=()
+fi


Bug: Environment Variable Injection Vulnerability

The use of eval "FLAGS_ARRAY=($CHROMIUM_FLAGS)" in both scripts introduces a command injection vulnerability. Since CHROMIUM_FLAGS can be set via environment variables, malicious input could lead to arbitrary shell code execution.

Additional Locations (1)

images/chromium-headful/run-docker.sh#L27-L32

that is fine. It is not for production use case

server/lib/nekoclient/client.go

cursor · 2025-10-09T04:31:32Z

images/chromium-headful/run-docker.sh

+  fi
+done
+FLAGS_JSON+=']}'
+echo "$FLAGS_JSON" > "$FLAGS_FILE"


Bug: Shell Code Injection via User-Controlled Flags

The eval "FLAGS_ARRAY=($CHROMIUM_FLAGS)" command in both scripts introduces a command injection vulnerability. Since CHROMIUM_FLAGS can be user-controlled, malicious input could lead to arbitrary shell code execution.

Additional Locations (1)

images/chromium-headful/run-unikernel.sh#L33-L52

again, not user facing and this is fine

cursor · 2025-10-09T04:31:33Z

server/cmd/api/api/display.go

+	// Note: Using bash -c (not -lc) to avoid login shell overriding DISPLAY env var
+	cmd := exec.CommandContext(ctx, "bash", "-c", "xrandr | grep -E '\\*' | awk '{print $1}'")
+	cmd.Env = append(os.Environ(), fmt.Sprintf("DISPLAY=%s", display))
+


Bug: Insecure Command Execution in Resolution Fetching

The getCurrentResolution function directly uses exec.CommandContext with bash -c instead of the ProcessExec API. This approach is fragile and vulnerable to command injection due to unescaped DISPLAY and xrandr output parsing. It also bypasses internal process controls and logging, and handles the DISPLAY environment variable inconsistently.

not user facing path. This is fine

cursor · 2025-10-09T04:31:33Z

server/e2e/e2e_chromium_test.go

 		"--privileged",
-		"--network=host",
+		"-p", "10001:10001", // API server
+		"-p", "9222:9222", // DevTools proxy


Bug: API URL Mismatch After Network Change

The runContainer function switched from --network=host to explicit port mapping (-p 10001:10001). This means the hardcoded apiBaseURL (http://127.0.0.1:10001) no longer correctly points to the container's API server from the host, causing API calls in tests that use runContainer to fail.

I had to do this to run this on Mac. I will revert if this is not necessary

cursor · 2025-10-09T04:31:33Z

server/cmd/api/api/display.go

+	s.ProcessExec(ctx, oapi.ProcessExecRequestObject{Body: &removeEnvReq})
+
+	// Add the environment line with WIDTH and HEIGHT
+	addEnvCmd := []string{"-lc", fmt.Sprintf(`sed -i '/\[program:xvfb\]/a environment=WIDTH="%d",HEIGHT="%d",DPI="96",DISPLAY=":1"' /etc/supervisor/conf.d/services/xvfb.conf`, width, height)}


Bug: Vulnerability in sed Command Injection

The sed commands in setResolutionXvfb use fmt.Sprintf to inject width and height into shell commands without escaping. This allows for command injection. The direct string manipulation of the supervisor config is also fragile, risking breakage if the format changes.

that is fine. Not user facing

cursor · 2025-10-09T22:06:13Z

images/chromium-headful/run-unikernel.sh

+  eval "FLAGS_ARRAY=($CHROMIUM_FLAGS)"
+else
+  FLAGS_ARRAY=()
+fi


Bug: Shell Injection via CHROMIUM_FLAGS

The eval command in run-unikernel.sh and run-docker.sh parses CHROMIUM_FLAGS. This creates a security vulnerability, allowing arbitrary command injection if CHROMIUM_FLAGS contains malicious shell code, as it can be controlled by user input or environment variables.

Additional Locations (1)

images/chromium-headful/run-docker.sh#L27-L32

Sayan-

overall lgtm! last couple of q's

Sayan- · 2025-10-10T18:26:42Z

server/cmd/api/api/api.go

+	// Initialize Neko authenticated client
+	adminPassword := os.Getenv("NEKO_ADMIN_PASSWORD")
+	if adminPassword == "" {
+		adminPassword = "admin" // Default from neko.yaml
+	}
+	nekoAuthClient, err := nekoclient.NewAuthClient("http://127.0.0.1:8080", "admin", adminPassword)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create neko auth client: %w", err)
+	}
+


why do we prefer to initial this here instead of passing it in?

no reasons lol

Sayan- · 2025-10-10T18:30:05Z

server/openapi.yaml

+        refresh_rate:
+          type: integer
+          enum: [60, 30, 25]
+          description: Display refresh rate in Hz. If omitted, uses the highest available rate for the resolution.


did we also want 10 in here?

Sayan- · 2025-10-10T18:30:30Z

server/lib/nekoclient/client.go

+// AuthClient wraps the Neko OpenAPI client and handles authentication automatically.
+// It manages token caching and refresh on 401 responses.
+type AuthClient struct {
+	client   *nekooapi.ClientWithResponses
+	tokenMu  sync.Mutex
+	token    string
+	username string
+	password string
+}


this is way easier to read, thanks for iterating!

cursor · 2025-10-10T18:52:50Z

server/lib/oapi/oapi.go

+	N25 PatchDisplayRequestRefreshRate = 25
+	N30 PatchDisplayRequestRefreshRate = 30
+	N60 PatchDisplayRequestRefreshRate = 60
+)


Bug: Missing Enum Value Causes Validation Errors

The generated PatchDisplayRequestRefreshRate enum constants are incomplete. The value 10 is missing from the generated Go code, despite being defined in the OpenAPI spec. This prevents clients from using 10 Hz and may lead to validation issues.

cursor · 2025-10-10T18:59:29Z

images/chromium-headful/run-unikernel.sh

+    else
+      FLAGS_JSON+=",\"$flag\""
+    fi
+  fi


Bug: Shell Command Injection via eval

The eval command used to parse CHROMIUM_FLAGS in both run-unikernel.sh and run-docker.sh introduces a command injection vulnerability. If CHROMIUM_FLAGS contains malicious shell commands, eval will execute them, potentially leading to arbitrary code execution.

Additional Locations (1)

images/chromium-headful/run-docker.sh#L27-L34

cursor · 2025-10-10T18:59:29Z

images/chromium-headful/run-docker.sh

+  fi
+done
+FLAGS_JSON+=']}'
+echo "$FLAGS_JSON" > "$FLAGS_FILE"


Bug: JSON Flag Parsing Fails on Special Characters

The JSON generation for Chromium flags doesn't escape special characters within flag values. If a flag contains double quotes, backslashes, or other JSON special characters, the resulting JSON will be invalid. This could cause the downstream flag parsing system to fail or misinterpret flags.

Additional Locations (1)

images/chromium-headful/run-unikernel.sh#L44-L47

hiroTamada added 2 commits September 29, 2025 17:14

intial commit

5a9dc14

support headless mode

d13e764

Sayan- reviewed Sep 30, 2025

View reviewed changes

server/openapi.yaml Outdated Show resolved Hide resolved

server/openapi.yaml Outdated Show resolved Hide resolved

server/docs/resolution-change-alternatives.md Outdated Show resolved Hide resolved

server/cmd/api/api/display.go Outdated Show resolved Hide resolved

Sayan's comments

fc57289

hiroTamada marked this pull request as ready for review October 4, 2025 04:51

mesa-dot-dev bot reviewed Oct 4, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

hiroTamada changed the title ~~WIP: POC for configurable viewport~~ Configurable viewport Oct 4, 2025

moar refactoring

f4747f6

This comment was marked as outdated.

Sign in to view

hiroTamada added 2 commits October 6, 2025 22:02

Merge branch 'main' into hiro/configurable_viewport

a581364

Fix run scripts

a214bcf

This comment was marked as outdated.

Sign in to view

locahost to 127.0.0.1

b12c7c7

This comment was marked as outdated.

Sign in to view

hiroTamada commented Oct 7, 2025

View reviewed changes

moar

7d491fe

This comment was marked as outdated.

Sign in to view

hiroTamada requested a review from rgarcia October 7, 2025 23:09

avoid race condition

fe4b7f3

rgarcia reviewed Oct 8, 2025

View reviewed changes

server/cmd/api/api/display.go Show resolved Hide resolved

rgarcia reviewed Oct 8, 2025

View reviewed changes

server/cmd/api/api/display.go Outdated Show resolved Hide resolved

rgarcia reviewed Oct 8, 2025

View reviewed changes

Sayan- reviewed Oct 8, 2025

View reviewed changes

hiroTamada added 2 commits October 8, 2025 15:38

pr comments

209ffa9

add e2e tests

8ff2300

hiroTamada requested review from Sayan- and rgarcia October 8, 2025 20:04

Change deps version

792beee

This comment was marked as outdated.

Sign in to view

moar

99111f6

This comment was marked as outdated.

Sign in to view

moar

5358c43

This comment was marked as outdated.

Sign in to view

cursor comments

613f0f9

cursor bot reviewed Oct 9, 2025

View reviewed changes

moar

2c71f0c

cursor bot reviewed Oct 9, 2025

View reviewed changes

Add lower resolution configs

1e34150

cursor bot reviewed Oct 9, 2025

View reviewed changes

Sayan- reviewed Oct 10, 2025

View reviewed changes

one moar

6a7b560

Sayan- approved these changes Oct 10, 2025

View reviewed changes

cursor bot reviewed Oct 10, 2025

View reviewed changes

hiroTamada force-pushed the hiro/configurable_viewport branch from d1189ff to 9802b64 Compare October 10, 2025 18:55

Fix tests

513dcbb

hiroTamada force-pushed the hiro/configurable_viewport branch from 9802b64 to 513dcbb Compare October 10, 2025 18:55

cursor bot reviewed Oct 10, 2025

View reviewed changes

Sayan- approved these changes Oct 10, 2025

View reviewed changes

hiroTamada merged commit a730519 into main Oct 10, 2025
11 of 14 checks passed

hiroTamada deleted the hiro/configurable_viewport branch October 10, 2025 19:38

Configurable viewport #73

Configurable viewport #73

Uh oh!

Conversation

hiroTamada commented Sep 30, 2025 • edited by mesa-dot-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Why we made these changes

What changed?

Validation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mesa-dot-dev bot left a comment

Choose a reason for hiding this comment

Analysis

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sayan- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sayan- Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiroTamada Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hiroTamada commented Sep 30, 2025 •

edited by mesa-dot-dev bot

Loading

Sayan- left a comment •

edited

Loading

Sayan- Oct 8, 2025 •

edited

Loading

hiroTamada Oct 8, 2025 •

edited

Loading

Bug: Vulnerability in `sed` Command Injection

Bug: Shell Injection via `CHROMIUM_FLAGS`