Use a neutral name to hopefully avoid false alarm #586
Conversation
| def get_lab_app( | ||
| env_client_id="LAB_APP_CLIENT_ID", | ||
| env_client_secret="LAB_APP_CLIENT_SECRET", | ||
| env_name2="LAB_APP_CLIENT_SECRET", # A var name that hopefully avoids false alarm |
There was a problem hiding this comment.
LAB_APP_CLIENT_SECRET Will this be reported next? I would only think it should be the value more than the variable name itself. We used to have this issue in the Labs, and we had to remove the word secret from our code. Is changing LAB_APP_CLIENT_SECRET to something else too hard?
There was a problem hiding this comment.
LAB_APP_CLIENT_SECRETWill this be reported next? I would only think it should be the value more than the variable name itself. We used to have this issue in the Labs, and we had to remove the wordsecretfrom our code.
That is a fair point, @gladjohn . We do not really know what the CodeQL looks into. On the other hand, the two existing (false) alarms were specifically on the lines of logging env_client_secret, not on the line of "LAB_APP_CLIENT_SECRET".
Is changing
LAB_APP_CLIENT_SECRETto something else too hard?
It would require us to change more files (.env) spreading across many machines that we are using, also changing the env var setup in our GitHub Actions. Let's do this variable name change first, and see whether it is good enough.
Thanks @bgavrilMS for noticing the recent alarm 13 and alarm 14.
They are both false alarms. In this PR, we change the name of the variable.
The CodeQL has been configured to auto scan "PRs to dev" branch, so, a green test result would supposedly mean our fix is good. But then again, none of the recent CodeQL action result was red. So, we don't really know what the color means.
P.S.: The current test automation failure is caused by a different reason. The msidlab8 seems down. Lab team has been informed.It is fixed now.