Make tests.sh strictly POSIX-portable, fix real quoting/logic bugs - #2
Merged
Conversation
Addresses a full review (credit: Sol) of tests.sh against POSIX sh:
- Remove `local` (not POSIX; widely supported but not guaranteed)
- Replace the `getopts :hua-:` long-option hack (relies on unspecified
getopts behavior for non-alphanumeric option chars) with a plain
case/shift loop that handles short options, long options, and `--`
uniformly -- simpler than running getopts and manual parsing side by
side, and fully POSIX either way
- Replace every `echo` with `printf`, since echo's handling of `-n` and
backslashes is implementation-defined per POSIX (BSD vs System V)
- Quote "$PHPUNIT_BIN", "$ME_DIR", and function arguments throughout --
unquoted expansions broke on paths containing whitespace
- Recompute ME_DIR via `CDPATH= cd -P ... && pwd -P` with real failure
handling, replacing a `cd "$dir"; pwd` pattern where a failed cd let
pwd silently report the previous directory instead of erroring
- Fix printf calls that used dynamic data as the format string itself
(a literal `%` in a path would have been read as a format directive)
- Replace the coverage-option text-generation (building a string then
relying on word-splitting to turn it back into argv) with `set --`,
so a coverage path containing whitespace can't corrupt phpunit's args
- Narrow cmd_status_filter's "not a shell/signal status" range from a
Linux-specific 126..165 guess to a portable 1..125, since POSIX does
not define signal-number-to-exit-status encoding
- Fix inconsistent phpunit-*.xml suffix handling: passing a suite name
with or without the .xml extension now both resolve to the same file
- Correct the usage line: `--` only terminates the wrapper's own option
scanning (before <TEST-SUITE>), it was never consumable after it
Also fixes a real latent bug the restructuring surfaced: phpunit_coverage_check()
returned 0 (success) on every branch, including "coverage should be
skipped." That was inert in the original script because
print_phpunit_coverage_opt duplicated the same guard inline and its
exit status was never checked -- but using phpunit_coverage_check as an
actual boolean gate (the correct way to build the coverage flags)
surfaced it: coverage flags were being added even when xdebug wasn't
available, which PHPUnit 13 treated as fatal ("No tests executed!").
Fixed by making the skip/unavailable branches return 1.
Verified in php:8.5-cli: all 629 tests pass (default, --print-coverage,
and explicit `phpunit` suite modes), error paths give correct messages
and exit codes, and a run under a path containing a space confirmed the
quoting fixes hold end-to-end.
ackspony
added a commit
that referenced
this pull request
Aug 28, 2026
… major finding added A follow-up model review re-checked the original document against source and empirical testing rather than trusting it, and: - Confirmed all 5 original "verified findings" hold (with one added caveat: HtmlEncoder silently drops invalid-UTF-8 keys/values to empty strings rather than escaping them). - Resolved open question #2 (checksum) as a real bug: XmlEncoder's document-level checksum hashes the literal string "null", never the actual payload -- confirmed two different payloads produce the identical fx:md5 constant. - Resolved open question #4 (finfo/libmagic) as two real bugs: the `dumpOk` option is dead code (never gates anything, the dump path always runs), and any invalid-UTF-8 leaf string routes attacker-chosen raw bytes into finfo::buffer() unconditionally on default settings. - Resolved open question #1 (class-name invariant): holds against injection, but anonymous-class encoding leaks the full source file path and line number into the XML output. - Found something the first pass missed entirely: MarkdownEncoder's escapeMarkdown() never touches <, >, or &, so bin/json2md has a direct path from attacker JSON on stdin to raw HTML/script in the rendered Markdown output, for any downstream renderer with raw-HTML enabled (several common ones by default). Related: multiline detection only checks for \n, not \r, so a bare \r can smuggle a heading or HTML block past the inline-value escaping path. Only the depth/size resource-limit question remains open as a genuine design call rather than a bug.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rewrites
tests.shagainst a full POSIX-portability review (credit: Sol) and fixes several real bugs it surfaced.Portability fixes
local(not POSIX)getopts :hua-:long-option hack (unspecified behavior for non-alphanumeric option chars per POSIX) with a plaincase/shiftloop handling short options, long options, and--uniformlyechowithprintf(echo's-n/backslash handling is implementation-defined per POSIX)printfcalls that used dynamic data as the format string itselfCorrectness fixes
"$PHPUNIT_BIN","$ME_DIR", and function arguments throughout — unquoted expansions broke on paths containing whitespaceME_DIRviaCDPATH= cd -P ... && pwd -Pwith real failure handling, replacing acd "$dir"; pwdpattern where a failedcdletpwdsilently report the previous directoryset --, so a coverage path containing whitespace can't corrupt phpunit's argscmd_status_filter's exit-status range from a Linux-specific126..165guess to a portable1..125(POSIX doesn't define signal-to-exit-status encoding)phpunit-*.xmlsuffix handling so passing a suite name with or without.xmlboth resolve correctly--only terminates the wrapper's own option scanning, before<TEST-SUITE>Bug found via real testing
Restructuring the coverage flags through
phpunit_coverage_check()as an actual boolean gate (the correct way to build them) surfaced a latent bug: every branch of that function returned 0 (success), including "coverage should be skipped." It was inert in the original script becauseprint_phpunit_coverage_optduplicated the same guard inline without its exit status ever being checked — but as a real gate, it meant coverage flags got added even when xdebug wasn't available, which PHPUnit 13 treats as fatal ("No tests executed!"). Fixed by making the skip/unavailable branches return 1.Verification
Run in
php:8.5-cli: all 629 tests pass in default,--print-coverage, and explicitphpunit-suite modes; error paths give correct messages/exit codes; a run under a path containing a space confirmed the quoting fixes hold end-to-end.🤖 Generated with Claude Code