Skip to content

Added commented section of Python code#3

Merged
WritingPanda merged 2 commits into
mainfrom
WritingPanda-patch-1
Oct 27, 2023
Merged

Added commented section of Python code#3
WritingPanda merged 2 commits into
mainfrom
WritingPanda-patch-1

Conversation

@WritingPanda

Copy link
Copy Markdown

Added PyGoat code and commented it out to make it easier for people to find, uncomment, and then make a PR to push to the python repo for testing out CodeQL's capabilities.

Added PyGoat code and commented it out to make it easier for people to find, uncomment, and then make a PR to push to the python repo for testing out CodeQL's capabilities.
@WritingPanda

Copy link
Copy Markdown
Author

My concern here: I'm pasting code from another open source repo and putting it in here for use in our commercial purposes. I'm wondering if that is violating some license somewhere. :(

I can rewrite the code so that it does the same thing, but it's coming from me instead of PyGoat. That's a possible solution.

@securingdev securingdev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you add a URL and line numbers of where you are retrieving this from?

This is SUPER important to include as part of the "magic" behind CodeQL and helping this point land.

I almost feel like we shouldn't include this in this way, if only because it reduces the impact that this portion of the training has. Seeing is believing, and if it's not in here beforehand, they won't suspect any "sleight of hand" tricks from us.

@WritingPanda

Copy link
Copy Markdown
Author

Could you add a URL and line numbers of where you are retrieving this from?

This is SUPER important to include as part of the "magic" behind CodeQL and helping this point land.

I almost feel like we shouldn't include this in this way, if only because it reduces the impact that this portion of the training has. Seeing is believing, and if it's not in here beforehand, they won't suspect any "sleight of hand" tricks from us.

I'm not sure I 100% agree with this assessment. They won't see this commented code when they start running the scans. After they uncomment, they'll be able to see the "magic". I am having trouble seeing where there is a loss of experience. Could you help me understand that better?

@leftrightleft

Copy link
Copy Markdown

I like that the code is commented out inline. These bootcamps contain a lot of information for both the learner and the presenters to digest and communicate. By including the code in the file, we're making the process simpler for the attendees.

In fact, I would go even farther in the simplification:

# @flaskapp.route('/login')
# def login_logging():
#     name = request.args.get('username')
#     author = request.args.get('password')
   
#     query =  "SELECT * FROM book_auth WHERE user='"+username+"'AND password='"+password+"'"
#     print(sql_query)

This way, you're not in violation of any license things, the learner isn't distracted trying to understand the code in PyGoat, and you can still talk about the fact that this isn't an SQLi issue.

NOTE: I haven't tested that this code fires an alert...

@securingdev

Copy link
Copy Markdown

So here's my thinking of it (putting my "customer hat on").

If the code is not there before I see it, I see a scan has run, and then I go and add something new and scan it - I both get to see that this is truly net-new code, and I get to witness the "magic" of CodeQL finding a vulnerability in the PR.

But more than that, it finds a different finding than I am expecting - and I have access / can see all of the other code that I originally pulled this from. It totally dismantles the "gotcha" types of things some clever security professionals try to pull when they add malicious code to a repo to "show that CodeQL doesn't work" and then we disprove that entire concept.

@WritingPanda

Copy link
Copy Markdown
Author

I'm not sure that experience is taken away with the code in the repo that is commented out. I think that's the part I'm having trouble understanding.

@securingdev securingdev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Given the addition of the link to where the content is retrieved from, I think this should be sufficient 👍

@securingdev

Copy link
Copy Markdown

Go ahead and merge @WritingPanda when you see fit 👍

@WritingPanda WritingPanda merged commit 07cadc7 into main Oct 27, 2023
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.

3 participants