Skip to content

Conversation

@kevin-montrose
Copy link
Contributor

I noticed while looking into another potential change* that the *CodeEvaluations get confused by named parameters if the parameter is not in it's default position.

Basically, if you have a method (and appropriate configuration)

void TheSecondParameterIsInjectable(int foo, string bar);

the invocation

TheSecondParameterIsInjectable(bar: scaryUserInput, foo: 123);

won't be flagged. The reverse case (an erroneous flag) is also possible.

This PR fixes that (and adds a small test).

The logic is complicated, so I'm not positive this is correct. I'll poke at some more weird invocation-styles (optional parameters, and extension methods mixing all the types) if I can free up some time. But since all tests are passing, this may already be ready to merge.

*I think making behavior application conditional (like the attributes in the Csrf branch) will cover a few remaining gnarly places in the Stack Overflow codebases, especially around redirects.
I'm not quite ready to submit that yet, however.

@JarLob JarLob merged commit 1c69fbf into security-code-scan:vs2017 Aug 14, 2019
@JarLob
Copy link
Contributor

JarLob commented Aug 14, 2019

Thanks for bringing it up!

JarLob pushed a commit that referenced this pull request Aug 14, 2019
* Named parameters confuses behaviors, this fixes (CSharp|VB)CodeEvaluation
so it looks at the logical position of an argument instead of it's lexical
one.
@kevin-montrose kevin-montrose deleted the vs2017-fix-named-parameter-positions branch August 15, 2019 15:22
kevin-montrose added a commit to kevin-montrose/security-code-scan that referenced this pull request Aug 15, 2019
kevin-montrose added a commit to kevin-montrose/security-code-scan that referenced this pull request Aug 15, 2019
kevin-montrose added a commit to kevin-montrose/security-code-scan that referenced this pull request Aug 15, 2019
kevin-montrose added a commit to kevin-montrose/security-code-scan that referenced this pull request Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants