Skip to content

[CSharp] Additional data extensions for sink models#13093

Merged
jcogs33 merged 16 commits intogithub:mainfrom
GeekMasher:csharp-ext
Jun 22, 2023
Merged

[CSharp] Additional data extensions for sink models#13093
jcogs33 merged 16 commits intogithub:mainfrom
GeekMasher:csharp-ext

Conversation

@GeekMasher
Copy link
Copy Markdown
Contributor

@GeekMasher GeekMasher commented May 9, 2023 •
edited
Loading

I added support for logging, ldap, command-injection, and url-redirect sinks for the data extensions. The names come from the list in the Java External Flow.

Edit:

Updated names to match #12916 :

  • command-injection
  • ldap-injection
  • log-injection
  • url-redirection

@GeekMasher GeekMasher requested a review from a team as a code owner May 9, 2023 16:43
@jcogs33
Copy link
Copy Markdown
Contributor

jcogs33 commented May 9, 2023

The names come from the list in the Java External Flow.

Would you mind updating logging to log-injection, ldap to ldap-injection and url-redirect to url-redirection to align with the updates to the Java sink kind names which will be merged in #12916? 🙏
Apologies for the hassle! 🙈

@michaelnebel
Copy link
Copy Markdown
Contributor

michaelnebel commented May 10, 2023

The names come from the list in the Java External Flow.

Would you mind updating logging to log-injection, ldap to ldap-injection and url-redirect to url-redirection to align with the updates to the Java sink kind names which will be merged in #12916? 🙏 Apologies for the hassle! 🙈

Sorry for my duplicate request via slack :-)
Also could you update the sink kind validation in ExternalFlow.qll.
Thank you!

@GeekMasher
Copy link
Copy Markdown
Contributor Author

GeekMasher commented May 10, 2023

@jcogs33 @michaelnebel Makes complete sense and happy we are following the same names across the board. My changes are pushed now

@michaelnebel
Copy link
Copy Markdown
Contributor

michaelnebel commented May 10, 2023

@jcogs33 @michaelnebel Makes complete sense and happy we are following the same names across the board. My changes are pushed now

Could you also adjust the change notes and update the validation in ExternalFlow.qll?

michaelnebel
michaelnebel reviewed May 10, 2023
View reviewed changes
Comment thread csharp/ql/lib/change-notes/2023-05-09-models-as-data.md Outdated
michaelnebel
michaelnebel previously approved these changes May 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic - thank you! :-)

@GeekMasher GeekMasher mentioned this pull request May 10, 2023
Merged
@felickz felickz mentioned this pull request May 11, 2023
Closed
@GeekMasher
Copy link
Copy Markdown
Contributor Author

GeekMasher commented Jun 8, 2023

@jcogs33 @michaelnebel I have updated this PR now and pulled in the changes from #12916. Can you both review and let me know if anything else needs changing before merging?

michaelnebel
michaelnebel previously approved these changes Jun 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

jcogs33
jcogs33 reviewed Jun 9, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@jcogs33 jcogs33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple questions. 🙂

Comment thread csharp/ql/lib/semmle/code/csharp/dataflow/ExternalFlow.qll Outdated
Comment thread csharp/ql/lib/semmle/code/csharp/security/dataflow/CommandInjectionQuery.qll Outdated
@GeekMasher @jcogs33
Update csharp/ql/lib/semmle/code/csharp/security/dataflow/CommandInje…
861ac17 
…ctionQuery.qll

Co-authored-by: Jami <57204504+jcogs33@users.noreply.github.com>
jcogs33
jcogs33 previously approved these changes Jun 20, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@jcogs33 jcogs33 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now. (assuming the CI checks all pass)

michaelnebel
michaelnebel previously approved these changes Jun 21, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@GeekMasher GeekMasher dismissed stale reviews from michaelnebel and jcogs33 via 0fcc1cb June 22, 2023 12:30
michaelnebel
michaelnebel approved these changes Jun 22, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jcogs33 jcogs33 merged commit 3fed279 into github:main Jun 22, 2023
@GeekMasher GeekMasher deleted the csharp-ext branch June 22, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelnebel michaelnebel michaelnebel approved these changes

@jcogs33 jcogs33 jcogs33 approved these changes

Assignees

No one assigned

Labels

C# documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GeekMasher @jcogs33 @michaelnebel
⬇