You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "HyukjinKwon (via GitHub)" <gi...@apache.org> on 2026/09/08 07:31:46 UTC
[PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
HyukjinKwon opened a new pull request, #58604:
URL: https://github.com/apache/spark/pull/58604
### What changes were proposed in this pull request?
Add `spark.sql.restrictedMode.enabled` (internal, default `false` = unchanged). When enabled,
`reflect`/`java_method` and `TRANSFORM ... USING` are rejected during analysis with a new
`UNSUPPORTED_FEATURE.SQL_RESTRICTED_MODE` error condition. `ADD JAR` / JAR-backed
`CREATE FUNCTION` and the class-loading/URL JDBC data-source options are wired to the same flag
but not yet gated (follow-up).
### Why are the changes needed?
Provides an opt-in mode that disables dynamic code and external-script execution features in SQL
for deployments that want a more constrained SQL surface. Off by default, so there is no behavior
change unless enabled.
### Does this PR introduce _any_ user-facing change?
No by default. When the mode is enabled, the listed features are rejected with a clear error.
### How was this patch tested?
New `RestrictedModeSuite`: `reflect` and `TRANSFORM` are rejected when the mode is on and allowed
when off.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Isaac
This pull request and its description were written by Isaac.
Co-authored-by: Isaac <no...@databricks.com>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #58604:
URL: https://github.com/apache/spark/pull/58604#discussion_r3963645372
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -328,6 +328,9 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
}
preemptedError.clear()
try {
+ if (SQLConf.get.restrictedModeEnabled) {
+ checkRestrictedMode(inlinedPlan)
+ }
Review Comment:
[P1] Enforce restricted mode in the single-pass analyzer
This check runs only through the legacy analyzer. Both single-pass modes use ResolverRunner, which supports reflection expressions and marks the resulting plan analyzed without calling CheckAnalysis. Restricted mode can therefore accept the functions it promises to reject. Apply the restriction across both analyzer paths and add an ordinary reflection SELECT test across analyzer modes; the current tests explicitly call legacy analysis or use commands that fall back to it.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -339,6 +342,45 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
plan.setAnalyzed()
}
+ /**
+ * Rejects SQL features that load or execute externally provided code or scripts when the
+ * restricted execution mode is enabled. Unlike `checkAnalysis0`, which skips already-analyzed
+ * sub-plans (`case p if p.analyzed`), this walks the whole plan -- including analyzed sub-plans
+ * reused from a temporary view or a cached Dataset, and the bodies of analysis-only commands
+ * (CTAS, `CACHE TABLE ... AS SELECT`, `CREATE`/`ALTER VIEW`) which move into `innerChildren`
+ * once analyzed -- and descends into subquery plans, so these features cannot slip through.
+ */
+ private def checkRestrictedMode(plan: LogicalPlan): Unit = {
+ def checkExpression(expr: Expression): Unit = expr.foreach {
+ // A try_reflect call stays a RuntimeReplaceable wrapper until optimization, so match it
+ // before its CallMethodViaReflection replacement to report the right function name.
+ case t: TryReflect =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(t.prettyName)} function")
+ case c: CallMethodViaReflection =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(c.prettyName)} function")
Review Comment:
[P1] Reject reflection before class initialization
Legacy analysis can initialize the referenced class before reaching this rejection. Alias resolution reads Expression.resolved, which invokes CallMethodViaReflection.checkInputDataTypes; that calls classExists/findMethod, which use Utils.classForName with initialize=true. A previously uninitialized class already on the classpath can thus execute its initializer despite the eventual restricted-mode error. Add an early restriction before class lookup, retaining this final traversal. A fixture whose initializer increments a counter would verify rejection occurs without initialization.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -339,6 +342,45 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
plan.setAnalyzed()
}
+ /**
+ * Rejects SQL features that load or execute externally provided code or scripts when the
+ * restricted execution mode is enabled. Unlike `checkAnalysis0`, which skips already-analyzed
+ * sub-plans (`case p if p.analyzed`), this walks the whole plan -- including analyzed sub-plans
+ * reused from a temporary view or a cached Dataset, and the bodies of analysis-only commands
+ * (CTAS, `CACHE TABLE ... AS SELECT`, `CREATE`/`ALTER VIEW`) which move into `innerChildren`
+ * once analyzed -- and descends into subquery plans, so these features cannot slip through.
+ */
+ private def checkRestrictedMode(plan: LogicalPlan): Unit = {
+ def checkExpression(expr: Expression): Unit = expr.foreach {
+ // A try_reflect call stays a RuntimeReplaceable wrapper until optimization, so match it
+ // before its CallMethodViaReflection replacement to report the right function name.
+ case t: TryReflect =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(t.prettyName)} function")
+ case c: CallMethodViaReflection =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(c.prettyName)} function")
+ case s: SubqueryExpression =>
+ checkPlan(s.plan)
+ case _ =>
+ }
+ def checkPlan(p: LogicalPlan): Unit = p.foreach {
+ case _: ScriptTransformation =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ "The TRANSFORM ... USING clause")
+ case node =>
+ node.expressions.foreach(checkExpression)
+ // The body of an analysis-only command (CTAS, `CACHE TABLE ... AS SELECT`,
+ // `CREATE`/`ALTER VIEW`) moves from `children` into `innerChildren` once the command is
+ // analyzed, so `foreach` (which follows `children`) would not otherwise reach it.
+ node.innerChildren.foreach {
+ case inner: LogicalPlan => checkPlan(inner)
Review Comment:
[P2] Traverse each subquery only once
QueryPlan.innerChildren already returns the node’s subqueries, which checkExpression also visits through SubqueryExpression.plan. Every nesting level therefore doubles traversal of its descendants, making validation exponential for allowed nested scalar queries. Four nested SELECTs cause 30 plan-node visits for eight distinct nodes. Preserve command-body coverage while traversing each subquery once, and add coverage for nested queries containing no restricted features.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
Posted by "flyovers-bot (via GitHub)" <gi...@apache.org>.
flyovers-bot commented on PR #58604:
URL: https://github.com/apache/spark/pull/58604#issuecomment-5594770349
<!-- flyovers:flyover -->
[](https://flyovers.dev/flyover/7e065299-07ea-41b5-ac23-2e312f837616)
**[Watch the flyover →](https://flyovers.dev/flyover/7e065299-07ea-41b5-ac23-2e312f837616)** · 5:17 · 6 chapters · 7 files
An animated walkthrough of this diff: context, the problem, the change, implementation care, code walkthrough, and impact.
<sub>eacf135 · updated automatically by flyovers</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #58604:
URL: https://github.com/apache/spark/pull/58604#discussion_r3955464252
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2676,6 +2676,18 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val RESTRICTED_MODE_ENABLED = buildConf("spark.sql.restrictedMode.enabled")
+ .internal()
+ .doc("When true, SQL features that load or execute externally provided code, scripts, or " +
+ "jars from the query itself are disabled: the reflect and java_method functions and the " +
+ "TRANSFORM ... USING clause are rejected. This is an opt-in profile for endpoints that run " +
+ "queries from many users; it composes with the individual data-source option controls " +
+ "(for example spark.sql.kafka.disallowedOptions and the avro schema-URL scheme allowlist). " +
Review Comment:
`spark.sql.kafka.disallowedOptions` does not exist anywhere in the repository -- this doc string is the only occurrence. I could not find an "avro schema-URL scheme allowlist" either.
Could you drop these examples, or point at configs that actually exist?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/RestrictedModeSuite.scala:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions.{AttributeReference, CallMethodViaReflection, Literal}
+import org.apache.spark.sql.catalyst.plans.logical.{ScriptInputOutputSchema, ScriptTransformation}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.StringType
+
+class RestrictedModeSuite extends AnalysisTest {
+
+ private val restricted = SQLConf.RESTRICTED_MODE_ENABLED.key
+
+ private def reflectCall: CallMethodViaReflection =
+ CallMethodViaReflection(Seq(Literal("java.lang.Math"), Literal("abs"), Literal(-1)))
+
+ private def transformPlan: ScriptTransformation =
+ ScriptTransformation(
+ "cat",
+ Seq(AttributeReference("value", StringType)()),
+ TestRelations.testRelation,
+ ScriptInputOutputSchema(Nil, Nil, None, None, Nil, Nil, None, None, schemaLess = false))
+
+ test("reflect/java_method are rejected only when restricted mode is enabled") {
+ withSQLConf(restricted -> "true") {
+ val e = intercept[AnalysisException](reflectCall.checkInputDataTypes())
+ assert(e.getMessage.contains("restrictedMode.enabled"))
Review Comment:
A substring match on the message verifies neither the new error condition name nor its parameters -- it would still pass if the error came from an unrelated code path that happened to mention the config. `checkError` is the convention here (82 uses in this test directory alone):
```scala
checkError(
exception = e,
condition = "UNSUPPORTED_FEATURE.SQL_RESTRICTED_MODE",
parameters = Map(
"feature" -> "...",
"config" -> "\"spark.sql.restrictedMode.enabled\""))
```
Same for the `TRANSFORM` assertion below.
Two cases also look worth adding: `try_reflect` (blocked via the `TryReflect` replacement, but untested), and an end-to-end SQL test for `SELECT TRANSFORM ... USING` -- the current test builds the `ScriptTransformation` node directly, so it would not catch a plan shape that reaches execution without passing through this `CheckAnalysis` traversal.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -375,6 +375,10 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
plan.foreachUp {
case p if p.analyzed => // Skip already analyzed sub-plans
+ case _: ScriptTransformation if SQLConf.get.restrictedModeEnabled =>
Review Comment:
This case sits below `case p if p.analyzed => // Skip already analyzed sub-plans`, and `AnalysisHelper.setAnalyzed()` marks children recursively:
```scala
private[sql] def setAnalyzed(): Unit = {
...
children.foreach(_.setAnalyzed())
}
```
So a `ScriptTransformation` that lives inside an already-analyzed sub-plan never reaches this case. That covers DataFrame-backed temp views (`createOrReplaceTempView` stores the analyzed plan) and reused `Dataset`s, so a view created while the mode was off can still be queried after the mode is turned on.
The `reflect` gate has the analogous problem: `checkInputDataTypes()` is reached through the `Expression.resolved` lazy val, so the conf value is memoized at first resolution.
If the mode is meant to be a boundary rather than a hint, the check probably needs to run somewhere that pre-analyzed plans cannot skip. At minimum it is worth calling out the limitation.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala:
##########
@@ -84,6 +84,10 @@ case class CallMethodViaReflection(
override def prettyName: String = getTagValue(FunctionRegistry.FUNC_ALIAS).getOrElse("reflect")
override def checkInputDataTypes(): TypeCheckResult = {
+ if (SQLConf.get.restrictedModeEnabled) {
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ "The reflect and java_method functions")
Review Comment:
Two issues with this string.
**Grammar.** Substituted into `"<feature> is not allowed because <config> is enabled."` and prefixed by the `UNSUPPORTED_FEATURE` parent message, users see:
> The feature is not supported: The reflect and java_method function**s** **is** not allowed because "spark.sql.restrictedMode.enabled" is enabled.
**`try_reflect` is missing.** `TryReflect` replaces itself with `CallMethodViaReflection(params, failOnError = false)`, so `try_reflect` is blocked by this check too, but the message never mentions it.
Using `prettyName` fixes both at once, since `prettyName` already resolves to `reflect` / `java_method` / `try_reflect` via `FUNC_ALIAS`:
```scala
throw QueryCompilationErrors.restrictedModeFeatureError(
s"The ${toSQLId(prettyName)} function")
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2676,6 +2676,18 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val RESTRICTED_MODE_ENABLED = buildConf("spark.sql.restrictedMode.enabled")
Review Comment:
This should very likely be `buildStaticConf` rather than `buildConf`.
The doc below describes this as "an opt-in profile for endpoints that run queries from many users", but as a runtime `SQLConf` any of those users can disable the whole thing with
```sql
SET spark.sql.restrictedMode.enabled=false;
```
`internal()` only hides the config from the docs and `SET -v`; it does not make it read-only. Spark already uses `buildStaticConf` for exactly this kind of setting -- see `spark.sql.legacy.hive.thriftServer.allowSettingSystemProperties` and `spark.sql.hive.thriftServer.http.sniHostCheckEnabled` in `StaticSQLConf.scala`. With a static conf, `RuntimeConfig.requireNonStaticConf` rejects the `SET`.
As written, the gate can be turned off by the very users it is meant to constrain, which undercuts the rest of the PR.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -329,6 +329,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map.empty)
}
+ def restrictedModeFeatureError(feature: String): Throwable = {
+ new AnalysisException(
+ errorClass = "UNSUPPORTED_FEATURE.SQL_RESTRICTED_MODE",
+ messageParameters = Map(
+ "feature" -> feature,
+ "config" -> toSQLConfVal(SQLConf.RESTRICTED_MODE_ENABLED.key)))
Review Comment:
This is a config *key*, so it should be `toSQLConf`, not `toSQLConfVal`. The convention is spelled out in `QueryErrorsBase`'s own header comment:
> 5. SQL configs ... For example: "spark.sql.ansi.enabled".
> 6. Any values of datasource options or SQL configs shall be double quoted. For example: "true", "CORRECTED".
`Count.scala` shows the correct pairing:
```scala
toSQLConf(SQLConf.ALLOW_PARAMETERLESS_COUNT.key),
toSQLConfVal(true.toString))
```
Both helpers happen to emit the same double quotes today, so this is not a behavior change -- but the two names mean opposite things.
Separately, on the error condition itself: `<feature>` taking free-form English prose is the only such parameter in `error-conditions.json`. Splitting into per-feature sub-conditions would match how the rest of the file is written.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #58604:
URL: https://github.com/apache/spark/pull/58604#discussion_r3957190535
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -339,6 +342,37 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
plan.setAnalyzed()
}
+ /**
+ * Rejects SQL features that load or execute externally provided code or scripts when the
+ * restricted execution mode is enabled. Unlike the checks in `checkAnalysis0`, this walks the
+ * whole plan -- including already-analyzed sub-plans reused from a temporary view or a cached
+ * Dataset -- and descends into subquery plans, so these features cannot slip through on a plan
+ * that was analyzed while the mode was off.
+ */
+ private def checkRestrictedMode(plan: LogicalPlan): Unit = {
+ def checkExpression(expr: Expression): Unit = expr.foreach {
+ // A try_reflect call stays a RuntimeReplaceable wrapper until optimization, so match it
+ // before its CallMethodViaReflection replacement to report the right function name.
+ case _: TryReflect =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId("try_reflect")} function")
+ case c: CallMethodViaReflection =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(c.prettyName)} function")
+ case s: SubqueryExpression =>
+ checkPlan(s.plan)
+ case _ =>
+ }
+ def checkPlan(p: LogicalPlan): Unit = p.foreach {
Review Comment:
`p.foreach` follows `children`, and that misses the query of every `AnalysisOnlyCommand`.
`checkAnalysis` runs *after* `markAsAnalyzed`, and at that point (`Command.scala`):
```scala
override final def children: Seq[LogicalPlan] = if (isAnalyzed) Nil else childrenToAnalyze
override def innerChildren: Seq[QueryPlan[_]] = if (isAnalyzed) childrenToAnalyze else Nil
```
so the body has moved out of `children` into `innerChildren`. That covers `V2CreateTableAsSelectPlan` (CTAS / RTAS), `CacheTableAsSelect`, `CreateView` / `CreateViewCommand`, and `AlterViewAs` / `AlterViewAsCommand`.
So these look like they are not rejected:
```sql
CREATE TABLE t AS SELECT reflect('java.lang.Runtime', 'getRuntime');
CACHE TABLE t AS SELECT TRANSFORM(a) USING 'evil.sh' AS (b) FROM src;
```
and there is no second chance later: the stored `query` is already analyzed, so `Analyzer.executeAndCheck` returns early (`if (plan.analyzed) plan`) and `checkAnalysis` never runs again before the command executes it. `CREATE VIEW` is less severe since the body is re-analyzed on read, but CTAS and `CACHE TABLE ... AS SELECT` run the query right there.
Descending into `innerChildren` as well should close this:
```scala
def checkPlan(p: LogicalPlan): Unit = p.foreach {
case _: ScriptTransformation =>
throw QueryCompilationErrors.restrictedModeFeatureError(
"The TRANSFORM ... USING clause")
case node =>
node.expressions.foreach(checkExpression)
node.innerChildren.foreach {
case inner: LogicalPlan => checkPlan(inner)
case _ =>
}
}
```
Worth a test for each of CTAS and `CACHE TABLE ... AS SELECT`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -339,6 +342,37 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
plan.setAnalyzed()
}
+ /**
+ * Rejects SQL features that load or execute externally provided code or scripts when the
+ * restricted execution mode is enabled. Unlike the checks in `checkAnalysis0`, this walks the
+ * whole plan -- including already-analyzed sub-plans reused from a temporary view or a cached
+ * Dataset -- and descends into subquery plans, so these features cannot slip through on a plan
+ * that was analyzed while the mode was off.
+ */
+ private def checkRestrictedMode(plan: LogicalPlan): Unit = {
+ def checkExpression(expr: Expression): Unit = expr.foreach {
+ // A try_reflect call stays a RuntimeReplaceable wrapper until optimization, so match it
+ // before its CallMethodViaReflection replacement to report the right function name.
+ case _: TryReflect =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId("try_reflect")} function")
Review Comment:
Nit: `TryReflect.prettyName` is already `"try_reflect"`, so this could be `case t: TryReflect => ... toSQLId(t.prettyName)`, matching the `CallMethodViaReflection` branch just below.
(The ordering comment above it is accurate, by the way -- `InheritAnalysisRules` makes `replacement` the child, and `Expression.foreach` is pre-order, so the wrapper is matched first.)
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/RestrictedModeSuite.scala:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions.{AttributeReference, Literal}
+import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project, ScriptInputOutputSchema, ScriptTransformation}
+import org.apache.spark.sql.internal.{SQLConf, StaticSQLConf}
+import org.apache.spark.sql.types.StringType
+
+class RestrictedModeSuite extends AnalysisTest {
+
+ private val restricted = StaticSQLConf.RESTRICTED_MODE_ENABLED.key
+ // The config appears in the error message double-quoted, as `toSQLConf` renders it.
+ private val configName = "\"" + restricted + "\""
+
+ // The mode is a static config, so `withSQLConf` (which rejects static keys) cannot toggle it.
+ // Set it directly on the active conf instead, restoring the previous value afterwards.
+ private def withRestrictedMode[T](enabled: Boolean)(f: => T): T = {
Review Comment:
This helper writes the static config straight onto the active `SQLConf`, which is a reasonable way to toggle it in a catalyst unit test -- but it also means nothing in the suite covers the guarantee this PR is built on, namely that a session *cannot* turn the mode off.
A test asserting that `SET spark.sql.restrictedMode.enabled=false` fails with `CANNOT_MODIFY_CONFIG` would be worth adding (in `sql/core`, since it needs a session).
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/RestrictedModeSuite.scala:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions.{AttributeReference, Literal}
+import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project, ScriptInputOutputSchema, ScriptTransformation}
+import org.apache.spark.sql.internal.{SQLConf, StaticSQLConf}
+import org.apache.spark.sql.types.StringType
+
+class RestrictedModeSuite extends AnalysisTest {
+
+ private val restricted = StaticSQLConf.RESTRICTED_MODE_ENABLED.key
+ // The config appears in the error message double-quoted, as `toSQLConf` renders it.
+ private val configName = "\"" + restricted + "\""
+
+ // The mode is a static config, so `withSQLConf` (which rejects static keys) cannot toggle it.
+ // Set it directly on the active conf instead, restoring the previous value afterwards.
+ private def withRestrictedMode[T](enabled: Boolean)(f: => T): T = {
+ val conf = SQLConf.get
+ val previous = if (conf.contains(restricted)) Some(conf.getConfString(restricted)) else None
+ conf.setConfString(restricted, enabled.toString)
+ try f finally {
+ previous match {
+ case Some(value) => conf.setConfString(restricted, value)
+ case None => conf.unsetConf(restricted)
+ }
+ }
+ }
+
+ private def functionProject(name: String): LogicalPlan =
+ Project(
+ Seq(UnresolvedAlias(
+ UnresolvedFunction(
+ name,
+ Seq(Literal("java.lang.Math"), Literal("abs"), Literal(-1)),
+ isDistinct = false))),
+ TestRelations.testRelation)
+
+ private def transformPlan: ScriptTransformation =
+ ScriptTransformation(
+ "cat",
+ Seq(AttributeReference("value", StringType)()),
+ TestRelations.testRelation,
+ ScriptInputOutputSchema(Nil, Nil, None, None, Nil, Nil, None, None, schemaLess = false))
+
+ private def checkRestrictedError(e: AnalysisException, feature: String): Unit = {
+ checkError(
+ exception = e,
+ condition = "UNSUPPORTED_FEATURE.SQL_RESTRICTED_MODE",
+ parameters = Map("feature" -> feature, "config" -> configName))
+ }
+
+ test("reflect / java_method / try_reflect are rejected only when restricted mode is enabled") {
+ Seq("reflect", "java_method", "try_reflect").foreach { fn =>
+ withRestrictedMode(true) {
+ val analyzer = getAnalyzer
+ val e = intercept[AnalysisException] {
+ analyzer.checkAnalysis(analyzer.execute(functionProject(fn)))
+ }
+ checkRestrictedError(e, s"The `$fn` function")
+ }
+ withRestrictedMode(false) {
+ val analyzer = getAnalyzer
+ analyzer.checkAnalysis(analyzer.execute(functionProject(fn)))
+ }
+ }
+ }
+
+ test("TRANSFORM ... USING is rejected only when restricted mode is enabled") {
+ withRestrictedMode(true) {
+ val analyzer = getAnalyzer
+ val e = intercept[AnalysisException] {
+ analyzer.checkAnalysis(analyzer.execute(transformPlan))
+ }
+ checkRestrictedError(e, "The TRANSFORM ... USING clause")
+ }
+ withRestrictedMode(false) {
+ val analyzer = getAnalyzer
+ // Analysis succeeds (no restricted-mode error) when the profile is off.
+ analyzer.checkAnalysis(analyzer.execute(transformPlan))
+ }
+ }
+
+ test("restricted mode is enforced even for an already-analyzed sub-plan") {
Review Comment:
This exercises a path that cannot occur at runtime. `Analyzer.executeAndCheck` starts with
```scala
if (plan.analyzed) {
plan
} else {
```
so a top-level plan that is entirely analyzed never reaches `checkAnalysis` at all -- passing one in directly only tests the traversal in isolation.
The case that does happen is an analyzed *sub*-plan under a fresh parent (referencing a temp view, reusing a Dataset). Wrapping `analyzed` in something like a `Project` before calling `checkAnalysis` would cover the real shape, and would also be the case that `checkAnalysis0`'s `case p if p.analyzed` skips -- which is the reason this method exists.
Related: now that the config is static it cannot change within a JVM, so "a plan that was analyzed while the mode was off" (here and in the `checkRestrictedMode` scaladoc) cannot really arise. The traversal is still worth having for the `p.analyzed` skip, but the justification in both comments overstates the case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #58604:
URL: https://github.com/apache/spark/pull/58604#issuecomment-5593626536
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/dad9ee82213ff062b32cb5ca550755089339cd48
- merged into branch-4.x https://github.com/apache/spark/commit/a336c052dd54a25e2c09879d9ef9d33c985f55c5
*Posted by `merge_spark_pr.py`*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #58604:
URL: https://github.com/apache/spark/pull/58604#issuecomment-5593628849
Merged to branch-4.3 too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #58604:
URL: https://github.com/apache/spark/pull/58604#issuecomment-5594965368
Thanks @sunchao for the careful post-merge review. I've addressed all three findings in a follow-up: #58642.
1. **Enforce across both analyzer paths.** `HybridAnalyzer.apply` now routes a restricted-mode session to the fixed-point analyzer (where `checkRestrictedMode` runs) in every single-pass mode, including the fully-enabled mode that skips `ResolverGuard`. Test: enforcement holds with `spark.sql.analyzer.singlePassResolver.enabled`/`enabledTentatively`.
2. **Reject before class initialization.** `CallMethodViaReflection.checkInputDataTypes` short-circuits in restricted mode, so a call that is going to be rejected never resolves the class via `Utils.classForName` (no static initializer). Test uses a fixture whose initializer sets a system property and asserts it stays unset.
3. **Traverse each subquery once.** The `innerChildren` recursion now skips plans that are already the node's subqueries (reached via `SubqueryExpression`), keeping validation linear; added coverage for a feature nested in a scalar subquery and for deeply nested subqueries with no restricted feature.
All changes are scoped to restricted mode, so default behavior is unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #58604: [SPARK-59319][SQL] Add an opt-in restricted SQL execution mode
URL: https://github.com/apache/spark/pull/58604
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org