search

sillsdev / chorus

End-user collaboration library via 3-way xml merging and hg dvcs under the hood
6 stars 26 forks source link

Do not log Send/Receive passwords #339

Closed rmunn closed 7 months ago

rmunn commented 7 months ago

Fixes #337.

Most code paths use ServerSettingsModel.RemovePasswordForLog to replace the user's Send/Receive password with a string of asterisks, but there was one code path where this was missed. This PR fixes that omission. It is also extra-cautious trying to make sure that the debug logging doesn't throw exceptions: if simply calling RemovePasswordForLog ends up throwing an exception somehow, instead of returning the unmodified URI (which might still have a password in it), we simply do not log the URI. This might be over-cautious, but it's probably better to be safe.

This change is Reviewable

github-actions[bot] commented 7 months ago

Test Results

       4 files  ±0     412 suites  ±0   2h 46m 4s :stopwatch: +8s    881 tests ±0     858 :heavy_check_mark: ±0    23 :zzz: ±0  0 :x: ±0  4 032 runs  ±0  3 898 :heavy_check_mark: ±0  134 :zzz: ±0  0 :x: ±0 

Results for commit cec33501. ± Comparison against base commit ccdf57d9.

jasonleenaylor commented 1 month ago

src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs line 1084 at r1 (raw file):

              return ServerSettingsModel.RemovePasswordForLog(address.URI);
          }
          catch { /* Really don't throw trying to get extra information to log */ }

I don't believe there is any possible way that ServerSettingsModel.RemovePasswordForLog or anything on this line can throw. A try/catch inside of a catch is pretty ugly if there isn't a good reason.