-
Notifications
You must be signed in to change notification settings - Fork 4
fix: validate preference input, bound logged fields, set an explicit redirect policy #378
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
8ef3d19
a7856e0
17b9ebb
5f7a991
45a3dae
14f6ccf
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 | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,12 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| return ALLOWED_PREFIXES.some((p) => key.startsWith(p)); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // Most keys one read may name. Every caller in the app asks for a single key | ||||||||||||||||||||||||||||||||||||||||||||||||
| // (see SettingsApp, i18n, mascot-client); the whole set is fetched with `all=1` | ||||||||||||||||||||||||||||||||||||||||||||||||
| // instead. Bound it so the size of a response follows the store rather than the | ||||||||||||||||||||||||||||||||||||||||||||||||
| // request. | ||||||||||||||||||||||||||||||||||||||||||||||||
| const MAX_KEYS_PER_READ = 32; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| // GET /setup-api/preferences?keys=wp_opacity,wp_bg_color | ||||||||||||||||||||||||||||||||||||||||||||||||
| // GET /setup-api/preferences?all=1 (returns all pref:* keys) | ||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -46,9 +52,19 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| return NextResponse.json({ error: "keys or all param required" }, { status: 400 }); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| const keys = keysParam.split(",").filter(isAllowed); | ||||||||||||||||||||||||||||||||||||||||||||||||
| if (keys.length > MAX_KEYS_PER_READ) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| return NextResponse.json( | ||||||||||||||||||||||||||||||||||||||||||||||||
| { error: `at most ${MAX_KEYS_PER_READ} keys per request` }, | ||||||||||||||||||||||||||||||||||||||||||||||||
| { status: 400 }, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| // One read of the store rather than one per key: config.get() re-reads and | ||||||||||||||||||||||||||||||||||||||||||||||||
| // re-parses the whole file synchronously on every call, so the work of a | ||||||||||||||||||||||||||||||||||||||||||||||||
| // request would otherwise follow the length of its `keys` parameter. | ||||||||||||||||||||||||||||||||||||||||||||||||
| const allConfig = await config.getAll(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
54
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win Count supplied keys before whitelist filtering. Line 54 removes unallowed keys before the limit check. A request with more than 32 unallowed keys bypasses the cap and still calls Proposed fix- const keys = keysParam.split(",").filter(isAllowed);
- if (keys.length > MAX_KEYS_PER_READ) {
+ const requestedKeys = keysParam.split(",");
+ if (requestedKeys.length > MAX_KEYS_PER_READ) {
return NextResponse.json(
{ error: `at most ${MAX_KEYS_PER_READ} keys per request` },
{ status: 400 },
);
}
+ const keys = requestedKeys.filter(isAllowed);As per path instructions, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||||||||||||||||||||||||||||||||||
| const result: Record<string, unknown> = Object.create(null); | ||||||||||||||||||||||||||||||||||||||||||||||||
| for (const key of keys) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| result[key] = await config.get(`pref:${key}`); | ||||||||||||||||||||||||||||||||||||||||||||||||
| result[key] = allConfig[`pref:${key}`]; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
KrasimirKralev marked this conversation as resolved.
Dismissed
|
||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| return NextResponse.json(sanitizePreferences(result)); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,29 @@ export function validateProjectId(id: string): boolean { | |
| return APP_ID_RE.test(id); | ||
| } | ||
|
|
||
| /** Longest project name the desktop label and the starter templates carry. */ | ||
| export const MAX_PROJECT_NAME_LENGTH = 60; | ||
|
|
||
| /** | ||
| * The name a project may be created with, or a ValidationError. | ||
| * | ||
| * Checked here rather than at each caller because initProject writes the | ||
| * directory and project.json before the name reaches the templates: a name the | ||
| * templates cannot render has to be refused while nothing has been created yet, | ||
| * so a rejected request leaves no project behind for the next attempt to | ||
| * collide with. The MCP door declares the same limit (`zText(60)` in | ||
| * mcp/tools/desktop.ts); this is where it is enforced. | ||
| */ | ||
| export function validateProjectName(name: unknown): string { | ||
| if (typeof name !== "string") throw new ValidationError("Project name must be a string"); | ||
| const trimmed = name.trim(); | ||
| if (!trimmed) throw new ValidationError("Project name required"); | ||
| if (trimmed.length > MAX_PROJECT_NAME_LENGTH) { | ||
| throw new ValidationError(`Project name must be at most ${MAX_PROJECT_NAME_LENGTH} characters`); | ||
| } | ||
| return trimmed; | ||
| } | ||
|
|
||
| /** Resolve a file path inside a project directory, preventing traversal. */ | ||
| function safePath(projectId: string, filePath: string): string { | ||
| if (!validateProjectId(projectId)) throw new ValidationError("Invalid project ID"); | ||
|
|
@@ -119,6 +142,8 @@ export async function initProject( | |
| opts?: { color?: string; description?: string; template?: "blank" | "app" } | ||
| ): Promise<ProjectMeta> { | ||
| if (!validateProjectId(projectId)) throw new ValidationError("Invalid project ID"); | ||
| // Before anything is created on disk — see validateProjectName. | ||
| const projectName = validateProjectName(name); | ||
|
|
||
| const dir = projectDir(projectId); | ||
| const exists = await fs.stat(dir).catch(() => null); | ||
|
|
@@ -129,7 +154,7 @@ export async function initProject( | |
| const now = new Date().toISOString(); | ||
| const meta: ProjectMeta = { | ||
| projectId, | ||
| name, | ||
| name: projectName, | ||
| color: opts?.color || "#f97316", | ||
| description: opts?.description || "", | ||
| created: now, | ||
|
|
@@ -147,14 +172,14 @@ export async function initProject( | |
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>${escapeHtml(name)}</title> | ||
| <title>${escapeHtml(projectName)}</title> | ||
| <style> | ||
| * { margin: 0; padding: 0; box-sizing: border-box; } | ||
| body { font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', sans-serif; background: #1a1a2e; color: #e0e0e0; min-height: 100vh; display: flex; align-items: center; justify-content: center; } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <h1>${escapeHtml(name)}</h1> | ||
| <h1>${escapeHtml(projectName)}</h1> | ||
| </body> | ||
| </html>`, | ||
| "utf-8" | ||
|
|
@@ -168,7 +193,7 @@ export async function initProject( | |
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>${escapeHtml(name)}</title> | ||
| <title>${escapeHtml(projectName)}</title> | ||
| <link rel="stylesheet" href="style.css"> | ||
| </head> | ||
| <body> | ||
|
|
@@ -207,8 +232,8 @@ h1 { | |
| // template-literal metacharacters (` $ \) and newlines untouched, so a name | ||
| // like "`;fetch('/setup-api/...')`" would break out of the literal and run | ||
| // as code when the built app loads on the ClawBox origin (stored XSS). | ||
| const commentName = name.replace(/[\r\n]+/g, " "); | ||
| const innerName = jsTemplateEscape(escapeHtml(name)); | ||
| const commentName = projectName.replace(/[\r\n]+/g, " "); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win XSS (CWE-79): Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') Reachability: External Reachability pathSanitize all JavaScript line terminators before generating The request-derived 🤖 Prompt for AI Agents |
||
| const innerName = jsTemplateEscape(escapeHtml(projectName)); | ||
| await fs.writeFile( | ||
| path.join(dir, "app.js"), | ||
| `// ${commentName} — ClawBox Web App | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,15 @@ const LOGIN_RETRY_COOLDOWN_MS = 10_000; | |
| // promise to every later caller until the server is restarted. Every request | ||
| // this module issues is therefore bounded. | ||
| const REQUEST_TIMEOUT_MS = 8_000; | ||
| // Every request below sets this. Node's fetch defaults to "follow", which makes | ||
| // a redirect invisible to the caller — the response that comes back is the one | ||
| // from wherever Location pointed, not from the path we asked for, and a | ||
| // redirected request can carry its body and headers there. Resolving redirects | ||
| // manually keeps each call's answer the answer to the call it made: a 3xx from | ||
| // the dashboard means the request did not reach the API, which is what the | ||
| // callers below already treat as "not signed in". Same rule and same reason as | ||
| // mcp/lib/api.ts. | ||
| const REDIRECT_POLICY = "manual" as const; | ||
|
Comment on lines
+33
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/lib/hermes-dashboard-auth.ts --items all
rg -n -C 4 '\bDASH_ORIGIN\b|dashboardFetch\s*\(' src/lib/hermes-dashboard-auth.ts .Repository: ID-Robots/clawbox Length of output: 8090 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- hermes-dashboard-auth.ts ---'
sed -n '1,110p' src/lib/hermes-dashboard-auth.ts
printf '%s\n' '--- HERMES_DASH_HOST / HERMES_PORT references ---'
rg -n -C 3 'HERMES_DASH_HOST|HERMES_PORT|HERMES_DASH_USERNAME' . \
-g '!node_modules' -g '!dist' -g '!build'Repository: ID-Robots/clawbox Length of output: 17318 Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information Reachability: Internal Reachability pathRestrict
🤖 Prompt for AI Agents |
||
|
|
||
| async function readPassword(): Promise<string> { | ||
| try { | ||
|
|
@@ -46,10 +55,13 @@ async function login(): Promise<string | null> { | |
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ provider: "basic", username: USERNAME, password: pw, next: "/" }), | ||
| redirect: REDIRECT_POLICY, | ||
| // The caller's `init.signal` only covers `attempt()` below — login runs | ||
| // before it and would otherwise be unbounded. | ||
| signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), | ||
| }); | ||
| // A redirect here is not a login: the cookies are read off THIS response, so | ||
| // treat anything that is not a 2xx as "no session". | ||
| if (!res.ok) return null; | ||
| const setCookies = typeof res.headers.getSetCookie === "function" ? res.headers.getSetCookie() : []; | ||
| const cookie = setCookies.map((c) => c.split(";", 1)[0]).filter(Boolean).join("; "); | ||
|
|
@@ -69,6 +81,7 @@ export async function dashboardFetch(apiPath: string, init?: RequestInit): Promi | |
| const attempt = () => | ||
| fetch(`${DASH_ORIGIN}${apiPath}`, { | ||
| ...init, | ||
| redirect: REDIRECT_POLICY, | ||
| // Callers that don't bring their own deadline still get one — no request | ||
| // from this module may be able to hang indefinitely. | ||
| signal: init?.signal ?? AbortSignal.timeout(REQUEST_TIMEOUT_MS), | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.