Skip to content

fix: cache the app JWT with a custom client, reject a bad installation ID - #62

Merged
jferrl merged 1 commit into
mainfrom
fix/inconsistencies
Sep 17, 2026
Merged

jferrl merged 1 commit into
mainfrom
fix/inconsistencies

Conversation

@jferrl

@jferrl jferrl commented Sep 17, 2026 •

Copy link
Copy Markdown
Owner

Summary

Two paths diverged from their own equivalents. WithHTTPClient wired the raw application token source into the transport, while NewInstallationTokenSource wraps that same source in oauth2.ReuseTokenSource, so passing a custom HTTP client re-signed a JWT on every installation-token request. Separately, a zero App ID is rejected at construction but a zero installation ID was not, so it reached GitHub as /app/installations/0/access_tokens and returned a 404 that reads like an outage rather than a misconfiguration.

Changes

  • WithHTTPClient wraps the source in oauth2.ReuseTokenSource, matching the default transport; a test asserts one signing across three installation-token fetches, where it was three before
  • A non-positive installation ID fails through the existing configuration-error path before any network call, and a test asserts the server receives zero requests
  • Replaces a hand-rolled substring matcher in the tests with strings.Contains

Rationale

Both fixes route through mechanisms the package already has: the same ReuseTokenSource wrapper the default transport uses, and the same setConfigErr path that already reports a nil HTTP client and an unparseable base URL. Neither adds public API or a new failure mode.

Migration Notes

A non-positive installation ID now fails at the first Token() call instead of returning a 404 from GitHub. Both tests were written to fail against the current code first, then fixed.

…n ID

Two inconsistencies where a path diverged from its equivalent.

WithHTTPClient wired the raw application token source into the transport
while NewInstallationTokenSource wraps the same source in
oauth2.ReuseTokenSource. Supplying a custom HTTP client therefore re-signed
a JWT on every installation-token request instead of reusing one. It now
wraps the source the same way.

A zero App ID is rejected at construction, but a zero installation ID was
not, so it reached GitHub as /app/installations/0/access_tokens and came
back as a 404 that reads like an outage rather than a misconfiguration.
Non-positive IDs now fail through the existing configuration-error path,
before any network call.

Also replaces a hand-rolled substring matcher in the tests with
strings.Contains.
@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.50%. Comparing base (c69bfba) to head (a1e9c32).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   96.44%   96.50%   +0.06%     
==========================================
  Files           4        4              
  Lines         281      286       +5     
==========================================
+ Hits          271      276       +5     
  Misses         10       10              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jferrl
jferrl merged commit 14058b0 into main Sep 17, 2026
10 checks passed
@jferrl
jferrl deleted the fix/inconsistencies branch September 17, 2026 20:45
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.

1 participant