Skip to content

Fix UtilCheckerFuzzer: Security exception Update I18nFormatUtil.java #7031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Shivam7-1 wants to merge 7 commits into from
+23 −12
Closed

Fix UtilCheckerFuzzer: Security exception Update I18nFormatUtil.java #7031

Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
851c120
Update I18nFormatUtil.java
Shivam7-1 Apr 10, 2025
ef8068b
Remove trailing whitespace
mernst Apr 16, 2025
d4b934c
Merge ../checker-framework-branch-master into patch-1
mernst Apr 16, 2025
05e84bf
Merge ../checker-framework-branch-master into patch-1
mernst Mar 3, 2026
3e11626
IllegalFormatException => IllegalArgumentException
mernst Mar 3, 2026
a7e95a6
Fix import
mernst Mar 3, 2026
e4bc6a4
Merge ../checker-framework-branch-master into patch-1
mernst Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions ...er-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.IllegalFormatException;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -36,40 +35,52 @@ public class I18nFormatUtil {
*/
@SuppressWarnings("nullness:argument") // It's not documented, but passing null as the
// argument array is supported.
public static void tryFormatSatisfiability(String format) throws IllegalFormatException {
public static void tryFormatSatisfiability(String format) throws IllegalArgumentException {
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Declaring throws for unchecked exception is unconventional.

IllegalArgumentException extends RuntimeException, so declaring it in the throws clause has no compile-time effect. While it serves as documentation, it's not idiomatic Java. Consider either removing the throws clause or adding a @throws Javadoc tag instead for documentation purposes.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@checker-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java`
at line 38, The method signature for tryFormatSatisfiability currently declares
"throws IllegalArgumentException", which is an unchecked exception; remove the
throws clause from the method declaration in
I18nFormatUtil.tryFormatSatisfiability to follow Java conventions and keep
behavior unchanged, and if you want to document the possible
IllegalArgumentException add a `@throws` IllegalArgumentException line to the
method Javadoc instead.

MessageFormat.format(format, (Object[]) null);
}

/**
* Returns a {@link I18nConversionCategory} for every conversion found in the format string.
*
* @param format the format string to parse
* @throws IllegalFormatException if the format is not syntactically valid
* @throws IllegalArgumentException if the format is not syntactically valid
*/
public static I18nConversionCategory[] formatParameterCategories(String format)
throws IllegalFormatException {
throws IllegalArgumentException {

tryFormatSatisfiability(format);
I18nConversion[] cs = MessageFormatParser.parse(format);

I18nConversion[] cs;
try {
cs = MessageFormatParser.parse(format);
} catch (Exception e) {
// Defensive programming: fail gracefully on parse errors
throw new IllegalArgumentException("Invalid format string: " + format);
}
Comment on lines +53 to +59
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java.util.IllegalFormatException is an abstract exception type and cannot be instantiated. This code will not compile as written. Use a concrete exception type (e.g., IllegalArgumentException with the original exception as the cause) or rethrow the parser exception after normalizing it to a single unchecked type used by callers.

Copilot uses AI. Check for mistakes.
Comment on lines +57 to +59
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception message concatenates the full (potentially attacker-controlled) format string. For very large inputs this can significantly increase memory usage and produce overly-large logs; consider truncating the string and/or including only its length (and possibly a small prefix) in the error message.

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +59
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Catching generic Exception loses important context.

Two issues with this exception handling:

  1. Catching Exception is too broad—it catches all checked and unchecked exceptions indiscriminately.
  2. The original exception is not chained, losing valuable debugging information (stack trace, root cause).
Proposed fix to preserve exception context 
     I18nConversion[] cs;
     try {
       cs = MessageFormatParser.parse(format);
-    } catch (Exception e) {
+    } catch (IllegalArgumentException e) {
       // Defensive programming: fail gracefully on parse errors
-      throw new IllegalArgumentException("Invalid format string: " + format);
+      throw new IllegalArgumentException("Invalid format string: " + format, e);
     }

If other exception types need handling, consider catching them explicitly rather than using a blanket Exception catch.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@checker-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java`
around lines 54 - 59, The current catch of Exception in I18nFormatUtil around
the call to MessageFormatParser.parse(format) is too broad and drops the
original exception; replace it by catching the specific parse-related
exception(s) thrown by MessageFormatParser.parse (e.g., a ParseException or the
exact checked exception type it declares) and, if you also need to guard against
unchecked errors, catch RuntimeException separately; when rethrowing use
exception chaining by constructing the IllegalArgumentException with the
original exception as the cause (e.g., new IllegalArgumentException("Invalid
format string: "+format, e)) so the stack trace/root cause is preserved.


int maxIndex = -1;
Map<Integer, I18nConversionCategory> conv = new HashMap<>(cs.length);

for (I18nConversion c : cs) {
int index = c.index;
if (index < 0 || index > 1000) { // Arbitrary upper bound to prevent abuse
throw new IllegalArgumentException(
Comment on lines +66 to +67
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hard-coded upper bound 1000 is a magic number that changes accepted formats and will be hard to adjust later. Consider extracting it into a named constant (e.g., MAX_ARGUMENT_INDEX) with a brief rationale, so the security/DoS intent is explicit and configurable.

Copilot uses AI. Check for mistakes.
"Format string contains illegal argument index: " + index);
Comment on lines +66 to +68
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw new IllegalFormatException(...) will not compile here either (the type is abstract). If you want to signal an invalid format/index, throw IllegalArgumentException (and consider keeping the exception cause) so callers can handle it consistently.

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +68
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior is introduced here (wrapping parser failures and rejecting large argument indices). There are existing JUnit tests for I18nFormatUtil (e.g., checker/src/test/java/.../I18nFormatterUnitTest.java), but none appear to cover these new failure modes; please add tests asserting isFormat(...)/hasFormat(...) behavior for (1) malformed patterns that previously crashed the parser and (2) an out-of-range index (e.g., {1001}).

Copilot uses AI. Check for mistakes.
}
Comment on lines +66 to +69
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider documenting the 1000 index limit in the method's Javadoc.

The arbitrary upper bound of 1000 is a reasonable safeguard against resource exhaustion attacks (as discovered by the fuzzer). However, this limit should be documented in the method's Javadoc so callers are aware of the constraint. Additionally, consider extracting 1000 to a named constant for clarity.

Proposed improvement 
+  private static final int MAX_FORMAT_INDEX = 1000;
+
   /**
    * Returns a {`@link` I18nConversionCategory} for every conversion found in the format string.
    *
    * `@param` format the format string to parse
-   * `@throws` IllegalArgumentException if the format is not syntactically valid
+   * `@throws` IllegalArgumentException if the format is not syntactically valid, or if any argument
+   *     index exceeds {`@value` `#MAX_FORMAT_INDEX`}
    */

And update the check:

-      if (index < 0 || index > 1000) { // Arbitrary upper bound to prevent abuse
+      if (index < 0 || index > MAX_FORMAT_INDEX) {
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@checker-util/src/main/java/org/checkerframework/checker/i18nformatter/util/I18nFormatUtil.java`
around lines 66 - 69, The magic upper bound 1000 used in I18nFormatUtil (the
condition "if (index < 0 || index > 1000)") should be extracted to a named
constant (e.g., MAX_ARGUMENT_INDEX) and documented in the method/class Javadoc;
update the check to use that constant and add a Javadoc sentence on the method
(or class) that explains the reason for and value of the limit (to prevent
resource-exhaustion/fuzzing attacks) so callers are aware of the constraint.


Integer indexKey = index;
conv.put(
indexKey,
I18nConversionCategory.intersect(
c.category,
conv.containsKey(indexKey) ? conv.get(indexKey) : I18nConversionCategory.UNUSED));
I18nConversionCategory existing = conv.getOrDefault(indexKey, I18nConversionCategory.UNUSED);
I18nConversionCategory merged = I18nConversionCategory.intersect(c.category, existing);

conv.put(indexKey, merged);
maxIndex = Math.max(maxIndex, index);
}

I18nConversionCategory[] res = new I18nConversionCategory[maxIndex + 1];
for (int i = 0; i <= maxIndex; i++) {
Integer indexKey = i;
res[i] = conv.containsKey(indexKey) ? conv.get(indexKey) : I18nConversionCategory.UNUSED;
res[i] = conv.getOrDefault(i, I18nConversionCategory.UNUSED);
}

return res;
}

Expand Down
Loading