jamesfredley commented on code in PR #15968: URL: https://github.com/apache/grails-core/pull/15968#discussion_r3590926955
########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/GormQuerySafetyWarnings.groovy: ########## @@ -0,0 +1,67 @@ +/* + * 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 + * + * https://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.grails.datastore.gorm.query + +import groovy.transform.CompileStatic + +import java.util.concurrent.ConcurrentHashMap + +import org.slf4j.Logger + +@CompileStatic +final class GormQuerySafetyWarnings { + + private static final String GSTRING_VALUE_PLACEHOLDER = '${...}' + private static final Set<String> WARNED_GSTRING_QUERY_SHAPES = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>()) + + private GormQuerySafetyWarnings() { + } + + static boolean warnIfGStringQuery(Logger logger, CharSequence query, String operation) { + if (!(query instanceof GString) || ((GString) query).values.length == 0) { + return false + } + + String queryShape = buildQueryShape((GString) query) + if (!logger.warnEnabled) { + return false + } + + String warningKey = "${operation}\n${queryShape}" + if (!WARNED_GSTRING_QUERY_SHAPES.add(warningKey)) { + return false + } Review Comment: Addressed in 244cecd5a5. `warnIfGStringQuery` now checks `logger.warnEnabled` **before** building the query shape or the warning key and returns early when warn logging is disabled, so no shape/key work happens on the disabled path. The dedup set is bounded by `MAX_WARNED_QUERY_SHAPES` (1000): the size check, clear-on-overflow, and `add` run inside a `synchronized` block so it stays thread-safe and can no longer grow without limit. A new `GormQuerySafetyWarningsSpec` case floods >1000 distinct shapes and asserts a previously-warned shape warns again after the bounded reset (rather than being suppressed forever). ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy: ########## @@ -480,6 +481,8 @@ class HibernateGormStaticApi<D> extends GormStaticApi<D> { if (hints.isEmpty() && querySettings != null) { hints = querySettings.findAll { AvailableHints.getDefinedHints().contains(it.key) } } + String operation = isNative ? 'native SQL query' : (isUpdate ? 'executeUpdate' : 'find/executeQuery') + GormQuerySafetyWarnings.warnIfGStringQuery(log, hql, operation) Map<String, Object> coercedParams = namedParams?.collectEntries { k, v -> [k.toString(), v] } ?: [:] def ctx = HqlQueryContext.prepare(persistentEntity, hql, coercedParams, positionalParams, querySettings, hints, isNative, isUpdate) Review Comment: Addressed in 244cecd5a5. `HibernateGormStaticApiSpec` now includes "Test GString query warning is emitted once without interpolated values": it attaches a Logback `ListAppender` to the `HibernateGormStaticApi` logger, runs the same `GString` `executeQuery` twice, and asserts exactly one `WARN` is emitted, that the message contains `GString-interpolated query`, and that it does **not** contain the interpolated secret value. The module's test logging binding was switched to `logback-classic` with a `logback-test.xml` that mirrors the previous slf4j-simple levels (INFO root plus the two DEBUG categories), so the suite's log output 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
