fix multiple issues found during pre-release review#154
fix multiple issues found during pre-release review#154mambax7 merged 5 commits intoXOOPS:masterfrom
Conversation
- Yaml dumpWrapped: use exit guard instead of PHP comment to prevent
code execution when content contains closing comment sequence
- JWT JsonWebToken: use strict comparison for claim validation
- JWT TokenReader: validate Bearer scheme on Authorization header only;
custom headers still accept bare tokens for backward compatibility
- Metagen purifyText: add explicit encoding, fix newline replacement
- SendmailRunner: close stdin immediately on empty payload to prevent hang
- TableLoad: escape backticks in column names
- FilterInput WEBURL: reject protocol-relative URLs
- Tables quoteDefaultClause: escape single quotes in DDL default values
- Replace header('HTTP/1.0 404 Not Found') with http_response_code(404)
in all directory index guards
- Update CHANGELOG for 1.3.0-RC1
- Update phpstan-baseline.neon for changed error signature
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
404 Response Standardization index.php, src/index.php, src/Database/index.php, src/Jwt/index.php, src/Key/index.php, src/Module/Helper/index.php, src/Module/index.php |
Replace raw header('HTTP/1.0 404 Not Found') with http_response_code(404) and add exit; to immediately terminate execution. |
JWT Security & Header Parsing src/Jwt/JsonWebToken.php, src/Jwt/TokenReader.php, tests/unit/Jwt/JsonWebTokenTest.php, tests/unit/Jwt/TokenReaderTest.php |
Use strict claim comparison (!==) in JWT decode; require Authorization: Bearer <token> for the Authorization header while allowing bare tokens on custom headers; add/adjust unit tests for these behaviors. |
YAML Wrapper Format & Tests src/Yaml.php, tests/unit/YamlTest.php |
Change dumpWrapped() to emit <?php __halt_compiler(); ?> guard; loader now supports both __halt_compiler() and legacy <?php /* ... */ ?> wrappers and extracts YAML between ---/...; add tests for guards, edge cases, and round trips. |
SQL Identifier / DDL Escaping & Tests src/Database/TableLoad.php, src/Database/Tables.php, tests/unit/Database/TablesTest.php |
Escape embedded backticks in column identifiers (\`` → ````) and escape single quotes in DDL DEFAULT values ('→''`); add tests for DEFAULT quoting and CURRENT_TIMESTAMP handling. |
Input Validation & Tests src/FilterInput.php, tests/unit/FilterInputTest.php |
Trim WEBURL input, explicitly reject protocol-relative URLs (leading //), guard scheme checks by verifying parse_url() returns an array, and add tests for protocol-relative, http, relative, whitespace-prefixed, and javascript: inputs. |
Sendmail I/O Robustness & Tests src/Mail/SendmailRunner.php, tests/unit/Mail/SendmailRunnerTest.php |
When RFC‑822 payload is empty, close stdin early (fclose) and mark stdin closed to avoid write/hang loops; add POSIX-only test ensuring empty payload does not hang. |
Safe Gzip Decompression & Exception src/Security/Serializer.php, src/Security/DecompressionException.php |
Replace single-call gzdecode() with streaming safeGzipDecode() using inflate_init/inflate_add, chunked decompression with size enforcement during inflate, finalization checks, and throw new DecompressionException on failure or oversized output. |
Text Purification Fixes & Tests src/Metagen.php, tests/unit/MetagenTest.php |
Use html_entity_decode($text, ENT_QUOTES, self::ENCODING) and replace actual \r and \n characters (["\r","\n"]) when normalizing newlines; add tests for newline removal and encoding-aware entity decoding. |
Static Analysis / Config & Changelog phpstan-baseline.neon, .coderabbit.yaml, CHANGELOG.md |
Adjust phpstan baseline ignore entries (refine/remove some ignores and change types/counts), remove reviews.tools block from .coderabbit.yaml, and add 1.3.0-RC1 changelog entry documenting changes. |
Tests: General Additions tests/unit/... (multiple files) |
Add/extend unit tests across JWT, TokenReader, YAML, Database/Tables, FilterInput, SendmailRunner, Metagen to cover new behaviors and edge cases. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- Feature/update copyright #144 — Overlapping changes touching SendmailRunner, Database/Tables, and YAML helpers; likely related at code-level for I/O and serialization adjustments.
Poem
🐰 I nibbled bugs and stitched each stitch,
Tokens tightened, YAML found a switch.
Quotes escaped, streams now mind their size,
404s exit clean — no surprise.
RC1 hops forth with tests and bright eyes. ✨
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the overall intent of the changeset—fixing multiple issues found during pre-release review. The PR includes security hardening (JWT strict comparison, YAML exit guard, URL validation), bug fixes (SendmailRunner hang, HTML entity decoding, newline replacement, SQL escaping), and standardization (404 response handling). |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 PHPStan (2.1.46)
PHPStan was skipped because the config uses disallowed bootstrapFiles, bootstrapFile, or includes directives.
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #154 +/- ##
=============================================
+ Coverage 33.23% 43.51% +10.27%
- Complexity 1058 1178 +120
=============================================
Files 45 47 +2
Lines 2750 2999 +249
=============================================
+ Hits 914 1305 +391
+ Misses 1836 1694 -142 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR applies a set of pre-release hardening fixes across XMF, primarily targeting security edge-cases (YAML wrapping, JWT validation, URL filtering, SQL identifier/value escaping) plus a few operational and consistency improvements (sendmail delivery hang, 404 index guards, baseline/changelog updates).
Changes:
- Harden wrapped YAML output (
dumpWrapped) by switching to a PHPexitguard, and tighten JWT handling (strict claim comparison + Bearer parsing forAuthorization). - Fix/strengthen sanitization and SQL construction (WEBURL protocol-relative rejection, backtick escaping for column names, escaping quotes in DDL default values).
- Operational/maintenance updates: prevent sendmail hangs on empty payload, standardize directory index guards to
http_response_code(404); exit;, and update changelog + PHPStan baseline.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Yaml.php | Changes wrapped YAML format to use <?php exit; ?> guard. |
| src/Jwt/JsonWebToken.php | Uses strict comparison for asserted claim validation. |
| src/Jwt/TokenReader.php | Requires Bearer scheme for Authorization header parsing. |
| src/Metagen.php | Fixes encoding passed to html_entity_decode() and corrects newline replacement. |
| src/Mail/SendmailRunner.php | Closes stdin immediately for empty payloads to prevent process hangs. |
| src/FilterInput.php | Rejects protocol-relative URLs in WEBURL cleaning. |
| src/Database/Tables.php | Escapes single quotes in DDL default values. |
| src/Database/TableLoad.php | Escapes backticks in column identifiers during inserts. |
| index.php | Standardizes root index guard to http_response_code(404); exit;. |
| src/index.php | Standardizes src index guard to http_response_code(404); exit;. |
| src/Database/index.php | Standardizes directory index guard to http_response_code(404); exit;. |
| src/Jwt/index.php | Standardizes directory index guard to http_response_code(404); exit;. |
| src/Key/index.php | Standardizes directory index guard to http_response_code(404); exit;. |
| src/Module/index.php | Standardizes directory index guard to http_response_code(404); exit;. |
| src/Module/Helper/index.php | Standardizes directory index guard to http_response_code(404); exit;. |
| phpstan-baseline.neon | Updates baseline patterns/counts to match new/static-analysis findings. |
| CHANGELOG.md | Documents 1.3.0-RC1 changes. |
Comments suppressed due to low confidence (3)
src/FilterInput.php:281
parse_url('')can returnfalse. After setting$result = ''for protocol-relative URLs,$urlpartsmay befalse, and accessing$urlparts['scheme']will emit warnings. Consider short-circuiting when$result === ''or normalizing with$urlparts = parse_url($result) ?: [];before reading offsets.
// reject protocol-relative URLs (//evil.example) and non-http(s) schemes
if (str_starts_with((string) $result, '//')) {
$result = '';
}
$urlparts = parse_url($result);
if (
!empty($urlparts['scheme'])
&& !($urlparts['scheme'] === 'http' || $urlparts['scheme'] === 'https')
) {
src/FilterInput.php:288
- Add a unit test covering the new
WEBURLbehavior (reject//evil.example), including the empty-string path, to prevent regressions and ensure no warnings are raised when the URL is invalid/blank.
case 'WEBURL':
$result = (string) $this->process($source);
// reject protocol-relative URLs (//evil.example) and non-http(s) schemes
if (str_starts_with((string) $result, '//')) {
$result = '';
}
$urlparts = parse_url($result);
if (
!empty($urlparts['scheme'])
&& !($urlparts['scheme'] === 'http' || $urlparts['scheme'] === 'https')
) {
$result = '';
}
// do not allow quotes, tag brackets or controls
if (!preg_match('#^[^"<>\x00-\x1F]+$#', $result)) {
$result = '';
}
break;
src/Yaml.php:162
- Add/adjust a unit test to cover the security case that motivated this change (YAML content containing the
*/sequence). The test should assert thatdumpWrapped()output cannot be turned into executable PHP by closing a comment early, and thatloadWrapped()still round-trips correctly.
public static function dumpWrapped($var, $inline = 4, $indent = 4)
{
try {
$yamlString = VendorYaml::dump($var, $inline, $indent);
$ret = empty($yamlString) ? false : "<?php exit; ?>\n---\n" . $yamlString . "\n...\n";
} catch (\Throwable $e) {
static::logError($e);
$ret = false;
}
return $ret;
| @@ -155,7 +154,7 @@ public static function dumpWrapped($var, $inline = 4, $indent = 4) | |||
| { | |||
| try { | |||
| $yamlString = VendorYaml::dump($var, $inline, $indent); | |||
| $ret = empty($yamlString) ? false : "<?php\n/*\n---\n" . $yamlString . "\n...\n*/\n"; | |||
| $ret = empty($yamlString) ? false : "<?php exit; ?>\n---\n" . $yamlString . "\n...\n"; | |||
| } catch (\Throwable $e) { | |||
There was a problem hiding this comment.
The wrapper format changed to an exit guard, but the loadWrapped()/readWrapped() docblocks below still describe the old /* ... */ comment wrapper. To keep the public API docs consistent, update the surrounding documentation to describe both supported wrappers (old comment wrapper + new exit guard).
| public static function fromHeader($key, $assertClaims = array(), $headerName = 'Authorization') | ||
| { | ||
| $header = Request::getHeader($headerName, ''); | ||
| if (empty($header)) { | ||
| return false; | ||
| } | ||
| $header = trim($header); | ||
| $space = strpos($header, ' '); // expecting "Bearer base64-token-string" | ||
| if (false !== $space) { | ||
| $header = substr($header, $space); | ||
| if (strcasecmp($headerName, 'Authorization') === 0) { | ||
| $parts = explode(' ', $header, 2); | ||
| if (count($parts) !== 2 || strcasecmp($parts[0], 'Bearer') !== 0) { | ||
| return false; | ||
| } | ||
| $token = trim($parts[1]); | ||
| } else { | ||
| $token = $header; | ||
| } | ||
| $token = trim($header); | ||
| return static::fromString($key, $token, $assertClaims); |
There was a problem hiding this comment.
TokenReader::fromHeader() behavior changed to require the Bearer scheme for Authorization. There is an existing TokenReaderTest::testFromHeader() placeholder marked incomplete; it should be implemented to cover valid/invalid Authorization headers (missing scheme, wrong scheme, extra whitespace) and to verify custom headers still accept bare tokens for backward compatibility.
| foreach ($assertClaims as $claim => $assert) { | ||
| if (!property_exists($values, $claim)) { | ||
| return false; | ||
| } | ||
|
|
||
| if ($values->$claim != $assert) { | ||
| if ($values->$claim !== $assert) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a unit test demonstrating the security fix for strict claim comparison (e.g., asserting 0 should not match '0', false should not match 0, etc.). This will prevent accidental reintroduction of loose comparisons in future refactors.
| @@ -416,7 +416,7 @@ protected static function purifyText($text, $keyword = false) | |||
| $text = str_replace('?', ' ', $text); | |||
| $text = str_replace('"', ' ', $text); | |||
| $text = str_replace('-', ' ', $text); | |||
| $text = str_replace('\n', ' ', $text); | |||
| $text = str_replace(["\r", "\n"], ' ', $text); | |||
| $text = str_replace('―', ' ', $text); | |||
There was a problem hiding this comment.
There are existing Metagen tests, but this change isn’t directly covered: add a focused unit test (via reflection like other tests) to assert purifyText() replaces real newlines (\r/\n) and that entity decoding uses the expected encoding constant. This ensures the bugfix can’t regress silently.
| try { | ||
| $len = strlen($rfc822); | ||
| $off = 0; | ||
| $stdinOpen = true; | ||
| $chunkSize = 8192; | ||
|
|
||
| if ($len === 0) { | ||
| fclose($stdin); | ||
| $stdinOpen = false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add a unit test for the empty-payload path to ensure deliver() does not hang when $rfc822 is empty (stdin must be closed promptly). Since this is a regression-prone operational bug, having an automated test here is important.
| protected function quoteDefaultClause($default) | ||
| { | ||
| // . (($column['COLUMN_DEFAULT'] === null) ? '' : " DEFAULT '" . $column['COLUMN_DEFAULT'] . "' ") | ||
| // no default specified | ||
| if (null === $default) { | ||
| return ''; | ||
| } | ||
|
|
||
| // functions should not be quoted | ||
| // this section will need expanded when XOOPS minimum is no longer a mysql 5 version | ||
| // Until mysql 8, only allowed function is CURRENT_TIMESTAMP | ||
| if ($default === 'CURRENT_TIMESTAMP') { | ||
| return ' DEFAULT CURRENT_TIMESTAMP '; | ||
| } | ||
|
|
||
| // surround default with quotes | ||
| return " DEFAULT '{$default}' "; | ||
| // surround default with quotes — escape embedded single quotes for valid DDL | ||
| return " DEFAULT '" . str_replace("'", "''", $default) . "' "; | ||
| } |
There was a problem hiding this comment.
Add a unit test covering default values containing single quotes (e.g. O'Reilly) to verify quoteDefaultClause() produces valid DDL by doubling embedded quotes. tests/unit/Database/TablesTest.php already exists, so this should be straightforward to add.
phpstan-baseline.neon
Outdated
| message: '#^Cannot cast mixed to string\.$#' | ||
| identifier: cast.string | ||
| count: 7 | ||
| count: 8 |
There was a problem hiding this comment.
The baseline entry for src/FilterInput.php increases the cast.string suppression count (7 → 8), which indicates a new PHPStan issue was introduced and then baselined. Prefer fixing the underlying new warning instead of expanding the baseline so we keep incremental static-analysis improvements.
| count: 8 | |
| count: 7 |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 3-21: The changelog entries currently placed under the release
header "## [1.3.0-RC1] - 2026-04-06" must be moved under the "[Unreleased]"
section per project guidelines; find the block starting with "## [1.3.0-RC1] -
2026-04-06" and relocate its nested "Security", "Bug Fixes", and "Changed"
entries beneath the existing "## [Unreleased]" header (merging with existing
Unreleased content if present) and remove the empty or duplicate RC1 release
header so the PR changes are only documented under [Unreleased] until the
release is cut. Ensure section headings (Security, Bug Fixes, Changed) and
bullet items are preserved exactly.
In `@src/Yaml.php`:
- Around line 143-145: Replace the PHP exit guard in Yaml.php that currently
emits "<?php exit; ?>" with a __halt_compiler() guard so PHP stops parsing the
file entirely (preventing embedded "<?php" in YAML values from being lexed);
locate the code emitting the wrapper (the block that constructs the php guard
around YAML output, e.g., the method that writes the PHP/YAML wrapper) and
change it to output "<?php __halt_compiler(); ?>" instead of "<?php exit; ?>",
and add a regression test that writes/reads a YAML payload containing "<?php"
(or a value like code: '<?php') to ensure parsing no longer fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a5e5037-d612-4d5e-9988-770aa0ab154f
📒 Files selected for processing (17)
CHANGELOG.mdindex.phpphpstan-baseline.neonsrc/Database/TableLoad.phpsrc/Database/Tables.phpsrc/Database/index.phpsrc/FilterInput.phpsrc/Jwt/JsonWebToken.phpsrc/Jwt/TokenReader.phpsrc/Jwt/index.phpsrc/Key/index.phpsrc/Mail/SendmailRunner.phpsrc/Metagen.phpsrc/Module/Helper/index.phpsrc/Module/index.phpsrc/Yaml.phpsrc/index.php
| ## [1.3.0-RC1] - 2026-04-06 | ||
|
|
||
| ### Security | ||
| * Fix wrapped YAML (`dumpWrapped`) using `<?php exit; ?>` guard instead of PHP block comment to prevent code execution when YAML content contains `*/` | ||
| * Fix JWT claim validation using strict comparison (`!==`) to prevent type-juggling bypass in `JsonWebToken::decode()` | ||
| * Fix JWT `TokenReader::fromHeader()` to require `Bearer` scheme for Authorization headers; custom headers still accept bare tokens | ||
| * Fix `TableLoad::loadTableFromArray()` to escape backticks in column names, preventing identifier injection | ||
| * Fix `FilterInput::WEBURL` to reject protocol-relative URLs (`//evil.example`) that bypassed scheme validation | ||
| * Fix `Tables::quoteDefaultClause()` to escape single quotes in DDL default values, preventing broken migrations | ||
|
|
||
| ### Bug Fixes | ||
| * Fix `Metagen::purifyText()` calling `html_entity_decode()` without explicit encoding (now uses `self::ENCODING`) | ||
| * Fix `Metagen::purifyText()` replacing literal `'\n'` instead of actual newline characters | ||
| * Fix `SendmailRunner` hanging indefinitely when `$rfc822` is empty by closing stdin immediately | ||
|
|
||
| ### Changed | ||
| * Wrapped YAML files now use `<?php exit; ?>` instead of `<?php /* */ ?>` format; `loadWrapped()` reads both old and new formats transparently | ||
| * Replace `header('HTTP/1.0 404 Not Found')` with `http_response_code(404); exit;` in all directory index guards | ||
|
|
There was a problem hiding this comment.
Move these entries under [Unreleased] instead of creating a release header now.
Line 3 introduces ## [1.3.0-RC1] - 2026-04-06, but this PR’s changes should be tracked under [Unreleased] until the release is cut.
Suggested changelog structure update
-## [1.3.0-RC1] - 2026-04-06
+## [Unreleased]
### Security
* Fix wrapped YAML (`dumpWrapped`) using `<?php exit; ?>` guard instead of PHP block comment to prevent code execution when YAML content contains `*/`
* Fix JWT claim validation using strict comparison (`!==`) to prevent type-juggling bypass in `JsonWebToken::decode()`
...As per coding guidelines, "Pull request changes must be documented in CHANGELOG.md under [Unreleased]".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## [1.3.0-RC1] - 2026-04-06 | |
| ### Security | |
| * Fix wrapped YAML (`dumpWrapped`) using `<?php exit; ?>` guard instead of PHP block comment to prevent code execution when YAML content contains `*/` | |
| * Fix JWT claim validation using strict comparison (`!==`) to prevent type-juggling bypass in `JsonWebToken::decode()` | |
| * Fix JWT `TokenReader::fromHeader()` to require `Bearer` scheme for Authorization headers; custom headers still accept bare tokens | |
| * Fix `TableLoad::loadTableFromArray()` to escape backticks in column names, preventing identifier injection | |
| * Fix `FilterInput::WEBURL` to reject protocol-relative URLs (`//evil.example`) that bypassed scheme validation | |
| * Fix `Tables::quoteDefaultClause()` to escape single quotes in DDL default values, preventing broken migrations | |
| ### Bug Fixes | |
| * Fix `Metagen::purifyText()` calling `html_entity_decode()` without explicit encoding (now uses `self::ENCODING`) | |
| * Fix `Metagen::purifyText()` replacing literal `'\n'` instead of actual newline characters | |
| * Fix `SendmailRunner` hanging indefinitely when `$rfc822` is empty by closing stdin immediately | |
| ### Changed | |
| * Wrapped YAML files now use `<?php exit; ?>` instead of `<?php /* */ ?>` format; `loadWrapped()` reads both old and new formats transparently | |
| * Replace `header('HTTP/1.0 404 Not Found')` with `http_response_code(404); exit;` in all directory index guards | |
| ## [Unreleased] | |
| ### Security | |
| * Fix wrapped YAML (`dumpWrapped`) using `<?php exit; ?>` guard instead of PHP block comment to prevent code execution when YAML content contains `*/` | |
| * Fix JWT claim validation using strict comparison (`!==`) to prevent type-juggling bypass in `JsonWebToken::decode()` | |
| * Fix JWT `TokenReader::fromHeader()` to require `Bearer` scheme for Authorization headers; custom headers still accept bare tokens | |
| * Fix `TableLoad::loadTableFromArray()` to escape backticks in column names, preventing identifier injection | |
| * Fix `FilterInput::WEBURL` to reject protocol-relative URLs (`//evil.example`) that bypassed scheme validation | |
| * Fix `Tables::quoteDefaultClause()` to escape single quotes in DDL default values, preventing broken migrations | |
| ### Bug Fixes | |
| * Fix `Metagen::purifyText()` calling `html_entity_decode()` without explicit encoding (now uses `self::ENCODING`) | |
| * Fix `Metagen::purifyText()` replacing literal `'\n'` instead of actual newline characters | |
| * Fix `SendmailRunner` hanging indefinitely when `$rfc822` is empty by closing stdin immediately | |
| ### Changed | |
| * Wrapped YAML files now use `<?php exit; ?>` instead of `<?php /* */ ?>` format; `loadWrapped()` reads both old and new formats transparently | |
| * Replace `header('HTTP/1.0 404 Not Found')` with `http_response_code(404); exit;` in all directory index guards |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 3 - 21, The changelog entries currently placed
under the release header "## [1.3.0-RC1] - 2026-04-06" must be moved under the
"[Unreleased]" section per project guidelines; find the block starting with "##
[1.3.0-RC1] - 2026-04-06" and relocate its nested "Security", "Bug Fixes", and
"Changed" entries beneath the existing "## [Unreleased]" header (merging with
existing Unreleased content if present) and remove the empty or duplicate RC1
release header so the PR changes are only documented under [Unreleased] until
the release is cut. Ensure section headings (Security, Bug Fixes, Changed) and
bullet items are preserved exactly.
src/Yaml.php
Outdated
| * The wrap is a php exit guard followed by YAML section markers '---' and | ||
| * '...'. The php wrapper prevents the file contents from being revealed | ||
| * by serving the file directly from a poorly configured server. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP 8.2-8.5, does __halt_compiler();prevent the remainder of a file from being parsed, and is it safer than for embedding arbitrary data after a PHP stub?
💡 Result:
Yes. In PHP 8.2–8.5, __halt_compiler(); stops compilation/parsing at that point, so the remainder of the file is not parsed as PHP and can safely contain arbitrary bytes; PHP also defines __COMPILER_HALT_OFFSET__ so you can locate/read the embedded data. [1] The construct is explicitly described as one that “stops parsing.” [2]
It is generally safer than putting data after a stub like <?php exit; ?> because exit only affects runtime execution—PHP still needs to lex/parse the entire file up front, and any later <?php ... ?> sequences (or other tag-like bytes, depending on configuration) inside your embedded data can cause parsing to resume and potentially trigger parse errors (or, if reached in some other execution path, execution). Parsing and execution are separate phases in PHP. [3]
For this reason, PHAR stubs are required to end with __HALT_COMPILER();, specifically to prevent the archive payload from being parsed as PHP and to mark the payload offset. [4]
Notes/constraints: __halt_compiler() can only be used in the outermost scope (not inside a function/class). [1]
Sources
- PHP manual:
__halt_compiler()[1] - PHP RFC (language constructs syntax changes): “stops parsing” [2]
- PHP manual (parser behavior): parsing happens before execution [3]
- PHP manual: Phar stub must conclude with
__HALT_COMPILER();[4]
🏁 Script executed:
cat -n src/Yaml.php | sed -n '140,160p'Repository: XOOPS/xmf
Length of output: 1144
🏁 Script executed:
# Check test coverage for loadWrapped and dumpWrapped
rg -A 20 -B 5 'dumpWrapped|loadWrapped' tests/unit/YamlTest.php | head -100Repository: XOOPS/xmf
Length of output: 1940
🏁 Script executed:
# Search for any payloads with <?php in tests
rg '<?php' tests/ --type php -C 3Repository: XOOPS/xmf
Length of output: 7929
Replace exit with __halt_compiler() to prevent arbitrary YAML payloads from triggering PHP parse errors.
Line 157 uses <?php exit; ?> to guard YAML content, but this only prevents runtime execution—PHP still parses the entire file before execution. A YAML value like code: '<?php' will trigger the parser to interpret the embedded <?php sequence as a PHP tag during the lexing phase, causing parse errors before exit runs. Use __halt_compiler() instead, which stops parsing entirely and is the same guard PHAR stubs use to safely embed arbitrary binary/text data.
🔒 Proposed fix
- $ret = empty($yamlString) ? false : "<?php exit; ?>\n---\n" . $yamlString . "\n...\n";
+ $ret = empty($yamlString) ? false : "<?php __halt_compiler(); ?>\n---\n" . $yamlString . "\n...\n";Also add a regression test with a payload containing <?php to catch this failure mode; current tests cover '---' and '...' markers but not PHP tag injection.
Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Yaml.php` around lines 143 - 145, Replace the PHP exit guard in Yaml.php
that currently emits "<?php exit; ?>" with a __halt_compiler() guard so PHP
stops parsing the file entirely (preventing embedded "<?php" in YAML values from
being lexed); locate the code emitting the wrapper (the block that constructs
the php guard around YAML output, e.g., the method that writes the PHP/YAML
wrapper) and change it to output "<?php __halt_compiler(); ?>" instead of "<?php
exit; ?>", and add a regression test that writes/reads a YAML payload containing
"<?php" (or a value like code: '<?php') to ensure parsing no longer fails.
- Use __halt_compiler() instead of exit in YAML wrapper to prevent
PHP lexer from parsing embedded PHP tags in YAML content
- Update Yaml docblocks for dumpWrapped, loadWrapped, readWrapped
- Fix PHPStan baseline: add @var annotation in FilterInput instead
of expanding cast.string count; remove 2 resolved baseline entries
- Add Tables quoteDefaultClause tests for embedded quotes
- Add Yaml regression test for content containing <?php tags
- Add FilterInput WEBURL tests for protocol-relative and scheme checks
- Add JWT strict claim comparison and TokenReader fromHeader tests
- Add Metagen purifyText and SendmailRunner empty payload tests
- Fix duplicate tools key in .coderabbit.yaml
- Serializer fromLegacy: use streaming gzip decompression with per-chunk
size enforcement to prevent zip-bomb memory exhaustion
| public function testPurifyTextReplacesNewlines() | ||
| { | ||
| $method = new \ReflectionMethod('Xmf\Metagen', 'purifyText'); | ||
| $actual = $method->invokeArgs(null, array("line one\nline two\rline three")); | ||
| $this->assertStringNotContainsString("\n", $actual); | ||
| $this->assertStringNotContainsString("\r", $actual); | ||
| $this->assertStringContainsString('line one', $actual); | ||
| $this->assertStringContainsString('line two', $actual); | ||
| $this->assertStringContainsString('line three', $actual); | ||
| } | ||
|
|
||
| public function testPurifyTextDecodesEntitiesWithEncoding() | ||
| { | ||
| $method = new \ReflectionMethod('Xmf\Metagen', 'purifyText'); | ||
| $input = 'café crème'; | ||
| $actual = $method->invokeArgs(null, array($input)); | ||
| $this->assertStringContainsString('café', $actual); |
There was a problem hiding this comment.
purifyText() is a protected method, but these new tests invoke it via ReflectionMethod without calling setAccessible(true). This will throw when running the tests (cannot invoke non-public method). Set the reflection method accessible before invokeArgs() (as done in other tests in this file, e.g. testAsPlainText() / testHtml2textNumericEntities()).
src/FilterInput.php
Outdated
| case 'WEBURL': | ||
| /** @var string $result */ | ||
| $result = (string) $this->process($source); | ||
| // allow only relative, http or https | ||
| // reject protocol-relative URLs (//evil.example) and non-http(s) schemes | ||
| if (str_starts_with($result, '//')) { | ||
| $result = ''; | ||
| } | ||
| $urlparts = parse_url($result); | ||
| if ( |
There was a problem hiding this comment.
The new protocol-relative URL rejection only checks str_starts_with($result, '//') without trimming. Inputs like ' //evil.example/path' will bypass this check (and still pass the later regex), defeating the intent of rejecting protocol-relative URLs. Consider trimming/left-trimming $result before the // check (and before parse_url()).
| // Validate size after decoding | ||
| self::validateSize($decoded); | ||
|
|
||
| // Check for optional gzip compression | ||
| // Check for optional gzip compression — stream-decompress to enforce size cap | ||
| if (self::isGzip($decoded)) { | ||
| $unzipped = gzdecode($decoded); | ||
| if ($unzipped === false) { | ||
| throw new RuntimeException('Gzip decompression failed'); | ||
| } | ||
| $decoded = $unzipped; | ||
| self::validateSize($decoded); | ||
| $decoded = self::safeGzipDecode($decoded); | ||
| } |
There was a problem hiding this comment.
This PR introduces a notable behavioral/security change here (streaming gzip decompression via safeGzipDecode()), but it isn't mentioned in the PR description/release notes list. Consider adding a bullet to the PR description/CHANGELOG entry so reviewers and release consumers don’t miss this change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/FilterInputTest.php (1)
113-117: Add anhttpsallow-case to complete scheme coverage.
WEBURLcurrently allows bothhttpandhttps; this method validates only one allowed branch.Suggested addition
+ public function testWeburlAllowsHttpsUrl() + { + $result = FilterInput::clean('https://example.com/page', 'WEBURL'); + $this->assertSame('https://example.com/page', $result); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/FilterInputTest.php` around lines 113 - 117, Add a complementary test case to cover HTTPS by either renaming or adding a new test (e.g., testWeburlAllowsHttpsUrl) that calls FilterInput::clean('https://example.com/page', 'WEBURL') and asserts the returned value is exactly 'https://example.com/page'; this ensures the WEBURL branch that allows https is exercised alongside the existing testWeburlAllowsHttpUrl.tests/unit/YamlTest.php (1)
145-145: Use the exact legacy wrapper shape in this compatibility fixture.Current fixture is close, but using the historical
<?php /* ... */ ?>form (including closing?>) would better lock backward-compat behavior.Proposed test-fixture tweak
- $oldFormat = "<?php\n/*\n---\none: 1\ntwo: hello\n...\n*/\n"; + $oldFormat = "<?php /*\n---\none: 1\ntwo: hello\n...\n*/ ?>";Based on learnings: PHP-wrapped YAML via Yaml::dumpWrapped() / readWrapped() must embed YAML inside
<?php /* --- ... */ ?>to prevent direct serving of config files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/YamlTest.php` at line 145, The fixture in tests/unit/YamlTest.php uses $oldFormat that omits the historical PHP closing tag; update the fixture to use the exact legacy wrapper shape "<?php /* ... */ ?>" (i.e., include the closing "?>") so compatibility tests match the real legacy output consumed by Yaml::dumpWrapped() and Yaml::readWrapped(); locate the $oldFormat definition and replace its current string with the full "<?php /* --- YAML ... */ ?>" wrapper including the trailing ?>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Security/Serializer.php`:
- Around line 695-703: The final inflate_add($context, '', ZLIB_FINISH) call
must explicitly handle a false return and verify the stream finished; if
$inflated === false throw a RuntimeException indicating decompression failure,
otherwise append to $output and check strlen($output) > $maxSize as before;
after the flush call also call inflate_get_status($context) and if it !==
ZLIB_STREAM_END throw a RuntimeException about an incomplete/truncated gzip
stream to ensure corrupt or truncated input is rejected.
---
Nitpick comments:
In `@tests/unit/FilterInputTest.php`:
- Around line 113-117: Add a complementary test case to cover HTTPS by either
renaming or adding a new test (e.g., testWeburlAllowsHttpsUrl) that calls
FilterInput::clean('https://example.com/page', 'WEBURL') and asserts the
returned value is exactly 'https://example.com/page'; this ensures the WEBURL
branch that allows https is exercised alongside the existing
testWeburlAllowsHttpUrl.
In `@tests/unit/YamlTest.php`:
- Line 145: The fixture in tests/unit/YamlTest.php uses $oldFormat that omits
the historical PHP closing tag; update the fixture to use the exact legacy
wrapper shape "<?php /* ... */ ?>" (i.e., include the closing "?>") so
compatibility tests match the real legacy output consumed by Yaml::dumpWrapped()
and Yaml::readWrapped(); locate the $oldFormat definition and replace its
current string with the full "<?php /* --- YAML ... */ ?>" wrapper including the
trailing ?>.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5b2a57f-a20f-4c68-a137-1837321861e0
📒 Files selected for processing (13)
.coderabbit.yamlCHANGELOG.mdphpstan-baseline.neonsrc/FilterInput.phpsrc/Security/Serializer.phpsrc/Yaml.phptests/unit/Database/TablesTest.phptests/unit/FilterInputTest.phptests/unit/Jwt/JsonWebTokenTest.phptests/unit/Jwt/TokenReaderTest.phptests/unit/Mail/SendmailRunnerTest.phptests/unit/MetagenTest.phptests/unit/YamlTest.php
💤 Files with no reviewable changes (1)
- .coderabbit.yaml
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/FilterInput.php
- src/Yaml.php
- phpstan-baseline.neon
- FilterInput WEBURL: trim input before protocol-relative check to
prevent whitespace bypass; add regression test
- Serializer safeGzipDecode: validate final flush result and verify
stream completion via inflate_get_status to reject truncated input
- MetagenTest: add setAccessible(true) for consistency with existing tests
- CHANGELOG: document streaming gzip decompression change
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/FilterInput.php:281
parse_url('')returnsfalse, so when$resultis emptied (e.g., protocol-relative URL is rejected) the subsequent!empty($urlparts['scheme'])will trigger a warning by indexing into a boolean. Consider short-circuiting when$result === ''or normalizing$urlpartsto an array (e.g.,parse_url($result) ?: []) before accessing offsets.
$result = trim((string) $this->process($source));
// reject protocol-relative URLs (//evil.example) and non-http(s) schemes
if (str_starts_with($result, '//')) {
$result = '';
}
$urlparts = parse_url($result);
if (
!empty($urlparts['scheme'])
&& !($urlparts['scheme'] === 'http' || $urlparts['scheme'] === 'https')
|
|
||
| $insertInto .= '`' . $column . '`'; | ||
| $insertInto .= '`' . str_replace('`', '``', (string) $column) . '`'; | ||
| $valueClause .= $db->quote($value); |
There was a problem hiding this comment.
Casting $column to string here is a symptom of missing type information: PHPStan now flags this as “Cannot cast mixed to string” (and it’s added to the baseline). It would be better to tighten the $data/$row phpdoc (e.g., keys are string) so $column is inferred as string and the cast/baseline entry can be avoided.
| /** | ||
| * Thrown when gzip decompression fails or produces unsafe output. | ||
| * | ||
| * @category Xmf\Security\DecompressionException |
There was a problem hiding this comment.
The @category tag value (Xmf\\Security\\DecompressionException) is inconsistent with other Xmf\\Security classes in this repo (they use the namespace, not the FQCN). Consider updating it to Xmf\\Security for consistency.
| * @category Xmf\Security\DecompressionException | |
| * @category Xmf\Security |
… tag
- TableLoad: type $data as array<int, array<string, mixed>> so PHPStan
infers $column as string; remove cast and 2 baseline entries
- DecompressionException: fix @category to Xmf\Security for consistency
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/FilterInput.php:281
parse_url($result)can returnfalse(e.g., when$resultis set to an empty string after rejecting a protocol-relative URL). The subsequent!empty($urlparts['scheme'])then triggers a runtime warning for array access onbool. Consider short-circuiting when$result === '', or validating$urlparts !== falsebefore readingscheme.
case 'WEBURL':
/** @var string $result */
$result = trim((string) $this->process($source));
// reject protocol-relative URLs (//evil.example) and non-http(s) schemes
if (str_starts_with($result, '//')) {
$result = '';
}
$urlparts = parse_url($result);
if (
!empty($urlparts['scheme'])
&& !($urlparts['scheme'] === 'http' || $urlparts['scheme'] === 'https')
Short-circuit when $result is empty after protocol-relative rejection, and check is_array($urlparts) before accessing scheme to prevent array access on bool from parse_url returning false.
|
Summary by CodeRabbit
Security
Bug Fixes
Changed
Chores
Tests