You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 67a95f4
Browse filesBrowse the repository at this point in the historyBrowse files
feat: pin a default connection; fix Editor misdiagnosing malformed SQL as a permissions error (#107)
Two independent changes, one commit each.
---
## 1. `feat:` pin a connection as your per-user default
Requested feature. A pin toggle in **Manage Connections** and in the
sidebar connection switcher; a pinned connection is the one DeepSQL
opens on every load.
**The pin is per user, not a flag on the connection.** A connection can
be shared through `connection_access_grant`, so a column on
`database_connection` would let one person's choice decide what everyone
else opens on — and shared connections are `canManageConfig=false` for
their recipients, so exactly the people who most want a default could
not set one. New `connection_pin` table, one row per user, unique
constraint on `username`; pinning a second connection moves the row
rather than adding one.
**`PUT|DELETE /connections/{id}/pin` are gated on
`assertCanUseConnection`**, not `assertCanManageConnectionConfig` —
choosing where you land is a preference, not a change to the connection.
`GET /connections` carries `pinned` per caller, so no surface needs a
second request; `deleteConnection` clears every pin on the connection
alongside its grants.
**The pin has to beat an already-selected connection, not just an empty
one.** `useDashboardStore` persists `connectionId`, so after a reload
something is always selected and the existing auto-select never ran.
`useConnectionManager` now applies the pin once per page load, tracked
at module scope rather than in a ref — the hook is called from a dozen
sections, and a per-instance guard would let a later-mounted section
yank the user back to the pin after they deliberately switched.
Switching mid-session still sticks.
The sidebar toggle exists because Manage Connections needs
`MANAGE_CONNECTIONS` to open at all; without it a Developer or Data
Engineer, who typically holds exactly one granted connection, would have
no way to set a default.
### One thing reviewers should look at
`ConnectionScopedAuthorizationSafetyTest` now flags `GET /connections`,
because the handler resolves the caller's *pinned* connection id and the
scanner matches `(?i)connection_?id` anywhere in a handler body. That
endpoint takes **no arguments at all** — it returns whatever
`getConnectionsForUser(username, isAdmin)` gives. It is exempted in
`AUTHORIZED_ELSEWHERE`, and a new
`connectionListingTakesNoCallerSuppliedId` test re-derives the claim so
the exemption cannot rot into cover for a real gap. The scanner was not
weakened and no meaningless assert was added.
### Verified against a live backend
On a throwaway Postgres/backend stack, not inferred:
- `ddl-auto` creates `connection_pin` with its unique index on boot
- pinning a second connection flips the first to `false`
- unpinning a *non*-pinned connection leaves the real pin alone
- deleting a connection clears its pin row
- a DEVELOPER with **no grant** gets **403, not 500**
- a DEVELOPER holding only a grant (`canManageConfig: false`) pins
successfully, and their pin does **not** appear on the admin's list
---
## 2. `fix:` Editor reported malformed SQL as a permissions denial
Reported from the field. A user pasted a SELECT that still carried the
double quotes it had in source code (`"select h.id, ...`) and was told
**"Only admins can execute DDL or DML from the SQL Editor"** — which
reads as a permissions problem and sent people looking for a role fix.
The statement is neither DDL nor DML. With an unclosed `"` the whole
thing is one quoted identifier, so it is not valid SQL at all.
**Two keyword heuristics disagreed, and the disagreement was resolved as
"mutation":**
```
detectQueryType = SELECT isReadOnlyQuery = false parse = FAILED
```
`QueryNormalizer.detectQueryType` sanitizes a prefix away and answers
`SELECT`; the provider's `isReadOnlyQuery` strips only *comments*, still
sees the leading quote, and answers false. `mutating = !readOnly && type
!= UNKNOWN` then labelled a SELECT a mutation.
`classifyStatement` now records that the parser rejected the statement
and, when the detected verb is read-only and no hidden write was found,
returns `notParseable`; `enforce` throws `STATEMENT_NOT_PARSEABLE` ahead
of both the read-only and confirmation branches, with a message naming
the likely cause.
### This does not weaken the guard
**The statement is still blocked, admins included** — only the diagnosis
changed. An admin is deliberately *not* offered a confirmation prompt
for a statement nothing managed to classify, since confirming past the
guard is the one way this could become a bypass.
The reclassification is gated on `isReadOnlyVerb(queryType)` **and**
`hiddenWrite == null`. Both halves are tested:
- `anUnparseableWriteIsStillTreatedAsAMutation` — `DELETE FROM hotel
WHERE (((` still returns `EDITOR_MUTATION_FORBIDDEN`
- `aMalformedDataModifyingCteIsStillBlockedAsAWrite` — a broken `WITH x
AS (DELETE ...)` is still a blocked write, not a reported typo
The MCP guard already reported this case honestly ("Only read-only SQL
is allowed …") and was left alone.
---
## Testing
- `QueryExecutionPolicyServiceTest`: **49 pass** (43 before, 6 added),
including every pre-existing guard case — data-modifying CTEs, `SELECT
INTO`, `DROP` blocking, EXPLAIN-wrapped writes.
- **Full backend suite diffed against pristine HEAD**: 1531 vs 1512
tests, **failure sets byte-identical**. The 99 failures are pre-existing
environmental context-load failures (no DB/Redis in the test container),
unchanged by this branch.
- Commit 1 built and tested **in isolation** in a detached worktree, so
it is bisect-safe rather than only green as part of the combined tree.
- Frontend: `npm run build` clean; 0 lint errors in changed files.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Copy file name to clipboardExpand all lines: CLAUDE.md
+64Lines changed: 64 additions & 0 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -499,6 +499,51 @@ so "View as" resolves membership as the target user).
499
499
"authentication is not authorization" trap `BrainController` documents — there is still
500
500
no filter doing it for you.
501
501
502
+
### Default connection (pinning)
503
+
504
+
A user can pin one connection as their default, from the pin column in **Manage
505
+
Connections** or the pin toggle in the sidebar connection switcher.
506
+
507
+
-**The pin is per user, not per connection.**`connection_pin` keys on username with a
508
+
unique constraint (`V120__create_connection_pin.sql`; applied by `ddl-auto` from the
509
+
`ConnectionPin` entity — verified on a scratch database, the table and its unique index
510
+
are created on boot). A column on `database_connection` would have been wrong twice
511
+
over: a connection shared through `connection_access_grant` would let one user's choice
512
+
decide what everyone else opens on, and a shared connection is `canManageConfig=false`
513
+
for its recipients, so exactly the people who most want a default could not set one.
514
+
-**One pin per user is the point.**`ConnectionPinService.pin` moves the existing row
515
+
rather than inserting a second; the unique constraint is the backstop, and a losing
516
+
concurrent insert re-reads and updates instead of surfacing a 500.
517
+
-**`PUT|DELETE /connections/{id}/pin` are gated on `assertCanUseConnection`**, not
518
+
`assertCanManageConnectionConfig` — choosing where you land is a preference, not a
519
+
change to the connection. Verified live: a DEVELOPER holding only a grant on a
520
+
connection (`canManageConfig: false`) pins it and gets 200, while the same user pinning
521
+
a connection they hold no grant on gets **403, not 500** — the `ResponseStatusException`
522
+
rethrow before the catch-all is doing its job.
523
+
-**Unpin is scoped to the connection named.** A stale click in a background tab must not
524
+
clear a pin the user has since moved elsewhere.
525
+
-**`GET /connections` carries `pinned` per caller**, so no surface needs a second
526
+
request, and two users listing the same shared connection see different values —
527
+
confirmed live. `deleteConnection` clears every pin on the connection alongside its
528
+
grants.
529
+
-**`ConnectionScopedAuthorizationSafetyTest` flags `GET /connections` now**, because the
530
+
handler resolves the caller's *pinned* connection id and the scanner matches
531
+
`(?i)connection_?id` anywhere in a handler body. That endpoint takes no arguments at all
532
+
— it returns whatever `getConnectionsForUser(username, isAdmin)` gives — so it is in
533
+
`AUTHORIZED_ELSEWHERE`, and `connectionListingTakesNoCallerSuppliedId` re-derives that
534
+
claim so the exemption cannot rot into cover for a real gap. Do not resolve this by
535
+
adding a meaningless assert, and do not weaken the scanner.
536
+
-**The pin must beat an already-selected connection, not just an empty one.**
537
+
`useDashboardStore` persists `connectionId`, so after a reload something is always
538
+
selected — the original auto-select ran only when nothing was. `useConnectionManager`
539
+
therefore applies the pin once per page load (`pinAppliedThisLoad`, module scope, reset
540
+
by `resetConnectionPinApplied()` in the auth reset). Module scope and not a ref: the
541
+
hook is called from a dozen sections, and a per-instance guard would let a
542
+
later-mounted section yank the user back to the pin after they deliberately switched.
543
+
Switching mid-session still sticks; the pin re-applies on the next load.
544
+
- Pinned connections sort first in `useConnectionManager`, so every consumer — the sidebar
545
+
switcher included — shows the default at the top.
546
+
502
547
### Admin profile switch
503
548
Admins can **View as** a sub-user from the top-right of the home layout (`ProfileSwitch`) to verify connection ACLs, chat/editor policies, and role-gated nav.
504
549
@@ -680,6 +725,25 @@ it against a real database — not a theoretical hardening pass.
680
725
it killed **every** active query on the connection, including other users' work.
681
726
The cancel endpoint is scoped to the connection *and* the user who started the
682
727
run, so an execution id is not a kill primitive for someone else's query.
728
+
-**A statement DeepSQL cannot parse is a syntax error, not DDL/DML — say so.** A user
729
+
pasted a SELECT still carrying the double quotes it had in source code
730
+
(`"select h.id, ...`) and got **"Only admins can execute DDL or DML from the SQL
731
+
Editor"**, which reads as a permissions problem and sends people hunting for a role fix.
732
+
Two keyword heuristics disagreed and the code resolved the disagreement as "mutation":
733
+
`QueryNormalizer.detectQueryType` sanitizes a prefix away and answered `SELECT`, while
734
+
the provider's `isReadOnlyQuery` strips only *comments*, still saw the leading `"`, and
735
+
answered false — so `mutating = !readOnly && type != UNKNOWN` labelled a SELECT a
736
+
mutation. `classifyStatement` now records that the parser rejected the statement and,
737
+
when the detected verb is read-only and no hidden write was found, returns
738
+
`notParseable`; `enforce` throws `STATEMENT_NOT_PARSEABLE` ahead of both the read-only
739
+
and confirmation branches. **The statement is still blocked, for admins too** — only the
740
+
diagnosis changed, and an admin is deliberately *not* offered a confirmation prompt for
741
+
something nothing managed to classify. The reclassification is gated on
742
+
`isReadOnlyVerb(queryType)` and `hiddenWrite == null`, which is what keeps it from
743
+
becoming a bypass: an unparseable `DELETE`, and a malformed data-modifying CTE, both keep
744
+
their mutation handling (covered by `anUnparseableWriteIsStillTreatedAsAMutation` and
745
+
`aMalformedDataModifyingCteIsStillBlockedAsAWrite`). The MCP guard already reported this
746
+
case honestly ("Only read-only SQL is allowed …") and was left alone.
683
747
-**Keep the client timeout under the proxy's.**`docker/nginx/default.conf` gives
684
748
up at `proxy_read_timeout 300s`; the Editor used to ask for 600s, so a 6-minute
685
749
query returned an opaque 504 while still running. `QUERY_TIMEOUT_SECONDS = 240`
0 commit comments