MSSQL: write-access check and startup warning (never blocks) #342

New issue

Closed

opened 2026-11-10 23:43:27 +00:00 by david · 1 comment

david commented

2026-11-10 23:43:27 +00:00

Owner

Summary

Add a permissions module that runs one fixed, read-only query at startup to detect whether the SQL Server login has write rights, and warn (never block) if it does.

Background

Decision recorded in docs/adr/0002-mssql-read-only-credentials-with-minimal-guard.md: read-only credentials are the write control, and the guard is only an accident check. The extension must tell the user when that control is missing, but must never refuse to load or run queries because of it. Design: extensions/mssql/docs/DESIGN.md ("Write-access warning"); plan task 2.7a and 2.7b in extensions/mssql/docs/IMPLEMENTATION_PLAN.md. [SECURITY RISK] The check does not detect table-level grants or ownership chaining, so a clean result does not prove the login is read-only. Use Australian English. TDD with a fake pool.

Depends on: #324 (pool config) and #323 (audit logger, for the permission_warning entry)
Blocks: #329 (index.ts wiring)

Documentation Required

docs/reference/mssql/ — https://raw.githubusercontent.com/tediousjs/node-mssql/master/README.md : node-mssql README (pool and request usage). Vendored on main; fetch the URL if absent.
Microsoft Learn T-SQL reference for IS_SRVROLEMEMBER, IS_ROLEMEMBER and HAS_PERMS_BY_NAME (verify exact semantics and return values before writing the query).

Implementation Details

Create extensions/mssql/permissions.ts exporting checkWriteAccess(pool): Promise<{ status: "ok" | "write-access" | "unverified", rights: string[] }>.

One fixed query, no user input, no string concatenation. Check IS_SRVROLEMEMBER('sysadmin'); IS_ROLEMEMBER for db_owner, db_datawriter and db_ddladmin; and HAS_PERMS_BY_NAME(DB_NAME(), 'DATABASE', <perm>) for INSERT, UPDATE, DELETE, ALTER and EXECUTE.
Return the names of the rights found. If the query throws, return unverified (never throw to the caller).
Export a formatWarning(result) helper. The text names the rights only. It never includes the password, the URL, or raw driver error text containing credentials.
Do not block: no code path may stop the extension loading or registering tools.
Do not log the warning to the audit log here. Return the result; #329 writes the permission_warning entry through the audit logger.

Acceptance Criteria

No write rights found returns ok and no warning text.
Each right (each role and each permission) found returns write-access and the warning names it.
A query error returns unverified with a warning that write access could not be verified.
checkWriteAccess never throws.
The query is fixed and parameter-free.
The README note (in #331) says a clean result does not prove the login is read-only.

Test Plan

node --test extensions/mssql/permissions.test.ts with a fake pool: no rights, each right individually, several rights together, a thrown error, and a rejected promise. Assert the warning text never contains a planted password or URL.

## Summary Add a `permissions` module that runs one fixed, read-only query at startup to detect whether the SQL Server login has write rights, and warn (never block) if it does. ## Background Decision recorded in `docs/adr/0002-mssql-read-only-credentials-with-minimal-guard.md`: read-only credentials are the write control, and the guard is only an accident check. The extension must tell the user when that control is missing, but must **never refuse to load or run queries** because of it. Design: `extensions/mssql/docs/DESIGN.md` ("Write-access warning"); plan task 2.7a and 2.7b in `extensions/mssql/docs/IMPLEMENTATION_PLAN.md`. [SECURITY RISK] The check does not detect table-level grants or ownership chaining, so a clean result does not prove the login is read-only. Use Australian English. TDD with a fake pool. **Depends on:** #324 (pool config) and #323 (audit logger, for the `permission_warning` entry) **Blocks:** #329 (index.ts wiring) ## Documentation Required - `docs/reference/mssql/` — https://raw.githubusercontent.com/tediousjs/node-mssql/master/README.md : node-mssql README (pool and request usage). Vendored on `main`; fetch the URL if absent. - Microsoft Learn T-SQL reference for `IS_SRVROLEMEMBER`, `IS_ROLEMEMBER` and `HAS_PERMS_BY_NAME` (verify exact semantics and return values before writing the query). ## Implementation Details Create `extensions/mssql/permissions.ts` exporting `checkWriteAccess(pool): Promise<{ status: "ok" | "write-access" | "unverified", rights: string[] }>`. - One fixed query, no user input, no string concatenation. Check `IS_SRVROLEMEMBER('sysadmin')`; `IS_ROLEMEMBER` for `db_owner`, `db_datawriter` and `db_ddladmin`; and `HAS_PERMS_BY_NAME(DB_NAME(), 'DATABASE', <perm>)` for `INSERT`, `UPDATE`, `DELETE`, `ALTER` and `EXECUTE`. - Return the names of the rights found. If the query throws, return `unverified` (never throw to the caller). - Export a `formatWarning(result)` helper. The text names the rights only. It never includes the password, the URL, or raw driver error text containing credentials. - Do not block: no code path may stop the extension loading or registering tools. - Do not log the warning to the audit log here. Return the result; #329 writes the `permission_warning` entry through the audit logger. ## Acceptance Criteria - [ ] No write rights found returns `ok` and no warning text. - [ ] Each right (each role and each permission) found returns `write-access` and the warning names it. - [ ] A query error returns `unverified` with a warning that write access could not be verified. - [ ] `checkWriteAccess` never throws. - [ ] The query is fixed and parameter-free. - [ ] The README note (in #331) says a clean result does not prove the login is read-only. ## Test Plan `node --test extensions/mssql/permissions.test.ts` with a fake pool: no rights, each right individually, several rights together, a thrown error, and a rejected promise. Assert the warning text never contains a planted password or URL.

david added the

feature

label

2026-11-10 23:43:27 +00:00

david referenced this issue

2026-11-10 23:44:05 +00:00

MSSQL: index.ts wiring (load env, refuse on invalid limits, pool, shutdown) #329

david referenced this issue

2026-11-10 23:44:05 +00:00

MSSQL: audit log writer (.pi/mssql-audit.log, mode 0600) #323

david referenced this issue

2026-11-10 23:44:17 +00:00