implement tri-state verification in FTP detector #1604
Conversation
| ftpErr := &textproto.Error{} | ||
| return errors.As(e, &ftpErr) && ftpErr.Code == ftpNotLoggedIn |
| Redacted: "ftp://dlpuser:*************************@ftp.dlptest.com", | ||
| Redacted: "ftp://dlpuser:********@ftp.dlptest.com", |
There was a problem hiding this comment.
Curious why some of the tests were adjusted? Did our redaction code update?
There was a problem hiding this comment.
i guess so? they were failing on main and i didn't think that figuring out why was worth it
| for i := range got { | ||
| got[i].Raw = nil | ||
|
|
||
| if (got[i].VerificationError != nil) != tt.wantVerificationErr { | ||
| t.Fatalf("wantVerificationError = %v, verification error = %v", tt.wantVerificationErr, got[i].VerificationError) | ||
| } | ||
| } | ||
| if diff := pretty.Compare(got, tt.want); diff != "" { | ||
| t.Errorf("URI.FromData() %s diff: (-got +want)\n%s", tt.name, diff) | ||
| opts := cmpopts.IgnoreFields(detectors.Result{}, "VerificationError") | ||
| if diff := cmp.Diff(got, tt.want, opts); diff != "" { | ||
| t.Errorf("FTP.FromData() %s diff: (-got +want)\n%s", tt.name, diff) | ||
| } |
There was a problem hiding this comment.
Nit / question: Could we add "Raw" to the cmpopts.IgnoreFields and remove setting it to nil on line 137? If so, we could convert that loop into iterating over values also.
There was a problem hiding this comment.
good thought moving Raw into the ignorelist, but i don't follow the second part of your suggestion
There was a problem hiding this comment.
I think the only reason we were iterating over the indices was to clear out the Raw value in the object, so now that Raw is being ignored we don't need to do that and could update the loop to be:
for _, gotResult := range got {
if (gotResult.Verification != nil) != tt.wantVerificationErr {
// ...
}
}There was a problem hiding this comment.
oh, no, it's dumber than that. the root problem is that we know when we want to see a verification error, but we don't always know the exact shape of the error, so there's a top-level "is there an error" flag. but verification errors are per result, not per overall chunk flow, so we loop through and check the expected flag against each individual result object.
there's probably a better way to do it, though - i haven't thought about it too hard.
This PR implements tri-state verification in the FTP detector. The verification timeout was made injectable to support a new test case.
The dogfood/Github action detection of "new" secrets is just picking up new test cases that use the existing test FTP secrets, which are already cleartext elsewhere in the codebase. (They're publicly available.)