[Sensitive Information] US Passport Number#58
Conversation
| string key = 2; // OAuth Application Secret (prefix: sq0csp-) | ||
| } | ||
|
|
||
| message USPassportNumber { |
There was a problem hiding this comment.
I don't think you need to create a proto type for these, since the type is always sensitiveinformation.SensitiveInformation
There was a problem hiding this comment.
oversight from the merge. i'll remove it now
|
Yes it does, beside the .proto looks ready to be merged |
There was a problem hiding this comment.
Why are the proto changes required?
There was a problem hiding this comment.
there was a letf-over proto object that was not needed, but compiled in. the new version contains the up-to-date compiled proto objects
There was a problem hiding this comment.
Given that proto objects are not modified by the PR after all, can't we just leave them as-is?
There was a problem hiding this comment.
i had a wrong assumption about this. reverted the file to the version in main,
| FromMatch: func(blob []byte, keywordMatch bool) (sensitiveinformation.SensitiveInformation, bool) { | ||
| likelihood := sensitiveinformation.LikelihoodUnlikely | ||
| if keywordMatch { | ||
| likelihood = sensitiveinformation.LikelihoodVeryLikely |
There was a problem hiding this comment.
I am willing to bet they'll want to downgrade it to LikelihoodLikely. VeryLikely is rather for stuff we were able to verify checksum, or something similar.
|
|
||
| var ( | ||
| keywordsRe = simpleregex.KeywordsRe([]string{ | ||
| `\b\w*passport\w*\b`, |
There was a problem hiding this comment.
How is \b\w*passport\w*\b different from just passport ?
There was a problem hiding this comment.
the basic idea is to match passport within a boundary with an arbitrary prefix and/or suffix. for example, that regex will match all of these:
passportpassport_nous_passportus_passport_num
There was a problem hiding this comment.
Makes sense. But isn't the same achieved by just using passport?
In other words: can you point out what is filtered out by this construction?
There was a problem hiding this comment.
technically yes, with the exception that i'd need to remove the boundary (\b). let me see if there's any space for some optimization
There was a problem hiding this comment.
@SzymonDrosdzol made some updates. i'd appreciate a look when you get the chance
7bd9574 to
0274912
Compare
3 participants
Related to #57
I merged the behavior of both branches. Once this one is reviewed, the other PR/branch can be removed.