fix: make @php/@endphp block masking string-literal safe - #164
Conversation
mask_php_blocks located the closing @endphp with a plain substring search, so a literal @endphp inside a PHP string within a @php … @endphp block closed the masked region early — leaving the real PHP tail wrongly treated as renameable Blade markup. Replace the substring search with find_block_terminator, a single-pass O(n) scanner that skips PHP string literals (single-/double-quoted, heredoc, nowdoc) and comments (//, #, /* … */) before matching @endphp, with a symmetric word-boundary guard mirroring the @php opener. Fixes: #93
|
@mikebronner — escalating an acceptance-criteria dispute on #93, not a code defect. The implementation is correct and well-tested; one AC item names the wrong function, and resolving it is a contract amendment (your call), so I'm not approving or requesting changes. The snag — AC #5. It reads:
But Options
Recommendation: option 1. The implementation is right; only the AC's function reference is wrong. Amending AC #5 to Context: 8 of 9 AC items met, CI green (LSP test/fmt/clippy pass). Verified: scan is genuinely O(n) (no quadratic heredoc backtracking), no reachable panic on adversarial input, and the word-prefix / escaped-quote tests are honest discriminators. Two optional non-blocking test gaps to fold in once this resolves: a fake |
There was a problem hiding this comment.
✅ Approved — AC #5 amended per Mike's decision; the implementation is correct.
AC #5 named the wrong verification function. It asked for variable_spans to return no spans in the post-fake-@endphp region — but variable_spans → mask_non_code intentionally masks only {{-- --}} comments and @verbatim, not @php blocks, because an inline @php $x=…; @endphp assignment is a file-scoped renameable variable (the #55 design; enforced by php_block_variable_is_file_scoped). Forcing variable_spans to mask @php blocks would regress that. The fake-@endphp protection correctly lives in is_template_variable → mask_non_template → mask_php_blocks — exactly what this PR fixes. AC #5 has been reworded to name that gate (and AC #6 already covered it).
The implementation is right: find_block_terminator replaces the naive find("@endphp") substring search with a single-pass O(n) scanner that skips single/double-quoted strings (honoring \'/\"/\\ escapes), heredoc/nowdoc bodies (PHP 7.3+ indented closers), and ///#//* */ comments, plus a symmetric @endphp word-boundary guard (@endphpunit no longer matches). The $ghost-stays-block-local tests across single-quote, double-quote, and heredoc are honest discriminators. Verified O(n) (no quadratic heredoc backtracking) and no reachable panic on adversarial input.
8/9 ACs met outright; AC #5's intent was met and its wording is now corrected. CI green (LSP test/fmt/clippy, wasm, CodeQL). Merging.
Two optional, non-AC test gaps (a fake @endphp in a /* */ block comment, and a <<<"LABEL" double-quoted heredoc) — both code paths already implemented, just uncovered — will be spun out as a minor test-hardening follow-up.
…eredoc legs. Two already-working but uncovered paths in `find_block_terminator`'s @php-block masking: a literal @endphp inside a `/* … */` block comment, and inside a double-quoted-label heredoc (`<<<"LABEL"`). Existing tests only reach the `//` line-comment, bare-label heredoc, and nowdoc legs. Bundled from #93/#164 (unrelated area, folded into #156's PR by request).
…one shared module (#161) * chore: start work on #156 * refactor: ♻️ Consolidate triplicated path_within_root guard into one module. The canonical-first root-containment check lived in three copies with three divergent fallbacks: main.rs (fail-closed, #155), slot_navigation.rs (raw-textual), and an inline `retain` in salsa_impl.rs (lexical, for speculative candidates). Three copies can drift, and any future hardening had to land in all three and stay in sync. Extract one `path_containment` module with a shared canonical-first core and two public entry points: - `path_within_root` — fail-closed; the security guard, used by main.rs (no behavior change) and slot_navigation.rs (upgrade: raw-textual → fail-closed). - `path_within_root_lexical` — normalize_path lexical fallback that admits not-yet-created candidates, used by salsa_impl.rs's component-path filter (which must not fail-close). All main.rs call-sites (including the one #157 added in collect_route_declaration_targets) now route through the shared guard. Unit tests cover in-root, sibling-root, interior-`..` escape, and the fail-closed dangling-under-root-symlink leg. Fixes: #156 * test: ✅ Cover find_block_terminator block-comment and double-quoted heredoc legs. Two already-working but uncovered paths in `find_block_terminator`'s @php-block masking: a literal @endphp inside a `/* … */` block comment, and inside a double-quoted-label heredoc (`<<<"LABEL"`). Existing tests only reach the `//` line-comment, bare-label heredoc, and nowdoc legs. Bundled from #93/#164 (unrelated area, folded into #156's PR by request).
Summary
Implements #93.
mask_php_blockslocated the closing@endphpwith a plain substring search, so a literal@endphpinside a PHP string within a@php … @endphpblock closed the masked region early — leaving the real PHP tail wrongly treated as renameable Blade markup (the over-broad-rename bug Holmes flagged in #55/PR #89).Changes
find("@endphp")substring search inmask_php_blockswithfind_block_terminator— a single-pass, O(n) scanner that finds the real@endphpin PHP code, skipping over:'…') and double-quoted strings ("…"), honoring backslash escapes (\',\",\\);<<<LABEL … LABEL) and nowdoc (<<<'LABEL' … LABEL) bodies, with PHP 7.3+ indented-closer support;//,#(not#[attributes), and/* … */comments — so an apostrophe in a comment (// don't …) is never misread as a string opener.@endphp(mirroring the existing@phpopener guard) so@endphpunitno longer matches.skip_php_quoted,skip_to_line_end,skip_block_comment,skip_heredoc— all byte-level (multi-byte-UTF-8 safe, since the ASCII delimiters never collide with continuation bytes).Acceptance Criteria
mask_php_blocksis string-context aware —@endphpinside a single-quoted PHP string does not close the masked region@endphpin the heredoc body is ignored@endphp(before the true closer) are masked, not treated as renameable occurrences — see note belowis_template_variablereturnsfalsefor a variable whose only occurrences fall in a@phpblock containing a string-literal@endphpbefore the real closerblade_var_rename/tests.rs, mirroringphp_word_prefix_does_not_anchor_a_mask_blockblade_var_renametests pass unchangedvariable_spans) — review decision neededAC #5 is worded as
variable_spansreturning no spans in the post-fake-@endphpregion. Butvariable_spans(viamask_non_code) intentionally does not mask@phpblocks at all — the existing testphp_block_variable_is_file_scopedrequires a@php-assigned$totalto be returned as a span so a file-scoped rename rewrites its assignment. Masking@phpblocks invariable_spanswould break that.The protection AC #5/#6 describe lives in
is_template_variable→mask_non_template→mask_php_blocks(theprepare_renameadmissibility gate), which is exactly what this PR fixes: a variable confined to the masked block is now correctly rejected atprepare_rename, so the rename never reaches span computation. I implemented and tested AC #5/#6 against that gate rather than changingvariable_spans. Flagging in case you intended a broadervariable_spanschange.Test Plan
@endphpword-boundary + comment-apostrophe regression + escaped-quote/no-backtracking)cargo test— 1767 unit + 301 binary tests pass; all 50blade_var_renametests greencargo fmt --checkclean;cargo clippyclean (CI runs-D warnings)integration_testsfailures (missingtest-project/.envfixtures) reproduce identically onorigin/main— unrelated to this changeFixes #93