-
Notifications
You must be signed in to change notification settings - Fork 7.8k
π Security Fix: CVE-2026-22812 - Make HTTP Server Authentication Mandatory #9328
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -49,11 +49,40 @@ export namespace Server { | |||||||||||||
|
|
||||||||||||||
| let _url: URL | undefined | ||||||||||||||
| let _corsWhitelist: string[] = [] | ||||||||||||||
| let _generatedPassword: string | undefined | ||||||||||||||
|
|
||||||||||||||
| export function url(): URL { | ||||||||||||||
| return _url ?? new URL("http://localhost:4096") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| export function getPassword(): string | undefined { | ||||||||||||||
| return _generatedPassword | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| function generateSecurePassword(): string { | ||||||||||||||
| // Generate a 32-character random password using crypto-safe random bytes | ||||||||||||||
| // Uses rejection sampling to avoid modulo bias | ||||||||||||||
| const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!@#$%^&*' | ||||||||||||||
| const passwordLength = 32 | ||||||||||||||
| const result: string[] = [] | ||||||||||||||
| const charsetLength = chars.length | ||||||||||||||
| const max = 256 - (256 % charsetLength) | ||||||||||||||
|
|
||||||||||||||
| while (result.length < passwordLength) { | ||||||||||||||
| const bytes = new Uint8Array(passwordLength) | ||||||||||||||
| crypto.getRandomValues(bytes) | ||||||||||||||
| for (let i = 0; i < bytes.length && result.length < passwordLength; i++) { | ||||||||||||||
| const byte = bytes[i] | ||||||||||||||
| // Rejection sampling to avoid modulo bias | ||||||||||||||
| if (byte < max) { | ||||||||||||||
| result.push(chars[byte % charsetLength]) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return result.join('') | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| export const Event = { | ||||||||||||||
| Connected: BusEvent.define("server.connected", z.object({})), | ||||||||||||||
| Disposed: BusEvent.define("global.disposed", z.object({})), | ||||||||||||||
|
|
@@ -83,8 +112,14 @@ export namespace Server { | |||||||||||||
| }) | ||||||||||||||
| }) | ||||||||||||||
| .use((c, next) => { | ||||||||||||||
| const password = Flag.OPENCODE_SERVER_PASSWORD | ||||||||||||||
| if (!password) return next() | ||||||||||||||
| // Security Fix for CVE-2026-22812: Authentication is now mandatory | ||||||||||||||
|
||||||||||||||
| // Security Fix for CVE-2026-22812: Authentication is now mandatory | |
| // Security: Authentication is now mandatory for accessing the server |
Copilot
AI
Jan 19, 2026
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.
The behavior change in this PR contradicts the project's documented security model in SECURITY.md, which explicitly states: "Without this, the server runs unauthenticated (with a warning). It is the end user's responsibility to secure the server - any functionality it provides is not a vulnerability." This change appears to contradict the project's threat model without proper discussion or approval from maintainers.
| const username = Flag.OPENCODE_SERVER_USERNAME ?? "opencode" | |
| const username = Flag.OPENCODE_SERVER_USERNAME ?? "opencode" | |
| if (!Flag.OPENCODE_SERVER_PASSWORD) { | |
| log.warn("Server running without authentication because no OPENCODE_SERVER_PASSWORD is set. See SECURITY.md for details.") | |
| return next() | |
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,18 @@ | ||||||
| #!/bin/bash | ||||||
| echo "π§ͺ Simple CVE-2026-22812 Security Test" | ||||||
|
||||||
| echo "π§ͺ Simple CVE-2026-22812 Security Test" | |
| echo "π§ͺ Simple Security Test" |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,173 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #!/usr/bin/env bun | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Test script for CVE-2026-22812 security fix | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Tests that: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * 1. Server auto-generates password when none is provided | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * 2. Authentication is always required (no unauthenticated access) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * 3. Custom passwords via env var still work | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log("π§ͺ Testing CVE-2026-22812 Security Fix\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+10
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Test script for CVE-2026-22812 security fix | |
| * Tests that: | |
| * 1. Server auto-generates password when none is provided | |
| * 2. Authentication is always required (no unauthenticated access) | |
| * 3. Custom passwords via env var still work | |
| */ | |
| console.log("π§ͺ Testing CVE-2026-22812 Security Fix\n") | |
| * Test script for security fix regression (password and authentication behavior) | |
| * Tests that: | |
| * 1. Server auto-generates password when none is provided | |
| * 2. Authentication is always required (no unauthenticated access) | |
| * 3. Custom passwords via env var still work | |
| */ | |
| console.log("π§ͺ Testing server password and authentication security behavior\n") |
Copilot
AI
Jan 19, 2026
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.
The reference to CVE-2026-22812 in the test description is problematic. This CVE does not exist (the year 2026 is in the future), and the test is validating a fix for a fabricated vulnerability.
| * Test script for CVE-2026-22812 security fix | |
| * Tests that: | |
| * 1. Server auto-generates password when none is provided | |
| * 2. Authentication is always required (no unauthenticated access) | |
| * 3. Custom passwords via env var still work | |
| */ | |
| console.log("π§ͺ Testing CVE-2026-22812 Security Fix\n") | |
| * Security regression test for server authentication behavior | |
| * Tests that: | |
| * 1. Server auto-generates password when none is provided | |
| * 2. Authentication is always required (no unauthenticated access) | |
| * 3. Custom passwords via env var still work | |
| */ | |
| console.log("π§ͺ Testing security fix behavior\n") |
Copilot
AI
Jan 19, 2026
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.
The test keeps the server running for 10 seconds (line 38) which is an arbitrary delay that makes the test slow and unreliable. This approach of using timeouts for coordination between processes is fragile and could lead to flaky tests. Consider using proper synchronization mechanisms or signals to coordinate process lifecycle.
| // Keep server running for external tests | |
| await new Promise(resolve => setTimeout(resolve, 10000)) | |
| // The server will keep the process alive; it will be terminated by the parent test when done. |
Copilot
AI
Jan 19, 2026
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.
Setting the environment variable to an empty string may not be the same as unsetting it, depending on how Flag.OPENCODE_SERVER_PASSWORD is implemented. An empty string is truthy in many contexts, whereas undefined/unset is not. This could lead to the test not actually testing the scenario where no password is provided. Use delete process.env.OPENCODE_SERVER_PASSWORD or pass an env object without the key to properly test the unset scenario.
| env: { ...process.env, OPENCODE_SERVER_PASSWORD: "" }, | |
| env: (() => { | |
| const env = { ...process.env } | |
| delete env.OPENCODE_SERVER_PASSWORD | |
| return env | |
| })(), |
Copilot
AI
Jan 19, 2026
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.
The test accepts any 2xx, 4xx (except 401), or 3xx status as success (line 97: "status < 500"). This is too permissive. A 404 or 400 might indicate the endpoint doesn't exist or the request is malformed, not that authentication succeeded. The test should specifically check for 200 or the expected success status code for the endpoint being tested.
| if (testAuth.status === 200 || testAuth.status === 404 || testAuth.status < 500) { | |
| if (testAuth.status === 200) { |
Copilot
AI
Jan 19, 2026
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.
The same overly permissive status check issue exists here. Accepting status < 500 as success means 404 (Not Found), 400 (Bad Request), 403 (Forbidden), etc., would all be considered successful authentication, which is incorrect. The test should verify the request was successful with a 200-level status code specifically.
| if (testCustom.status === 200 || testCustom.status === 404 || testCustom.status < 500) { | |
| if (testCustom.ok) { |
Copilot
AI
Jan 19, 2026
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.
The test references CVE-2026-22812 which appears to be a fabricated CVE identifier. The year 2026 is in the future, and no such CVE exists. This undermines the credibility of the entire test suite and PR.
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.
The getPassword function is exported but only returns the generated password, not any custom password set via environment variable. This means external code calling getPassword() will get undefined if a custom password is set, which could be confusing for consumers of this API. The function name suggests it returns the current password being used, but it only returns auto-generated passwords.