Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Temporal API key to scanner #83

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mattkim
Copy link

@mattkim mattkim commented Nov 3, 2023

I added patterns for Temporal API keys.

Every Temporal API key has a prefix "tmprl_" follow by two alphanumeric ids.

Pending tests passing.

@mattkim mattkim changed the title add Temporal api key to scanner add Temporal API key to scanner Nov 3, 2023
@aegilops
Copy link
Collaborator

aegilops commented Nov 9, 2023

I did take a look at this, and a check over GitHub Code Search with the regex revealed the potential for false positives, even without deeper testing.

Could you tighten up on length constraints on the two alphanumeric parts of the pattern please? At the moment the lack of a lower limit means it can easily match on variable names in code.

Knowing the exact lengths of those alphanumeric parts could really make the FP potential plummet.

Copy link
Collaborator

@aegilops aegilops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did take a look at this, and a check over GitHub Code Search with the regex revealed the potential for false positives, even without deeper testing.

Could you tighten up on length constraints on the two alphanumeric parts of the pattern please? At the moment the lack of a lower limit means it can easily match on variable names in code.

Knowing the exact lengths of those alphanumeric parts could really make the FP potential plummet.

@aegilops
Copy link
Collaborator

@mattkim if you've got time to take a look at the requested changes I asked for then I can look at retesting and merging the pattern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants