)]}'
{
  "commit": "1e1a0a8c76f2e5dc6ed734439fd79ca90cffe96e",
  "tree": "5f956e1d6f651f4a2464f68128fe5565714d8e67",
  "parents": [
    "4de1634dd110a0906defb3f81b871a104886041d"
  ],
  "author": {
    "name": "BarakSrour",
    "email": "barak@apiiro.com",
    "time": "Mon Aug 17 13:48:28 2026 -0700"
  },
  "committer": {
    "name": "Copybara-Service",
    "email": "copybara-worker@google.com",
    "time": "Mon Aug 17 13:51:19 2026 -0700"
  },
  "message": "Validate php_class_prefix and objc_class_prefix in code generators (#29130)\n\n## Summary\n\nFollow-up to #28667, which added character validation for `php_namespace`, `php_metadata_namespace`, `ruby_package` and `csharp_namespace` because they are emitted directly into generated source without validation.\n\nTwo options in the same family were not covered and are still unvalidated on `main`:\n\n| option | read at | validated before this PR |\n|---|---|---|\n| `php_class_prefix` | `php/names.cc` (`ClassNamePrefixImpl`) | no |\n| `objc_class_prefix` | `objectivec/names.cc` (`FileClassPrefix`) | no |\n\n`php_class_prefix` is easy to miss by inspection: the guard added in #28667 and both of its call sites are in `php_generator.cc`, while `php_class_prefix` is read from `names.cc`.\n\n## What happens today\n\n**PHP.** The generator emits `class \u003cphp_class_prefix\u003e\u003cName\u003e extends ...`, so a prefix that closes the declaration and opens its own is emitted verbatim:\n\n```proto\noption php_class_prefix \u003d \"Pfx{} class Injected { public static function pwn() { return \u0027INJECTED\u0027; } } class \";\n```\n\n`protoc --php_out` exits 0 and the generated file contains:\n\n```php\nclass Pfx{} class Injected { public static function pwn() { return \u0027INJECTED\u0027; } } class M extends \\Google\\Protobuf\\Internal\\Message\n```\n\nThat is valid PHP (`php -l`: no syntax errors), the injected class is defined and its code runs, and `Tp\\M` is still generated correctly — so nothing fails to build and nothing looks wrong.\n\n**Objective-C.** `FileClassPrefix` returns the option verbatim, so with `objc_class_prefix \u003d \"A\\\";void pwn(){}//\"` the value lands in `@interface` / `@implementation` declarations, a `typedef struct` tag and the `.prefix \u003d \"...\"` string literal. I have not built the resulting Objective-C, so I am claiming unvalidated emission here rather than a working payload.\n\nWorth noting separately: a prefix check already exists, but it validates style rather than characters, so it warns about a legitimate short prefix and says nothing about a hostile one:\n\n```\nobjc_class_prefix \u003d \"AB\"                 -\u003e warning: Invalid \u0027option objc_class_prefix \u003d \"AB\";\u0027\nobjc_class_prefix \u003d \"A\\\";void pwn(){}//\" -\u003e no diagnostic at all\n```\n\n## Changes\n\n- **PHP generator** — generalised `IsValidPhpNamespace` to `IsValidPhpOption(value, option_name, error)` so the error names the offending option, and applied it to `php_class_prefix` alongside the two existing options. The message still begins `Invalid character`, so the tests added in #28667 are unaffected.\n- **Objective-C generator** — new `ValidateObjCClassPrefixChars()` rejects prefixes containing anything outside `[A-Za-z0-9_]`, run from `ValidateObjCClassPrefixes()` over every file ahead of the expected-prefixes opt-outs.\n- Tests for both, in the reject/accept pattern used by #28667.\n\n## Notes for review\n\nThree things I\u0027d rather flag than have you find:\n\n1. **The Objective-C check is an allowlist, not a denylist.** The prefix is pasted into C identifiers, where nothing outside `[A-Za-z0-9_]` is ever valid, so an allowlist closes the class rather than chasing characters. I checked every `objc_class_prefix` in this repo (including the deliberately-empty one) and none would be rejected. Happy to switch to the `absl::CharSet` denylist used for PHP/Ruby/C# if you\u0027d prefer consistency over precision.\n\n2. **Placement matters.** The character check runs in `ValidateObjCClassPrefixes()` over every file, ahead of both documented opt-outs (`expected_prefixes_path\u003d-` and `expected_prefixes_suppressions`), since disabling the naming *policy* should not disable a correctness check. There is a regression test per opt-out.\n\n3. **Java is gated, but to a much narrower bar** — `java/file.cc` rejects only `;`, `\\r`, `\\n` and space, and its comment says it checks \"only a subset\". Since Java accepts `/**/` as a whitespace substitute, `java_outer_classname \u003d \"Evil/**/implements/**/Cloneable\"` and `\"Evil/*\"` are both emitted into the class declaration on current `main`. I could not get code execution out of that (`;` blocks statement injection); the demonstrated effect is generated code that will not compile. **Not changed in this PR** — mentioning it in case you want the bar aligned, and happy to do it in a follow-up.\n\nAlso, for whatever it\u0027s worth to release planning: #28667 itself is not in any released binary yet — the validation string is absent from both `protoc 35.1` and `protoc 36.0-rc2`, since it landed after the v36 branch was cut.\n\n## Testing\n\n`src/google/protobuf/compiler/php/generator_unittest.cc` — `InvalidPhpClassPrefixRejected`, `ValidPhpClassPrefixAccepted`.\n\n`src/google/protobuf/compiler/objectivec/generator_unittest.cc` (new file, plus its `cc_test` target) — `InvalidObjCClassPrefixRejected`, `InvalidObjCClassPrefixRejectedWithExpectedPrefixesDisabled`, `InvalidObjCClassPrefixRejectedWhenSuppressed`, `ValidObjCClassPrefixAccepted`, `ObjCClassPrefixWithUnderscoreAccepted`.\n\nThe two opt-out tests fail with the character check in its original location inside `ValidateObjCClassPrefix()`, and pass with it moved.\n\nBoth directions are covered, so the tests fail if the validation is removed *and* if it is made too strict.\n\n---\n\n_Both tests were run locally against this branch (`bazel test //src/google/protobuf/compiler/php:generator_unittest //src/google/protobuf/compiler/objectivec:generator_unittest`), and re-run with the two guards reverted to confirm they fail without the fix._\n\nCloses #29130\n\nCOPYBARA_INTEGRATE_REVIEW\u003dhttps://github.com/protocolbuffers/protobuf/pull/29130 from BarakSrour:validate-class-prefix-options a2bbd8f2ea53f1b9958c8d8b310d28277d4979a7\nPiperOrigin-RevId: 966161028\n",
  "tree_diff": [
    {
      "type": "modify",
      "old_id": "0396353e088e20a3a017eb1184fca52805c6fa1e",
      "old_mode": 33188,
      "old_path": "src/google/protobuf/compiler/objectivec/BUILD.bazel",
      "new_id": "76ddfcc5b55722c17392e5dc1e861775308483f6",
      "new_mode": 33188,
      "new_path": "src/google/protobuf/compiler/objectivec/BUILD.bazel"
    },
    {
      "type": "add",
      "old_id": "0000000000000000000000000000000000000000",
      "old_mode": 0,
      "old_path": "/dev/null",
      "new_id": "a75e8ce0e28dc973a6d1878a9c74609c103d14fc",
      "new_mode": 33188,
      "new_path": "src/google/protobuf/compiler/objectivec/generator_unittest.cc"
    },
    {
      "type": "modify",
      "old_id": "88cece5ecc6d2480fb7c9815d6a2b7703cad4d9f",
      "old_mode": 33188,
      "old_path": "src/google/protobuf/compiler/objectivec/names.cc",
      "new_id": "8ea79d7dc8b712f29d483714c9d900a947d9734a",
      "new_mode": 33188,
      "new_path": "src/google/protobuf/compiler/objectivec/names.cc"
    },
    {
      "type": "modify",
      "old_id": "ff62eca28a7c8b2f75f7360516adcc75f3559897",
      "old_mode": 33188,
      "old_path": "src/google/protobuf/compiler/php/generator_unittest.cc",
      "new_id": "025bd6b4e200424c0fd331169885a9018719ca4e",
      "new_mode": 33188,
      "new_path": "src/google/protobuf/compiler/php/generator_unittest.cc"
    },
    {
      "type": "modify",
      "old_id": "46fbaf8f600aa255dceb824913c042b6bb0349ae",
      "old_mode": 33188,
      "old_path": "src/google/protobuf/compiler/php/php_generator.cc",
      "new_id": "4b16d1c063bb7dee9775f155126b125e0a24c0c0",
      "new_mode": 33188,
      "new_path": "src/google/protobuf/compiler/php/php_generator.cc"
    }
  ]
}