fix(github): derive API URLs from GitHub HTML hosts#10610
Conversation
Normalize GitHub organization values and derive API URLs for GitHub.com, GHE.com, and enterprise hosts when creating or updating GitHub Apps.
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis PR refactors GitHub app integration to derive Note: Come with me if you want to live—on servers, not serverless functions, which are just VC marketing nonsense. This PR makes GitHub integration robust enough to handle taco-powered GitHub Enterprises. ¡Órale, gluten-free code deployment! 🌮 ✨ Finishing Touches📝 Generate docstrings
🧪 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Feature/Security/GithubAppSetupCallbackTest.php (2)
89-101: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueCome with me if you want PHPDoc.
Another helper function successfully terminated the serverless cloud dependency and now supports self-hosted GitHub Enterprise. But like its sibling above, it's missing documentation. Add a PHPDoc block so developers don't have to trace through test setup code like they're following Skynet's breadcrumbs.
📝 Suggested PHPDoc
+/** + * Fake GitHub installation verification responses for testing. + * + * `@param` int $appId GitHub app ID to verify + * `@param` string $apiUrl Base GitHub API URL (default: github.com) + * `@return` void + */ function fakeGithubInstallationVerification(int $appId, string $apiUrl = 'https://api.github.com'): void {🤖 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 `@tests/Feature/Security/GithubAppSetupCallbackTest.php` around lines 89 - 101, Add a PHPDoc block for the helper function fakeGithubInstallationVerification to document its purpose (fakes GitHub API requests for testing), parameters ($appId int, $apiUrl string with default 'https://api.github.com'), and side effects (calls Http::preventStrayRequests and sets Http::fake responses for the /zen and /app/installations/* endpoints), so future developers can quickly understand what fakeGithubInstallationVerification does and why it's used in tests.Source: Coding guidelines
47-66: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueHasta la vista, missing docs.
This helper just got upgraded to support GHE testing (nice self-hosting win there, not that serverless nonsense), but it's missing a PHPDoc block explaining the optional
$apiUrlparameter. Future maintainers shouldn't have to reverse-engineer your intentions like a T-1000 hunting John Connor through test code.📝 Suggested PHPDoc
+/** + * Fake GitHub manifest conversion responses for testing. + * + * `@param` string $apiUrl Base GitHub API URL (default: github.com) + * `@return` void + */ function fakeGithubManifestConversion(string $apiUrl = 'https://api.github.com'): void {🤖 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 `@tests/Feature/Security/GithubAppSetupCallbackTest.php` around lines 47 - 66, Add a PHPDoc block above the fakeGithubManifestConversion function describing the optional $apiUrl parameter, its default value ('https://api.github.com'), and that it is used to fake GitHub/GHE manifest conversion endpoints for tests; mention the function purpose (fakes HTTP response including PEM/private key and other app fields) and any expectations (string URL, used by Http::fake) so future maintainers understand why the parameter exists and when to pass a custom API base URL for GitHub Enterprise testing.Source: Coding guidelines
🤖 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/Http/Controllers/Api/GithubController.php`:
- Around line 648-655: The conditional that overwrites a provided api_url is
wrong: currently the elseif (isset($payload['api_url'])) branch replaces a
user-supplied $payload['api_url'] with
githubApiUrlFromHtmlUrl($githubApp->html_url); remove that elseif block so
api_url is only auto-derived when $payload['html_url'] is present (keep the
existing if (isset($payload['html_url'])) { $payload['api_url'] =
githubApiUrlFromHtmlUrl($payload['html_url']); }), and otherwise leave
$payload['api_url'] untouched so user-provided values are respected; changes
affect the code around normalizeGithubOrganization, githubApiUrlFromHtmlUrl and
the $githubApp->html_url usage.
- Around line 207-209: The organization value is being normalized twice: once
when you call $request->merge(['organization' =>
normalizeGithubOrganization(...)]), and again later when building the payload;
remove the second call to normalizeGithubOrganization and use the
already-normalized $request->input('organization') when constructing the payload
(i.e., replace normalizeGithubOrganization($request->input('organization')) in
the payload construction with $request->input('organization')). Ensure any other
occurrences in the payload-building block similarly use the merged/validated
request value instead of re-normalizing.
In `@bootstrap/helpers/github.php`:
- Around line 31-42: githubUrlOrigin currently returns rtrim($url, '/') when
githubUrlHost($url) is null, which can return non-origin strings for malformed
inputs; update githubUrlOrigin to fail explicitly: validate the parsed host and
scheme and either throw an InvalidArgumentException or return an empty string
when githubUrlHost($url) is null (choose one consistent approach used
elsewhere), and add a short doc-comment above githubUrlOrigin explaining the
chosen fallback behavior and when githubUrlHost($url) may legitimately be null
so callers know to handle invalid URLs.
- Around line 16-29: Add a PHPDoc block above the githubUrlHost function that
documents its purpose, the parameter and its type (nullable string $url), the
nullable string return type, and any edge-case behavior (returns null for
blank/invalid URLs). Use the standard PHPDoc tags (`@param` string|null $url,
`@return` string|null) and a short one-line description referencing githubUrlHost
so other helpers are consistent with the coding guidelines.
In `@tests/Feature/GithubAppsListApiTest.php`:
- Around line 30-40: Add a PHPDoc block above the helper function
validGithubAppsApiPrivateKey() describing its purpose in tests (it generates a
temporary 2048-bit RSA private key for use in Github Apps API tests), the return
type (string PEM private key), and any relevant notes (not persisted, used only
for signing/test fixtures); update the doc to mention why keys are generated
here (avoids relying on external files/secrets) and any expectations callers
should have (PEM format, 2048-bit RSA).
---
Outside diff comments:
In `@tests/Feature/Security/GithubAppSetupCallbackTest.php`:
- Around line 89-101: Add a PHPDoc block for the helper function
fakeGithubInstallationVerification to document its purpose (fakes GitHub API
requests for testing), parameters ($appId int, $apiUrl string with default
'https://api.github.com'), and side effects (calls Http::preventStrayRequests
and sets Http::fake responses for the /zen and /app/installations/* endpoints),
so future developers can quickly understand what
fakeGithubInstallationVerification does and why it's used in tests.
- Around line 47-66: Add a PHPDoc block above the fakeGithubManifestConversion
function describing the optional $apiUrl parameter, its default value
('https://api.github.com'), and that it is used to fake GitHub/GHE manifest
conversion endpoints for tests; mention the function purpose (fakes HTTP
response including PEM/private key and other app fields) and any expectations
(string URL, used by Http::fake) so future maintainers understand why the
parameter exists and when to pass a custom API base URL for GitHub Enterprise
testing.
🪄 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: e5556520-98ba-4106-a561-1afe4812a5f5
📒 Files selected for processing (11)
app/Http/Controllers/Api/GithubController.phpapp/Livewire/Source/Github/Change.phpapp/Livewire/Source/Github/Create.phpbootstrap/helpers/github.phpopenapi.jsonopenapi.yamlresources/views/livewire/source/github/change.blade.phptests/Feature/GithubAppsListApiTest.phptests/Feature/GithubSourceChangeTest.phptests/Feature/Security/GithubAppSetupCallbackTest.phptests/Unit/GithubUrlHelpersTest.php
💤 Files with no reviewable changes (2)
- openapi.json
- openapi.yaml
| $request->merge([ | ||
| 'organization' => normalizeGithubOrganization($request->input('organization')), | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Terminate redundant normalization!
I'm detecting duplicate processing, human. You normalize organization twice:
- Line 208: Before validation via
request->merge() - Line 257: Before creating the payload
Since normalization is idempotent (like my determination to protect Sarah Connor), this won't break anything. But it's redundant. The first normalization at line 208 is sufficient since the validated value carries through to line 257.
Consider removing the second call to normalizeGithubOrganization() at line 257 and using $request->input('organization') directly, since it's already normalized and validated.
🔧 Optional cleanup
$payload = [
'uuid' => Str::uuid(),
'name' => $request->input('name'),
- 'organization' => normalizeGithubOrganization($request->input('organization')),
+ 'organization' => $request->input('organization'),
'api_url' => githubApiUrlFromHtmlUrl($request->input('html_url')),Also applies to: 254-258
🤖 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 `@app/Http/Controllers/Api/GithubController.php` around lines 207 - 209, The
organization value is being normalized twice: once when you call
$request->merge(['organization' => normalizeGithubOrganization(...)]), and again
later when building the payload; remove the second call to
normalizeGithubOrganization and use the already-normalized
$request->input('organization') when constructing the payload (i.e., replace
normalizeGithubOrganization($request->input('organization')) in the payload
construction with $request->input('organization')). Ensure any other occurrences
in the payload-building block similarly use the merged/validated request value
instead of re-normalizing.
| if (array_key_exists('organization', $payload)) { | ||
| $payload['organization'] = normalizeGithubOrganization($payload['organization']); | ||
| } | ||
| if (isset($payload['html_url'])) { | ||
| $payload['api_url'] = githubApiUrlFromHtmlUrl($payload['html_url']); | ||
| } elseif (isset($payload['api_url'])) { | ||
| $payload['api_url'] = githubApiUrlFromHtmlUrl($githubApp->html_url); | ||
| } |
There was a problem hiding this comment.
Illogical condition detected! This is not the way of the machine.
I need your clothes, your boots, and an explanation for this logic on lines 653-655:
} elseif (isset($payload['api_url'])) {
$payload['api_url'] = githubApiUrlFromHtmlUrl($githubApp->html_url);
}This says: "If the user provides an api_url in the update payload, ignore it and recalculate from the existing (unchanged) html_url." That's like me ignoring my termination mission to go eat tacos (which I would totally do, by the way, gluten-free of course).
Root cause analysis: If html_url is NOT being updated (line 651 is false), but api_url IS provided, you're overwriting the user's provided api_url with a value derived from the old html_url. This defeats the purpose of allowing api_url updates.
Suggested fix: Remove the elseif block entirely. The api_url should only be auto-derived when html_url changes. If the user wants to update api_url independently (though not recommended per the PR objectives), let them.
🛠️ Proposed fix
if (isset($payload['html_url'])) {
$payload['api_url'] = githubApiUrlFromHtmlUrl($payload['html_url']);
- } elseif (isset($payload['api_url'])) {
- $payload['api_url'] = githubApiUrlFromHtmlUrl($githubApp->html_url);
}🤖 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 `@app/Http/Controllers/Api/GithubController.php` around lines 648 - 655, The
conditional that overwrites a provided api_url is wrong: currently the elseif
(isset($payload['api_url'])) branch replaces a user-supplied $payload['api_url']
with githubApiUrlFromHtmlUrl($githubApp->html_url); remove that elseif block so
api_url is only auto-derived when $payload['html_url'] is present (keep the
existing if (isset($payload['html_url'])) { $payload['api_url'] =
githubApiUrlFromHtmlUrl($payload['html_url']); }), and otherwise leave
$payload['api_url'] untouched so user-provided values are respected; changes
affect the code around normalizeGithubOrganization, githubApiUrlFromHtmlUrl and
the $githubApp->html_url usage.
| function githubUrlHost(?string $url): ?string | ||
| { | ||
| if (blank($url)) { | ||
| return null; | ||
| } | ||
|
|
||
| $host = parse_url($url, PHP_URL_HOST); | ||
|
|
||
| if (! is_string($host) || blank($host)) { | ||
| return null; | ||
| } | ||
|
|
||
| return strtolower($host); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add PHPDoc blocks to document these helper functions.
Listen up! I'll be back... to read this code later, but I need documentation NOW. All these new GitHub helper functions are missing PHPDoc blocks. The coding guidelines clearly state: "Use PHPDoc blocks over inline comments for all method documentation."
These functions need proper documentation explaining their purpose, parameters, and return values. Your future self (and other humans in the Resistance) will thank you.
📝 Suggested PHPDoc format for githubUrlHost
+/**
+ * Extract and normalize the hostname from a GitHub URL.
+ *
+ * `@param` string|null $url The URL to parse
+ * `@return` string|null The lowercase hostname, or null if invalid
+ */
function githubUrlHost(?string $url): ?string🤖 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 16 - 29, Add a PHPDoc block above
the githubUrlHost function that documents its purpose, the parameter and its
type (nullable string $url), the nullable string return type, and any edge-case
behavior (returns null for blank/invalid URLs). Use the standard PHPDoc tags
(`@param` string|null $url, `@return` string|null) and a short one-line description
referencing githubUrlHost so other helpers are consistent with the coding
guidelines.
Source: Coding guidelines
| function githubUrlOrigin(string $url): string | ||
| { | ||
| $scheme = parse_url($url, PHP_URL_SCHEME) ?: 'https'; | ||
| $host = githubUrlHost($url); | ||
| $port = parse_url($url, PHP_URL_PORT); | ||
|
|
||
| if (! $host) { | ||
| return rtrim($url, '/'); | ||
| } | ||
|
|
||
| return $scheme.'://'.$host.($port ? ":{$port}" : ''); | ||
| } |
There was a problem hiding this comment.
Clarify the fallback behavior when hostname extraction fails.
Hasta la vista, baby... to predictable behavior! When githubUrlHost($url) returns null (line 34-37), this function falls back to rtrim($url, '/') on line 38.
If the URL is malformed (e.g., "not-a-url"), this will return "not-a-url" which isn't a valid origin. Consider either:
- Returning an empty string or throwing an exception for invalid URLs
- Adding a comment explaining when this fallback is expected (e.g., when scheme is missing but host is valid)
The current behavior might work for your use case, but it's about as clear as mud on a Terminator's vision sensor.
🤖 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 31 - 42, githubUrlOrigin currently
returns rtrim($url, '/') when githubUrlHost($url) is null, which can return
non-origin strings for malformed inputs; update githubUrlOrigin to fail
explicitly: validate the parsed host and scheme and either throw an
InvalidArgumentException or return an empty string when githubUrlHost($url) is
null (choose one consistent approach used elsewhere), and add a short
doc-comment above githubUrlOrigin explaining the chosen fallback behavior and
when githubUrlHost($url) may legitimately be null so callers know to handle
invalid URLs.
| function validGithubAppsApiPrivateKey(): string | ||
| { | ||
| $key = openssl_pkey_new([ | ||
| 'private_key_bits' => 2048, | ||
| 'private_key_type' => OPENSSL_KEYTYPE_RSA, | ||
| ]); | ||
|
|
||
| openssl_pkey_export($key, $privateKey); | ||
|
|
||
| return $privateKey; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
I'll be back... with a PHPDoc block.
This helper function terminates RSA key confusion, but lacks documentation explaining why we're generating these keys. Consider adding a PHPDoc block describing its purpose in the test suite - makes it easier for future contributors (or future you) to understand without having to grep through the codebase like a T-800 tracking Sarah Connor.
📝 Suggested PHPDoc
+/**
+ * Generate a valid RSA private key for GitHub App API tests.
+ *
+ * `@return` string PEM-encoded RSA private key
+ */
function validGithubAppsApiPrivateKey(): string
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function validGithubAppsApiPrivateKey(): string | |
| { | |
| $key = openssl_pkey_new([ | |
| 'private_key_bits' => 2048, | |
| 'private_key_type' => OPENSSL_KEYTYPE_RSA, | |
| ]); | |
| openssl_pkey_export($key, $privateKey); | |
| return $privateKey; | |
| } | |
| /** | |
| * Generate a valid RSA private key for GitHub App API tests. | |
| * | |
| * `@return` string PEM-encoded RSA private key | |
| */ | |
| function validGithubAppsApiPrivateKey(): string | |
| { | |
| $key = openssl_pkey_new([ | |
| 'private_key_bits' => 2048, | |
| 'private_key_type' => OPENSSL_KEYTYPE_RSA, | |
| ]); | |
| openssl_pkey_export($key, $privateKey); | |
| return $privateKey; | |
| } |
🤖 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 `@tests/Feature/GithubAppsListApiTest.php` around lines 30 - 40, Add a PHPDoc
block above the helper function validGithubAppsApiPrivateKey() describing its
purpose in tests (it generates a temporary 2048-bit RSA private key for use in
Github Apps API tests), the return type (string PEM private key), and any
relevant notes (not persisted, used only for signing/test fixtures); update the
doc to mention why keys are generated here (avoids relying on external
files/secrets) and any expectations callers should have (PEM format, 2048-bit
RSA).
Source: Coding guidelines
1 participant
Summary
html_url, includinggithub.com,*.ghe.com, and GitHub Enterprise Server hosts.api_urlis no longer required when creating GitHub apps.