tree 5f956e1d6f651f4a2464f68128fe5565714d8e67
parent 4de1634dd110a0906defb3f81b871a104886041d
author BarakSrour <barak@apiiro.com> 1786999708 -0700
committer Copybara-Service <copybara-worker@google.com> 1786999879 -0700

Validate php_class_prefix and objc_class_prefix in code generators (#29130)

## Summary

Follow-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.

Two options in the same family were not covered and are still unvalidated on `main`:

| option | read at | validated before this PR |
|---|---|---|
| `php_class_prefix` | `php/names.cc` (`ClassNamePrefixImpl`) | no |
| `objc_class_prefix` | `objectivec/names.cc` (`FileClassPrefix`) | no |

`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`.

## What happens today

**PHP.** The generator emits `class <php_class_prefix><Name> extends ...`, so a prefix that closes the declaration and opens its own is emitted verbatim:

```proto
option php_class_prefix = "Pfx{} class Injected { public static function pwn() { return 'INJECTED'; } } class ";
```

`protoc --php_out` exits 0 and the generated file contains:

```php
class Pfx{} class Injected { public static function pwn() { return 'INJECTED'; } } class M extends \Google\Protobuf\Internal\Message
```

That 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.

**Objective-C.** `FileClassPrefix` returns the option verbatim, so with `objc_class_prefix = "A\";void pwn(){}//"` the value lands in `@interface` / `@implementation` declarations, a `typedef struct` tag and the `.prefix = "..."` string literal. I have not built the resulting Objective-C, so I am claiming unvalidated emission here rather than a working payload.

Worth 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:

```
objc_class_prefix = "AB"                 -> warning: Invalid 'option objc_class_prefix = "AB";'
objc_class_prefix = "A\";void pwn(){}//" -> no diagnostic at all
```

## Changes

- **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.
- **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.
- Tests for both, in the reject/accept pattern used by #28667.

## Notes for review

Three things I'd rather flag than have you find:

1. **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'd prefer consistency over precision.

2. **Placement matters.** The character check runs in `ValidateObjCClassPrefixes()` over every file, ahead of both documented opt-outs (`expected_prefixes_path=-` and `expected_prefixes_suppressions`), since disabling the naming *policy* should not disable a correctness check. There is a regression test per opt-out.

3. **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 = "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.

Also, for whatever it's 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.

## Testing

`src/google/protobuf/compiler/php/generator_unittest.cc` — `InvalidPhpClassPrefixRejected`, `ValidPhpClassPrefixAccepted`.

`src/google/protobuf/compiler/objectivec/generator_unittest.cc` (new file, plus its `cc_test` target) — `InvalidObjCClassPrefixRejected`, `InvalidObjCClassPrefixRejectedWithExpectedPrefixesDisabled`, `InvalidObjCClassPrefixRejectedWhenSuppressed`, `ValidObjCClassPrefixAccepted`, `ObjCClassPrefixWithUnderscoreAccepted`.

The two opt-out tests fail with the character check in its original location inside `ValidateObjCClassPrefix()`, and pass with it moved.

Both directions are covered, so the tests fail if the validation is removed *and* if it is made too strict.

---

_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._

Closes #29130

COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/29130 from BarakSrour:validate-class-prefix-options a2bbd8f2ea53f1b9958c8d8b310d28277d4979a7
PiperOrigin-RevId: 966161028