From 3f01fade7c442f9d1e22d1d6f8c3964db22e6fe7 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 16 Dec 2021 10:32:18 +0000 Subject: [PATCH] Introduce `StringFormattingCheck` checkstyle rule (#81603) The new rule Checks for calls to `String#formatted(Object...)` that include format specifiers that are not locale-safe. This method always uses the default `Locale`, and so for our purposes it is safer to use `String#format(Locale, String, Object...)`. Note that this rule can currently only detect violations when calling `formatted()` on a string literal or text block. In theory, it could be extended to detect violations in local variables or statics. Note that this change also forbids `.formatted()` in server code, so we are only permitted to use `.formatted()` in test code. --- .../checkstyle/StringFormattingCheck.java | 89 +++++++++++++++++++ .../src/main/resources/checkstyle.xml | 6 ++ .../forbidden/es-server-signatures.txt | 4 + .../indices/IndicesLifecycleListenerIT.java | 5 +- .../index/mapper/KeywordFieldMapper.java | 9 +- .../elasticsearch/indices/SystemIndices.java | 5 +- .../bucket/BucketsAggregator.java | 5 +- .../heuristic/NXYSignificanceHeuristic.java | 5 +- .../metrics/NumericMetricsAggregator.java | 5 +- .../transport/RemoteConnectionStrategy.java | 4 +- .../ingest/PipelineConfigurationTests.java | 5 +- .../yaml/section/ClientYamlTestSuite.java | 33 +++---- .../section/ClientYamlTestSuiteTests.java | 33 +++---- .../xpack/ccr/ESCCRRestTestCase.java | 9 +- .../xpack/core/ml/MlStatsIndex.java | 6 +- .../persistence/AnomalyDetectorsIndex.java | 6 +- .../action/realm/ClearRealmCacheResponse.java | 5 +- .../action/role/ClearRolesCacheResponse.java | 5 +- .../test/enrich/CommonEnrichRestTestCase.java | 9 +- .../test/eql/EqlRestTestCase.java | 21 ++--- .../elasticsearch/xpack/ql/util/Graphviz.java | 13 +-- .../xpack/security/authc/TokenService.java | 9 +- .../xpack/sql/client/RemoteFailureTests.java | 5 +- 23 files changed, 210 insertions(+), 86 deletions(-) create mode 100644 build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/StringFormattingCheck.java diff --git a/build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/StringFormattingCheck.java b/build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/StringFormattingCheck.java new file mode 100644 index 0000000000000..48fa3ad6ee485 --- /dev/null +++ b/build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/StringFormattingCheck.java @@ -0,0 +1,89 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.gradle.internal.checkstyle; + +import com.puppycrawl.tools.checkstyle.StatelessCheck; +import com.puppycrawl.tools.checkstyle.api.AbstractCheck; +import com.puppycrawl.tools.checkstyle.api.DetailAST; +import com.puppycrawl.tools.checkstyle.api.TokenTypes; + +import java.util.Locale; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Checks for calls to {@link String#formatted(Object...)} that include format specifiers that + * are not locale-safe. This method always uses the default {@link Locale}, and so for our + * purposes it is safer to use {@link String#format(Locale, String, Object...)}. + *

+ * Note that this rule can currently only detect violations when calling formatted() + * on a string literal or text block. In theory, it could be extended to detect violations in + * local variables or statics. + */ +@StatelessCheck +public class StringFormattingCheck extends AbstractCheck { + + public static final String FORMATTED_MSG_KEY = "forbidden.formatted"; + + @Override + public int[] getDefaultTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getRequiredTokens() { + return new int[] { TokenTypes.METHOD_CALL }; + } + + @Override + public void visitToken(DetailAST ast) { + checkFormattedMethod(ast); + } + + // Originally pinched from java/util/Formatter.java but then modified. + // %[argument_index$][flags][width][.precision][t]conversion + private static final Pattern formatSpecifier = Pattern.compile("%(?:\\d+\\$)?(?:[-#+ 0,\\(<]*)?(?:\\d+)?(?:\\.\\d+)?([tT]?[a-zA-Z%])"); + + private void checkFormattedMethod(DetailAST ast) { + final DetailAST dotAst = ast.findFirstToken(TokenTypes.DOT); + if (dotAst == null) { + return; + } + + final String methodName = dotAst.findFirstToken(TokenTypes.IDENT).getText(); + if (methodName.equals("formatted") == false) { + return; + } + + final DetailAST subjectAst = dotAst.getFirstChild(); + + String stringContent = null; + if (subjectAst.getType() == TokenTypes.TEXT_BLOCK_LITERAL_BEGIN) { + stringContent = subjectAst.findFirstToken(TokenTypes.TEXT_BLOCK_CONTENT).getText(); + } else if (subjectAst.getType() == TokenTypes.STRING_LITERAL) { + stringContent = subjectAst.getText(); + } + + if (stringContent != null) { + final Matcher m = formatSpecifier.matcher(stringContent); + while (m.find()) { + char specifier = m.group(1).toLowerCase(Locale.ROOT).charAt(0); + + if (specifier == 'd' || specifier == 'e' || specifier == 'f' || specifier == 'g' || specifier == 't') { + log(ast, FORMATTED_MSG_KEY, m.group()); + } + } + } + } +} diff --git a/build-tools-internal/src/main/resources/checkstyle.xml b/build-tools-internal/src/main/resources/checkstyle.xml index 95a32e026c1ff..22a0d2912e4d7 100644 --- a/build-tools-internal/src/main/resources/checkstyle.xml +++ b/build-tools-internal/src/main/resources/checkstyle.xml @@ -130,6 +130,12 @@ --> + + + +