-
Notifications
You must be signed in to change notification settings - Fork 35
copy/paste fix (v1) #36
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Changed
This PR addresses a clipboard issue where non-host users couldn't paste text. It introduces a new backend API endpoint, POST /computer/paste, which uses xclip and xdotool to perform the paste operation directly on the host system. On the client side, a new Vue plugin (globalPaste.ts) is added to intercept Ctrl+V/Cmd+V events, read the clipboard content, and send it to this new local API endpoint. CORS has also been enabled on the Go server to allow requests from the web client.
Risks / Concerns
The review identified two high-severity security risks:
- The CORS policy in
server/cmd/api/main.gois overly permissive (AllowedOrigins: "*"with credentials), creating a potential CSRF vulnerability. It should be restricted to specific, trusted origins. - The client-side code in
images/chromium-headful/client/src/neko/index.tslogs the full content of the clipboard to the browser console, which could expose sensitive user data.
14 files reviewed | 2 comments | Review on Mesa | Edit Reviewer Settings
| } | ||
|
|
||
| protected [EVENT.CONTROL.CLIPBOARD]({ text }: ControlClipboardPayload) { | ||
| console.log('[clipboard] EVENT.CONTROL.CLIPBOARD:', text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the security implications of logging clipboard data. Clipboard content could contain sensitive information like passwords, API keys, or personal data. While this logging is helpful for debugging, you may want to either:
- Truncate/sanitize the logged text (e.g.,
console.log('[clipboard] EVENT.CONTROL.CLIPBOARD:', text.substring(0, 50) + '...')) - Add a flag to disable clipboard logging in production
- Use a more secure logging method that doesn't expose sensitive data in browser console
This is particularly important since the browser console can be accessed by users or browser extensions.
Type: Security | Severity: High
|
|
||
| r := chi.NewRouter() | ||
|
|
||
| r.Use(cors.Handler(cors.Options{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security concern: Using AllowedOrigins: "*" with AllowCredentials: true is a dangerous combination that can lead to CSRF attacks. Any website can make authenticated requests to this API. Consider either:
- Remove
AllowCredentials: trueif credentials aren't needed - Specify exact origins instead of
"*"(e.g.,[]string{"http://localhost:3000"}for the Vue client) - If this is truly localhost-only, consider restricting origins to localhost variants
| r.Use(cors.Handler(cors.Options{ | |
| r.Use(cors.Handler(cors.Options{ | |
| AllowedOrigins: []string{"http://localhost:3000", "http://127.0.0.1:3000"}, | |
| AllowedMethods: []string{"GET", "POST", "PUT", "DELETE", "OPTIONS"}, | |
| AllowedHeaders: []string{"Accept", "Authorization", "Content-Type", "X-CSRF-Token"}, | |
| ExposedHeaders: []string{"X-Recording-Started-At", "X-Recording-Finished-At"}, | |
| AllowCredentials: true, | |
| MaxAge: 300, | |
| })) |
Type: Security | Severity: High
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Clipboard URL Hardcoding and Paste Interception Issues
The commit introduces a hardcoded http://localhost:10001/computer/paste URL in both remote.ts (for fallback clipboard updates) and globalPaste.ts (for Ctrl/Cmd+V interception). This assumes the server runs on localhost:10001, which is problematic as the server's host/port is configurable, leading to connection failures in non-localhost or containerized deployments.
Additionally, the globalPaste plugin has two issues:
- It unconditionally calls
e.preventDefault()for Ctrl/Cmd+V, overriding native browser paste behavior. This breaks standard paste functionality in text inputs and prevents fallback if the custom clipboard read or API call fails. - It sends an API request even when
navigator.clipboard.readText()returns an empty string, causing unnecessary network traffic and server 400 errors.
images/chromium-headful/client/src/plugins/globalPaste.ts#L8-L19
images/chromium-headful/client/src/store/remote.ts#L45-L46
Bug: CORS Misconfiguration: Insecure Wildcard Origin
The CORS configuration is invalid and insecure: AllowedOrigins is set to "*" while AllowCredentials is true. This violates the CORS specification, causing browsers to reject cross-origin requests and creating a security vulnerability by allowing any website to make authenticated requests (e.g., CSRF). The configuration must either set AllowCredentials to false or specify explicit allowed origins.
server/cmd/api/main.go#L45-L53
Was this report helpful? Give feedback by reacting with 👍 or 👎
rgarcia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Alternative A" is more straightforward, i.e. giving the "dummy" user a profile that allows clipboard use in neko
| try { | ||
| const text = await navigator.clipboard.readText() | ||
| console.log(`[vue:globalPaste]:payload:` , text) | ||
| await fetch('http://localhost:10001/computer/paste', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using localhost here won't work in prod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on it 👍
note :
idea of external controls (outside of neko events) exposed via API might be useful down the line ; as a controller for agents to stream events to/from via sockets. not via localhost on the instance like in here - would be routed to and token-gated by the kernel api , and connected to by the ^ container and user/agent outside of the vue app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes definitely--this is why we added the click/move mouse stuff into the API. I think eventually we will go deeper on that front
| try { | ||
| const text = await navigator.clipboard.readText() | ||
| console.log(`[vue:globalPaste]:payload:` , text) | ||
| await fetch('http://localhost:10001/computer/paste', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Hardcoded URL Causes Production Environment Issues
The http://localhost:10001/computer/paste URL is hardcoded in remote.ts and globalPaste.ts. This prevents the application from functioning correctly in production, containerized, or distributed environments where the API server is not running on localhost:10001. This issue was previously identified in PR discussions.
Locations (2)
| ExposedHeaders: []string{"X-Recording-Started-At", "X-Recording-Finished-At"}, | ||
| AllowCredentials: true, | ||
| MaxAge: 300, | ||
| })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: CORS Misconfiguration Leads to Security Vulnerability
The CORS configuration is overly permissive, allowing all origins (*) while simultaneously setting AllowCredentials: true. This combination is invalid according to the CORS specification and creates a significant security vulnerability, enabling cross-origin credential theft and attacks.
(Issue #31)
clipboardv1_test.mp4
Issue Trace
9:29PM WRN message handler has failed error="is not the host" event=clipboard/set module=websocket session_id=dummy-vPhbw submodule=handleronkernel/kernel-images:images/chromium-headful/Dockerfile:FROM ghcr.io/m1k1o/neko/chromium:3.0.6 AS nekom1k1o/neko:server/internal/websocket/handler/clipboard.go| see hereSuggested Fixes
Alternative A (not tried)
onkernel/kernel-images:images/chromium-headful/client/src/components/connect.vueAlternative B (implemented)
POST /computer/paste : { text }to the Go/serverto manage clipboard + trigger paste , enabled CORS to be called by the web clientvueclient to capture clipboard paste events and call the local servernote
WITH_KERNEL_IMAGES_API=true ENABLE_WEBRTC=true ./run-docker.shTL;DR
Fixed clipboard paste functionality for non-host clients by introducing a new local API endpoint and client-side paste interception.
Why we made these changes
The existing
nekoclipboard mechanism restricted paste operations to the "host" session, preventing other clients (like the dummy client in the chromium container) from pasting content, as evidenced by the "is not the host" error. This change enables universal paste functionality.What changed?
POST /computer/pasteendpoint to handle clipboard setting and simulateCtrl+V(usingxclipandxdotool).go-chi/corsdependency and updated OpenAPI spec.GlobalPasteVue plugin to intercept global paste events (Ctrl+V/Cmd+V)./computer/pasteendpoint.video.vueandremote.tsto use the new local fallback service for clipboard updates.Validation
Verified with provided video demonstration (Issue #31).