Fix false-positive script injection warnings when using builtin functions with stable outputs #459
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.
Problem
Currently there are cases when
actionlint
falsely assumes expression to be unsafe when used inside scripts and warns about possible script injection.For instance, following step description:
would lead to warning:
Why do I assume this to be false positive
The only place which I suppose relevant to the issue is this: https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections
Thus, if the point of this lint rule is to ensure that script injection is not possible, example above is definitely false positive.
It's false positive because there's no way for attacker to get some arbitrary output from the
contains
function.What is done in this PR?
parent
field to every expression AST nodeparent
field to traverse the AST and search forFunctionCallNode
parentsCallee
considered safe for script injection attacks - we skip all further insecure input checks in this AST leaf.Which functions considered safe?
Basically, I've chosen only functions which are returning booleans. Maybe there's more, but I decided to start with these:
contains
startsWith
endsWith
P.S.
If there's a better way to implement this - I would love to apply any fixes required for this PR to get into
main
.I've added the
parent
to not pollute the calls tosema.check
with additional argument just for this case.I hope additional memory usage would not be that high, since it's just an additional pointer per AST node.