Even I was curious as to know when and how many times does this if len(match) != 2 is equating to true. So I added a print statement for every instance of it for all the detectors in the first week of Jan this year.

I have only seen this message for jira token detector few times for domainPat and emailPat. But I haven't done the root cause analysis for the same.

The JIRA email is slightly different because it's checking the length of the split and not the match groups. I don't really understand it's purpose, though.

@rgmz I believe this check is to prevent index errors like the one demonstrated here https://go.dev/play/p/MS8AyzGb1uq. Rather than removing the check, it would be better if we change the condition to if len(match) < 2 to ensure at least one capture group is present in the regex.

That still has the problem of silently ignoring legitimate errors. I believe that logic errors should explicitly fail, rather than quietly continuing.

That still has the problem of silently ignoring legitimate errors. I believe that logic errors should explicitly fail, rather than quietly continuing.

I disagree. It's less of a logic error and more of a incorrectly written regex issue. A single index error on a detector shouldn't cause the whole scan to fail imo.

Some patterns are precise and don't have any capture groups, so you'd only have one match (the entire matching string).

A single index error on a detector shouldn't cause the whole scan to fail imo.

This error means that the detector can never succeed and will miss valid secrets. It would be easy to detect in the build/test process, so a user should never actually ecounter this issue.

Some patterns are precise and don't have any capture groups, so you'd only have one match (the entire matching string).

Then the check could be safely removed if present.

For me to feel comfortable merging this blanket removal would require a test that ensures we would not get an index out of range error.

It would be easy to detect in the build/test process, so a user should never actually ecounter this issue.

I agree with this

For me to feel comfortable merging this blanket removal would require a test that ensures we would not get an index out of range error.

Ideally, each detector would have unit tests but only a few do at the moment.

The TestX_FromChunk case should cover this, however, it isn't clear what triggers those cases. They don't seem to run with test:
https://github.com/trufflesecurity/trufflehog/actions/runs/8912166932/job/24475012246

For me to feel comfortable merging this blanket removal would require a test that ensures we would not get an index out of range error.

@zricethezav Are we able to merge this now that ever detector has unit tests? #3771 was the last of the batch.