fix(git): use cloud install path for ghe apps - #10576
Conversation
Move GitHub App JWT generation and slug synchronization into shared helpers so installation URLs use the canonical GitHub slug. Encode GHE organization path segments and keep the app-scoped fallback for blank organizations.
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis PR adds clock-drift validation and GitHub App JWT generation helpers, plus syncGithubAppName() to fetch and persist a GitHub App's slug/name (also updating the related PrivateKey). generateGithubToken() now asserts clock sync. getInstallationPath() calls syncGithubAppName() and builds install URLs that detect .ghe.com hosts and optionally include a URL-encoded organization segment. The Livewire component's updateGithubAppName() was simplified to validate inputs, clear cached relations, and delegate name synchronization to the new helper. Tests were added/updated to cover enterprise URL shapes and mocked /app responses. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Livewire/Source/Github/Change.php`:
- Around line 315-319: Update the success (and any related info) user-facing
message so it references the GitHub App "private key" rather than "SSH key": in
the block that sets $this->name from $appSlug (where $this->dispatch is called)
change the success copy to mention "Private key name" (and adjust the
info/fallback message if it references SSH key) to correctly reflect that this
action renames the GitHub App PrivateKey record.
- Around line 307-313: The code is reading persisted $this->github_app fields
instead of the Livewire form values (appId/privateKeyId); update the checks and
sync call to use the Livewire properties: replace the
PrivateKey::...->find($this->github_app->private_key_id) lookup with
PrivateKey::ownedByCurrentTeam()->find($this->privateKeyId) (or the actual
Livewire property name for the edited private key), and pass a temporary github
app object or clone of $this->github_app with its app_id and private_key_id set
from $this->appId and $this->privateKeyId into syncGithubAppName (or otherwise
call syncGithubAppName using the live form values) so synchronization uses the
unsaved form values prior to submit().
In `@bootstrap/helpers/github.php`:
- Around line 149-155: Before calling the GitHub /app endpoint, run the same
clock-skew preflight used by generateGithubToken() so the host clock is
validated/adjusted (the Date header check) prior to creating/signing the JWT;
update the code around the Http::...->get("{$source->api_url}/app") call in
syncGithubAppName()/bootstrap/helpers/github.php to invoke that clock-skew
guard, and do the same for the other /app call area referenced (around the
177-182 block) so both paths use the same Date-header preflight before making
requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf6b0f73-e6b4-4c47-bd2c-88d57e93aef3
📒 Files selected for processing (3)
app/Livewire/Source/Github/Change.phpbootstrap/helpers/github.phptests/Feature/GithubSourceChangeTest.php
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bootstrap/helpers/github.php (1)
17-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClock sync check becomes a friendly target practice if /zen request fails.
Listen to me very carefully.
Carbon::parse(null)returns the current time, not an exception. If the/zenendpoint is unreachable, returns a non-2xx status, or doesn't include aDateheader,$timeDiffwill be approximately zero. The clock check will pass when it shouldn't—like letting a T-800 into a bunker because the door sensor was offline.Also, no timeout specified. Self-hosted servers deserve better than hanging indefinitely waiting for GitHub. That's how terminators get you.
🛡️ Proposed fix with proper error handling
function assertGithubClockInSync(string $apiUrl): void { - $response = Http::get("{$apiUrl}/zen"); + $response = Http::timeout(10)->get("{$apiUrl}/zen"); + + if (! $response->successful()) { + throw new Exception( + 'Failed to verify clock synchronization with GitHub API. '. + 'HTTP status: '.$response->status() + ); + } + + $dateHeader = $response->header('date'); + if (blank($dateHeader)) { + throw new Exception('GitHub API did not return a Date header for clock synchronization.'); + } + $serverTime = CarbonImmutable::now()->setTimezone('UTC'); - $githubTime = Carbon::parse($response->header('date')); + $githubTime = Carbon::parse($dateHeader); $timeDiff = abs($serverTime->diffInSeconds($githubTime));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bootstrap/helpers/github.php` around lines 17 - 33, The clock-sync check in assertGithubClockInSync currently assumes Http::get("{$apiUrl}/zen") succeeds and that response->header('date') is present; if the request fails, Carbon::parse(null) yields now and makes the check silently pass. Update assertGithubClockInSync to (1) call Http::get with a timeout option, (2) verify the response is successful (2xx) and that response->header('date') is non-empty, (3) throw a clear Exception if the request fails or the Date header is missing, and only then call Carbon::parse on the header and compute the diff; keep the existing error message formatting when throwing for out-of-sync clocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@bootstrap/helpers/github.php`:
- Around line 17-33: The clock-sync check in assertGithubClockInSync currently
assumes Http::get("{$apiUrl}/zen") succeeds and that response->header('date') is
present; if the request fails, Carbon::parse(null) yields now and makes the
check silently pass. Update assertGithubClockInSync to (1) call Http::get with a
timeout option, (2) verify the response is successful (2xx) and that
response->header('date') is non-empty, (3) throw a clear Exception if the
request fails or the Date header is missing, and only then call Carbon::parse on
the header and compute the diff; keep the existing error message formatting when
throwing for out-of-sync clocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6744b25-37f3-47f6-a33d-fc48dd637e97
📒 Files selected for processing (2)
app/Livewire/Source/Github/Change.phpbootstrap/helpers/github.php
5888fee to
403f8ab
Compare
|
Thank you for the PR! 💜 |
2 participants
Changes
Issues
/github-apps/path and 404s #10573Category
Preview
Not applicable; this is backend URL generation for GitHub App installation links.
AI Assistance
If AI was used:
Testing
Contributor Agreement
Important