Skip to content

Commit ab458e5

Browse files
tianzhouclaude
andcommitted
fix: surface SQL parse errors instead of misleading permission denied
When SQL has syntax errors (e.g., LIMIT before WHERE), the parser fails and the permission system was silently defaulting to 'admin', causing a confusing "requires admin permission" error. Now parse failures throw directly with the actual syntax error message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 6013be8 commit ab458e5

3 files changed

Lines changed: 12 additions & 8 deletions

File tree

server/lib/sql-permissions.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -234,8 +234,8 @@ export async function detectRequiredPermissions(sql: string): Promise<SqlAnalysi
234234
}
235235

236236
return { permissions, statementCount: parsed.statements.length, transactionSafe }
237-
} catch {
238-
// Parse failed - require admin for safety
239-
return { permissions: new Set(['admin']), statementCount: 1, transactionSafe: false }
237+
} catch (err) {
238+
const message = err instanceof Error ? err.message : String(err)
239+
throw new Error(`SQL syntax error: ${message}`)
240240
}
241241
}

server/services/query-service.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,12 @@ export const queryServiceHandlers: ServiceImpl<typeof QueryService> = {
151151
}
152152

153153
// Determine required permissions and statement analysis
154-
const analysis = await detectRequiredPermissions(req.sql);
154+
let analysis: Awaited<ReturnType<typeof detectRequiredPermissions>>;
155+
try {
156+
analysis = await detectRequiredPermissions(req.sql);
157+
} catch (err) {
158+
throw new ConnectError(err instanceof Error ? err.message : String(err), Code.InvalidArgument);
159+
}
155160
requirePermissions(user, req.connectionId, analysis.permissions, `execute query`);
156161

157162
const details = getConnectionDetails(req.connectionId);

tests/sql-permissions.test.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -172,10 +172,9 @@ describe('detectRequiredPermissions', () => {
172172
expect(transactionSafe).toBe(false)
173173
})
174174

175-
it('returns admin for unparseable SQL', async () => {
176-
const { permissions, transactionSafe } = await detectRequiredPermissions('THIS IS NOT VALID SQL !!!')
177-
expect(permissions).toEqual(new Set(['admin']))
178-
expect(transactionSafe).toBe(false)
175+
it('throws for unparseable SQL', async () => {
176+
await expect(detectRequiredPermissions('THIS IS NOT VALID SQL !!!'))
177+
.rejects.toThrow('SQL syntax error')
179178
})
180179

181180
it('returns read for empty SQL', async () => {

0 commit comments

Comments
 (0)