Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create codeql-analysis.yml #1096

Merged
merged 8 commits into from
Oct 8, 2022
Merged

Create codeql-analysis.yml #1096

merged 8 commits into from
Oct 8, 2022

Conversation

pandyaved98
Copy link
Contributor

There are some Critical and High Priority bugs.

@pandyaved98 pandyaved98 temporarily deployed to external July 13, 2022 09:37 Inactive
@dacbd
Copy link
Contributor

dacbd commented Jul 15, 2022

Thank you, there are certainly some improvements that can be made from that scanning report. The proper domain detection for example would be a great PR if you want to take that on as well 😉.

As far as the command injection, we might be able to do more? but I feel the nature of the tool doesn't include taking unsanitized input from systems, and if a user did put cml in particular cml runner behind a service/system that took non-predetermined input the onus would be on that system to sanitize the input. Thoughts @0x2b3bfa0 ?

@dacbd
Copy link
Contributor

dacbd commented Jul 15, 2022

@pandyaved98 can you update the PR more closely reflect: https://github.com/iterative/terraform-provider-iterative/blob/master/.github/workflows/codeql-analysis.yml

Copy link
Contributor

@dacbd dacbd left a comment

Choose a reason for hiding this comment

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

@pandyaved98

This comment was marked as duplicate.

@0x2b3bfa0
Copy link
Member

Thoughts @0x2b3bfa0?

None of these issues seem critical from a security standpoint; i.e. command execution is required beforehand, rendering them futile. 🤷🏼‍♂️

Still, it would be nice to escape properly the affected values. 👍🏼

@dacbd
Copy link
Contributor

dacbd commented Jul 18, 2022

some of the values are pretty explicit in what they accept perhaps some form of input validation could be used?

@dacbd
Copy link
Contributor

dacbd commented Jul 22, 2022

@pandyaved98 if you are willing to make the requested modifications please do so; if you don't have the time or are unable to, I'll merge this to an intermediate branch to tweak. That way your contribution is not in limbo for too long.

@dacbd dacbd added the awaiting-response Waiting for user feedback label Jul 27, 2022
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to external September 6, 2022 23:30 Inactive
@0x2b3bfa0
Copy link
Member

🔔 @pandyaved98, feel free to ping us if you need workflow run approvals; I took some days of vacation and missed the last workflow run request. 🙈

@pandyaved98 pandyaved98 temporarily deployed to external September 8, 2022 06:59 Inactive
@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 11 times, most recently from baee693 to 1c5325f Compare September 12, 2022 19:25
@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 4 times, most recently from eb1e5a3 to 9e8bdf4 Compare September 12, 2022 19:59
@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 5 times, most recently from 7243fd2 to 1e081f3 Compare September 30, 2022 04:22
@0x2b3bfa0 0x2b3bfa0 force-pushed the master branch 3 times, most recently from e2bed3a to fe9a06f Compare October 8, 2022 04:28
@dacbd dacbd self-requested a review October 8, 2022 19:30
.github/workflows/codeql-analysis.yml Outdated Show resolved Hide resolved
@dacbd dacbd self-requested a review October 8, 2022 19:35
Copy link
Contributor

@dacbd dacbd left a comment

Choose a reason for hiding this comment

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

enforce style changes

@dacbd dacbd added the blocked Dependent on something else label Oct 8, 2022
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to external October 8, 2022 22:08 Inactive
@dacbd dacbd enabled auto-merge (squash) October 8, 2022 22:16
@dacbd dacbd merged commit c9ecc0c into iterative:master Oct 8, 2022
@casperdcl casperdcl added the external-request You asked, we did label Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-response Waiting for user feedback blocked Dependent on something else external-request You asked, we did
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants