Add fallback for allowedHostnames environment variable

[objecttothis]

Bug: getenv('app.allowedHostnames') returns false intermittently even though .env has it set.

Key insight from DotEnv.php line 97:

if (getenv($name, true) === false) {
putenv("{$name}={$value}");
}

Guard skips putenv if value already exists. But App.php line 292 uses getenv('app.allowedHostnames') without true flag. Different lookup path.

More importantly — App.php reads getenv() but never checks $_ENV or $_SERVER. DotEnv always sets those. So fix: make App.php check $_ENV/$_SERVER too.

Let me check one more thing to confirm the intermittency source.

Run Command
Confirm getenv misses $_ENV/$_SERVER values
Confirmed. When putenv is skipped (guard on line 97), getenv() returns false — even though $_ENV and $_SERVER have value.

Fix: App.php constructor should check $_ENV/$_SERVER as fallbacks, not rely solely on getenv().

Now writing plan.

Write File
i-got-this-error-drifting-riddle.md
CONTENT

Fix: allowedHostnames intermittently not read from .env

Context

Error log shows allowedHostnames is not configured even though .env has app.allowedHostnames = 'localhost'. Happens intermittently.

Root cause: CI4's DotEnv setVariable() has a guard (line 97) that skips putenv() if getenv($name, true) already returns a value. When putenv is skipped, getenv() (without true) returns false — it can't see values only in $_ENV/$_SERVER. But DotEnv always sets $_ENV and $_SERVER.

App.php constructor (line 290-292) only checks getenv(), never $_ENV/$_SERVER. So when the putenv guard triggers, getenv('app.allowedHostnames') returns false and allowedHostnames stays empty.

Fix

File: app/Config/App.php (lines 290-293)

Change the env reading to also check $_ENV and $_SERVER as fallbacks, matching how CI4's own DotEnv stores values.

Replace:

$envAllowedHostnames = getenv('ALLOWED_HOSTNAMES');
if ($envAllowedHostnames === false || trim($envAllowedHostnames) === '') {
    $envAllowedHostnames = getenv('app.allowedHostnames');
}

With:

$envAllowedHostnames = getenv('ALLOWED_HOSTNAMES');
if ($envAllowedHostnames === false || trim($envAllowedHostnames) === '') {
    $envAllowedHostnames = $_ENV['ALLOWED_HOSTNAMES'] ?? $_SERVER['ALLOWED_HOSTNAMES'] ?? false;
}
if ($envAllowedHostnames === false || trim($envAllowedHostnames) === '') {
    $envAllowedHostnames = getenv('app.allowedHostnames');
}
if ($envAllowedHostnames === false || trim($envAllowedHostnames) === '') {
    $envAllowedHostnames = $_ENV['app.allowedHostnames'] ?? $_SERVER['app.allowedHostnames'] ?? false;
}

Verification

1. Run existing tests: vendor/bin/phpunit tests/Config/AppTest.php
2. Verify error no longer appears in log on page load
   Tool Search
   QUERY
   select:ExitPlanMode
   MAX_RESULTS
   1
   Root cause confirmed: getenv() returns false when putenv was skipped by DotEnv's guard, even though $_ENV/$_SERVER have the value. Intermittent because it depends on whether a previous request in same PHP worker already called putenv for that key.

Fix: check $_ENV/$_SERVER as fallbacks in App.php constructor.

Summary by CodeRabbit

• Bug Fixes
  • More robust parsing and normalization of hostname environment values, including proper handling of empty, falsey, or whitespace-only inputs and multi-source environment lookups.
• Tests
  • Expanded test coverage for hostname/env parsing and production-environment behavior to prevent regressions.
• Documentation
  • Minor formatting updates to hostname configuration docs.

[coderabbitai]

📝 Walkthrough

Walkthrough

App config now reads allowed hostnames through a centralized getEnvString helper (checking env(), $_ENV, $_SERVER, getenv) and prefers ALLOWED_HOSTNAMES over app.allowedHostnames, parsing CSVs and treating blank/whitespace as unset. Tests are tightened to isolate and validate these cases, including production env behavior.

Changes

Allowed Hostnames Configuration

Layer / File(s)	Summary
Environment variable precedence logic app/Config/App.php	Introduces getEnvString(string $key): ?string and updates App::__construct() to obtain ALLOWED_HOSTNAMES (or fallback app.allowedHostnames) via that helper, parsing comma-separated hostnames and ignoring blank/whitespace-only values.
Test environment setup and case adjustments tests/Config/AppTest.php	setUp()/tearDown() now clear ALLOWED_HOSTNAMES / app.allowedHostnames across putenv, $_ENV, and $_SERVER; production test sets $_SERVER['CI_ENVIRONMENT']='production'; new tests added for literal false, null, and whitespace-only inputs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• [Bug]: .env app.allowedHostNames does not work as intended #4480: Both PRs modify handling of app.allowedHostnames and environment-sourced whitelist logic.

Possibly related PRs

• opensourcepos/opensourcepos#4433: Modifies app/Config/App.php around allowed hostnames initialization and validation.
• opensourcepos/opensourcepos#4482: Also updates env parsing for ALLOWED_HOSTNAMES/app.allowedHostnames and related tests.
• opensourcepos/opensourcepos#4544: Prioritizes and parses ALLOWED_HOSTNAMES with corresponding test updates.

Poem

🐰 I nibble env strings in the night,
ALLOWED_HOSTNAMES first in sight,
commas trimmed, blanks swept away,
tests tidy the burrow for day,
hop—config neat—now all is right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title directly and clearly summarizes the main change: adding a fallback mechanism for the allowedHostnames environment variable handling.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bugfix-allowedhostnames-fix

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

app/Config/App.php (1)

359-373: ⚡ Quick win

Document why the second lookup is load-bearing (not redundant).

In CodeIgniter 4, env() reads $_ENV[$key] ?? $_SERVER[$key] ?? getenv($key) and then converts the magic words 'true'/'false'/'empty'/'null' via match(strtolower($value)) (returning booleans, '', or null). Since getEnvString() only returns non-empty strings, literal values like "false"/"null" would otherwise be type-juggled away—so the raw fallback must stay to preserve the original literal string.

📝 Suggested clarifying comment

     private function getEnvString(string $key): ?string
     {
+        // env() type-juggles the magic words "true"/"false"/"empty"/"null"
+        // into non-string values, so when it returns a non-string we re-read
+        // the raw value to preserve a legitimate literal hostname string.
         $value = env($key);

         if (is_string($value) && trim($value) !== '') {
             return $value;
         }

         $raw = $_ENV[$key] ?? $_SERVER[$key] ?? getenv($key);
         if (is_string($raw) && trim($raw) !== '') {
             return $raw;
         }

         return null;
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Config/App.php` around lines 359 - 373, Add a clarifying comment inside
the getEnvString method explaining that the second/raw lookup (using $_ENV[$key]
?? $_SERVER[$key] ?? getenv($key)) is intentionally required because env()
normalizes magic words (e.g., "true"/"false"/"empty"/"null") into booleans,
empty string or null, which would drop literal string values like "false" or
"null"; the raw lookup preserves the original literal string for return when
getEnvString must only return non-empty strings. This comment should be placed
immediately before the $raw = $_ENV... line in getEnvString to document why the
fallback is not redundant.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@app/Config/App.php`:
- Around line 359-373: Add a clarifying comment inside the getEnvString method
explaining that the second/raw lookup (using $_ENV[$key] ?? $_SERVER[$key] ??
getenv($key)) is intentionally required because env() normalizes magic words
(e.g., "true"/"false"/"empty"/"null") into booleans, empty string or null, which
would drop literal string values like "false" or "null"; the raw lookup
preserves the original literal string for return when getEnvString must only
return non-empty strings. This comment should be placed immediately before the
$raw = $_ENV... line in getEnvString to document why the fallback is not
redundant.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f0ae257b-05b9-4f1b-b2e1-611aebd67150

📥 Commits

Reviewing files that changed from the base of the PR and between fb99861 and a5fa3b9.

📒 Files selected for processing (2)

• app/Config/App.php
• tests/Config/AppTest.php