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

[MRG] Work around LGTM errors: Clear-text logging of sensitive information #681

Merged
merged 1 commit into from Dec 21, 2021
Merged

[MRG] Work around LGTM errors: Clear-text logging of sensitive information #681

merged 1 commit into from Dec 21, 2021

Conversation

DimitriPapadopoulos
Copy link
Contributor

Disable false positives that cannot be fixed by LGTM, because their heuristic detects strings such "id" that are actually very common in DICOM, for example in "UID".

Tasks

  • Unit tests added that reproduce issue or prove feature is working
  • Fix or feature added
  • Documentation and examples updated (if relevant)
  • Unit tests passing and coverage at 100% after adding fix/feature
  • Type annotations updated and passing with mypy
  • Apps updated and tested (if relevant)

Disable false positives that cannot be fixed by LGTM, because their
heuristic detects strings such "id" that are actually very common in
DICOM, for example in "UID".
@codecov
Copy link

codecov bot commented Dec 19, 2021

Codecov Report

Merging #681 (f8f7c39) into master (d36ac19) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #681   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines         8620      8620           
=========================================
  Hits          8620      8620           

@scaramallion scaramallion mentioned this pull request Dec 21, 2021
3 tasks
@DimitriPapadopoulos DimitriPapadopoulos deleted the py/clear-text-logging-sensitive-data branch December 21, 2021 06:22
@DimitriPapadopoulos DimitriPapadopoulos restored the py/clear-text-logging-sensitive-data branch December 21, 2021 06:43
@DimitriPapadopoulos
Copy link
Contributor Author

Was this pull request closed on purpose? I suspect it wasn't closed on purpose in #717, but of course it's OK if that's the case - but I would humbly ask for a chance to explain why I find this pull request useful.

@scaramallion
Copy link
Member

Ah, sorry, got the numbers mixed up, it was meant to be #677

But yeah, I wasn't really convinced of the worth of adding another config file anyway. Explain away!

@DimitriPapadopoulos
Copy link
Contributor Author

The output of the LGTM.com static analyser is visible to anyone. It is associated to GitHub and is not opt-in:
https://lgtm.com/projects/g/pydicom/pynetdicom/

You could of course decide you don't care. Another course of action would be to have a look at the output of static analysis tools and fix the actual real positives (probably a minority) and silence the false positives. The purpose of this config file is to silence the false positives. The valid names for the config file are either lgtm.yml or .lgtm.yml (hidden file).

@scaramallion
Copy link
Member

Fair enough, you've convinced me

@scaramallion scaramallion reopened this Dec 21, 2021
@scaramallion scaramallion merged commit ba60768 into pydicom:master Dec 21, 2021
@DimitriPapadopoulos DimitriPapadopoulos deleted the py/clear-text-logging-sensitive-data branch December 21, 2021 22:49
@DimitriPapadopoulos
Copy link
Contributor Author

Hopefully I will now be able to fix a few more real positives and find a way to silence false positives in a way that doesn't hide future real positives.

@DimitriPapadopoulos
Copy link
Contributor Author

Because Semmle has joined GitHub, LGTM.com will be deprecated and replaced by GitHub code scanning.

The next step for LGTM.com: GitHub code scanning!

As far as I can understand, in simple cases such as this one, automated pull requests will be created to help us migrate:

We will do our best to help migrate repositories that actively use LGTM.com to flag potential security issues in their pull requests. For those repositories, we will create pull requests that add a GitHub Actions workflow that runs code scanning.

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.

None yet

2 participants