fix: security hardening, config defaults, and test coverage across connectors and Rust core - #2551
Draft
tusharsharma20021114-rgb wants to merge 3 commits into
Conversation
…nnectors and Rust core
github-actions
Bot
added
dependencies
Contributor
WalkthroughThe PR strengthens Rust linting and error propagation, centralizes connector row-limit handling, adds SQL safety checks and configuration normalization, introduces optional MCP bearer authentication, updates the WASM MDL precondition, and documents the pull-request labeler trigger. ChangesRust safety and lint enforcement
Connector limit handling
SQL policy and configuration
MCP bearer authentication
Pull request labeler workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WrenEngine
participant basic_safety_check
participant Connector
participant Database
Client->>WrenEngine: submit SQL and limit
WrenEngine->>basic_safety_check: validate SQL policy
basic_safety_check-->>WrenEngine: allow or raise WrenError
WrenEngine->>Connector: execute SQL with normalized limit
Connector->>Database: send SQL and bound parameters
Database-->>Connector: return rows
Connector-->>Client: return query result
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/wren/src/wren/connector/trino.py (1)
484-511: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
qmarkfor LIMIT parameters here and in Athena
_apply_limit_param(..., param_style="format")emits%s, but Trino only accepts?, and PyAthena needsparamstyle="qmark"for positional parameters. Anyquery()/dry_run()with a limit will fail otherwise.
core/wren/src/wren/connector/trino.py: switch both calls toparam_style="qmark".core/wren/src/wren/connector/athena.py: switch toparam_style="qmark"and passparamstyle="qmark"tocursor.execute().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/src/wren/connector/trino.py` around lines 484 - 511, Update core/wren/src/wren/connector/trino.py lines 484-511 in both query and dry_run to call _apply_limit_param with param_style="qmark" instead of "format". Also update core/wren/src/wren/connector/athena.py lines 305-318 to use param_style="qmark" and pass paramstyle="qmark" to cursor.execute().
🧹 Nitpick comments (5)
core/wren/src/wren/serve_cli.py (1)
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the documentation to reflect bearer-token auth support.
As per the downstream behavior evidence, the documentation files
docs/core/guides/mcp.mdanddocs/core/reference/cli.mdexplicitly state that HTTP transport "ships no bearer-token auth in this version — keep it local". Consider updating these documentation files to reflect the newly introduced--api-keysupport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/src/wren/serve_cli.py` around lines 89 - 104, Update the HTTP transport authentication statements in the MCP guide and CLI reference to document the newly supported --api-key bearer-token authentication, removing the outdated claim that bearer-token auth is unavailable while preserving the existing local/no-auth guidance where applicable.core/wren/src/wren/engine.py (1)
115-115: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove redundant
basic_safety_check(sql)calls. Bothqueryanddry_runredundantly callbasic_safety_check(sql)immediately before callingself.dry_plan(sql, properties). Sincedry_planalready executes this safety check unconditionally, the SQL gets parsed twice per request. Remove these calls to avoid unnecessary overhead.
core/wren/src/wren/engine.py#L115-L115: removebasic_safety_check(sql)fromquery.core/wren/src/wren/engine.py#L132-L132: removebasic_safety_check(sql)fromdry_run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/src/wren/engine.py` at line 115, Remove the redundant basic_safety_check(sql) calls from both query and dry_run in core/wren/src/wren/engine.py (lines 115 and 132). Leave each method delegating safety validation to self.dry_plan(sql, properties), which already performs the check.core/wren/src/wren/policy.py (1)
204-205: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a defensive bounds check for empty statements.
If
sqlglot.parse()evaluates to an empty list,stmts[0]will raise anIndexError. Adding a quick guard prevents a potential unexpected crash.🛡️ Proposed fix to add the bounds check
- stmt = stmts[0] - if stmt is None: + if not stmts or stmts[0] is None: raise WrenError( ErrorCode.INVALID_SQL, "Could not parse SQL statement", phase=ErrorPhase.SQL_POLICY_CHECK, ) + stmt = stmts[0]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/src/wren/policy.py` around lines 204 - 205, Add an empty-list guard before indexing stmts in the policy parsing flow: return the existing no-statement result when stmts is empty, then keep the current stmt = stmts[0] and None handling unchanged for non-empty results.core/wren-core/core/src/mdl/utils.rs (1)
179-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
to_string()mapping.The
BTreeSetis populated withStrings due to the mapping logic on line 182. Calling.map(|m| m.to_string())again afterinto_iter()is unnecessary and triggers redundant memory allocations.♻️ Proposed refactor
let models = required_fields .iter() .filter_map(|c| c.relation.as_ref().map(|r| r.table().to_string())) .collect::<BTreeSet<_>>() // Collect into a BTreeSet to remove duplicates .into_iter() // Convert BTreeSet back into an iterator - .map(|m| m.to_string()) .collect::<Vec<String>>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren-core/core/src/mdl/utils.rs` around lines 179 - 187, Remove the redundant `.map(|m| m.to_string())` from the model collection pipeline after `BTreeSet::into_iter()`. Since `relation.table().to_string()` already produces `String` values, collect the iterator directly into `Vec<String>` while preserving deduplication and ordering.core/wren-core/core/src/mdl/mod.rs (1)
477-478: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid creating a new Tokio runtime per query transformation.
Instantiating a new Tokio runtime via
Runtime::new()is an expensive operation involving thread spawning and setup. Iftransform_sqlis executed per-query, this will add measurable latency and reduce overall throughput. Consider passing a shared handle to an existing runtime or using a lazily initialized static global runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren-core/core/src/mdl/mod.rs` around lines 477 - 478, Update the query-transformation flow around transform_sql to reuse an existing Tokio runtime or shared runtime handle instead of constructing a new Runtime per invocation. Pass the shared runtime dependency through the relevant callers, or lazily initialize one shared runtime if that matches the existing architecture, while preserving the current async transformation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren-core/core/src/logical_plan/analyze/model_generation.rs`:
- Around line 72-75: Replace internal_err! and plan_err! with their
*_datafusion_err! variants in every listed map_err and ok_or_else closure across
model_generation.rs, plan.rs, and relation_chain.rs, preserving each existing
error message and surrounding logic so the closures return underlying DataFusion
errors rather than Result values.
In `@core/wren-core/core/src/mdl/dialect/inner_dialect.rs`:
- Around line 388-389: Cache the compiled identifier regex instead of rebuilding
it on each call to identifier_quote_style. In
core/wren-core/core/src/mdl/dialect/inner_dialect.rs:388-389 and
core/wren-core/core/src/mdl/dialect/wren_dialect.rs:46-47, define a static
OnceLock (or equivalent lazy static) and retrieve the initialized Regex through
it, preserving the existing pattern and matching behavior at both sites.
In `@core/wren-core/core/src/mdl/function/remote_function.rs`:
- Around line 42-44: Update the coercion extraction in get_signature to avoid
propagating errors with ?. Since the preceding check guarantees no Err values
remain, collect only successful entries with filter_map into the existing vector
type while preserving the current signature-building behavior.
In `@core/wren-core/core/src/mdl/mod.rs`:
- Around line 248-250: Replace plan_err! with plan_datafusion_err! in the
TableReference conversion at core/wren-core/core/src/mdl/mod.rs lines 248-250
and in the corresponding error closures at
core/wren-core/core/src/mdl/lineage.rs lines 147-152 and 181-186, plus
core/wren-core/core/src/mdl/utils.rs lines 188-193, so each ok_or_else closure
returns a DataFusionError value rather than a nested Result.
In `@core/wren/src/wren/connector/base.py`:
- Around line 43-96: Resolve the contradictory None-limit behavior by choosing
and applying one consistent contract across _normalize_limit,
_apply_limit_param, _apply_limit_inline, and connector query methods such as
MSSqlConnector and MySqlConnector. If queries must be bounded, remove None
bypasses and safely apply the configured maximum, handling existing MySQL LIMIT
clauses; otherwise make _normalize_limit return None for limit=None, update its
documentation, and have callers uniformly preserve unbounded SQL.
In `@core/wren/src/wren/connector/postgres.py`:
- Around line 272-276: Update PostgresConnector.dry_run to call
_apply_limit_param with param_style="format" so the generated placeholder
matches psycopg’s %s parameter syntax; leave the existing cursor execution flow
unchanged.
In `@core/wren/src/wren/mcp_server.py`:
- Around line 700-703: The authentication settings in the server setup use
hardcoded URLs instead of the configured bind address. Update the
ServeContext/build_server flow to carry the CLI host and port into the
AuthSettings construction, then build both issuer_url and resource_server_url
from the actual host and port while preserving existing defaults.
- Around line 683-690: Update verify_token to compare token and self._api_key
with secrets.compare_digest instead of ==, preserving the existing AccessToken
return for matches and None for non-matches; ensure the secrets module is
imported.
In `@core/wren/src/wren/serve_cli.py`:
- Around line 154-160: Update serve_mcp to reject api_key when transport is not
"http", immediately after the existing transport validation. Emit a clear error
to stderr and exit with a failure status, while preserving the existing HTTP
behavior and stdio flow when no API key is provided.
---
Outside diff comments:
In `@core/wren/src/wren/connector/trino.py`:
- Around line 484-511: Update core/wren/src/wren/connector/trino.py lines
484-511 in both query and dry_run to call _apply_limit_param with
param_style="qmark" instead of "format". Also update
core/wren/src/wren/connector/athena.py lines 305-318 to use param_style="qmark"
and pass paramstyle="qmark" to cursor.execute().
---
Nitpick comments:
In `@core/wren-core/core/src/mdl/mod.rs`:
- Around line 477-478: Update the query-transformation flow around transform_sql
to reuse an existing Tokio runtime or shared runtime handle instead of
constructing a new Runtime per invocation. Pass the shared runtime dependency
through the relevant callers, or lazily initialize one shared runtime if that
matches the existing architecture, while preserving the current async
transformation behavior.
In `@core/wren-core/core/src/mdl/utils.rs`:
- Around line 179-187: Remove the redundant `.map(|m| m.to_string())` from the
model collection pipeline after `BTreeSet::into_iter()`. Since
`relation.table().to_string()` already produces `String` values, collect the
iterator directly into `Vec<String>` while preserving deduplication and
ordering.
In `@core/wren/src/wren/engine.py`:
- Line 115: Remove the redundant basic_safety_check(sql) calls from both query
and dry_run in core/wren/src/wren/engine.py (lines 115 and 132). Leave each
method delegating safety validation to self.dry_plan(sql, properties), which
already performs the check.
In `@core/wren/src/wren/policy.py`:
- Around line 204-205: Add an empty-list guard before indexing stmts in the
policy parsing flow: return the existing no-statement result when stmts is
empty, then keep the current stmt = stmts[0] and None handling unchanged for
non-empty results.
In `@core/wren/src/wren/serve_cli.py`:
- Around line 89-104: Update the HTTP transport authentication statements in the
MCP guide and CLI reference to document the newly supported --api-key
bearer-token authentication, removing the outdated claim that bearer-token auth
is unavailable while preserving the existing local/no-auth guidance where
applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d548bc69-6799-4517-8cf6-529a160124bb
📒 Files selected for processing (44)
.github/workflows/labeler.yamlcore/wren-core-base/Cargo.tomlcore/wren-core-wasm/src/lib.rscore/wren-core/Cargo.tomlcore/wren-core/benchmarks/Cargo.tomlcore/wren-core/core/Cargo.tomlcore/wren-core/core/src/logical_plan/analyze/model_generation.rscore/wren-core/core/src/logical_plan/analyze/plan.rscore/wren-core/core/src/logical_plan/analyze/relation_chain.rscore/wren-core/core/src/logical_plan/unparser.rscore/wren-core/core/src/mdl/context.rscore/wren-core/core/src/mdl/dataset.rscore/wren-core/core/src/mdl/dialect/inner_dialect.rscore/wren-core/core/src/mdl/dialect/wren_dialect.rscore/wren-core/core/src/mdl/function/remote_function.rscore/wren-core/core/src/mdl/lineage.rscore/wren-core/core/src/mdl/mod.rscore/wren-core/core/src/mdl/type_planner.rscore/wren-core/core/src/mdl/utils.rscore/wren-core/sqllogictest/Cargo.tomlcore/wren-core/wren-example/Cargo.tomlcore/wren/src/wren/config.pycore/wren/src/wren/connector/athena.pycore/wren/src/wren/connector/base.pycore/wren/src/wren/connector/canner.pycore/wren/src/wren/connector/clickhouse.pycore/wren/src/wren/connector/databricks.pycore/wren/src/wren/connector/datafusion.pycore/wren/src/wren/connector/duckdb.pycore/wren/src/wren/connector/mssql.pycore/wren/src/wren/connector/mysql.pycore/wren/src/wren/connector/oracle.pycore/wren/src/wren/connector/postgres.pycore/wren/src/wren/connector/redshift.pycore/wren/src/wren/connector/snowflake.pycore/wren/src/wren/connector/trino.pycore/wren/src/wren/engine.pycore/wren/src/wren/mcp_server.pycore/wren/src/wren/policy.pycore/wren/src/wren/serve_cli.pycore/wren/tests/conftest.pycore/wren/tests/unit/test_config.pycore/wren/tests/unit/test_connector_base.pycore/wren/tests/unit/test_mysql_helpers.py
…ck caching, Cargo.toml lints, Python parameterized limits, mcp auth hardening
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren/src/wren/policy.py`:
- Around line 204-210: Update the safety gate around the parsed stmt assignment
in the policy-check flow to explicitly allow only read-only statement kinds and
reject every other parsed statement before _plan runs. Preserve the existing
INVALID_SQL handling for missing or unparsable statements, and use the parser’s
statement-kind symbols rather than relying only on DDL/DML/COPY exclusions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 29a489bb-76b0-4008-bd1d-76bd7938971d
📒 Files selected for processing (29)
core/wren-core-base/Cargo.tomlcore/wren-core/benchmarks/Cargo.tomlcore/wren-core/core/Cargo.tomlcore/wren-core/core/src/logical_plan/analyze/model_generation.rscore/wren-core/core/src/logical_plan/analyze/plan.rscore/wren-core/core/src/logical_plan/analyze/relation_chain.rscore/wren-core/core/src/mdl/dialect/inner_dialect.rscore/wren-core/core/src/mdl/dialect/wren_dialect.rscore/wren-core/core/src/mdl/function/remote_function.rscore/wren-core/core/src/mdl/lineage.rscore/wren-core/core/src/mdl/mod.rscore/wren-core/core/src/mdl/utils.rscore/wren-core/sqllogictest/Cargo.tomlcore/wren-core/wren-example/Cargo.tomlcore/wren/src/wren/connector/athena.pycore/wren/src/wren/connector/base.pycore/wren/src/wren/connector/postgres.pycore/wren/src/wren/connector/trino.pycore/wren/src/wren/engine.pycore/wren/src/wren/mcp_server.pycore/wren/src/wren/policy.pycore/wren/src/wren/serve_cli.pycore/wren/tests/unit/test_athena_connector.pycore/wren/tests/unit/test_athena_limit_pushdown.pycore/wren/tests/unit/test_canner_semicolon.pycore/wren/tests/unit/test_connector_base.pycore/wren/tests/unit/test_duckdb_file_listing.pycore/wren/tests/unit/test_postgres_semicolon_unlimited.pycore/wren/tests/unit/test_redshift_semicolon.py
💤 Files with no reviewable changes (1)
- core/wren/src/wren/engine.py
🚧 Files skipped from review as they are similar to previous changes (16)
- core/wren-core/benchmarks/Cargo.toml
- core/wren-core/core/Cargo.toml
- core/wren-core/wren-example/Cargo.toml
- core/wren-core-base/Cargo.toml
- core/wren/src/wren/connector/athena.py
- core/wren-core/sqllogictest/Cargo.toml
- core/wren-core/core/src/logical_plan/analyze/model_generation.rs
- core/wren-core/core/src/logical_plan/analyze/relation_chain.rs
- core/wren/src/wren/serve_cli.py
- core/wren-core/core/src/mdl/function/remote_function.rs
- core/wren-core/core/src/mdl/utils.rs
- core/wren-core/core/src/mdl/lineage.rs
- core/wren/src/wren/mcp_server.py
- core/wren/src/wren/connector/base.py
- core/wren/src/wren/connector/postgres.py
- core/wren-core/core/src/logical_plan/analyze/plan.rs
Author
|
what are the unresolved issues in this PR can you list them in one place what are the things that need to be fixed and how important they are. |
…_safety_check, update docs for --api-key auth
github-actions
Bot
added
the
documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren/src/wren/policy.py`:
- Around line 212-215: Update the safety gate in the policy validation logic to
accept read-only query expressions by checking against exp.Query and
exp.Subquery instead of only exp.Select and exp.Explain. Preserve the existing
POLICY_VIOLATION error for non-query statements while allowing UNION, INTERSECT,
EXCEPT, and top-level parenthesized SELECT queries.
In `@docs/core/reference/cli.md`:
- Line 430: Add a --api-key row to the wren serve mcp options table, describing
that it accepts a secret and requires bearer-token authentication for HTTP
requests. Keep the existing prose and table formatting consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd009aa3-61d0-4a67-9df6-f0ddec008dd0
📒 Files selected for processing (3)
core/wren/src/wren/policy.pydocs/core/guides/mcp.mddocs/core/reference/cli.md
Comment on lines
+212
to
+215
| if not isinstance(stmt, (exp.Select, exp.Explain)): | ||
| raise WrenError( | ||
| ErrorCode.POLICY_VIOLATION, | ||
| f"Only read-only queries are allowed, got: {type(stmt).__name__}", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Allow UNION, INTERSECT, and EXCEPT queries in the safety gate.
The isinstance(stmt, (exp.Select, exp.Explain)) check inadvertently blocks UNION, INTERSECT, and EXCEPT queries. In sqlglot, set operations parse as subclasses of exp.SetOperation (e.g., exp.Union), which inherits from exp.Query but not exp.Select. This will cause BI tools and dashboards that issue UNION queries to fail.
To safely allow all read-only queries, use exp.Query (which is the base class for both Select and SetOperation). Including exp.Subquery is also recommended to support top-level parenthesized queries (e.g. (SELECT 1)).
🛠️ Proposed fix to allow all read-only queries
- if not isinstance(stmt, (exp.Select, exp.Explain)):
+ if not isinstance(stmt, (exp.Query, exp.Explain, exp.Subquery)):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not isinstance(stmt, (exp.Select, exp.Explain)): | |
| raise WrenError( | |
| ErrorCode.POLICY_VIOLATION, | |
| f"Only read-only queries are allowed, got: {type(stmt).__name__}", | |
| if not isinstance(stmt, (exp.Query, exp.Explain, exp.Subquery)): | |
| raise WrenError( | |
| ErrorCode.POLICY_VIOLATION, | |
| f"Only read-only queries are allowed, got: {type(stmt).__name__}", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/wren/src/wren/policy.py` around lines 212 - 215, Update the safety gate
in the policy validation logic to accept read-only query expressions by checking
against exp.Query and exp.Subquery instead of only exp.Select and exp.Explain.
Preserve the existing POLICY_VIOLATION error for non-query statements while
allowing UNION, INTERSECT, EXCEPT, and top-level parenthesized SELECT queries.
| For `--transport http`, connect the client to the Streamable HTTP endpoint at | ||
| `http://<host>:<port>` instead of spawning a process. Binds to `127.0.0.1` by | ||
| default; there is no bearer-token auth in this version — treat it as local-only. | ||
| default. Pass `--api-key <secret>` to enable bearer-token authentication. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add --api-key to the options table.
The prose documents the flag, but the wren serve mcp flag table still omits it. Add a row describing that it requires a bearer token for HTTP requests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/core/reference/cli.md` at line 430, Add a --api-key row to the wren
serve mcp options table, describing that it accepts a secret and requires
bearer-token authentication for HTTP requests. Keep the existing prose and table
formatting consistent.
This was referenced Jul 20, 2026
Collaborator
|
@tusharsharma20021114-rgb, thanks for working on this. The PR makes sense to me. There are some conflicts and CI failure. Could you fix them? |
goldmedal
marked this pull request as draft
July 31, 2026 03:04
Collaborator
|
To clean my review queue, I change the PR status to draft. After addressing the CI failure, you can request me again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive security and stability hardening across the Python connector layer, Rust core (wren-core), config system, and MCP server. All changes add no new dependencies and maintain backward compatibility (deprecation warnings used where defaults changed).
1. SQL injection prevention — all 15 connectors
Every connector previously interpolated the LIMIT value directly into SQL via f-strings. This is a SQL injection vector when
limitoriginates from user input.Changes:
ConnectorABC._normalize_limit()— centralized limit sanitization with clamp toMAX_ROW_LIMIT,None→MAX_ROW_LIMIT, negative →MAX_ROW_LIMITConnectorABC._apply_limit_param()— wraps SQL with parameterized LIMIT (supports%s,?,:numstyles)ConnectorABC._apply_limit_inline()— wraps SQL with inline LIMIT (for connectors that don't support parameters in subqueries)_escape_sql()2. Config system hardening
strict_modedefault flipped toTrueload_config()default changed fromFalsetoTrue— unregistered table references are rejected by defaultstrict_modekey is absent from config JSONWrenConfig()) was alreadystrict_mode=True— onlyload_config()was wrong__post_init__normalizationdenied_functionsandallowed_source_functionslowercased in__post_init__.lower()calls fromload_config()Denied-function error messages
3. MCP server auth — bearer token support
--api-keyCLI flag for bearer token auth on Streamable HTTP transport_ApiKeyVerifierclass inmcp_server.py--bearer-tokenusage for Claude Code and Codex4. Engine-level SQL safety check
basic_safety_check(sql)added todry_plan(),query(), anddry_run()5. Rust clippy hardening — 33 fixes across 12 files
Configuration
unwrapped_used = "deny"andexpect_used = "deny"in workspace and standalone crate Cargo.tomlsFixes by file
mdl/mod.rs—// SAFETY:commentmdl/function/remote_function.rs— 4x unwrap →?or matchmdl/type_planner.rs— 3x unwrap →?orunwrap_or_defaultmdl/context.rs— 5x unwrap →?orunwrap_or_defaultmdl/dataset.rs— 2x unwrap →?mdl/lineage.rs— 2x unwrap →?mdl/utils.rs— 7x unwrap →?mdl/dialect/inner_dialect.rs— 1x unwrap →?mdl/dialect/wren_dialect.rs— 1x unwrap →unwrap_or_defaultlogical_plan/unparser.rs— 2x unwrap →?logical_plan/analyze/plan.rs— 2x expect →?logical_plan/analyze/relation_chain.rs— 1x unwrap →?logical_plan/analyze/model_generation.rs— 2x unwrap →?6. CI safety
pull_request_targetusage7. WASM ACL
core/wren-core-wasm/src/lib.rs8. Test coverage
New:
test_connector_base.py(13 tests)Covers
_normalize_limit—None,0, negative, overflow, float, bool, custom max.Updated:
test_mysql_helpers.pyReplaced dead tests for removed functions with
_apply_limit_mysqlparameterized tests.Updated:
test_config.py(3 new tests + assertion fixes)__post_init__normalization testsstrict_mode=TruedefaultUpdated:
conftest.pywren_coreMagicMock for environments without compiled Rust binary.Migration notes
strict_modedefault change: Setstrict_mode: falseexplicitly in config JSON to keep old behavior--api-keyis opt-in, existing setups are unaffectedSummary by CodeRabbit
New Features
LIMIT.strict_modenow defaults to enabled, with allow/deny function lists normalized to lowercase.Bug Fixes
Tests / Chores
unwrap()/expect().