Stop user values reaching the filesystem and the generated code raw - #139
Stop user values reaching the filesystem and the generated code raw#139NaokiTsuchiya wants to merge 8 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe compiler centralizes generated script-name validation through ChangesCompiled script naming
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Compiler
participant InstanceScript
participant ScriptName
participant Scripts
participant CompiledInjector
Compiler->>InstanceScript: generate dependency expressions
InstanceScript->>ScriptName: validate dependency index
ScriptName-->>InstanceScript: return safe file component
Scripts->>ScriptName: validate script index
ScriptName-->>Scripts: return safe file component
CompiledInjector->>ScriptName: validate runtime dependency index
ScriptName-->>CompiledInjector: return safe file component or InvalidQualifier
CompiledInjector->>CompiledInjector: convert InvalidQualifier to Unbound
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #139 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 95 98 +3
===========================================
Files 11 12 +1
Lines 238 247 +9
===========================================
+ Hits 238 247 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A dependency index is `Interface-qualifier`, and the qualifier is arbitrary user input from `->annotatedWith()`. Only backslashes were replaced, so a qualifier containing `/` survived into the path: `FilePutContents` then made it a real directory via `mkdir(dirname($f), 0777, true)`, and once that directory existed a later compile of `x/../../escaped` let `rename()` place a script outside the script directory, overwriting whatever was there. The read side rebuilt the same unnormalised path, so the escaped script still resolved and anything treating the script directory as the whole artifact missed it. A NUL byte aborted the compile with a raw `ValueError` from `rename()`. `ScriptName` percent-encodes every byte outside `[A-Za-z0-9_.-]` after the existing backslash replacement, which keeps the name a single path segment and stops an encoded index landing on the file of an unencoded one. Bytes >= 0x80 are left alone: they are legal in a PHP identifier and no path separator is reachable from them. Write side, read side and generated code must agree, so all five call sites share the helper. Indexes built from class names and plain qualifiers contain no encodable byte, so compiling this project's own fixtures produces byte-identical file names.
…ing it
`InstanceScript` built the index and file path arguments with
sprintf("'%s'", ...), so a binding qualifier holding an apostrophe closed the
literal and everything after it was compiled as PHP. A qualifier of
"q'.PHP_EOL.'" produced
prototype($scriptDir, $singletons, 'Ray\Compiler\FakeEngineInterface-q'.PHP_EOL.'', ...)
which `php -l` accepts, so an arbitrary expression ran whenever the compiled
script was required. Encoding the file name does not help here; this is a
separate raw interpolation.
`var_export()` escapes the value instead. The only change to ordinary output is
`\` becoming `\\` inside the index literal, which is the same string in a
single-quoted PHP literal, so the compiled scripts keep their meaning and the
expected code in VisitorCompilerTest moves with it.
The injection point array holds a class, method and parameter name, which are
PHP identifiers and cannot carry a quote, so it stays as it is.
8fa8b64 to
63617b1
Compare
`addInstanceArg()` put `serialize()` output straight into a quoted literal, so
an apostrophe anywhere in an instance-bound array or object closed it and the
compiled script stopped being PHP:
$this->bind('')->annotatedWith('payload')->toInstance(['k' => "it's"]);
// unserialize('a:1:{s:1:"k";s:4:"it's";}')
// ParseError: syntax error, unexpected identifier "s", expecting ")"
Same defect as the dependency index, same fix. Scalars already went through
`var_export()`; now the serialized branch does too. Output only changes where
the payload holds a quote or a backslash, so the expected code in
VisitorCompilerTest is unaffected.
With this, no user-controlled value is interpolated into generated code raw.
What is left is the injection point array and the class and method names, which
are PHP identifiers and cannot carry a quote.
NaokiTsuchiya
changed the title
A binding qualifier is an identifier, not data: bytes outside [A-Za-z0-9_.-] (and >= 0x80) have no legitimate meaning in one, so ScriptName now throws InvalidQualifier at compile time instead of percent-encoding them. With the index validated, the plain interpolation in InstanceScript is safe again and the generated code stays byte-identical to 1.x. The var_export() of serialized instance values remains: that value is data, not an identifier. CompiledInjector converts InvalidQualifier to Unbound so the runtime contract of getInstance() is unchanged.
|
Thank you for this thorough work — the write-up made the problem and its history unusually easy to follow, and the We would like to propose validate-don't-sanitize as an alternative to percent-encoding, and have pushed a commit on top of this branch (
The runtime contract is also unchanged: a name that could never compile still surfaces as All tests, Psalm, PHPStan and CS pass on the updated branch. What do you think? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/InstanceScript.php`:
- Around line 99-104: Encode dependency indexes with var_export($index, true)
before interpolating them as unquoted generated-PHP arguments in the
prototype/singleton emission near InstanceScript.php lines 99-104 and the AOP
binding emission near lines 144-146. Add or update tests in
tests/QualifierScriptTest.php lines 43-58 to compile and resolve qualifiers
containing a trailing backslash and consecutive backslashes.
🪄 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 Plus
Run ID: 816edf10-e50d-48fa-8462-551c0c4b4faf
📒 Files selected for processing (16)
src/CompiledInjector.phpsrc/Exception/InvalidQualifier.phpsrc/InstanceScript.phpsrc/ScriptName.phpsrc/Scripts.phptests/Fake/FakeInstanceValueConsumer.phptests/Fake/FakeInstanceValueModule.phptests/Fake/FakePathQualifier.phptests/Fake/FakeQualifierConsumerInterface.phptests/Fake/FakeQualifierModule.phptests/Fake/FakeQualifierPathsModule.phptests/Fake/FakeQualifierPathsProvider.phptests/Fake/FakeQualifierPathsRoot.phptests/InstanceValueScriptTest.phptests/QualifierScriptTest.phptests/ScriptNameTest.php
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/Fake/FakePathQualifier.php
- tests/Fake/FakeQualifierPathsModule.php
- tests/Fake/FakeQualifierModule.php
- tests/Fake/FakeQualifierPathsRoot.php
- tests/Fake/FakeQualifierPathsProvider.php
- src/Scripts.php
- src/CompiledInjector.php
from() reads as the named-constructor idiom (BackedEnum::from(): static) but returns a plain string. forIndex() states the parameter role and breaks that association.
|
Follow-up: the helper method has been renamed
For the record: I put the question (forIndex / map / fromIndex / keep from) to two other AI reviewers (Claude and Codex). All three of us independently landed on |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ScriptName.php (1)
26-31: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEscape dependency indexes before embedding them in generated PHP.
The original
$indexis used directly insrc/InstanceScript.php::addDependencyArg()andpushAspectBind(), so backslashes are interpreted as PHP single-quoted string escapes (\\becomes\). This can make the generated script register a dependency under a different key. Pass the original index throughvar_export($index, true)at these call sites; keepScriptName::forIndex()scoped to filename normalization.🤖 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 `@src/ScriptName.php` around lines 26 - 31, The generated PHP must preserve dependency indexes containing backslashes. Update the call sites in InstanceScript::addDependencyArg() and pushAspectBind() to pass the original index through var_export($index, true) before embedding it, while keeping ScriptName::forIndex() limited to filename normalization.
🤖 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.
Outside diff comments:
In `@src/ScriptName.php`:
- Around line 26-31: The generated PHP must preserve dependency indexes
containing backslashes. Update the call sites in
InstanceScript::addDependencyArg() and pushAspectBind() to pass the original
index through var_export($index, true) before embedding it, while keeping
ScriptName::forIndex() limited to filename normalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eaf9ec3a-3965-450f-bf6d-d3b84b455b78
📒 Files selected for processing (5)
src/CompiledInjector.phpsrc/InstanceScript.phpsrc/ScriptName.phpsrc/Scripts.phptests/ScriptNameTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Scripts.php
- src/CompiledInjector.php
- tests/ScriptNameTest.php
- src/InstanceScript.php
Validation cannot reject a backslash: class-string qualifiers (annotatedWith(Foo::class)) legitimately carry one. Emitted raw into a single-quoted literal, a trailing backslash escapes the closing quote (ParseError) and consecutive backslashes collapse, shifting the singleton key between compile time and runtime. Validate what reaches the filesystem (ScriptName), escape what reaches generated code (var_export). Reported by the CodeRabbit review.
|
@coderabbitai Any blocker? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== PR diff summary ==='
git diff --stat HEAD^ HEAD || true
git status --short
printf '%s\n' '=== Relevant file outlines ==='
for file in \
src/ScriptName.php \
src/CompiledInjector.php \
src/InstanceScript.php \
src/Scripts.php \
src/Exception/InvalidQualifier.php \
tests/ScriptNameTest.php \
tests/QualifierScriptTest.php \
tests/InstanceValueScriptTest.php
do
if [ -f "$file" ]; then
echo "--- $file"
ast-grep outline "$file" --items all || true
fi
done
printf '%s\n' '=== Relevant implementation and tests ==='
for file in \
src/ScriptName.php \
src/CompiledInjector.php \
src/InstanceScript.php \
src/Scripts.php \
src/Exception/InvalidQualifier.php \
tests/ScriptNameTest.php \
tests/QualifierScriptTest.php \
tests/InstanceValueScriptTest.php
do
if [ -f "$file" ]; then
echo "--- $file"
cat -n "$file"
fi
done
printf '%s\n' '=== All ScriptName and generated literal call sites ==='
rg -n -C 3 --glob '*.php' \
'ScriptName::forIndex\s*\(|str_replace\s*\(\s*[\"'\'']\\\\\\\\|var_export\s*\(\s*(\$index|serialize\()|unserialize\s*\(' \
src tests || trueLength of output: 39483 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Compiler and visitor outlines ==='
for file in src/CompileVisitor.php src/Compiler.php; do
if [ -f "$file" ]; then
echo "--- $file"
ast-grep outline "$file" --items all
cat -n "$file"
fi
done
printf '%s\n' '=== Visitor construction and dependency dispatch ==='
rg -n -C 5 --glob '*.php' \
'new\s+CompileVisitor\s*\(|->accept\s*\(\s*new\s+CompileVisitor|->accept\s*\(\s*\$[A-Za-z_][A-Za-z0-9_]*\s*\)|function\s+visitInstance\b|visitInstance\s*\(' \
src tests
printf '%s\n' '=== Instance binding compilation paths ==='
rg -n -C 5 --glob '*.php' \
'toInstance\s*\(|new\s+Instance\s*\(|InstanceScript|CompileVisitor' \
src testsLength of output: 45104
return sprintf('return unserialize(\'%s\');', serialize($value));A serialized value containing return sprintf('return unserialize(%s);', var_export(serialize($value), true));and update the Other than that, the validate-don’t-sanitize approach, runtime conversion to ✏️ Learnings added
|
CompileVisitor::visitInstance() had the same defect InstanceScript::addInstanceArg() was fixed for in 5a070ae: serialize() output went into a quoted literal unescaped, so a quote or backslash in a top-level toInstance() value broke the compiled script for that binding's own script file (exercised via getInstance() called directly on the index, not via constructor injection). Reported by CodeRabbit on PR ray-di#139.
|
@CodeRabbit Fixed in commit 9847155 — |
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! This chat is LLM-generated and may be inaccurate. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ray-di/Ray.Compiler"
pr_number="139"
threads=$(gh api graphql \
-F owner="${repo%%/*}" \
-F name="${repo#*/}" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 1) {
nodes { author { login } }
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| select(.comments.nodes[0].author.login == "coderabbitai")
| .id')
if [ -z "$threads" ]; then
echo "No open review threads authored by coderabbitai."
exit 0
fi
while IFS= read -r thread_id; do
[ -n "$thread_id" ] || continue
gh api graphql \
-F threadId="$thread_id" \
-f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread { id isResolved }
}
}' \
--jq '.data.resolveReviewThread.thread | "Resolved thread: \(.id), isResolved=\(.isResolved)"'
done <<< "$threads"Length of output: 1362
To approve the PR, please post a top-level comment containing: This chat is LLM-generated and may be inaccurate. |
|
@coderabbitai approve |
✅ Action performedComments resolved. Approval is disabled; enable |
2 participants
Problem
A dependency index is
Interface-qualifier. Only\was replaced when building the script file name, so a qualifier from->annotatedWith()reached the filesystem raw — and both the index and->toInstance()values reached the generated code raw.annotatedWith('a/b')writes a directory, not a flat script. OnceInterface-x/exists, a laterx/../../escapedmakesrename()write outside the script dir — and the read side rebuilds the same unnormalised path, so it still resolves. Anything treating the script dir as the whole artifact misses it.ValueErrorfromrename().'closes the literal holding the index, and what follows is compiled as PHP in a scope holding$scriptDirand$singletons. A trailing\escapes the closing quote instead, and the script stops parsing. ($alone is inert — the literal is single-quoted.)toInstance(['k' => "it's"])does the same to the literal holding theserialize()output: the compiled script is no longer PHP.Fix
ScriptName::from()percent-encodes every byte outside[A-Za-z0-9_.-]after the existing\→_. The name stays one path segment, and an encoded index can never land on an unencoded one's file. Write side, read side and generated code share it.var_export()replaces both raw interpolations. No user-controlled value is interpolated into generated code any more; what is left is class and method names, which cannot carry a quote.Compatibility
Fixtures here compile to byte-identical file names (26 scripts). Only an index holding a byte outside
[A-Za-z0-9_.\-\x80-\xFF]is renamed — exactly the broken set. Generated scripts change only by\→\\inside the index literal, and inside a serialized value that holds a quote or a backslash; the same strings either way.Slash qualifiers work today, just not flat, so this needs a recompile, and so does a rollback. Minor, not patch.
Not fixed
\→_predates this and is not injective:annotatedWith(Foo\Bar::class)shares a file withannotatedWith('Foo_Bar'). Encoding\renames every script, encoding_every snake_case qualifier, and the collision takes two bindings spelling one name two ways. Characterised inScriptNameTest.Tests
Each of the five ways Ray.Di delivers a qualifier, plus instance values. All three commits green on their own.
Summary by CodeRabbit
ScriptName::forIndex(...).Unboundat runtime.