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.

Sign up for GitHub

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

Jump to bottom

Fix codeql #927

Closed
wants to merge 6 commits into from
Closed

Fix codeql #927

wants to merge 6 commits into from

Conversation

AkashKumar7902
Copy link
Member

Related Issue

  • Info about Issue or bug

Closes: #[issue number that will be closed through this PR]

Describe the changes you've made

A clear and concise description of what you have done to successfully close your assigned issue. Any new files? or anything you feel to let us know!

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, local variables)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Please let us know if any test cases are added

Please describe the tests(if any). Provide instructions how its affecting the coverage.

Describe if there is any unusual behaviour of your code(Write NA if there isn't)

A clear and concise description of it.

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Screenshots (if any)

Original Updated
original screenshot updated screenshot

@AkashKumar7902
feat: option to name test cases
4a396b7 
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902
fix: changed header name
bc45034 
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902
fix: failing codeql
4bc084c 
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902
fix: failing codeql
ff1f92d 
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902
Copy link
Member Author

@charankamarapu I am having trouble solving it. also why there are multiple instances of check failure clicking on "sensitive data returned by http request header" always point to the same line ?

@charankamarapu
Copy link
Member

charankamarapu commented Sep 30, 2023 •
edited

Regarding Uncontrolled data used in path expression - The check is failing because we are using the file name given by user directly and not checking whether it is even a path like structure or not. Lets say user can give some random string etc. To solve this place some checks for the path. You can refer to this line

@charankamarapu
Copy link
Member

Regarding sensitive data returned by http request header you are adding a http header to tcsName and logging it . That is nothing but you are revealing a req header in the log statement . The error is regarding that.

@charankamarapu
Copy link
Member

I hope everything is clear now

@AkashKumar7902
fix: uncontrolled data used in path expression
36e7b34 
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902
Copy link
Member Author

AkashKumar7902 commented Sep 30, 2023 •
edited

Regarding sensitive data returned by http request header you are adding a http header to tcsName and logging it . That is nothing but you are revealing a req header in the log statement . The error is regarding that.

Hey @charankamarapu
I have resolved the Uncontrolled data used in path expression security warning but i am not quite sure how can i resolve the sensitive data returned by http request header, should i add a character like '#' in the beginning of the data to obfuscate it or something else ?
Regards,
Akash Kumar

@charankamarapu
Copy link
Member

Hey @AkashKumar7902 even I don't think there is a way to suppress this. But there is a way to filter this out of scanning . GitHub Advanced Security uses CodeQL for code scanning, and you can use // lgtm [go/some-query-id] comment against the line where warning is being shown to suppress specific warnings. You'll need to replace go/some-query-id with the specific query ID of the warning. Again, please verify this with the GitHub documentation to ensure it's the correct syntax . If that too doesn't work I will mark it as false positive and approve . But why is this being done in other PR ? Commit everything in one PR (previous PR).

@AkashKumar7902
fix: codeql error
7e0b9ae 
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 1, 2023
View reviewed changes
pkg/platform/yaml/yaml.go Dismissed Show dismissed Hide dismissed
pkg/platform/yaml/yaml.go Dismissed Show dismissed Hide dismissed
pkg/platform/yaml/yaml.go Dismissed Show dismissed Hide dismissed
pkg/platform/yaml/yaml.go Dismissed Show dismissed Hide dismissed
pkg/platform/yaml/yaml.go

file, err := os.OpenFile(yamlPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, os.ModePerm)
if err != nil {
ys.Logger.Error("failed to open the created yaml file", zap.Error(err), zap.Any("yaml file name", fileName)) // lgtm [go/clear-text-logging]

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
pkg/platform/yaml/yaml.go
return err
}
data = append(data, d...)

_, err = file.Write(data)
if err != nil {
ys.Logger.Error("failed to write the yaml document", zap.Error(err), zap.Any("yaml file name", fileName))
ys.Logger.Error("failed to write the yaml document", zap.Error(err), zap.Any("yaml file name", fileName)) // lgtm [go/clear-text-logging]

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
flows to a logging call.
@AkashKumar7902
Copy link
Member Author

AkashKumar7902 commented Oct 1, 2023 •
edited

@charankamarapu Its still not working. I have verified the query id. Also i think we require to modify the workflow file to add configuration to supress those warnings [1] [2]

P.S.: Sorry for creating a new PR. As my internet connection is metered, I am unable to setup alt since it requires a large docker image of ubuntu. So i made this new PR to test out the codeql workflow. After you will approve this PR i will add all the redundant commits into the previous PR with a single commit so my original branch remains clean.

@charankamarapu
Copy link
Member

No problem. There is no need for being sorry😅 . Regarding the comment which you have added , I gave a reference that this kind of comments are used to suppress warning . But I didn't give you the exact comment . The comment which I gave you was for LGTM workflow and we are using codeQL working (although LGTM uses codeQL internally). Find a way to suppress the warning in the case of codeQL. I will be clear next time 👍. Also Try the references which you have mentioned github/codeql#10940
github/codeql#7937
by editing the workflow.
This can be our last try and if this doesn't work I will dismiss alerts via UI.

@charankamarapu
Copy link
Member

Hey @AkashKumar7902 I have actually gone through the docs of codeQL and it was mentioned there that they have knowing didn't add this feature of using comments to suppress warnings . They suggested to go via dismissing alerts in UI . Please add the changes from this PR to the other PR you have created and request for code review.

@charankamarapu
Copy link
Member

attaching reference - github/codeql#11427

@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2023
@charankamarapu charankamarapu reopened this Oct 6, 2023
@charankamarapu
Copy link
Member

closing this PR on basis of #903

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@AkashKumar7902 @charankamarapu