Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
|
@copilot resolve the merge conflicts in this pull request |
Resolved all 6 merge conflicts in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
gkorland
left a comment
There was a problem hiding this comment.
Code review: SQL Server support
Recommendation: needs rework. The security posture is largely fine — all introspection queries are properly parameterized and no credential leakage was found — but the loader is functionally broken end-to-end, and the branch has drifted semantically (not just textually) from staging.
One scoping note: the Dockerfile hunk that shows up in the diff (bookworm→trixie, libtinfo6, bash) is merge-base drift — those lines are already in staging. There are no ODBC-driver/apt-key changes to review here.
🔴 Critical
C1. Dict cursor indexed by integer → schema load fails against every real database
api/loaders/sqlserver_loader.py:69 (with :169 conn.cursor(as_dict=True))
sample_results = cursor.fetchall()
return [row[0] for row in sample_results if row[0] is not None]The cursor is created with as_dict=True, so rows are dicts keyed only by column name — pymssql's row2dict() explicitly filters to string keys:
def row2dict(row):
"""Filter dict so it only has string keys; used when as_dict == True"""
return dict([(k, v) for k, v in row.items() if hasattr(k, 'startswith')])So row[0] raises KeyError: 0 on the first non-empty column. The broad except Exception in load() (:186) converts that into the generic "Failed to load SQL Server database schema", hiding the root cause. Compare MySQLLoader._execute_sample_query, which correctly uses row[col_name] with DictCursor (the Postgres loader uses row[0] only because its cursor is tuple-based).
return [row[col_name] for row in sample_results if row[col_name] is not None]The tests don't catch this because they mock out every method that touches the database.
🟠 High
H1. No schema qualification anywhere — cross-schema collisions and broken sample queries
sqlserver_loader.py:202-255 (sys.tables, no schema filter), :304, :391, :62
Everything is keyed by the bare t.name, so:
- two same-named tables in different schemas silently overwrite each other in
entities; extract_columns_infofilters only onWHERE t.name = %s, returning the union of columns of every same-named table across all schemas, merged into one dict — wrong types, nullability and keys;_execute_sample_queryrunsFROM [{table_name}]unqualified, which fails with "Invalid object name" for anything outside the default schema;- generated SQL will reference unqualified names and fail at execution time.
PostgresLoader deliberately parses search_path and filters every catalog query by table_schema = %s. Suggest joining sys.schemas, keying entities as schema.table, passing schema_name as a second bound parameter, and qualifying the sample query.
H2. sqlserver missing from the sqlglot dialect map → ordinary SELECTs are classified destructive
api/core/pipeline.py:203-208 (_DIALECT_BY_DB_TYPE), interacting with the new '[' quote char in sql_sanitizer.py:184
detect_destructive_operation is fail-closed, and _sqlglot_dialect('sqlserver') returns None. Verified against the working tree:
detect_destructive_operation('SELECT TOP 10 * FROM users', 'sqlserver') -> ('UNKNOWN', True)
detect_destructive_operation('SELECT * FROM [my-table]', 'sqlserver') -> ('UNKNOWN', True)
detect_destructive_operation('SELECT a, b FROM t FOR JSON PATH', 'sqlserver')-> ('UNKNOWN', True)
sqlglot.parse('SELECT TOP 10 * FROM users', read='tsql') -> ['Select'] # fine
This PR's own quoting change compounds it: auto_quote_sql_identifiers runs before detect_destructive_operation, so every auto-quoted SQL Server query becomes unparseable. Read-only queries then trigger the destructive-confirmation flow and are refused outright on demo/general graphs. Fix: add "sqlserver": "tsql" (and "mssql") to _DIALECT_BY_DB_TYPE, with a regression test asserting ('SELECT', False).
H3. Loader API drift — this will not work after rebasing onto staging
Concrete breakages against current staging, beyond textual conflicts:
schema_loader.pycallsloader.load(user_id, url, db=db);SQLServerLoader.load(prefix, connection_url)has nodbparam →TypeError.text2sql.py:246callsrefresh_graph_schema(..., db=db)→ sameTypeError.:503usesfrom api.extensions import db; the current pattern isfrom api.core.db_resolver import resolve_db.load_to_graph(...)is called withoutdb=db.- Dispatch moved to
pipeline.get_database_type_and_loader, and the scheme allowlist is now_KNOWN_DB_SCHEMES. Without adding"sqlserver://"to both, the new frontend option fails withInvalidArgumentError("Invalid database URL format")or silently falls back toPostgresLoader.
🟡 Medium
M1. Unescaped ] in bracket-quoted identifiers — sqlserver_loader.py:58-66, sql_sanitizer.py:79-81. SQL Server escapes ] by doubling it. my]table → [my]table] terminates the quote early. Bounded (names come from the target DB's own catalog, executed with the user's own credentials) but it violates the "never interpolate identifiers" rule; Postgres uses psycopg2.sql.Identifier. Add _quote_ident(n) = "[" + n.replace("]", "]]") + "]".
M2. pymssql added as a core dependency with an eager import. The core block is documented as "required for SDK (minimal)"; pymssql is a compiled FreeTDS binding most SDK users will never need, and a top-level import means a missing/unbuildable wheel breaks import api.core. Follow the Snowflake pattern: server extra + lazy import inside dispatch + sdk_only guard.
M3. Connections leaked on every error path — :166-190 closes cursor/connection only on success. Inherited from MySQLLoader, but C1 makes it fire on every load. Also :596-611: if 'conn' in locals(): conn.rollback() raises if the failure occurred between connect() and cursor(). Use try/finally.
M4. Tests assert almost nothing. test_successful_load patches out both extract_tables_info and extract_relationships, so it only exercises URL parsing; mock_load_to_graph.assert_called_once() asserts on a mock of a mock. test_connection_error uses a plain Exception, so the pymssql.Error branch is never hit. Untested: _execute_sample_query (would have caught C1), extract_columns_info, extract_foreign_keys, execute_sql_query, refresh_graph_schema.
🔵 Low / nits
_is_already_quoted(sql_sanitizer.py:28-35) applies the('[', ']')pair unconditionally, so[weird]in a Postgres/MySQL schema now reportsneeds_quoting() == Falseand is emitted unquoted — a small regression for existing dialects. Scope the quote-pair check to the active dialect._parse_sqlserver_urldiscards the query string, so there's no way to requestEncrypt/TLS and noencryption=is passed topymssql.connect. Matches MySQL's behavior so not a regression, but SQL Server deployments usually expect an explicit setting.- Bare
ValueErrorfor malformed URLs instead ofInvalidArgumentError;SQLServerConnectionError(:20) is defined but never raised. - Three lines exceed the 120-char limit (
:257,:362,:407) — will fail the pylint gate.db_nameis accepted-but-unused in four methods behind# pylint: disable=unused-argument; either use it for schema scoping (see H1) or drop it. DatabaseModal.tsxlooks correct (noany, port 1433 wired), though the DB-specific nested ternaries now appear in three places — aRecord<string, {protocol, port}>map would stop the next vendor from missing one.
Security summary
No exploitable vulnerability found:
- ✅ All four introspection queries use
%splaceholders with bound parameters; no user input is interpolated. - ✅ No credentials logged; user-facing errors are generic and the parsed password never reaches a log or exception message.
- ✅ No new dangerous-statement allowance —
;-stacked statements,EXEC xp_cmdshell,sp_executesql,BULK INSERTandOPENROWSETall still classify as destructive (verified by execution). The dialect gap in H2 makes it more conservative, not less.
The only injection-shaped issue is M1, bounded to the user's own database, and should still be fixed.
Review performed against staging at the time of writing. Given H3, this needs to be rebased and re-validated against the current loader interface rather than merged textually.
There was a problem hiding this comment.
Pull request overview
Adds SQL Server as an additional database backend in QueryWeaver, wiring it through the backend loader selection logic and exposing it in the frontend DB connection UI, with unit tests and dependency updates to support the new driver.
Changes:
- Added
SQLServerLoader(pymssql-based) and registeredsqlserver://routing in schema loading / Text2SQL selection. - Updated identifier quoting utilities to support SQL Server bracket quoting.
- Extended the frontend Database connection modal to allow selecting SQL Server and generating
sqlserver://...URLs, plus added unit tests and dependency locks.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
api/loaders/sqlserver_loader.py |
New SQL Server loader implementation (schema extraction, query execution, schema refresh). |
api/core/schema_loader.py |
Routes sqlserver:// URLs to SQLServerLoader during schema load. |
api/core/text2sql.py |
Routes sqlserver:// URLs to SQLServerLoader for query execution path. |
api/sql_utils/sql_sanitizer.py |
Adds support for SQL Server bracket quoting and refactors quoted-identifier detection. |
app/src/components/modals/DatabaseModal.tsx |
Adds SQL Server option, placeholder URL, and default port in the UI. |
tests/test_sqlserver_loader.py |
Adds unit tests covering SQL Server URL parsing, serialization, schema modification detection, and load flow. |
pyproject.toml |
Adds pymssql dependency; bumps fastmcp minimum. |
uv.lock |
Locks pymssql, bumps fastmcp and adds transitive dependency (griffelib). |
Dockerfile |
Updates Python base image and installs extra system deps needed for the container build. |
.github/wordlist.txt |
Adds SQL Server-related terms for spellchecking. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| cursor.execute(""" | ||
| SELECT | ||
| t.name AS table_name, | ||
| ISNULL(ep.value, '') AS table_comment | ||
| FROM sys.tables t |
| sample_results = cursor.fetchall() | ||
| return [row[0] for row in sample_results if row[0] is not None] |
Adds a `sqlserver://` loader so QueryWeaver can introspect Microsoft SQL Server and Azure SQL instances and answer natural-language questions against them. Rebuilt on top of current staging and reworked to address the review findings on #538. - api/loaders/sqlserver_loader.py: new pymssql-based loader. Connections use `as_dict=True`, so rows are read by column name; positional access raises KeyError with that setting. - Schema scoping: `parse_schema_from_url` reads `?schema=` (default `dbo`). All catalog queries join `sys.schemas` and bind the schema as a parameter, and sample queries are schema-qualified, so same-named tables in other schemas can no longer collide. - Identifier quoting: `quote_ident` doubles a literal `]` so it cannot terminate a bracket delimiter early. - Connections are released in `finally` via `_close_quietly` / `_rollback_quietly` instead of `if 'conn' in locals()`. - api/core/pipeline.py: dispatch `sqlserver://` with an `sdk_only` guard and a lazy import, and map `sqlserver`/`mssql` to the `tsql` sqlglot dialect. Without the mapping the fail-closed destructive-operation guard classified ordinary reads such as `SELECT TOP 10 ...` as destructive. - api/core/schema_loader.py: accept the `sqlserver://` scheme. - api/sql_utils/sql_sanitizer.py: "already quoted" is now dialect-scoped, so `[weird]` is still quoted on PostgreSQL/MySQL where brackets are data; `get_quote_char` returns `[` for sqlserver/mssql. - pyproject.toml: pymssql lives in the `server` extra, not core deps, so the published SDK wheel is unaffected. - DatabaseModal.tsx: replace the nested protocol/port/placeholder ternaries with a `DB_PROFILES` map and expose the SQL Server option and its schema field. - tests: new `tests/test_sqlserver_loader.py` uses fakes that mimic pymssql dict rows, so the cursor contract is actually exercised; added T-SQL dialect and bracket-quoting regression tests. - docs/sqlserver_loader.md and README updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
282f5d1 to
184e0a0
Compare
|
Pushed a rebuild of this branch on top of current Here is how each review finding was addressed. CriticalC1 — dict-cursor bug in HighH1 — no schema qualification. All four catalog queries now join H2 — H3 — loader API drift. MediumM1 — unescaped M2 — M3 — connection leaks. Replaced the M4 — weak tests. The test file was rewritten around Nits
Additional fix found while rebuilding
Validation
|
CodeQL flagged the sample-value query as `py/sql-injection` (high): the schema name reaches it from the user-supplied connection URL, and T-SQL cannot bind identifiers as parameters. Adds `validate_ident`, an anchored allow-list matching the existing `SnowflakeLoader._validate_identifier` pattern. It accepts only characters that can legitimately appear in a SQL Server object name and rejects everything capable of escaping a bracket delimiter (`]`, quotes, semicolons, backslashes, control characters), plus empty and over-long names. `quote_ident` keeps doubling `]` as defence in depth. Validation runs before the statement is built, so a hostile identifier never reaches `cursor.execute`. `parse_schema_from_url` now validates the schema at parse time, and `sample_size` is checked to be a positive int. Also imports `api.core` ahead of the loader in the new test module. The package's `__init__` eagerly pulls in the pipeline, which imports the loaders, so importing a loader first left `graph_loader` half-built and the file could not be run on its own. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
api/loaders/sqlserver_loader.py:475
extract_foreign_keysis not fully schema-scoped: it filters the parent table schema (ps.name) but not the referenced table schema (rs.name). This can emit foreign keys pointing to tables outside the loaded schema, contradicting the loader’s “single-schema” behavior and potentially confusing downstream schema reasoning.
'type': data_type,
'null': is_nullable,
'key': key_type,
api/loaders/sqlserver_loader.py:96
SELECT DISTINCT TOP ... ORDER BY NEWID()is invalid in SQL Server becauseORDER BYexpressions must appear in theSELECTlist when usingDISTINCT. This sample-value query will fail at runtime; use a DISTINCT subquery (as the Postgres loader does) and apply the randomORDER BYin the outer query.
# More specific patterns for schema-affecting operations
SCHEMA_PATTERNS = [ # pylint: disable=duplicate-code
r'^\s*CREATE\s+TABLE',
r'^\s*CREATE\s+INDEX',
r'^\s*CREATE\s+UNIQUE\s+INDEX',
docs/sqlserver_loader.md:68
- This doc claims the loader extracts “Tables and views”, but the implementation currently queries only
sys.tables(nosys.views). Also, the sample-values SQL shown doesn’t match the implemented random-sampling query. Update the docs to reflect the actual behavior/query (or extend the loader to include views).
- Tables and views in the selected schema
- Columns with data types, nullability, defaults and primary-key flags
- Extended properties (`MS_Description`) used as table and column descriptions
- Foreign keys, including composite keys
- Many-to-many relationships inferred from junction tables
All catalog queries join `sys.schemas` and bind the schema name as a parameter, so
a connection only ever sees the requested schema. Tables in other schemas are not
extracted and cannot collide with same-named tables in the selected schema.
### Sample Values
Sample values are collected per column with a schema-qualified, bracket-quoted
query:
```sql
SELECT DISTINCT TOP 3 [column_name]
FROM [dbo].[table_name]
WHERE [column_name] IS NOT NULL;
</details>
| # ``api.core`` must be initialised before any loader module is imported. | ||
| # ``api.core.__init__`` eagerly pulls in the pipeline, which imports the | ||
| # loaders, so importing a loader first leaves ``graph_loader`` half-built. | ||
| import api.core # noqa: F401 pylint: disable=unused-import |
CodeQL still reported `py/sql-injection` after the allow-list validator: the schema name reaching the sample query originated in the user-supplied connection URL, and an anchored regex is not recognised as a barrier. The tables query now selects `s.name AS schema_name` back from `sys.schemas`, and that server-returned value is what gets interpolated into the sample query. The URL string is still used, but only as a bound query parameter, so it never reaches a statement body. This is also more correct: sampling now uses the server's canonical casing for the schema rather than whatever the URL happened to contain. Extracts `_build_column_description` and a `_KEY_TYPES` lookup out of `extract_columns_info` to keep it within the local-variable limit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
api/loaders/sqlserver_loader.py:60
validate_identallows dots for all identifier types, but_execute_sample_queryuses.as the schema/table separator. If a catalog table name contains a dot (legal in SQL Server when quoted),extract_columns_infowill passf"{schema}.{table_name}", producing multiple dots and the sample query will target the wrong object (or fail) in a non-obvious way. Consider rejecting dots in single-part schema/table identifiers so onlyschema.tablequalification is supported.
if not identifier or len(identifier) > 128:
raise ValueError(
f"Invalid {identifier_type}: {identifier!r}. "
"Must be between 1 and 128 characters."
)
if not re.fullmatch(r'[A-Za-z0-9_$#@ .\-]+', identifier):
raise ValueError(
f"Invalid {identifier_type}: {identifier!r}. Only letters, digits, "
"underscore, dollar, hash, at-sign, space, dot and dash are allowed."
)
docs/sqlserver_loader.md:53
- The SQL Server loader doc claims support for views and junction-table many-to-many inference, but the current implementation in
api/loaders/sqlserver_loader.pyonly queriessys.tablesand returns direct FK relationships. Please align the feature list with what’s actually implemented (or add view/M2M support).
- Tables and views in the selected schema
- Columns with data types, nullability, defaults and primary-key flags
- Extended properties (`MS_Description`) used as table and column descriptions
- Foreign keys, including composite keys
- Many-to-many relationships inferred from junction tables
docs/sqlserver_loader.md:116
- This catalog-query list references
sys.views, but the loader currently only queriessys.tables. Update this bullet so the doc matches the implemented queries.
The loader reads from SQL Server system catalog views:
- `sys.tables` / `sys.views` joined with `sys.schemas` — table list
- `sys.columns` joined with `sys.types` — column metadata
- `sys.indexes` / `sys.index_columns` — primary keys
- `sys.foreign_keys` / `sys.foreign_key_columns` — foreign keys
- `sys.extended_properties` (with `class = 1`) — table and column descriptions
`catalog_schema` was optional and fell back to the URL-derived schema, which kept the tainted value flowing into the interpolated sample query and left CodeQL's `py/sql-injection` alert open. It is now a required argument, so the only schema string that can reach a statement body is the one `sys.schemas` returned. The URL schema is used exclusively as a bound query parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
api/loaders/sqlserver_loader.py:59
validate_ident()claims underscores are allowed, but the allow-list regex does not include_, so legitimate identifiers likecol_1will be rejected (and the included tests forcol_1would fail). Add_to the character class to match the documented behavior.
if not identifier or len(identifier) > 128:
raise ValueError(
f"Invalid {identifier_type}: {identifier!r}. "
"Must be between 1 and 128 characters."
)
if not re.fullmatch(r'[A-Za-z0-9_$#@ .\-]+', identifier):
raise ValueError(
f"Invalid {identifier_type}: {identifier!r}. Only letters, digits, "
"underscore, dollar, hash, at-sign, space, dot and dash are allowed."
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/sqlserver_loader.md:49
- The documentation claims the loader extracts "Tables and views", but the implementation currently queries only
sys.tables(nosys.views). Either update the docs to say tables only, or extend the loader to include views as well.
- Tables and views in the selected schema
api/loaders/sqlserver_loader.py:536
extract_foreign_keyscurrently scopes only the parent table (ps.name = %s) but does not scope the referenced side, whileextract_relationshipsexplicitly requires both parent and referenced tables to be in the loaded schema. This can record foreign keys to tables outside the selected schema (and loses the referenced schema name in the returned dict), producing ambiguous or dangling FK metadata. Consider restrictingrs.nameto the same schema so FK metadata matches the schema-scoped load behavior.
WHERE ps.name = %s AND pt.name = %s
ORDER BY fk.name;
""", (schema, table_name))
|
CI is now fully green (10/10, including Playwright and CodeQL). Three Why it fired. T-SQL cannot bind identifiers as parameters, so the schema What was done, in order:
Also in these commits:
New tests cover the validator (legitimate names, breakout attempts, empty, Local: 305 passed, 1 skipped; pylint 10.00/10; Ready for review — needs an approval to satisfy branch protection on |
All findings from this review (C1, H1-H3, M1-M4 and the nits) have been addressed in the rebuilt branch, plus a CodeQL py/sql-injection alert found during the rebuild. CI is fully green. Dismissing as stale.
Summary
Adds Microsoft SQL Server (and Azure SQL) as a supported source database, so a
sqlserver://connection can be introspected into the schema graph and queriedwith natural language.
This branch was rebuilt on top of current
staging. The previous revisionwas ~4 months stale and conflicted with the dispatch-layer restructuring that
landed since, so rather than resolving conflicts hunk-by-hunk the feature was
reimplemented cleanly and every finding from the review was addressed. The PR is
now a single commit against current
stagingand is conflict-free.Changes
Loader —
api/loaders/sqlserver_loader.py(new)as_dict=True, so rows are read bycolumn name.
pymssql'srow2dictkeeps only string keys, so positionalaccess (
row[0]) raisesKeyErrorunder that setting.parse_schema_from_urlreads?schema=anddefaults to
dbo. All four catalog queries joinsys.schemasand bind theschema name as a parameter; sample queries are schema-qualified.
quote_identdoubles a literal]so it cannot terminate a bracketdelimiter early.
finallyvia_close_quietly/_rollback_quietly.urlparse, so percent-encoded credentials work; optional?encrypt=maps to pymssql'sencryptionparameter.ep.class = 1.Dispatch —
api/core/pipeline.py,api/core/schema_loader.pysqlserver://is routed toSQLServerLoaderwith ansdk_onlyguard and alazy import, mirroring Snowflake.
sqlserver/mssqlmap to thetsqlsqlglot dialect in_DIALECT_BY_DB_TYPE.sqlserver://added to_KNOWN_DB_SCHEMES.Sanitizer —
api/sql_utils/sql_sanitizer.py[weird]is still quoted onPostgreSQL/MySQL where brackets are ordinary data.
quote_identifierescapes]by doubling for the[dialect.get_quote_charreturns[forsqlserver/mssql.Dependencies
pymssql~=2.3.13added to theserverextra, not core dependencies, sothe published SDK wheel is unaffected.
uv.lockregenerated.Frontend —
app/src/components/modals/DatabaseModal.tsxDB_PROFILESmap.Server and emits
?schema=.Docs
docs/sqlserver_loader.mdadded, matching the Snowflake/Postgres loader docs.Testing
tests/test_sqlserver_loader.pyrewritten aroundFakeCursor/FakeConnectionfakes that mimic pymssql's dict-only rows, so the cursor contract is genuinely
exercised rather than mocked away. Covers bracket escaping and injection
attempts, URL parsing, schema qualification, column/FK/relationship mapping,
value serialization, DDL detection, and the query-execution paths (select,
non-select, error, connection failure).
tests/test_destructive_detection.py:SELECT TOP 10 ...,[my-table],FOR JSON PATHandISNULL(...)areclassified
SELECT/ non-destructive, whileDROP,TRUNCATE,SELECT ... INTO,EXEC xp_cmdshelland stacked statements remaindestructive.
tests/test_sql_sanitizer.py, including thedialect-scoping regression in both directions.
Local results:
uv run --extra server --extra dev pytest tests/ -k "not e2e and not test_sdk"→ 281 passed, 1 skippeduv run pylinton all changed Python files → 10.00/10npx tsc --noEmitandnpm run lintinapp/→ cleanmake build-prod→ succeedspyspellingwith the repo's spellcheck config → passesMemory / Performance Impact
N/A — no changes to graph memory, allocators or the query engine.
pymssqlisimported lazily at dispatch time, so installs without the
serverextra areunaffected.
Related Issues
Supersedes the original revision of this PR; addresses all review findings on
#538.