Scan results cache secrets inside the app


As mentioned in the design considerations

If user uses the Security app to find a secret that was accidentally committed and then fixes the error by rewriting history, the secret should be gone entirely. We shouldn’t cache a copy of the offending secret in our own data structures, otherwise the secret would still be available to an attacker via our scan reports.

Unfortunately, we're currently caching the matched secrets inside InvalidLine.getLine This is used when displaying the failures in the "Security Scan" page.

Instead of displaying the actual found secrets, we should just have a link to the corresponding filename and line number at the right commit sha1. The current UI has links to the files, but it uses the branch name instead of the actual commit hash – we should use the actual commit.




George V @Mohami
June 3, 2020, 1:32 PM

That proposal sounds good too as long as the performance is OK. We also need to handle the case where the original vulnerability has been deleted

George V @Mohami
June 3, 2020, 1:38 PM

To avoid having to re-parse the file when displaying the vulnerabilities, we could also store the beginning and ending byte offsets in addition to the line number. That would make extracting the output from cat-file very easy

Andrey Levchenko
June 3, 2020, 2:16 PM

It turns out I missed urgency of the issue. So I’m going to skip blob parsing at all.
We can create a separate issue for that.

George V @Mohami
June 3, 2020, 2:38 PM

I think it’s better to implement it well, even if it adds a few days to the turnaround time. It’ll be less effort than having to go back and revisit this with another ticket. What do you think?

Mohammed Davoodi
June 3, 2020, 3:26 PM

I missed this ticket conversation but have been talking to Andrey on slack. We should implement the line loading now instead of later.


Andrey Levchenko


George V @Mohami





Github URL