fix: feat: add php_server's env vars to sandboxed environment

[henderkes]

closes #1674

was looking through old issues and found this one that already had a plan to solve it

[AlliBalliBaba]

Tbh the name 'prepared_env' has always confused me. "request_specific_env", "request_local_env", "request_context_env", "request_env" or something similar would make more sense IMO

[henderkes]

Tbh the name 'prepared_env' has always confused me. "request_specific_env", "request_local_env", "request_context_env", "request_env" or something similar would make more sense IMO

Prepared as in prepared by the php(_server) directive. I'm open to suggestions, but request_ is incorrect because it's not scoped to requests.

[AlliBalliBaba]

It is kind of scoped to the request since you can add request-specific caddy replacers to it. We currently lack an equivalent of the php_server scope from the caddy side in the frankenphp library side, but adding one would probably be too much for this PR.

[henderkes]

Hmm that's true, but it would be a bit confusing to call it request_env as that could be understood to be the sandboxed one.

Perhaps caddy_handler_env? Or just live with prepared_env?

[AlliBalliBaba]

Hmm yeah maybe let's keep it for now. It could be scoped_env if we ever add something like the request scoping from #2398

[AlliBalliBaba]

Looks like there's currently some race conditions in tests between restarts. Not yet sure when they were introduced (not this PR)

[henderkes]

Yeah that was already fixed by @alexandre-daubois by a rw lock.

[henderkes]

@dunglas should be ready for review+merge now.

[Copilot]

Pull request overview

This PR addresses #1674 by ensuring environment variables set via the env subdirective (and WithRequestEnv) are available through getenv() in addition to $_SERVER, by introducing a distinct “prepared environment” layer that is merged appropriately.

Changes:

• Add a per-thread prepared_env layer in the C runtime and update getenv/$_ENV population logic to include it.
• Update Go ↔ C integration to register “prepared env” early (before PHP userland runs) and merge it into $_SERVER.
• Add regression tests and test scripts covering getenv() visibility, variables_order behavior, and interaction with putenv().

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
testdata/env/prepared-env-survives-putenv.php	Adds a regression script ensuring prepared env remains visible after putenv().
testdata/env/prepared-env-getenv.php	Adds a regression script asserting prepared env is visible via getenv() and $_SERVER (and conditionally $_ENV).
frankenphp.h	Exposes new C APIs for prepared env registration/merge.
frankenphp.c	Implements prepared_env layer and updates getenv/$_ENV merging behavior.
frankenphp_test.go	Adds Go tests reproducing #1674 and verifying layering/variables_order behavior.
cgi.go	Registers prepared env earlier (for getenv()), and merges it into $_SERVER later.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[henderkes]

Technically right, this could be an issue if someone used frankenphp as a library incorrectly. But for the Caddy module it isn't actually a problem, otherwise we'd have been bitten by it in the sandbox already.

Will safety-fix in both places.

[henderkes]

Negative: there's no public API to ever pass non-NULL terminated strings here. The only public API surface is WithRequestEnv() which calls:

func PrepareEnv(env map[string]string) PreparedEnv {
	preparedEnv := make(PreparedEnv, len(env))
	for k, v := range env {
		preparedEnv[k+"\x00"] = v
	}

	return preparedEnv
}

It's not worth adding extra checks that can never happen to a hot path happening in every single request.