Commit Graph

5 Commits

Author SHA1 Message Date
Joseph Doherty 4dc2ad8084 feat(sql): implement the design §8.1 catalog identifier-validation gate (#496)
Until now ISqlDialect.QuoteIdentifier was the SOLE defence for authored
table/column names: an identifier went from the TagConfig blob straight into a
command text, bracket-quoted. The residual risk was bounded — a hostile name is
quoted into one nonexistent object, the query fails after the connection opens,
and the tag Bad-codes — but "bounded" is not "filtered", and the design promised
a filter. Both ISqlDialect and SqlServerDialect carried a doc paragraph saying so.

SqlCatalogGate + SqlCatalogLoader now resolve every authored identifier against
the live catalog at Initialize, and REPLACE it with the catalog's own spelling.
The identifier text in an emitted poll query is therefore a string this driver
read back out of ListSchemas/ListTables/ListColumns — not operator input. Quoting
becomes the backstop it was documented to be.

Decisions worth knowing before touching this:

- Substitution, not just validation. Matching is exact-ordinal first, then a
  UNIQUE case-insensitive hit: SQL Server's default collation is CI so case
  variants have always worked, and rejecting them would break valid deployments.
  An ambiguous CI match under a case-sensitive collation is refused rather than
  guessed — picking one would publish another column's data under the operator's
  node, which is worse than rejecting the tag.

- Charset check BEFORE catalog lookup. Each identifier goes through
  QuoteIdentifier for its rejection rules first, so a name carrying a control or
  Unicode format character is rejected WITHOUT its value being echoed into a log
  line (Trojan-Source). A name that passes is safe to render, which is why
  catalog-miss messages do name it — an operator hunting a typo has to see what
  they wrote. Both halves are pinned by tests.

- A rejected tag keeps its node. It is dropped from the POLLED table but stays in
  the AUTHORED table, so it still materializes and reads BadNodeIdUnknown —
  §8.1's specified outcome. My first wiring dropped it from both, which deleted
  the node instead; an existing test (ReinitializeAsync_recoversFromFaulted)
  caught it. A status code can only be published by a node that exists, and a
  missing address-space entry is far harder to diagnose than a Bad quality.

- An unreadable catalog FAULTS Initialize; it does not reject every tag. That is
  the absence of evidence about the tags, not evidence against them — rejecting
  all of them would serve a confidently-empty address space and send the operator
  hunting typos that do not exist. Zero visible schemas is treated the same way,
  because that is exactly what a missing GRANT looks like. Faulting lands
  DriverInstanceActor in Reconnecting with its retry timer running.

- Bounded load: one schema list, one default-schema scalar, then one ListTables
  per distinct authored schema and one ListColumns per distinct authored relation
  — never a full catalog enumeration, and nothing after Initialize. Every
  authored name reaches the catalog queries as a bound @schema/@table parameter,
  so building the allow-list cannot itself be an injection vector.

- ISqlDialect gains DefaultSchemaSql ("SELECT SCHEMA_NAME()"). An unqualified
  `TagValues` must resolve in whatever schema the SERVER reports; hardcoding dbo
  would be a silent lie on any estate that maps service accounts to their own
  default schema. It is a query, not a constant, because the answer is
  per-connection.

- Accepted v1 limitation: a 3-part db.schema.table (or linked-server name)
  addresses a catalog this connection cannot enumerate, so it cannot be
  allow-listed and is rejected with a message pointing at the fix — expose the
  data through a view in the connected database.

VerifyLivenessAsync's wall-clock pattern is extracted to RunBoundedAsync and
reused, so the new I/O gets the same R2-01 / STAB-14 protection rather than a
second hand-rolled copy. (It also fixes a latent bug in the extracted code: the
OperationCanceledException arm detached the wrong task.)

Tests: 24 pure gate tests + 9 end-to-end driver tests against the real SQLite
catalog + 3 new live tests against the real SQL Server on 10.100.0.35 (21 in that
suite now pass, exercising the real SELECT SCHEMA_NAME() + INFORMATION_SCHEMA
path). Verified falsifiable: bypassing the gate turns exactly the three rejection
assertions red and leaves the rest green.

SqlInjectionRegressionTests is deliberately NOT rewritten to expect
BadNodeIdUnknown. It drives SqlPollReader directly, below the gate, and pins the
quoting backstop on its own — defence in depth is only worth the name if each
layer holds independently. Rewriting those assertions would delete the backstop's
only coverage and leave the gate a single point of failure. Its scope note, which
said the gate does not exist, is updated to say why it stays where it is.
2026-07-25 16:07:53 -04:00
Joseph Doherty 898c7365c4 test(sql): close the blackhole gate's leaked-pause race + bound the shell command
Review follow-up on the blackhole gate. Two robustness gaps, both bounded to the
disposable dedicated container (never shared infra), fixed:

- If the outer token fired while 'docker pause' was in flight, 'paused' was set
  only AFTER the await, so a cancellation there left paused=false while the
  container could still complete the pause — the finally then skipped unpause and
  leaked a frozen container. Mark paused BEFORE the await, and make the finally's
  unpause best-effort (unpausing a never-actually-paused container errors
  harmlessly, and a cleanup failure must never mask the real test outcome).
- The pause/unpause shell commands had no wall-clock bound of their own — only the
  post-pause read was capped — so a hung SSH handshake would hang CI. Give
  RunShellCommandAsync its own 30s hard cap that kills the process and throws
  TimeoutException, distinct from an operator's own cancellation.

Offline skip still clean (1 skipped, 9ms, no socket/docker).

Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
2026-07-24 15:29:21 -04:00
Joseph Doherty 02f2dafe2a test(sql): correct the cancelled-token test's overclaim
Its doc said it pinned "the real provider's cancellation path", but the
token is cancelled before ReadAsync so the OCE is raised at the reader's
SemaphoreSlim.WaitAsync gate, before any SqlConnection opens — it never
exercises Microsoft.Data.SqlClient's in-flight command cancellation.
Softened (option (a)) to state accurately what it proves: an already-
cancelled token propagates as OCE and is never swallowed into a Bad
snapshot or an unreachable-database verdict. The in-flight/deadline path
is covered by SqlBlackholeTimeoutTests.

Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
2026-07-24 15:20:59 -04:00
Joseph Doherty 10a6a37ca5 test(sql): blackhole/timeout live-gate on a dedicated mssql container
Adds the frozen-peer / BadTimeout gate for the Sql driver — the single
highest-value integration test. Mirrors the S7 R2-01 blackhole gate:
docker pause a DEDICATED mssql mid-poll, assert the next read surfaces
BadTimeout within the client-side operationTimeout (≈3s) and well below
the server-side CommandTimeout backstop (30s), the driver degrades
(Degraded + host Stopped), and docker unpause recovers to Healthy/Running.

- Docker/docker-compose.yml: disposable `otopcua-sql-blackhole` mssql on
  :14333 (never the shared :14330 ConfigDb server) + one-shot seed.
- Docker/seed.sql: the two sample tables.
- SqlBlackholeTimeoutTests.cs: env-gated (SQL_BLACKHOLE_ENDPOINT); pauses
  ONLY the hard-coded container name; skips loudly if the endpoint is the
  shared port 14330; bounds its own wait so a wedged impl fails, not hangs.
  Offline: clean skip (no socket, no docker shell-out).

Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
2026-07-24 15:20:24 -04:00
Joseph Doherty 48cbc26c34 test(sql): env-gated central-SQL integration fixture + read round-trip
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
2026-07-24 15:06:55 -04:00