Skip to content

fix: Update GitHub token detector for new App token format#961

Open
Mallikarjunadevops wants to merge 1 commit into
Yelp:masterfrom
Mallikarjunadevops:fix-github-app-tokens
Open

fix: Update GitHub token detector for new App token format#961
Mallikarjunadevops wants to merge 1 commit into
Yelp:masterfrom
Mallikarjunadevops:fix-github-app-tokens

Conversation

@Mallikarjunadevops

Copy link
Copy Markdown

Issue 958: GitHubTokenDetector misses new GitHub App installation token format

What kind of change does this PR introduce?
Bug fix

What is the current behavior?
GitHubTokenDetector uses a regex that only matches classic 36-character prefixes (like ghp_...). It fails to match the new GitHub App installation token format (ghs_APPID_JWT) which is ~520 characters long. (Fixes #958)

What is the new behavior?
Updates the GitHub token regex in detect_secrets/plugins/github_token.py to allow matching up to 519 characters with periods, hyphens, etc. re.compile(r'(ghp|gho|ghu|ghs|ghr)_[A-Za-z0-9_.-]{36,519}'). This covers the new App installation token formats.

Does this PR introduce a breaking change?
No

Other information:
Added test case in tests/plugins/github_token_test.py with a dummy new token format ghs_12345_eyJhbGciOiJSUzI1NiJ9.A*100.B*100 and verified the detector successfully flags it.

Testing Evidence

Pytest completed successfully for all test cases in tests/plugins/github_token_test.py.

@omobolajiadeyan omobolajiadeyan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for covering the new installation-token rollout. I reproduced this branch locally, and there are two correctness issues to address before merge:

  1. GitHub changed only installation tokens (ghs_). The current pattern applies {36,519} plus JWT punctuation to ghp, gho, ghu, and ghr as well, so those token types can now consume arbitrary long dotted/hyphenated strings. GitHub's current recommended compatibility regex is specifically ghs_[A-Za-z0-9\.\-_]{36,}. It also deliberately has no upper bound because clients should treat the token as opaque; 519 reintroduces a length assumption and may truncate a valid token.
  2. The prefix alternation remains a capturing group. RegexBasedDetector.analyze_string() uses findall(), so this branch yields only the captured prefix. My direct check produced finding_values=['ghp'] for a classic token and finding_values=['ghs'] for the new sample, rather than the full secret. The existing count-only test does not catch this.

A compatible shape would be two non-capturing/full-match patterns, for example:

re.compile(r'(?:ghp|gho|ghu|ghr)_[A-Za-z0-9_]{36}')
re.compile(r'ghs_[A-Za-z0-9._-]{36,}')

The second pattern covers both classic and stateless ghs_ tokens. Please add assertions on the exact detected secret_value, not only len(output), plus a stateless sample longer than 519 characters to protect the opaque/unbounded requirement.

Local verification performed: python -m pytest tests/plugins/github_token_test.py -v (4 passed), followed by direct analyze_string() and analyze_line() inspection.

Official guidance:

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.

GitHubTokenDetector misses new GitHub App installation token format (ghs_APPID_JWT, ~520 chars) — false negative during active rollout

2 participants