Conversation
This commit introduces the foundational implementation of an X11 server, written in Go and compiled to WebAssembly. This is the first step towards enabling X11 forwarding within the browser, with the goal of allowing remote graphical applications to be displayed and interacted with in sshterm. The server runs in the browser, parsing the X11 protocol stream and translating drawing commands into JavaScript calls to render onto HTML5 <canvas> elements. This is an early-stage implementation and should be considered a proof-of-concept. It is sufficient to run simple applications like xeyes and xclock, but it is not yet a fully functional X server. Key Features (Initial Support): - Core Protocol: Basic X11 handshake, setup, and request handling. - Window Management: Basic support for creating, mapping, and configuring windows. Top-level windows have basic decorations and can be moved and resized. - Graphics Context (GC): Initial support for Graphics Contexts, including creation and modification. - Drawing Primitives: Implementation of several core drawing primitives like PutImage and CopyArea. - Text & Font Rendering: Basic text rendering and a simplified font system that maps X fonts to CSS properties. - Input Handling: Initial handling for mouse and keyboard events. - Clipboard: Basic clipboard integration for text. Known Limitations and Future Work: - Protocol Incompleteness: Many protocol opcodes are stubs. Advanced features like input grabbing, window manager hints, and complex event propagation are not correctly implemented. - Compatibility: The server currently only works with a handful of simple X11 applications. More complex applications will fail or render incorrectly. - Rendering & Fonts: Font handling and metrics are very basic. - Performance: No performance tuning has been done. - Security: Input is not fully validated. - Extensions: No X11 extensions are supported. Future work will focus on addressing these limitations to improve compatibility and stability, as detailed in X11-plan.md.
Refactored the X11 message handling code to improve organization and maintainability. - Separated request, reply, event, and error message structs into their own dedicated files (`*_messages.go`). - Created corresponding test files (`*_messages_test.go`) for each new file. - Moved request parsing functions into `request_messages.go` alongside the request structs. - Ensured that all structs within the new files are sorted by their associated opcode. - Fixed bugs in parsing functions that were ignoring the `req.data` field. - Restored lost test coverage.
This refactoring separates the X11 message structs (requests, replies, events, and errors) into their own dedicated files within the `go/internal/x11` package. - All request message structs are now in `request_messages.go`, with their tests in `request_messages_test.go`. - All reply message structs are now in `reply_messages.go`, with their tests in `reply_messages_test.go`. - All event message structs are now in `event_messages.go`, with their tests in `event_messages_test.go`. - All error message structs are now in `error_messages.go`, with their tests in `error_messages_test.go`. The structs within each `*_messages.go` file are sorted by their associated opcode or error code. Additionally, request parsing functions have been moved to `request_messages.go`, placed immediately after their corresponding struct definition, and their signatures have been updated to return `(*Struct, error)`. The main `x11.go` file has been updated to use these new parsing functions.
This commit introduces a comprehensive test suite for all X11 request parsing functions in `go/internal/x11/request_messages.go`. Previously, test coverage for this critical part of the X11 package was low, with many request types lacking any form of automated verification. To address this, a series of new test functions have been added to `go/internal/x11/request_messages_test.go`. Each new test is dedicated to a specific request type and is meticulously crafted based on the official `x11protocol.txt` specification. This ensures that the parsing logic is validated against a correct and reliable source of truth. During the process of writing these tests, a bug was discovered in the `parseStoreNamedColorRequest` function where the `flags` field was being read from an incorrect location in the request body, leading to a panic. This issue has been fixed, and the corresponding test case has been updated to reflect the correct implementation. With these changes, the X11 package is now more robust and reliable, and the risk of regressions in the request parsing logic has been significantly reduced.
This change implements all remaining unimplemented X11 requests in `go/internal/x11/request_messages.go`. It also includes the following: - Creation of `go/internal/x11/types.go` to define strongly-typed aliases for X11 resource identifiers and other protocol data structures. - Refactoring of the entire `request_messages.go` file and its consumers to use these new strong types. - Addition of reply message structs in `go/internal/x11/reply_messages.go` for requests that expect a response. - Update of the `X11FrontendAPI` interface and its mock and WASM stub implementations to include methods for the newly supported requests. - Addition of comprehensive tests to `go/internal/x11/request_messages_test.go` to cover the parsing of the newly implemented requests.
This change implements all remaining unimplemented X11 requests in `go/internal/x11/request_messages.go`. It also includes the following: - Creation of `go/internal/x11/types.go` to define strongly-typed aliases for X11 resource identifiers and other protocol data structures. - Refactoring of the entire `request_messages.go` file and its consumers to use these new strong types. - Addition of reply message structs in `go/internal/x11/reply_messages.go` for requests that expect a response. - Update of the `X11FrontendAPI` interface and its mock and WASM stub implementations to include methods for the newly supported requests. - Addition of comprehensive tests to `go/internal/x11/request_messages_test.go` to cover the parsing of the newly implemented requests. - Implementation of the `case` statements in `handleRequest` in `go/internal/x11/x11.go` to handle the newly parsed request types.
This commit enhances the X11 protocol implementation by adding all missing error and reply types, with corresponding unit tests. It also refactors the error handling to use a consistent naming scheme, addressing feedback from the previous review. - Refactored error types in `go/internal/x11/error_messages.go` and constants in `go/internal/x11/const.go` to have consistent, non-conflicting names (e.g., `ValueError` for the struct and `ValueErrorCode` for the constant). - Updated the `NewError` factory function, associated unit tests, and all call sites to use the new naming scheme. - Implemented all missing X11 reply types in `go/internal/x11/reply_messages.go`, using `go/internal/x11/x11protocol.txt` as a reference. Placeholders with `// TODO:` comments have been added for replies that require more complex implementation. - Added corresponding unit tests for all newly implemented reply types in `go/internal/x11/reply_messages_test.go`.
Adds slice boundary checks to all X11 request parsing functions in `go/internal/x11/request_messages.go` to prevent panics when processing malformed or undersized requests. - Added length checks at the beginning of each `parse...Request` function. - Refactored helper functions (`parseWindowAttributes`, `parseGCValues`, `parseKeyboardControl`) to propagate errors. - Added a comprehensive table-driven test (`TestRequestParsingErrors`) to ensure that an error is returned for each request type when the input slice is too small. - Corrected existing tests that were failing due to the new checks. This change makes the X11 request parsing more robust and prevents the application from crashing due to invalid client data.
Adds a comprehensive test suite for the `encodeMessage` method on all X11 reply types in `go/internal/x11/reply_messages.go`. This change also includes the following fixes: - Corrects a slice-out-of-bounds panic in `getKeyboardControlReply.encodeMessage`. - Fixes a bug in `listHostsReply.encodeMessage` where data was being overwritten. - Removes an unused `numFonts` field from the `listFontsReply` struct.
This is a work in progress. The build is currently broken due to duplicate declarations. Implements parsing and handling for the following X11 request types: - CopyColormapAndFree - AllocColorCells - AllocColorPlanes - CreateCursor - CopyPlane - ListExtensions - ChangePointerControl - GetPointerControl
This commit completes the implementation of the following X11 request parsers in `go/internal/x11/request_messages.go`: - CopyPlaneRequest - ListExtensionsRequest - ChangePointerControlRequest - GetPointerControlRequest The parsing logic for each request has been implemented according to the `x11protocol.txt` specification. Corresponding test cases have been added to `go/internal/x11/testdata/requests.json` to ensure the correctness of the new parsers. The test suite for the x11 package passes successfully.
This commit completes the implementation of the remaining X11 request parsers in `go/internal/x11/request_messages.go`. The following request types have been implemented and tested: - AllocColorCells - AllocColorPlanes - CreateCursor - CopyPlane - ListExtensions - ChangePointerControl - GetPointerControl Parsing logic and corresponding test cases, including error handling for undersized requests, have been added for each new parser. This work was based on the specifications in `go/internal/x11/x11protocol.txt`.
Adds placeholder stubs for all unimplemented X11 request types to the `handleRequest` function in `go/internal/x11/x11.go`. Each new case includes a `// TODO: implement` comment, and a placeholder reply is returned for requests that expect one. All cases in the `switch` statement have been sorted numerically by their `reqCode` to improve readability and maintainability. A blank line has also been added between each case.
This commit implements the CreateCursor and RecolorCursor X11 requests. CreateCursor is implemented by generating a data URI from the source and mask pixmaps and using it as a CSS cursor. RecolorCursor is implemented by storing the original pixmap data and regenerating the cursor with the new colors. A test case for CreateCursor has been added to the request parsing test suite.
This commit implements the following X11 Graphics Context and Pixmap operations in the HTML5 canvas frontend: - `CopyPlane`: Implemented the logic to copy a single bit-plane from a source drawable to a destination drawable, using the foreground and background pixels from the GC. - `SetDashes`: Implemented the logic to set the dash pattern for lines. The dash list from the request is stored and will be applied to the canvas context during drawing operations. - `SetClipRectangles`: Implemented the logic to set the clipping region for a GC. The rectangles are stored and applied as a clipping path to the canvas context during drawing operations. - `PutImage`: Enhanced `PutImage` to correctly handle the `XYPixmap` format by iterating through the bit-planes. This commit also refactors all drawing functions to accept a `gcID` instead of the full `GC` struct, centralizing GC state management in the frontend.
- Added `rootWindowWidth` and `rootWindowHeight` fields to `x11Server` struct. - Implemented `SetRootWindowSize` method on `x11Server` to allow the frontend to report window dimensions. - Modified `GetGeometryRequest` handler to return the correct geometry for the root window (ID 0) using the reported dimensions. - Updated `x11_frontend_wasm.go` to report the browser window's dimensions to the `x11Server` on initialization and on resize events.
- Added `rootWindowWidth` and `rootWindowHeight` fields to `x11Server` struct. - Implemented `SetRootWindowSize` method on `x11Server` to allow the frontend to report window dimensions. - Modified `GetGeometryRequest` handler to return the correct geometry for the root window (ID 0) using the reported dimensions. - Updated `x11_frontend_wasm.go` to report the browser window's dimensions to the `x11Server` on initialization and on resize events.
This commit implements the X11 Graphics Context (GC) by mapping its features to the HTML5 canvas API. Key changes: - A central `applyGC` function in `go/internal/x11/x11_frontend_wasm.go` now translates GC attributes (line width/style, cap/join styles, colors, and fill styles) to canvas properties. - All X11 drawing operations have been refactored to use `applyGC`, ensuring consistent application of GC settings. - The `parseGCValues` function in `go/internal/x11/request_messages.go` has been fixed to correctly initialize GC structs with default values as per the X11 protocol. This was a critical bug causing numerous test failures. - A panic in the test harness related to passing Go objects to JavaScript was resolved. - New tests have been added to `go/internal/testserver/x11_sim.go` to exercise all the newly implemented GC features, ensuring their correctness and preventing regressions. The following GC components have been implemented: - function - plane-mask - foreground - background - line-width - line-style - cap-style - join-style - fill-style - fill-rule - tile - stipple - font - subwindow-mode - graphics-exposures - clip-x-origin - clip-y-origin - clip-mask - dash-offset - dashes The following components were not implemented due to complexity or lack of direct mapping to the canvas API: - arc-mode - tile-stipple-x-origin - tile-stipple-y-origin
This change introduces the parsing of X11 Graphics Context (GC) attributes and their application to canvas drawing operations. The following GC attributes are now parsed from the X11 protocol: - function - plane-mask - foreground - background - line-width - line-style - cap-style - join-style - fill-style - fill-rule - tile - stipple - tile-stipple-x-origin - tile-stipple-y-origin - font - subwindow-mode - graphics-exposures - clip-x-origin - clip-y-origin - clip-mask - dash-offset - dashes - arc-mode A new test simulation, `simulateGCOperations`, has been added to `go/internal/testserver/x11_sim.go` to exercise these new features. Note: The unit tests for `TestRequestParsing` and `TestParseChangeGCRequest` are currently failing. This is a known issue.
…shterm into x11-checkpoint-20251105
- Increase WASM test Expect timeout from 5s to 30s to reduce flakiness in containerized environments. - Add max length validation for X11 requests to prevent excessive memory allocation. - Implement systematic X11 resource ID collision checks using a new resourceExists helper. - Add pixmap format tracking and data length validation for PutImage requests. - Add client ID validation for OpenFont requests.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces X11 forwarding support by implementing a Go/WASM-based X11 server that renders remote graphical applications directly to an HTML5 Canvas. The implementation covers protocol serialization, state management, and event routing integrated with the SSH client. The review feedback highlights several protocol compliance issues, including incorrect encoding of setup failure responses, hardcoded color layouts, and deviations from X11 specifications regarding window stacking and mapping behaviors. Additionally, improvements were suggested to handle the OnlyIfExists flag in atom interning and to prevent potential integer overflows in property slicing and color cell allocation logic.
- Store bitmap-format-scanline-unit and bitmap-format-scanline-pad from setup response. - Use stored scanline pad values in calculateImageSize instead of hardcoding 32. - Fixes validation failures for PutImage requests with small widths on clients using 8-bit scanline padding.
- Add comprehensive PixmapFormats (1, 4, 8, 16, 24, 32 bits) to default setup response. - Fix calculateImageSize to correctly determine bits-per-pixel from setup formats. - Refactor WASM frontend PutImage for ZPixmap to dynamically handle different bit depths and scanline padding. - Improve X11 standalone test to correctly wait for X11 window divs and log terminal content.
- Fix SetupResponse encoding for success and failure cases. - Fix window stacking logic in server (initial windows at bottom). - Maintain visible z-index for new windows in frontend (base 100). - Respect OnlyIfExists flag in InternAtom. - Return VisualID 0 for pixmaps in GetImageReply. - Fix potential integer overflows in GetProperty and AllocColorCells. - Use actual visual masks for plane mask check in applyGC.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements X11 forwarding support for the sshterm project, enabling remote graphical applications to display within the browser. It introduces the core X11 server logic, protocol wire definitions, and frontend integration via WebAssembly. The review identified several critical issues, including potential out-of-bounds panics in request handlers, lack of thread safety in concurrent operations, and missing validation for X11 protocol requirements such as atom existence and keycode ranges. Several suggestions were made to improve robustness, including adding bounds checks, implementing proper error handling for invalid requests, and ensuring thread-safe access to server state.
- Implement thread-safety with RWMutex in x11Server. - Enforce 32MB limit on server replies to prevent memory exhaustion. - Fix memory leak by cleaning up window properties on destruction. - Correct window stacking order (new windows at top). - Implement dynamic pixel calculation based on visual masks. - Add bounds checking and validation for ConfigureWindow, AllocColorPlanes, GetAtomName, and GetKeyboardMapping. - Implement circularity check in ReparentWindow. - Fix potential FID collisions in ListFontsWithInfo.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements X11 forwarding for the sshterm application, allowing remote graphical applications to be displayed in a web browser using Go compiled to WebAssembly and HTML5 Canvas. The implementation covers the X11 wire protocol, state management, and rendering logic. However, several critical issues regarding protocol compliance were identified. These include incorrect event code handling for the XInput extension, a lack of mandatory event delivery (such as ConfigureNotify) during window state changes, and insufficient validation for window dimensions and depths. Additionally, the architectural choice of a global window stack should be revised to a parent-relative model to correctly handle Z-order for subwindows.
- Refactor window stacking to a parent-relative model using window.children. - Remove obsolete global windowStack and associated methods. - Implement dynamic XInput event and error code offsets (starting at 64). - Add mandatory CreateNotify, DestroyNotify, MapNotify, UnmapNotify, and ConfigureNotify events. - Enforce strict validation for CreateWindow (non-zero size, InputOnly constraints). - Fix PutImage depth validation, including the XYBitmap exception. - Prevent integer overflows in coordinate translation and absolute position logic. - Initialize root window and visual state correctly in server and tests. - Remove global sequence tracking to ensure thread-safety in multi-client scenarios.
- Add protocol_compliance_test.go with comprehensive unit tests for: - Window validation (size, InputOnly constraints). - PutImage depth validation and XYBitmap handling. - Mandatory state change notifications. - Parent-relative stacking model. - Integer overflow prevention in coordinate calculations. - Property cleanup. - InternAtom OnlyIfExists flag. - Dynamic XInput event/error code offsets. - Refactor window event mask storage from a single global mask to a per-client map (eventMasks) to properly support X11's multi-client event distribution. - Update all event delivery logic (mouse, keyboard, notifications) to broadcast to all interested clients. - Fix a bug where DestroyNotify was not sent before window state deletion. - Update all existing test setups to correctly initialize the per-client eventMasks map.
- Update all manual &window{} instantiations in tests to initialize the eventMasks map.
- Remove redundant/duplicate test helper methods.
- Correctly apply the new multi-client event distribution logic within tests to resolve panics and test failures.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements X11 forwarding in sshterm, enabling remote graphical applications to be displayed in the browser using Go compiled to WebAssembly and HTML5 Canvas. The implementation covers X11 protocol handling, state management, and SSH channel integration, supported by a robust testing framework. The code review identified several critical issues regarding protocol compliance, specifically in the parsing and encoding of XInput events and the handling of variable-length generic events. Further feedback suggests refining the input focus notification logic to reach all interested clients, ensuring color allocation respects visual masks and non-contiguous requests, and providing clearer errors when X11 support is disabled.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces X11 forwarding support to sshterm, allowing remote graphical applications to be rendered directly in the browser using a Go-based WebAssembly server and HTML5 Canvas. The changes include a robust implementation of the X11 wire protocol, state management for windows and graphics contexts, and a comprehensive testing strategy. Technical feedback identified several areas for improvement, specifically regarding defensive programming: potential integer overflows in color cell and plane allocation logic, a possible underflow in keyboard mapping range checks when input counts are zero, and the necessity of handling errors during random cookie generation to maintain cryptographic integrity.
…error handling comments
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements X11 forwarding for the SSHTERM application, enabling remote graphical applications to render directly in the browser using a Go-based WebAssembly server and an HTML5 Canvas renderer. The implementation includes comprehensive protocol handling, state management for windows and resources, and a suite of visual and integration tests. The review feedback highlights several areas for improvement, including correcting a hardcoded value in the keyboard mapping reply, using more robust methods for random data generation, and refining visual class validation in color allocation handlers to better align with the X11 specification.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements X11 forwarding in sshterm, enabling remote graphical applications to be displayed in the browser through a Go-based X11 server compiled to WebAssembly. The implementation covers protocol handling, state management, and a rendering engine using HTML5 Canvas, along with comprehensive unit and visual tests. Feedback from the review identifies a potential resource ID collision due to a bit-shift mismatch, suggests performance optimizations for buffer allocations during message parsing, and highlights a possible race condition in the asynchronous clipboard selection logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements X11 forwarding support in sshterm, enabling remote graphical applications to be rendered in the browser using a Go/WASM X11 server and HTML5 Canvas. The implementation covers protocol handling, state management, and SSH integration. The review identified several critical issues, including potential integer overflows in message length calculations, incorrect color plane mask logic, and incomplete state rollback during failed pointer grabs. Feedback also noted concerns regarding protocol compliance for sequence numbers and the risks of using hardcoded temporary resource identifiers.
- Add overflow protection check in reply message and GenericEvent length calculation. - Support both PseudoColor and DirectColor visuals and implement robust plane mask allocation logic in handleAllocColorPlanes. - Revert all pointer grab state variables on pointer grab failure. - Set exact sequence number (oldClient.sequence) for SelectionClear events. - Dynamically allocate tempFID by scanning client resource ID namespace. - Update TestAllocColorPlanes_Visuals unit test accordingly.
1 participant
Summary of Changes
This pull request implements X11 forwarding for the SSH terminal, enabling users to run remote graphical applications and interact with them directly in their web browser. The implementation introduces a robust X11 server written in Go, compiled to WebAssembly, which handles the X11 protocol and communicates with a frontend renderer built on the HTML5 Canvas API. The changes include significant additions to the codebase to support protocol parsing, state management, and integration with SSH channels, alongside a comprehensive testing suite to ensure protocol compliance and rendering fidelity.
Highlights