fix: scan .phtml files as PHP - #2037
Open
arpitjain099 wants to merge 1 commit into
Open
arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
enry classifies .phtml as HTML+PHP, not PHP, and languagescanner.Scan returns early when the detected language is not in the language's EnryLanguages list. PHP declares only "PHP", so every .phtml file is dropped before it is read, with no skipped-file warning and no effect on the exit code. .phtml is the default template extension for Magento 2 and Laminas, so the templates that render user input in those projects have never been scanned. Byte-identical vuln.php and vuln.phtml: the .php copy reports php_lang_exec_using_user_input CRITICAL and php_lang_raw_output_using_user_input HIGH, the .phtml copy reports nothing. A .phtml with HTML wrapped around the PHP block, which is the shape these templates actually take, is detected correctly once the language is accepted, so tree-sitter-php already handles mixed content. javascript already lists three enry languages for the same reason. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This branch has not been deployed
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
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.
.phtmlfiles are never scanned. enry classifies them asHTML+PHPrather thanPHP:and
languagescanner.Scanbails before reading the file when the detected language is not in the list the language declares:PHP declares only
"PHP". There is no warning and no effect on the exit code, so the file is simply absent from the run.Byte-identical
vuln.phpandvuln.phtmlin one directory, through the built CLI:The
.phtmlcopy contributes nothing. After the change both report the same two findings..phtmlis the default template extension for Magento 2 and Laminas, so this is not an unusual file to have in a PHP codebase; it is where the output rendering lives, which is exactly where the XSS rules are aimed.A template in the shape those projects actually use, HTML with an embedded PHP block rather than a bare
<?phpfile, is also detected correctly once the language is accepted:so
tree-sitter-phpalready parses mixed content and the enry string was the only barrier.javascriptalready declares three enry languages for the same reason, so the multi-value list is the existing pattern rather than a new one.Verification
TestEnryLanguagesCoversPhtmlasserts both entries are present, and also asserts enry still classifies.phtmlasHTML+PHP, so if that ever changes upstream the test says so rather than silently passing. It fails onmain.go test ./pkg/languages/...is 12 packages ok with no failures, andgo vetandgofmt -lare clean.One thing I left alone:
GoclocLanguages()still returns["PHP"]. gocloc uses its own language names, so I did not want to guess at the right string for line-count statistics without checking what it calls these files. Happy to follow up if it matters.