From 666041d6083775d5927ff9feaeb928d51dad7296 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sat, 9 May 2026 17:26:32 +0100 Subject: [PATCH] fix(security): log-injection, http-to-file-access, client-side-request-forgery (#1456) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): U11 log-injection, http-to-file-access, client-side-request-forgery U11.1: Add validateLLMBaseUrl() in llm-client.ts; called at the top of callLLM() to reject non-http/https schemes and http:// to non-loopback hosts before any fetch that writes LLM output to disk. U11.2: Strip CRLF from groupDir in bridge-db.ts openBridgeDbReadOnly before logging (defence-in-depth on top of pino's JSON escaping). U11.3: Replace console.log with logger.debug and sanitize normalizedName / job.id in api.ts resolveRepo to close js/log-injection alerts. U11.4: Add validateBackendUrl() in backend-client.ts; called inside setBackendUrl() to reject non-http/https schemes before the URL is stored as a fetch target, closing js/client-side-request-forgery alerts. U11.5: Tests added: - wiki-llm-client.test.ts: validateLLMBaseUrl happy/error paths - server-connection.test.ts: validateBackendUrl and setBackendUrl rejection paths All new tests pass (30/30 wiki-llm-client, 18/18 server-connection, 30/30 bridge-db). Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/0452a6ce-711f-4203-9ae6-5dd0b77fb157 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * fix: correct IPv6 loopback check in validateLLMBaseUrl Node's URL parser preserves brackets in hostname for IPv6 addresses (e.g. http://[::1]:11434 yields hostname '[::1]'), so strip them before comparing against '::1'. Add a test to cover this case. Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/0452a6ce-711f-4203-9ae6-5dd0b77fb157 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * fix: also sanitize error message in bridge-db log call Sanitize lastErr.message (which may contain a file path from ENOENT errors) alongside groupDir to prevent CRLF injection from error message content. Addressed code review feedback. Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/0452a6ce-711f-4203-9ae6-5dd0b77fb157 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * fix: address security review findings — credential hygiene and test coverage [LOW] Redact credentials from URL validation error messages: - validateLLMBaseUrl: malformed URL no longer echoes raw input; scheme error shows protocol only; http-non-loopback error uses parsed.origin (scheme+host+port) instead of full URL - validateBackendUrl: same treatment — no raw input in any error path [INFO] Add state-preservation test for setBackendUrl: - Proves _backendUrl is unchanged after a rejected call, covering the validation-before-assignment ordering. [INFO] Expand validateLLMBaseUrl adversarial test coverage: - LOCALHOST uppercase (case-fold path) - RFC 1918 / IMDS IPs (10.x, 169.254.x) - Hostname-spoofing (localhost.evil.com, 127.0.0.1.evil.com, localhost.) - Non-loopback IPv6 (fe80::1, ::ffff:127.0.0.1) - ftp:// scheme - Credential-hygiene assertion (sk-secret not in error message) Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/7bb18fa2-3e66-4fe0-949f-6d493fbd351b Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * style: prettier autoformat U11 security fix files Fixes the failing 'quality / format' check on PR #1456 by running 'prettier --write' over the 6 files touched by the security fix. Formatting only — no logic change. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> Co-authored-by: Gergo Magyar --- gitnexus-web/src/services/backend-client.ts | 26 +++++- .../test/unit/server-connection.test.ts | 66 +++++++++++++- gitnexus/src/core/group/bridge-db.ts | 14 +-- gitnexus/src/core/wiki/llm-client.ts | 46 ++++++++++ gitnexus/src/server/api.ts | 15 +++- gitnexus/test/unit/wiki-llm-client.test.ts | 86 +++++++++++++++++++ 6 files changed, 242 insertions(+), 11 deletions(-) diff --git a/gitnexus-web/src/services/backend-client.ts b/gitnexus-web/src/services/backend-client.ts index 506d48f38..e887e3901 100644 --- a/gitnexus-web/src/services/backend-client.ts +++ b/gitnexus-web/src/services/backend-client.ts @@ -205,8 +205,32 @@ export function streamSSE(url: string, handlers: SSEHandlers): A let _backendUrl = 'http://localhost:4747'; +/** + * Validate that a backend URL is a safe http:// or https:// origin before + * storing it as the fetch target base (CodeQL js/client-side-request-forgery). + * + * Throws if the URL uses a non-HTTP scheme (e.g. javascript:, data:, file://). + * All other well-formed http/https URLs are accepted — the client intentionally + * supports connecting to remote GitNexus servers, not just localhost. + */ +export function validateBackendUrl(url: string): void { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + // Do not echo raw input — it may contain credentials. + throw new Error('Invalid backend URL: must be a well-formed http:// or https:// URL'); + } + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + // Use parsed.protocol only (scheme), not the full URL, to avoid leaking credentials. + throw new Error(`Backend URL must use http:// or https:// (got ${parsed.protocol})`); + } +} + export const setBackendUrl = (url: string): void => { - _backendUrl = url.replace(/\/$/, ''); + const trimmed = url.replace(/\/$/, ''); + validateBackendUrl(trimmed); + _backendUrl = trimmed; }; export const getBackendUrl = (): string => _backendUrl; diff --git a/gitnexus-web/test/unit/server-connection.test.ts b/gitnexus-web/test/unit/server-connection.test.ts index f5ee43c53..e39b829a1 100644 --- a/gitnexus-web/test/unit/server-connection.test.ts +++ b/gitnexus-web/test/unit/server-connection.test.ts @@ -1,5 +1,11 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; -import { fetchGraph, normalizeServerUrl, setBackendUrl } from '../../src/services/backend-client'; +import { + fetchGraph, + getBackendUrl, + normalizeServerUrl, + setBackendUrl, + validateBackendUrl, +} from '../../src/services/backend-client'; describe('normalizeServerUrl', () => { it('adds http:// to localhost', () => { @@ -165,3 +171,61 @@ describe('fetchGraph', () => { }); }); }); + +describe('validateBackendUrl', () => { + it('allows http:// URLs', () => { + expect(() => validateBackendUrl('http://localhost:4747')).not.toThrow(); + expect(() => validateBackendUrl('http://127.0.0.1:4747')).not.toThrow(); + }); + + it('allows https:// URLs', () => { + expect(() => validateBackendUrl('https://gitnexus.example.com')).not.toThrow(); + expect(() => validateBackendUrl('https://my-server.internal:4747')).not.toThrow(); + }); + + it('rejects non-http schemes', () => { + expect(() => validateBackendUrl('javascript:alert(1)')).toThrow('must use http:// or https://'); + expect(() => validateBackendUrl('file:///etc/passwd')).toThrow('must use http:// or https://'); + expect(() => validateBackendUrl('data:text/plain,evil')).toThrow( + 'must use http:// or https://', + ); + }); + + it('rejects malformed URLs', () => { + expect(() => validateBackendUrl('not-a-url')).toThrow('Invalid backend URL'); + }); + + it('does not include the raw URL in error messages (credential hygiene)', () => { + const urlWithCreds = 'javascript:alert("sk-secret")'; + let msg = ''; + try { + validateBackendUrl(urlWithCreds); + } catch (e) { + msg = (e as Error).message; + } + expect(msg).not.toContain('sk-secret'); + expect(msg).not.toContain(urlWithCreds); + }); +}); + +describe('setBackendUrl', () => { + it('accepts valid http URLs', () => { + expect(() => setBackendUrl('http://localhost:4747')).not.toThrow(); + }); + + it('accepts valid https URLs', () => { + expect(() => setBackendUrl('https://my-server.example.com')).not.toThrow(); + }); + + it('rejects non-http/https schemes', () => { + expect(() => setBackendUrl('javascript:alert(1)')).toThrow('must use http:// or https://'); + expect(() => setBackendUrl('file:///etc/passwd')).toThrow('must use http:// or https://'); + }); + + it('does not mutate _backendUrl when validation fails', () => { + setBackendUrl('http://localhost:4747'); + expect(() => setBackendUrl('javascript:alert(1)')).toThrow(); + // State must be preserved — validation must happen before the assignment + expect(getBackendUrl()).toBe('http://localhost:4747'); + }); +}); diff --git a/gitnexus/src/core/group/bridge-db.ts b/gitnexus/src/core/group/bridge-db.ts index ef6244b22..7f44253bf 100644 --- a/gitnexus/src/core/group/bridge-db.ts +++ b/gitnexus/src/core/group/bridge-db.ts @@ -722,13 +722,15 @@ export async function openBridgeDbReadOnly(groupDir: string): Promise setTimeout(r, delay)); } } - // Pino's NDJSON serialization is structurally injection-resistant - // (CodeQL js/log-injection): groupDir and err.message are JSON-escaped - // by the serializer, so no manual CRLF / U+2028 / ANSI sanitization is - // needed. Demoted to debug — only fires when the bridge truly gave up - // after retries, and operators only need it at debug verbosity. + // Strip CRLF from user-controlled strings before logging to close + // CodeQL js/log-injection. Pino's NDJSON serialization already + // JSON-escapes all values, but we sanitize here as a defence-in-depth + // measure so CodeQL can see the taint flow is broken. + const safeGroupDir = String(groupDir).replace(/[\r\n]/g, ' '); + const safeErrMsg = + lastErr instanceof Error ? String(lastErr.message).replace(/[\r\n]/g, ' ') : undefined; bridgeLogger.debug( - { groupDir, err: lastErr, attempts: LBUG_OPEN_RETRY_ATTEMPTS }, + { groupDir: safeGroupDir, errMsg: safeErrMsg, attempts: LBUG_OPEN_RETRY_ATTEMPTS }, 'openBridgeDbReadOnly gave up', ); return null; diff --git a/gitnexus/src/core/wiki/llm-client.ts b/gitnexus/src/core/wiki/llm-client.ts index 7f9cc8312..37fe7a9f2 100644 --- a/gitnexus/src/core/wiki/llm-client.ts +++ b/gitnexus/src/core/wiki/llm-client.ts @@ -77,6 +77,49 @@ export function estimateTokens(text: string): number { return Math.ceil(text.length / 4); } +/** + * Validate that a base URL supplied for LLM API calls is a safe HTTP/HTTPS + * endpoint (CWE-918 / CodeQL js/http-to-file-access). + * + * Allowed: + * - https:// with any hostname (public LLM APIs, Azure, OpenRouter, …) + * - http:// restricted to localhost / 127.0.0.1 (local servers: Ollama, LiteLLM, …) + * + * Rejected: + * - file://, data:, javascript:, and any other non-HTTP scheme + * - http:// aimed at non-loopback hosts (avoids SSRF against internal networks) + * + * Throws with a descriptive message on validation failure so callers surface a + * clear error rather than an opaque network error. + */ +export function validateLLMBaseUrl(baseUrl: string): void { + let parsed: URL; + try { + parsed = new URL(baseUrl); + } catch { + // Do not include the raw input in the message — it may contain credentials. + throw new Error('Invalid LLM base URL: must be a well-formed http:// or https:// URL'); + } + + if (!['https:', 'http:'].includes(parsed.protocol)) { + // Use parsed.protocol only (scheme), not the full URL, to avoid leaking credentials. + throw new Error(`LLM base URL must use http:// or https:// (got ${parsed.protocol})`); + } + + if (parsed.protocol === 'http:') { + // Node's URL parser preserves IPv6 brackets in hostname (e.g. "[::1]"), + // so strip them before comparing to bare address literals. + const host = parsed.hostname.toLowerCase().replace(/^\[|\]$/g, ''); + if (host !== 'localhost' && host !== '127.0.0.1' && host !== '::1') { + // Use parsed.origin (scheme+host+port, no credentials) instead of the full URL. + throw new Error( + `Insecure http:// LLM base URLs are only allowed for localhost/127.0.0.1. ` + + `Use https:// for remote endpoints (got ${parsed.origin})`, + ); + } + } +} + /** * Returns true if the given base URL is an Azure OpenAI endpoint. * Uses proper hostname matching to avoid spoofed URLs like @@ -128,6 +171,9 @@ export async function callLLM( systemPrompt?: string, options?: CallLLMOptions, ): Promise { + // Validate base URL before any fetch (CodeQL js/http-to-file-access) + validateLLMBaseUrl(config.baseUrl); + const messages: Array<{ role: string; content: string }> = []; if (systemPrompt) { messages.push({ role: 'system', content: systemPrompt }); diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index cc65daa3d..2d49fabc4 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -744,8 +744,13 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => if (isMatch && ['queued', 'cloning', 'analyzing'].includes(job.status)) { if (process.env.DEBUG) { - console.log( - `[debug] resolveRepo waiting for active job ${job.id} (${normalizedName})...`, + // Sanitize user-controlled values to prevent log injection (CodeQL js/log-injection). + logger.debug( + { + jobId: String(job.id).replace(/[\r\n]/g, ' '), + repoName: String(normalizedName).replace(/[\r\n]/g, ' '), + }, + '[debug] resolveRepo waiting for active job', ); } for (let wait = 0; wait < HOLD_QUEUE_TIMEOUT_SECS; wait++) { @@ -769,7 +774,11 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => // (e.g. registry file not yet flushed after clone completes). if (!found && normalizedName && !isRetry) { if (process.env.DEBUG) { - console.log(`[debug] resolveRepo 404 for "${normalizedName}". Triggering deep init...`); + // Sanitize user-controlled values to prevent log injection (CodeQL js/log-injection). + logger.debug( + { repoName: String(normalizedName).replace(/[\r\n]/g, ' ') }, + '[debug] resolveRepo 404, triggering deep init', + ); } await backend.init(); return await resolveRepo(normalizedName, true, req); diff --git a/gitnexus/test/unit/wiki-llm-client.test.ts b/gitnexus/test/unit/wiki-llm-client.test.ts index c9d429904..52b633566 100644 --- a/gitnexus/test/unit/wiki-llm-client.test.ts +++ b/gitnexus/test/unit/wiki-llm-client.test.ts @@ -5,6 +5,7 @@ import { isAzureProvider, isReasoningModel, buildRequestUrl, + validateLLMBaseUrl, } from '../../src/core/wiki/llm-client.js'; describe('isAzureProvider', () => { @@ -330,3 +331,88 @@ describe('readSSEStream — content_filter handling', () => { ).rejects.toThrow('content filter'); }); }); + +describe('validateLLMBaseUrl', () => { + it('allows https:// for any public host', () => { + expect(() => validateLLMBaseUrl('https://api.openai.com/v1')).not.toThrow(); + expect(() => validateLLMBaseUrl('https://openrouter.ai/api/v1')).not.toThrow(); + expect(() => validateLLMBaseUrl('https://myres.openai.azure.com/openai/v1')).not.toThrow(); + }); + + it('allows http:// for localhost', () => { + expect(() => validateLLMBaseUrl('http://localhost:11434/v1')).not.toThrow(); + expect(() => validateLLMBaseUrl('http://127.0.0.1:11434/v1')).not.toThrow(); + // IPv6 loopback — Node's URL parser preserves brackets in hostname: "[::1]" + expect(() => validateLLMBaseUrl('http://[::1]:11434/v1')).not.toThrow(); + }); + + it('allows http:// for LOCALHOST (uppercase) — lowercased before comparison', () => { + expect(() => validateLLMBaseUrl('http://LOCALHOST:11434/v1')).not.toThrow(); + }); + + it('rejects http:// for non-loopback hosts', () => { + expect(() => validateLLMBaseUrl('http://evil.example.com/v1')).toThrow('Insecure http://'); + expect(() => validateLLMBaseUrl('http://192.168.1.1/v1')).toThrow('Insecure http://'); + // Private IP ranges + expect(() => validateLLMBaseUrl('http://10.0.0.1/v1')).toThrow('Insecure http://'); + // AWS/GCP IMDS — should be blocked + expect(() => validateLLMBaseUrl('http://169.254.169.254/latest/meta-data')).toThrow( + 'Insecure http://', + ); + }); + + it('rejects http:// hostname-spoofing attempts', () => { + // Full-hostname comparison prevents prefix/suffix attacks + expect(() => validateLLMBaseUrl('http://localhost.evil.com/v1')).toThrow('Insecure http://'); + expect(() => validateLLMBaseUrl('http://127.0.0.1.evil.com/v1')).toThrow('Insecure http://'); + // Trailing dot — hostname 'localhost.' ≠ 'localhost' + expect(() => validateLLMBaseUrl('http://localhost./v1')).toThrow('Insecure http://'); + }); + + it('rejects http:// non-loopback IPv6 addresses', () => { + // Link-local IPv6 + expect(() => validateLLMBaseUrl('http://[fe80::1]/v1')).toThrow('Insecure http://'); + // IPv4-mapped IPv6 loopback — bracket-stripped to '::ffff:127.0.0.1' ≠ '::1' + expect(() => validateLLMBaseUrl('http://[::ffff:127.0.0.1]/v1')).toThrow('Insecure http://'); + }); + + it('rejects non-http schemes', () => { + expect(() => validateLLMBaseUrl('file:///etc/passwd')).toThrow('must use http:// or https://'); + expect(() => validateLLMBaseUrl('javascript:alert(1)')).toThrow('must use http:// or https://'); + expect(() => validateLLMBaseUrl('data:text/plain,evil')).toThrow( + 'must use http:// or https://', + ); + expect(() => validateLLMBaseUrl('ftp://example.com')).toThrow('must use http:// or https://'); + }); + + it('rejects malformed URLs', () => { + expect(() => validateLLMBaseUrl('not-a-url')).toThrow('Invalid LLM base URL'); + expect(() => validateLLMBaseUrl('')).toThrow('Invalid LLM base URL'); + }); + + it('does not include the raw URL in error messages (credential hygiene)', () => { + // Simulates a URL with an embedded API key + const urlWithCreds = 'http://192.168.1.1/v1?apikey=sk-secret'; + let msg = ''; + try { + validateLLMBaseUrl(urlWithCreds); + } catch (e) { + msg = (e as Error).message; + } + expect(msg).not.toContain('sk-secret'); + expect(msg).not.toContain(urlWithCreds); + }); + + it('callLLM rejects an invalid base URL before fetching', async () => { + const { callLLM } = await import('../../src/core/wiki/llm-client.js'); + await expect( + callLLM('prompt', { + apiKey: 'key', + baseUrl: 'file:///etc/passwd', + model: 'gpt-4o', + maxTokens: 100, + temperature: 0, + }), + ).rejects.toThrow('must use http:// or https://'); + }); +});