fix: feat: add php_server's env vars to sandboxed environment (#2451)

closes #1674

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

---------

Signed-off-by: Marc <m@pyc.ac>
Co-authored-by: Alliballibaba <alliballibaba@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: 3 people

cgi.go

@@ -4,10 +4,12 @@ package frankenphp
 // #cgo nocallback frankenphp_register_variable_safe
 // #cgo nocallback frankenphp_register_known_variable
 // #cgo nocallback frankenphp_init_persistent_string
+// #cgo nocallback frankenphp_add_to_prepared_env
 // #cgo noescape frankenphp_register_server_vars
 // #cgo noescape frankenphp_register_variable_safe
 // #cgo noescape frankenphp_register_known_variable
 // #cgo noescape frankenphp_init_persistent_string
+// #cgo noescape frankenphp_add_to_prepared_env
 // #include "frankenphp.h"
 // #include <php_variables.h>
 import "C"
@@ -162,11 +164,12 @@ func addHeadersToServer(ctx context.Context, request *http.Request, trackVarsArr
 	}
 }
 
-func addPreparedEnvToServer(fc *frankenPHPContext, trackVarsArray *C.zval) {
-	for k, v := range fc.env {
-		C.frankenphp_register_variable_safe(toUnsafeChar(k), toUnsafeChar(v), C.size_t(len(v)), trackVarsArray)
+// registerPreparedEnv exposes fc.env to getenv() before any PHP code runs.
+func registerPreparedEnv(env PreparedEnv) {
+	size := C.size_t(len(env))
+	for k, v := range env {
+		C.frankenphp_add_to_prepared_env(toUnsafeChar(k), C.size_t(len(k)-1), toUnsafeChar(v), C.size_t(len(v)), size)
 	}
-	fc.env = nil
 }
 
 //export go_register_server_variables
@@ -180,7 +183,9 @@ func go_register_server_variables(threadIndex C.uintptr_t, trackVarsArray *C.zva
 	}
 
 	// The Prepared Environment is registered last and can overwrite any previous values
-	addPreparedEnvToServer(fc, trackVarsArray)
+	if len(fc.env) != 0 {
+		C.frankenphp_merge_with_prepared_env(trackVarsArray)
+	}
 }
 
 // splitCgiPath splits the request path into SCRIPT_NAME, SCRIPT_FILENAME, PATH_INFO, DOCUMENT_URI
@@ -287,6 +292,10 @@ func go_update_request_info(threadIndex C.uintptr_t, info *C.sapi_request_info)
 		return nil
 	}
 
+	if len(fc.env) != 0 {
+		registerPreparedEnv(fc.env)
+	}
+
 	if m, ok := cStringHTTPMethods[request.Method]; ok {
 		info.request_method = m
 	} else {

frankenphp.c

@@ -101,6 +101,10 @@ HashTable *main_thread_env = NULL;
 __thread uintptr_t thread_index;
 __thread bool is_worker_thread = false;
 __thread HashTable *sandboxed_env = NULL;
+/* prepared_env holds entries from php(_server)'s `env KEY VAL`, exposed to
+ * getenv() and merged into $_ENV when 'E' is in variables_order. Separate from
+ * putenv() so those don't leak into $_ENV. */
+__thread HashTable *prepared_env = NULL;
 
 /* Published via SG(server_context) so ext-parallel children, which inherit
  * the parent's SG(server_context), can route SAPI callbacks back to the
@@ -519,9 +523,17 @@ bool frankenphp_shutdown_dummy_request(void) {
 }
 
 void get_full_env(zval *track_vars_array) {
-  zend_hash_extend(Z_ARR_P(track_vars_array),
-                   zend_hash_num_elements(main_thread_env), 0);
+  size_t total = zend_hash_num_elements(main_thread_env);
+  if (prepared_env != NULL) {
+    // perf: doesn't matter if we get the exact count, just >= needed
+    total += zend_hash_num_elements(prepared_env);
+  }
+  zend_hash_extend(Z_ARR_P(track_vars_array), total, 0);
   zend_hash_copy(Z_ARR_P(track_vars_array), main_thread_env, NULL);
+  if (prepared_env != NULL) {
+    zend_hash_copy(Z_ARR_P(track_vars_array), prepared_env,
+                   (copy_ctor_func_t)zval_add_ref);
+  }
 }
 
 /* Adapted from php_request_startup() */
@@ -626,6 +638,13 @@ PHP_FUNCTION(frankenphp_putenv) {
 
   if (sandboxed_env == NULL) {
     sandboxed_env = zend_array_dup(main_thread_env);
+    /* prepared_env overrides the OS env and putenv() overrides both, so layer
+     * the prepared vars onto the dup before sandboxed_env starts shadowing the
+     * other two layers in getenv(). */
+    if (prepared_env != NULL) {
+      zend_hash_copy(sandboxed_env, prepared_env,
+                     (copy_ctor_func_t)zval_add_ref);
+    }
   }
 
   /* cut at null byte to stay consistent with regular putenv */
@@ -661,6 +680,38 @@ PHP_FUNCTION(frankenphp_putenv) {
   RETURN_BOOL(success);
 } /* }}} */
 
+/* getenv() lookup: sandboxed_env if present (it already holds prepared + OS),
+ * otherwise prepared_env then main_thread_env. */
+static inline zval *frankenphp_lookup_env(const char *name, size_t name_len) {
+  if (sandboxed_env != NULL) {
+    return zend_hash_str_find(sandboxed_env, name, name_len);
+  }
+
+  zval *env_val = NULL;
+  if (prepared_env != NULL) {
+    env_val = zend_hash_str_find(prepared_env, name, name_len);
+  }
+  if (env_val == NULL) {
+    env_val = zend_hash_str_find(main_thread_env, name, name_len);
+  }
+
+  return env_val;
+}
+
+/* Returns a fresh copy of the full environment, merging the layers above. */
+static inline HashTable *frankenphp_dup_env(void) {
+  if (sandboxed_env != NULL) {
+    return zend_array_dup(sandboxed_env);
+  }
+
+  HashTable *env = zend_array_dup(main_thread_env);
+  if (prepared_env != NULL) {
+    zend_hash_copy(env, prepared_env, (copy_ctor_func_t)zval_add_ref);
+  }
+
+  return env;
+}
+
 /* {{{ Get the env from the sandboxed environment */
 PHP_FUNCTION(frankenphp_getenv) {
   zend_string *name = NULL;
@@ -672,14 +723,12 @@ PHP_FUNCTION(frankenphp_getenv) {
   Z_PARAM_BOOL(local_only)
   ZEND_PARSE_PARAMETERS_END();
 
-  HashTable *ht = sandboxed_env ? sandboxed_env : main_thread_env;
-
   if (!name) {
-    RETURN_ARR(zend_array_dup(ht));
+    RETURN_ARR(frankenphp_dup_env());
     return;
   }
 
-  zval *env_val = zend_hash_find(ht, name);
+  zval *env_val = frankenphp_lookup_env(ZSTR_VAL(name), ZSTR_LEN(name));
   if (env_val && Z_TYPE_P(env_val) == IS_STRING) {
     zend_string *str = Z_STR_P(env_val);
     zend_string_addref(str);
@@ -1203,6 +1252,13 @@ void frankenphp_register_server_vars(zval *track_vars_array,
   zend_hash_update_ind(ht, frankenphp_strings.remote_ident, &zv);
 }
 
+void frankenphp_merge_with_prepared_env(zval *track_vars_array) {
+  if (prepared_env != NULL) {
+    HashTable *ht = Z_ARRVAL_P(track_vars_array);
+    zend_hash_copy(ht, prepared_env, (copy_ctor_func_t)zval_add_ref);
+  }
+}
+
 /** Create an immutable zend_string that lasts for the whole process **/
 zend_string *frankenphp_init_persistent_string(const char *string, size_t len) {
   /* persistent strings will be ignored by the GC at the end of a request */
@@ -1316,9 +1372,7 @@ static void frankenphp_log_message(const char *message, int syslog_type_int) {
 }
 
 static char *frankenphp_getenv(const char *name, size_t name_len) {
-  HashTable *ht = sandboxed_env ? sandboxed_env : main_thread_env;
-
-  zval *env_val = zend_hash_str_find(ht, name, name_len);
+  zval *env_val = frankenphp_lookup_env(name, name_len);
   if (env_val && Z_TYPE_P(env_val) == IS_STRING) {
     zend_string *str = Z_STR_P(env_val);
     return ZSTR_VAL(str);
@@ -1381,6 +1435,22 @@ static inline void reset_sandboxed_environment() {
     zend_hash_release(sandboxed_env);
     sandboxed_env = NULL;
   }
+  if (prepared_env != NULL) {
+    zend_hash_release(prepared_env);
+    prepared_env = NULL;
+  }
+}
+
+/* Adds a key/value pair to the per-thread prepared environment, exposing
+ *  env vars from the php(_server) directive to getenv() and $_ENV. */
+void frankenphp_add_to_prepared_env(char *name, size_t name_len, char *val,
+                                    size_t val_len, size_t size) {
+  if (prepared_env == NULL) {
+    prepared_env = zend_new_array(size);
+  }
+  zval zv = {0};
+  ZVAL_STRINGL(&zv, val, val_len);
+  zend_hash_str_update(prepared_env, name, name_len, &zv);
 }
 
 static void *php_thread(void *arg) {

frankenphp.h

@@ -202,6 +202,9 @@ void frankenphp_register_variable_safe(char *key, char *var, size_t val_len,
                                        zval *track_vars_array);
 void frankenphp_register_server_vars(zval *track_vars_array,
                                      frankenphp_server_vars vars);
+void frankenphp_add_to_prepared_env(char *name, size_t name_len, char *val,
+                                    size_t val_len, size_t size);
+void frankenphp_merge_with_prepared_env(zval *track_vars_array);
 
 zend_string *frankenphp_init_persistent_string(const char *string, size_t len);
 int frankenphp_get_current_memory_limit();

frankenphp_test.go

@@ -767,6 +767,69 @@ func TestEnvIsNotResetInWorkerMode(t *testing.T) {
 	}, &testOptions{workerScript: "env/remember-env.php"})
 }
 
+// reproduction of https://github.com/php/frankenphp/issues/1674
+func TestPreparedEnvIsVisibleToGetenv_module(t *testing.T) {
+	testPreparedEnvIsVisibleToGetenv(t, &testOptions{nbParallelRequests: 1})
+}
+func TestPreparedEnvIsVisibleToGetenv_worker(t *testing.T) {
+	testPreparedEnvIsVisibleToGetenv(t, &testOptions{
+		workerScript: "env/prepared-env-getenv.php",
+	})
+}
+func testPreparedEnvIsVisibleToGetenv(t *testing.T, opts *testOptions) {
+	if opts.phpIni == nil {
+		opts.phpIni = map[string]string{}
+	}
+	opts.phpIni["variables_order"] = "EGPCS"
+	opts.requestOpts = append(opts.requestOpts,
+		frankenphp.WithRequestEnv(map[string]string{"FRANKENPHP_TEST_PHP_SERVER_ENV_IN_GETENV": "hello"}),
+	)
+
+	expectedEnv := "'hello'"
+	if opts.workerScript != "" {
+		// workers don't populate $_ENV regardless of variables_order
+		expectedEnv = "NULL"
+	}
+
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
+		body, _ := testGet("http://example.com/env/prepared-env-getenv.php", handler, t)
+		assert.Equal(t, fmt.Sprintf("getenv='hello'\nserver='hello'\nenv=%s\n", expectedEnv), body)
+	}, opts)
+}
+
+// $_ENV mustn't be filled with prepared_env without E in variables_order
+func TestPreparedEnvIsNotInEnvWithoutVariablesOrderE(t *testing.T) {
+	opts := &testOptions{
+		nbParallelRequests: 1,
+		phpIni:             map[string]string{"variables_order": "GPCS"},
+	}
+	opts.requestOpts = append(opts.requestOpts,
+		frankenphp.WithRequestEnv(map[string]string{"FRANKENPHP_TEST_PHP_SERVER_ENV_IN_GETENV": "hello"}),
+	)
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
+		body, _ := testGet("http://example.com/env/prepared-env-getenv.php", handler, t)
+		assert.Equal(t, "getenv='hello'\nserver='hello'\nenv=NULL\n", body)
+	}, opts)
+}
+
+func TestPreparedEnvSurvivesPutenv_module(t *testing.T) {
+	testPreparedEnvSurvivesPutenv(t, &testOptions{nbParallelRequests: 1})
+}
+func TestPreparedEnvSurvivesPutenv_worker(t *testing.T) {
+	testPreparedEnvSurvivesPutenv(t, &testOptions{
+		workerScript: "env/prepared-env-survives-putenv.php",
+	})
+}
+func testPreparedEnvSurvivesPutenv(t *testing.T, opts *testOptions) {
+	opts.requestOpts = append(opts.requestOpts,
+		frankenphp.WithRequestEnv(map[string]string{"FRANKENPHP_PREPARED": "prepared_value"}),
+	)
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
+		body, _ := testGet("http://example.com/env/prepared-env-survives-putenv.php", handler, t)
+		assert.Equal(t, "before='prepared_value'\nprepared='prepared_value'\nput='put_value'\n", body)
+	}, opts)
+}
+
 // reproduction of https://github.com/php/frankenphp/issues/1061
 func TestModificationsToEnvPersistAcrossRequests(t *testing.T) {
 	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) {

requestoptions.go

@@ -130,13 +130,38 @@ func WithRequestEnv(env map[string]string) RequestOption {
 }
 
 func WithRequestPreparedEnv(env PreparedEnv) RequestOption {
+	env = ensurePreparedEnv(env)
+
 	return func(o *frankenPHPContext) error {
 		o.env = env
 
 		return nil
 	}
 }
 
+// ensurePreparedEnv ensures every key is NUL-terminated
+// Empty keys are dropped
+func ensurePreparedEnv(env PreparedEnv) PreparedEnv {
+	for k := range env {
+		if k == "" || k[len(k)-1] != '\x00' {
+			fixed := make(PreparedEnv, len(env))
+			for k, v := range env {
+				if k == "" {
+					continue
+				}
+				if k[len(k)-1] != '\x00' {
+					k += "\x00"
+				}
+				fixed[k] = v
+			}
+
+			return fixed
+		}
+	}
+
+	return env
+}
+
 func WithOriginalRequest(r *http.Request) RequestOption {
 	return func(o *frankenPHPContext) error {
 		o.originalRequest = r

testdata/env/prepared-env-getenv.php

@@ -0,0 +1,12 @@
+<?php
+
+require_once __DIR__ . '/../_executor.php';
+
+return function () {
+    // Variables declared via the env subdirective in the php(_server) directive
+    // (or via WithRequestEnv) must be exposed to both $_SERVER and getenv().
+    // See https://github.com/php/frankenphp/issues/1674
+    echo "getenv=" . var_export(getenv('FRANKENPHP_TEST_PHP_SERVER_ENV_IN_GETENV'), true) . "\n";
+    echo "server=" . var_export($_SERVER['FRANKENPHP_TEST_PHP_SERVER_ENV_IN_GETENV'] ?? null, true) . "\n";
+    echo "env=" . var_export($_ENV['FRANKENPHP_TEST_PHP_SERVER_ENV_IN_GETENV'] ?? null, true) . "\n";
+};

testdata/env/prepared-env-survives-putenv.php

@@ -0,0 +1,10 @@
+<?php
+
+require_once __DIR__ . '/../_executor.php';
+
+return function () {
+    echo "before=" . var_export(getenv('FRANKENPHP_PREPARED'), true) . "\n";
+    putenv('FRANKENPHP_PUT=put_value');
+    echo "prepared=" . var_export(getenv('FRANKENPHP_PREPARED'), true) . "\n";
+    echo "put=" . var_export(getenv('FRANKENPHP_PUT'), true) . "\n";
+};