Skip to content

Commit 8760972

Browse files
fullstackjamfullstackjam
committed
fix: comprehensive security and bug fixes across auth, API, and frontend
CRITICAL: - Fix XSS in highlightBash via escapeHtml before regex highlighting - Reject JWT tokens without exp claim (was bypassable with exp:0) - Add rate limiting, size cap, and error handling to brewfile parse HIGH: - Preserve existing username on GitHub re-login (prevent overwrite) - Clear OAuth state cookies on state mismatch (both providers) - Fix CLI approve race condition with atomic UPDATE claim + db.batch - Server-generate all device codes (remove client-supplied code path) - Fix open redirect in validateReturnTo via decodeURIComponent - Wrap GitHub callback in try/catch matching Google callback pattern - Hide unlisted configs from public profile pages - Check response.ok before reloading after deleteConfig - Fix packageDescs mutation (create new Map instead of mutating) - Add performance indexes on configs and cli_auth_codes MEDIUM: - Add updated_at to snapshot UPDATE query - Cap slug dedup loop at 100 attempts - Fix formatDate unconditional Z append for D1 date strings - Fix stale event?.currentTarget in ConfigDetail copy button - Migrate ThemeToggle from Svelte 4 on:click to Svelte 5 onclick - Remove dead starCount GitHub API fetch from home page - Add name/description length validation to config creation - Centralize RESERVED_ALIASES constant across hooks and API endpoints - Change logout from GET to POST (CSRF prevention) - Replace N+1 queries with JOIN in hooks.server.ts - Fix auth store checkPromise reset on fetch failure - Add opportunistic expired cli_auth_codes cleanup
1 parent 79b0872 commit 8760972

File tree

24 files changed

+338
-354
lines changed

24 files changed

+338
-354
lines changed

‎migrations/0015_add_performance_indexes.sql‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
-- Add indexes for frequently queried columns to avoid full table scans
2+
3+
-- Visibility-based queries (explore page, profile page, alias resolution)
4+
CREATE INDEX IF NOT EXISTS idx_configs_visibility ON configs(visibility);
5+
CREATE INDEX IF NOT EXISTS idx_configs_visibility_updated_at ON configs(visibility, updated_at DESC);
6+
CREATE INDEX IF NOT EXISTS idx_configs_visibility_install_count ON configs(visibility, install_count DESC);
7+
8+
-- Featured sort (explore page default view)
9+
CREATE INDEX IF NOT EXISTS idx_configs_featured ON configs(featured DESC, install_count DESC);
10+
11+
-- CLI auth codes lookup by status (approve endpoint)
12+
CREATE INDEX IF NOT EXISTS idx_cli_auth_codes_status ON cli_auth_codes(status);
13+
CREATE INDEX IF NOT EXISTS idx_cli_auth_codes_expires_at ON cli_auth_codes(expires_at);

‎src/hooks.server.ts‎

Lines changed: 37 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { Handle } from '@sveltejs/kit';
22
import { generateInstallScript, generatePrivateInstallScript } from '$lib/server/install-script';
3+
import { RESERVED_ALIASES } from '$lib/server/validation';
34

45
const INSTALL_SCRIPT_URL = 'https://raw.githubusercontent.com/openbootdotdev/openboot/main/scripts/install.sh';
56

@@ -43,7 +44,7 @@ export const handle: Handle = async ({ event, resolve }) => {
4344
}
4445

4546
const shortAliasMatch = path.match(/^\/([a-z0-9-]+)$/);
46-
if (shortAliasMatch && !['dashboard', 'api', 'install', 'docs', 'cli-auth', 'explore'].includes(shortAliasMatch[1])) {
47+
if (shortAliasMatch && !(RESERVED_ALIASES as readonly string[]).includes(shortAliasMatch[1])) {
4748
const alias = shortAliasMatch[1];
4849
const env = event.platform?.env;
4950

@@ -90,31 +91,28 @@ export const handle: Handle = async ({ event, resolve }) => {
9091
const username = installShMatch[1];
9192
const slug = installShMatch[2];
9293

93-
const user = await env.DB.prepare('SELECT id FROM users WHERE username = ?').bind(username).first<{ id: string }>();
94-
if (user) {
95-
const config = await env.DB.prepare('SELECT custom_script, visibility, dotfiles_repo FROM configs WHERE user_id = ? AND slug = ?')
96-
.bind(user.id, slug)
97-
.first<{ custom_script: string; visibility: string; dotfiles_repo: string }>();
98-
99-
if (config) {
100-
if (config.visibility === 'private') {
101-
const script = generatePrivateInstallScript(env.APP_URL, username, slug);
102-
return withSecurityHeaders(new Response(script, {
103-
headers: { 'Content-Type': 'text/plain; charset=utf-8', 'Cache-Control': 'no-cache' }
104-
}));
105-
}
106-
107-
const script = generateInstallScript(username, slug, config.custom_script, config.dotfiles_repo || '');
108-
109-
env.DB.prepare('UPDATE configs SET install_count = install_count + 1 WHERE user_id = ? AND slug = ?').bind(user.id, slug).run().catch((e: unknown) => console.error('install count update failed:', e));
94+
const config = await env.DB.prepare(
95+
'SELECT c.custom_script, c.visibility, c.dotfiles_repo, c.user_id FROM configs c JOIN users u ON c.user_id = u.id WHERE u.username = ? AND c.slug = ?'
96+
).bind(username, slug).first<{ custom_script: string; visibility: string; dotfiles_repo: string; user_id: string }>();
11097

98+
if (config) {
99+
if (config.visibility === 'private') {
100+
const script = generatePrivateInstallScript(env.APP_URL, username, slug);
111101
return withSecurityHeaders(new Response(script, {
112-
headers: {
113-
'Content-Type': 'text/plain; charset=utf-8',
114-
'Cache-Control': 'no-cache'
115-
}
102+
headers: { 'Content-Type': 'text/plain; charset=utf-8', 'Cache-Control': 'no-cache' }
116103
}));
117104
}
105+
106+
const script = generateInstallScript(username, slug, config.custom_script, config.dotfiles_repo || '');
107+
108+
env.DB.prepare('UPDATE configs SET install_count = install_count + 1 WHERE user_id = ? AND slug = ?').bind(config.user_id, slug).run().catch((e: unknown) => console.error('install count update failed:', e));
109+
110+
return withSecurityHeaders(new Response(script, {
111+
headers: {
112+
'Content-Type': 'text/plain; charset=utf-8',
113+
'Cache-Control': 'no-cache'
114+
}
115+
}));
118116
}
119117
}
120118
}
@@ -129,31 +127,28 @@ export const handle: Handle = async ({ event, resolve }) => {
129127
const username = twoSegMatch[1];
130128
const slug = twoSegMatch[2];
131129

132-
const user = await env.DB.prepare('SELECT id FROM users WHERE username = ?').bind(username).first<{ id: string }>();
133-
if (user) {
134-
const config = await env.DB.prepare('SELECT custom_script, visibility, dotfiles_repo FROM configs WHERE user_id = ? AND slug = ?')
135-
.bind(user.id, slug)
136-
.first<{ custom_script: string; visibility: string; dotfiles_repo: string }>();
130+
const config = await env.DB.prepare(
131+
'SELECT c.custom_script, c.visibility, c.dotfiles_repo, c.user_id FROM configs c JOIN users u ON c.user_id = u.id WHERE u.username = ? AND c.slug = ?'
132+
).bind(username, slug).first<{ custom_script: string; visibility: string; dotfiles_repo: string; user_id: string }>();
137133

138134
if (config) {
139-
if (config.visibility === 'private') {
140-
const script = generatePrivateInstallScript(env.APP_URL, username, slug);
141-
return withSecurityHeaders(new Response(script, {
142-
headers: { 'Content-Type': 'text/plain; charset=utf-8', 'Cache-Control': 'no-cache' }
143-
}));
144-
}
145-
146-
const script = generateInstallScript(username, slug, config.custom_script, config.dotfiles_repo || '');
147-
148-
env.DB.prepare('UPDATE configs SET install_count = install_count + 1 WHERE user_id = ? AND slug = ?').bind(user.id, slug).run().catch((e: unknown) => console.error('install count update failed:', e));
149-
135+
if (config.visibility === 'private') {
136+
const script = generatePrivateInstallScript(env.APP_URL, username, slug);
150137
return withSecurityHeaders(new Response(script, {
151-
headers: {
152-
'Content-Type': 'text/plain; charset=utf-8',
153-
'Cache-Control': 'no-cache'
154-
}
138+
headers: { 'Content-Type': 'text/plain; charset=utf-8', 'Cache-Control': 'no-cache' }
155139
}));
156140
}
141+
142+
const script = generateInstallScript(username, slug, config.custom_script, config.dotfiles_repo || '');
143+
144+
env.DB.prepare('UPDATE configs SET install_count = install_count + 1 WHERE user_id = ? AND slug = ?').bind(config.user_id, slug).run().catch((e: unknown) => console.error('install count update failed:', e));
145+
146+
return withSecurityHeaders(new Response(script, {
147+
headers: {
148+
'Content-Type': 'text/plain; charset=utf-8',
149+
'Cache-Control': 'no-cache'
150+
}
151+
}));
157152
}
158153
}
159154
}

‎src/lib/components/ConfigDetail.svelte‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,11 +123,20 @@
123123
return gradients[index % gradients.length];
124124
}
125125
126+
function escapeHtml(str: string): string {
127+
return str
128+
.replace(/&/g, '&amp;')
129+
.replace(/</g, '&lt;')
130+
.replace(/>/g, '&gt;')
131+
.replace(/"/g, '&quot;')
132+
.replace(/'/g, '&#039;');
133+
}
134+
126135
function highlightBash(code: string): string {
127-
return code
136+
return escapeHtml(code)
128137
.replace(/^(#.*)$/gm, '<span class="comment">$1</span>')
129138
.replace(/\b(if|then|else|elif|fi|for|while|do|done|case|esac|function|return|exit|echo|export|source|cd|mkdir|chmod|chown|sudo|brew|npm|git|curl|wget)\b/g, '<span class="keyword">$1</span>')
130-
.replace(/(["'])(.*?)\1/g, '<span class="string">$1$2$1</span>')
139+
.replace(/(&#039;|&quot;)(.*?)\1/g, '<span class="string">$1$2$1</span>')
131140
.replace(/(\$\w+|\$\{[^}]+\})/g, '<span class="variable">$1</span>');
132141
}
133142
@@ -384,9 +393,9 @@
384393
<div class="code-block">
385394
<div class="code-header">
386395
<span>custom-script.sh</span>
387-
<button onclick={() => {
396+
<button onclick={(e: MouseEvent) => {
388397
navigator.clipboard.writeText(config.custom_script || '');
389-
const btn = event?.currentTarget as HTMLButtonElement;
398+
const btn = e.currentTarget as HTMLButtonElement;
390399
if (btn) {
391400
btn.textContent = 'Copied!';
392401
setTimeout(() => btn.textContent = 'Copy', 2000);

‎src/lib/components/SiteHeader.svelte‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
{:else if $auth.user}
2727
<a href="/dashboard" class="header-dashboard-link">Dashboard</a>
2828
<span class="header-separator">/</span>
29-
<a href="/api/auth/logout" class="header-logout-link">Logout</a>
29+
<form method="POST" action="/api/auth/logout" style="display:inline"><button type="submit" class="header-logout-link">Logout</button></form>
3030
{:else}
3131
<a href="/login" class="header-login-link">Sign in</a>
3232
{/if}
@@ -138,6 +138,14 @@
138138
transition: color 0.2s cubic-bezier(0.4, 0, 0.2, 1);
139139
}
140140
141+
button.header-logout-link {
142+
background: none;
143+
border: none;
144+
cursor: pointer;
145+
padding: 0;
146+
font-family: inherit;
147+
}
148+
141149
.header-dashboard-link:hover,
142150
.header-login-link:hover,
143151
.header-logout-link:hover {

‎src/lib/components/ThemeToggle.svelte‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
import { theme } from '$lib/stores/theme';
33
</script>
44

5-
<button class="theme-toggle" on:click={() => theme.toggle()} title="Toggle theme">
5+
<button class="theme-toggle" onclick={() => theme.toggle()} title="Toggle theme">
66
{#if $theme === 'dark'}
77
<svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2">
88
<path d="M21 12.79A9 9 0 1 1 11.21 3 7 7 0 0 0 21 12.79z" />

‎src/lib/server/auth.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export async function verifyToken(token: string, secret: string): Promise<TokenP
3030
if (!valid) return null;
3131

3232
const payload = JSON.parse(data) as TokenPayload;
33-
if (payload.exp && Date.now() > payload.exp) return null;
33+
if (!payload.exp || Date.now() > payload.exp) return null;
3434

3535
return payload;
3636
} catch {

‎src/lib/server/validation.test.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -209,8 +209,12 @@ describe('validateReturnTo', () => {
209209
expect(validateReturnTo('/page?foo=bar&baz=qux')).toBe(true);
210210
});
211211

212-
it('should accept paths with URL-encoded query params', () => {
213-
expect(validateReturnTo('/page?param=%20space')).toBe(true);
212+
it('should reject paths with URL-encoded unsafe characters', () => {
213+
expect(validateReturnTo('/page?param=%20space')).toBe(false);
214+
});
215+
216+
it('should reject percent-encoded double slash (open redirect)', () => {
217+
expect(validateReturnTo('/%2F%2Fevil.com')).toBe(false);
214218
});
215219

216220
it('should reject paths not starting with /', () => {

‎src/lib/server/validation.ts‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@ interface ValidationResult {
33
error?: string;
44
}
55

6+
/** Reserved aliases that conflict with top-level routes. Used by both hooks.server.ts and config creation. */
7+
export const RESERVED_ALIASES = ['api', 'install', 'dashboard', 'login', 'docs', 'cli-auth', 'explore'] as const;
8+
69
/** Validates custom script: max 10k chars, no null bytes. Base64-encoded at generation time to prevent shell injection. */
710
export function validateCustomScript(script: string | null | undefined): ValidationResult {
811
if (!script) {
@@ -77,16 +80,24 @@ export function validateReturnTo(path: string | null | undefined): boolean {
7780
return false;
7881
}
7982

80-
if (!path.startsWith('/')) {
83+
// Decode first to catch %2F%2F → // bypass attempts
84+
let decoded: string;
85+
try {
86+
decoded = decodeURIComponent(path);
87+
} catch {
88+
return false;
89+
}
90+
91+
if (!decoded.startsWith('/')) {
8192
return false;
8293
}
8394

84-
if (path.startsWith('//')) {
95+
if (decoded.startsWith('//')) {
8596
return false;
8697
}
8798

88-
// Allow path + optional query string with safe characters
89-
return /^\/[a-zA-Z0-9\-_/]*(\?[a-zA-Z0-9\-_=&%]*)?$/.test(path);
99+
// Allow path + optional query string with safe characters (no % to prevent double-encoding attacks)
100+
return /^\/[a-zA-Z0-9\-_/]*(\?[a-zA-Z0-9\-_=&]*)?$/.test(decoded);
90101
}
91102

92103
interface Package {

‎src/lib/stores/auth.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ function createAuthStore() {
3838
}
3939
} catch {
4040
set({ user: null, loading: false });
41-
} finally {
41+
// Reset on failure so next check() retries instead of returning stale error
4242
checkPromise = null;
4343
}
4444
})();

‎src/lib/test/db-mock.ts‎

Lines changed: 63 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,12 @@ export function createMockDB(data: MockData = {}): D1Database & { data: Record<s
3333
dump() {
3434
throw new Error('dump() not implemented in mock');
3535
},
36-
batch<T = unknown>(statements: D1PreparedStatement[]): Promise<D1Result<T>[]> {
37-
throw new Error('batch() not implemented in mock');
36+
async batch<T = unknown>(statements: D1PreparedStatement[]): Promise<D1Result<T>[]> {
37+
const results: D1Result<T>[] = [];
38+
for (const stmt of statements) {
39+
results.push(await stmt.run() as D1Result<T>);
40+
}
41+
return results;
3842
},
3943
exec(query: string): Promise<D1Result> {
4044
throw new Error('exec() not implemented in mock');
@@ -74,17 +78,18 @@ function createMockStatement(sql: string, tables: Record<string, any[]>): D1Prep
7478
},
7579

7680
async run<T = unknown>(): Promise<D1Result<T>> {
77-
// For INSERT/UPDATE/DELETE
81+
// For INSERT/UPDATE/DELETE - modified array length indicates change count
7882
const modified = executeQuery<T>(sql, bindings, tables);
83+
const changes = Array.isArray(modified) ? modified.length : 0;
7984
return {
8085
results: [],
8186
success: true,
8287
meta: {
8388
duration: 0.1,
84-
changes: modified ? 1 : 0,
85-
last_row_id: modified ? 1 : 0,
89+
changes,
90+
last_row_id: changes ? 1 : 0,
8691
rows_read: 0,
87-
rows_written: modified ? 1 : 0
92+
rows_written: changes ? 1 : 0
8893
}
8994
};
9095
},
@@ -252,31 +257,72 @@ function executeQuery<T>(sql: string, bindings: any[], tables: Record<string, an
252257

253258
const tableName = tableMatch[1];
254259
const setMatch = sql.match(/set\s+(.*?)\s+where/i);
255-
const whereMatch = sql.match(/where\s+(\w+)\s*=\s*\?/i);
256-
257-
if (setMatch && whereMatch) {
258-
const whereField = whereMatch[1];
259-
const whereValue = bindings[bindings.length - 1];
260+
const whereClause = sql.match(/where\s+(.*?)$/i);
260261

262+
if (setMatch && whereClause) {
263+
// Count SET bindings to determine where WHERE bindings start
261264
const setFields = setMatch[1].split(',').map((s) => s.trim());
262-
let bindingIndex = 0;
265+
let setBindingCount = 0;
266+
setFields.forEach((field) => {
267+
if (field.match(/=\s*\?/)) setBindingCount++;
268+
});
269+
270+
// Parse WHERE conditions
271+
const conditions = whereClause[1].split(/\s+and\s+/i);
272+
let whereBindingIndex = setBindingCount;
273+
274+
const matchesRow = (row: any): boolean => {
275+
for (const condition of conditions) {
276+
const eqMatch = condition.match(/(\w+)\s*=\s*\?/);
277+
if (eqMatch) {
278+
if (row[eqMatch[1]] !== bindings[whereBindingIndex++]) return false;
279+
continue;
280+
}
263281

282+
const literalEqMatch = condition.match(/(\w+)\s*=\s*'([^']*)'/);
283+
if (literalEqMatch) {
284+
if (row[literalEqMatch[1]] !== literalEqMatch[2]) return false;
285+
continue;
286+
}
287+
288+
const datetimeGtMatch = condition.match(/(\w+)\s*>\s*datetime\(['"]now['"]\)/i);
289+
if (datetimeGtMatch) {
290+
const fieldValue = row[datetimeGtMatch[1]];
291+
if (!fieldValue || fieldValue <= new Date().toISOString()) return false;
292+
continue;
293+
}
294+
}
295+
return true;
296+
};
297+
298+
let changeCount = 0;
264299
tables[tableName].forEach((row) => {
265-
if (row[whereField] === whereValue) {
300+
// Reset whereBindingIndex for each row
301+
whereBindingIndex = setBindingCount;
302+
if (matchesRow(row)) {
303+
let bindingIndex = 0;
266304
setFields.forEach((field) => {
267-
const bindingMatch = field.match(/(\w+)\s*=\s*\?/);
268-
if (bindingMatch && bindingIndex < bindings.length - 1) {
269-
row[bindingMatch[1]] = bindings[bindingIndex++];
305+
const bindingMatchField = field.match(/(\w+)\s*=\s*\?/);
306+
if (bindingMatchField && bindingIndex < setBindingCount) {
307+
row[bindingMatchField[1]] = bindings[bindingIndex++];
270308
return;
271309
}
272310

273311
const literalMatch = field.match(/(\w+)\s*=\s*'([^']*)'/);
274312
if (literalMatch) {
275313
row[literalMatch[1]] = literalMatch[2];
276314
}
315+
316+
const datetimeMatch = field.match(/(\w+)\s*=\s*datetime\(['"]now['"]\)/i);
317+
if (datetimeMatch) {
318+
row[datetimeMatch[1]] = new Date().toISOString().replace('T', ' ').replace(/\.\d+Z$/, '');
319+
}
277320
});
321+
changeCount++;
278322
}
279323
});
324+
// Return array with one element per changed row to signal change count
325+
return new Array(changeCount).fill({}) as T[];
280326
}
281327
return [] as T[];
282328
}
@@ -295,6 +341,7 @@ function executeQuery<T>(sql: string, bindings: any[], tables: Record<string, an
295341
const index = tables[tableName].findIndex((row) => row[fieldName] === value);
296342
if (index !== -1) {
297343
tables[tableName].splice(index, 1);
344+
return [{}] as T[];
298345
}
299346
}
300347
return [] as T[];

0 commit comments

Comments
 (0)