Refuse a translation key that would not form a Kotlin name

The generator escaped every translation value but emitted the key verbatim as
the member identifier, and identifierFor strips only . _ - -- so a key carrying
a space, a brace, a quote or a newline from a \uXXXX escape broke out of
val <name>: String get() = lookup(...) into an init block or property
initializer that runs when Strings is instantiated. Translation files are the
contribution surface that arrives by pull request, so this was a path from a
merged .properties change to code in every client.

The generated identifier is now held to letters-then-letters-and-digits before
any source is written, alongside the existing orphan and collision refusals.
Proven by replaying a file-writing payload against the built generator: it now
throws and writes nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Rsclub2_2andClaude Opus 5 committed 2026-10-05 22:36:02 +02:00
1 parent 9da7cd377e
commit 2654a1178e
4 files changed
+89

No files matched your search

@@ -50,6 +50,7 @@ fun generate(inputDirectory: File, outputDirectory: File): GenerationResult {
+ "language is where every key and every argument count comes from" + "language is where every key and every argument count comes from"
) )
refuseIllegalIdentifiers(base)
refuseIdentifierCollisions(base) refuseIdentifierCollisions(base)
val others = translations.filter { it.language != BASE_LANGUAGE } val others = translations.filter { it.language != BASE_LANGUAGE }
@@ -80,6 +81,21 @@ fun generate(inputDirectory: File, outputDirectory: File): GenerationResult {
) )
} }
private fun refuseIllegalIdentifiers(base: Translation) {
val illegal = illegalIdentifierKeys(base.entries.keys)
if (illegal.isEmpty()) {
return
}
throw TranslationException(
"strings_$BASE_LANGUAGE.properties has keys that do not form a usable name -- a key is "
+ "letters and digits with . _ - between words, nothing else. A key carrying "
+ "anything more would be written into the generated source as code: "
+ illegal.sorted().joinToString(", ")
)
}
private fun refuseIdentifierCollisions(base: Translation) { private fun refuseIdentifierCollisions(base: Translation) {
val collisions = identifierCollisions(base.entries.keys) val collisions = identifierCollisions(base.entries.keys)
@@ -59,5 +59,24 @@ fun identifierFor(key: String): String {
fun identifierCollisions(keys: Collection<String>): Map<String, List<String>> = fun identifierCollisions(keys: Collection<String>): Map<String, List<String>> =
keys.groupBy { identifierFor(it) }.filterValues { it.size > 1 } keys.groupBy { identifierFor(it) }.filterValues { it.size > 1 }
/**
* Keys whose identifier would not be a legal Kotlin name.
*
* [identifierFor] strips only `.`, `_` and `-`; every other character a `Properties` file can
* carry in a key -- a space, a brace, a quote, a `=`, a newline decoded from a `\uXXXX` escape
* -- passes through into identifier position unchanged. Emitted there it does not merely fail
* to compile: the member is written as `val <identifier>: String get() = lookup(...)`, so a
* key that reads `x: String get() = "" ... init { ... } ... val y` breaks out of the template
* and leaves a property initializer or an init block of the contributor's choosing, which
* runs when `Strings` is instantiated in the application. The value is already escaped before
* it becomes a literal; the identifier was not checked at all. So a generated name that is
* not letters-then-letters-and-digits is refused before any source is written.
*/
fun illegalIdentifierKeys(keys: Collection<String>): List<String> =
keys.filterNot { IDENTIFIER.matches(identifierFor(it)) }
/** A letter, then letters and digits: the plain Kotlin identifier this generator emits. */
private val IDENTIFIER = Regex("[A-Za-z][A-Za-z0-9]*")
/** `{0}`, `{12}`, `{0,number}` -- a brace, digits, then either a brace or a comma. */ /** `{0}`, `{12}`, `{0,number}` -- a brace, digits, then either a brace or a comma. */
private val PLACEHOLDER = Regex("""\{(\d+)\s*[,}]""") private val PLACEHOLDER = Regex("""\{(\d+)\s*[,}]""")
@@ -92,6 +92,32 @@ class GenerationTest {
assertTrue(failure.message!!.contains("guiStartupDesign"), failure.message) assertTrue(failure.message!!.contains("guiStartupDesign"), failure.message)
} }
@Test
fun aKeyCarryingKotlinSyntaxBreaksTheBuildRatherThanInjectingCode() {
/*
* The key, not the value, becomes the member name, and only . _ - are stripped from
* it. A \uXXXX escape lets a contributed .properties file carry a key that, dropped
* into `val <name>: String get() = ...`, closes the template and opens a property
* initializer -- Kotlin that runs when Strings is built in every client. The base
* file is the vector: only base keys become members. This must fail the build the way
* an orphan or a collision does, not ship.
*
* The escapes below decode to the key `inject: Int = run{ pwn() }`.
*/
val (input, output) = directories()
write(
input,
"strings_en.properties",
"ok=fine\n" +
"inject\\u003a\\u0020Int\\u0020\\u003d\\u0020run\\u007b\\u0020pwn()\\u0020\\u007d=value\n",
)
val failure = assertThrows(TranslationException::class.java) { generate(input, output) }
assertTrue(failure.message!!.contains("inject"), failure.message)
assertFalse(generated(output).exists(), "source was written despite an unusable key")
}
@Test @Test
fun aMissingBaseFileBreaksTheBuild() { fun aMissingBaseFileBreaksTheBuild() {
val (input, output) = directories() val (input, output) = directories()
@@ -103,4 +103,32 @@ class TranslationRulesTest {
fun distinctKeysReportNoCollision() { fun distinctKeysReportNoCollision() {
assertTrue(identifierCollisions(listOf("a.b", "c.d")).isEmpty()) assertTrue(identifierCollisions(listOf("a.b", "c.d")).isEmpty())
} }
@Test
fun aKeyThatWouldNotFormALegalIdentifierIsRejected() {
/*
* identifierFor strips only . _ -, so every other character a Properties file can
* carry in a key -- a space, a brace, a quote, a newline decoded from a \uXXXX escape
* -- would reach identifier position verbatim. Emitted there it does not merely fail
* to compile: it breaks out of `val <name>: String get() = ...` into whatever the key
* says, which is Kotlin that then runs. So the generated name is held to the shape of
* an identifier before it is ever written.
*/
val illegal = illegalIdentifierKeys(
listOf("settings.save", "a b", "x\": String get() = \"\"", "init { pwn() }\n val")
)
assertEquals(
listOf("a b", "init { pwn() }\n val", "x\": String get() = \"\""),
illegal.sorted(),
)
}
@Test
fun ordinaryKeysFormLegalIdentifiersAndAreNotRejected() {
// A digit-leading key is fine: identifierFor prefixes it, which is a letter again.
assertTrue(
illegalIdentifierKeys(listOf("settings.save", "gui.startup_design", "2.metres")).isEmpty()
)
}
} }