Repository navigation
Return INI values as written instead of substituting PHP constants - #248
Open
LukeTowers wants to merge 2 commits into
Open
LukeTowers wants to merge 2 commits into
LukeTowers wants to merge 2 commits into
Conversation
`Ini::parse()` hands its content to `parse_ini_string()` with the default scanner, which replaces any token matching the name of a defined PHP constant with that constant's value. The parser's output then depends on the state of the host process rather than on the document it was given: a settings block containing `title = PHP_VERSION` came back as the interpreter's version string instead of the text its author wrote, and a constant name used inside a bracketed key (`a[PHP_VERSION] = x`) became that value as an array key. This extends the placeholder technique the parser already uses for `${...}` interpolation - before parsing, a marker is prefixed to each token in the document that names a currently defined constant so the native parser cannot resolve it, and the marker is stripped again in a recursive restore walk (`restorePlaceholders()`, which runs after the existing `${` restoration). A token is only marked when `defined()` reports a constant of that name, and only as a whole token, so a document naming none of them is handed to the native parser byte for byte. Switching to `INI_SCANNER_RAW` was considered and rejected: it breaks multi-line quoted values, leaves `\"` escapes unprocessed and drops keyword coercion.
Behaviour changes:
- A token that happens to match the name of a defined PHP constant now stays as written, where it was previously replaced by that constant's value. This covers a token adjacent to a quoted run (`foo = CONST"x"`), repeated tokens in one value, tokens in array and nested-key values, and a constant name inside a bracketed key, which now becomes that key's own text. A bracketed key offset is also the one position where the native scanner resolves `on`, `off`, `yes`, `no` and `none` against the constant table rather than coercing them, so those are returned as written there too. The offset may be indented (`a[ yes] = x`): the native scanner skips that whitespace before it resolves the offset, so the marker does too.
- An INI operator expression over constant names - the `php.ini` bitmask idiom, `a = E_ALL & ~E_NOTICE` - is now evaluated as if those constants were undefined, so it yields `0` rather than a number computed from their values. Winter's own INI documents contain no such expression.
- A keyword-named constant standing between two `|` characters is now marked as well, because the offset rule applies to that delimiter too - `parsePreProcess()` uses it in keys. Where the previous parser lexed `a = b|yes|c` as a keyword and rejected the whole document, the value now parses as the bitwise-or of two unresolved tokens, `0`, which is the result `a = b|notakeyword|c` already gave. It takes a defined constant named `on`, `off`, `yes`, `no` or `none` to reach this at all.
- A failure inside the marking pre-pass, meaning a PCRE backtrack or recursion limit on a pathological document, now throws `RuntimeException`. Previously the pass could return null and that null would be handed to the native parser, which parses it as nothing at all.
- Everything else parses as before: in a value the keywords `true`, `false`, `on`, `off`, `yes`, `no`, `none` and `null` still coerce to `1`/`''`, and `true`, `false` and `null` do so in a bracketed key as well; quoted values, escape sequences and multi-line quoted values are unchanged; section headers and bare key names are untouched; `${...}` is still returned literally; and a document whose own text contains the parser's internal marker keeps it.
- `restoreDollarBrace()`, which has shipped since v1.2.13, keeps its name, its behaviour and its call contract: `parse()` still hands it the whole parsed array, so an override that walks array nodes as well as strings keeps working.
Tests in `tests/Parse/IniTest.php`: `testPhpConstantNamesAreNotSubstituted`, `testInterpreterConstantsAreNotSubstituted`, `testConstantNamesInBracketedKeysAreNotSubstituted`, `testKeywordNamedConstantsInBracketedKeysAreNotSubstituted`, `testKeywordsCoercedInBracketedKeysAreUnchanged`, `testQuotedConstantNamesAreUnchanged`, `testReservedKeywordsAreStillCoerced`, `testKeywordCoercionIsIndependentOfDefinedConstantNames`, `testShortConstantNamesAreNotMarkedInsideLongerTokens`, `testMultilineQuotedValuesAreUnaffected`, `testDocumentTextMatchingTheInternalMarkerIsPreserved`, `testReleasedRestoreHookIsStillCalled`, `testReleasedRestoreHookStillReceivesArrayNodes`, `testRenderedConstantNameRoundTrips`, `testKeywordNamedConstantsInIndentedBracketedKeysAreNotSubstituted` and `testAFailedIdentifierPrePassIsReported`.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe INI parser now escapes defined PHP constant names before calling Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change is ready to merge after normal checks; no actionable parsing regression was established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Parsed settings will now retain PHP constant names instead of substituting their process values. The existing protection against environment-variable interpolation remains, and no new privileged path was identified. Uses of these settings outside this package were not available to assess. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
`testAFailedIdentifierPrePassIsReported` forced the failure by lowering `pcre.backtrack_limit`, which does not produce one on every PCRE build: it passed locally and failed on all eight CI jobs. The replacement is a seam — `replaceIdentifiers()` — that the test overrides to return null directly, so the guard is exercised the same way everywhere.
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.
Ini::parse()hands its content toparse_ini_string()with the default scanner, which replaces any token matching the name of a defined PHP constant with that constant's value. The parser's output then depends on the state of the host process rather than on the document it was given: a settings block containingtitle = PHP_VERSIONcomes back as the interpreter's version string instead of the text its author wrote, and a constant name used inside a bracketed key (a[PHP_VERSION] = x) becomes that value as an array key.This extends the placeholder technique the parser already uses for
${...}interpolation. Before parsing, a marker is prefixed to each token in the document that names a currently defined constant, so the native parser cannot resolve it; the marker is stripped again in a recursive restore walk (restorePlaceholders(), which runs after the existing${restoration). A token is only marked whendefined()reports a constant of that name, and only as a whole token, so a document naming none of them is handed to the native parser byte for byte.INI_SCANNER_RAWwas considered and rejected: it breaks multi-line quoted values, leaves\"escapes unprocessed and drops keyword coercion.Behaviour changes
foo = CONST"x"), repeated tokens in one value, tokens in array and nested-key values, and a constant name inside a bracketed key, which now becomes that key's own text. A bracketed key offset is also the one position where the native scanner resolveson,off,yes,noandnoneagainst the constant table rather than coercing them, so those are returned as written there too, indented (a[ yes] = x) or not.php.inibitmask idiom,a = E_ALL & ~E_NOTICE— is now evaluated as if those constants were undefined, so it yields0rather than a number computed from their values. Winter's own INI documents contain no such expression.|characters is marked as well, because the offset rule applies to that delimiter too. Where the previous parser lexeda = b|yes|cas a keyword and rejected the whole document, the value now parses as0, the same resulta = b|notakeyword|calready gave. It takes a defined constant namedon,off,yes,noornoneto reach at all.RuntimeExceptionrather than being handed on as null, which the native parser would have parsed as nothing at all.true,false,on,off,yes,no,noneandnullstill coerce to1/'', andtrue,falseandnulldo so in a bracketed key as well; quoted values, escape sequences and multi-line quoted values are unchanged; section headers and bare key names are untouched;${...}is still returned literally; and a document whose own text contains the parser's internal marker keeps it.restoreDollarBrace(), which has shipped since v1.2.13, keeps its name, its behaviour and its call contract:parse()still hands it the whole parsed array, so an override that walks array nodes as well as strings keeps working.Tests
16 new tests in
tests/Parse/IniTest.php, includingtestPhpConstantNamesAreNotSubstituted,testInterpreterConstantsAreNotSubstituted,testConstantNamesInBracketedKeysAreNotSubstituted,testKeywordNamedConstantsInBracketedKeysAreNotSubstituted,testKeywordNamedConstantsInIndentedBracketedKeysAreNotSubstituted,testKeywordsCoercedInBracketedKeysAreUnchanged,testKeywordCoercionIsIndependentOfDefinedConstantNames,testShortConstantNamesAreNotMarkedInsideLongerTokens,testMultilineQuotedValuesAreUnaffected,testDocumentTextMatchingTheInternalMarkerIsPreserved,testReleasedRestoreHookStillReceivesArrayNodes,testRenderedConstantNameRoundTripsandtestAFailedIdentifierPrePassIsReported.The suite is green at 855 tests / 3430 assertions with the 9 pre-existing skips (baseline 839), and
phpcsis clean on both changed files. A parity check over 31 documents against the unpatched parser — quoted and multi-line values, every keyword in value and offset position, section headers, comments, numeric and hex forms, list keys,${...}— differs only in the four cases the behaviour list above describes. Measured cost on this checkout's real corpus: 0.82 ms → 1.99 ms for all 148 settings blocks together, 0.12 ms → 0.31 ms for the largest single block.Winter's
modules/cmshas two tests that fail until this merges, since they assert the parser's behaviour from the CMS side.Summary by CodeRabbit