feat: Support for github enterprise urls added (custom domains) #273
Conversation
📝 WalkthroughWalkthroughAdds GitHub Enterprise API URL and SkipTLSVerify options across CLI, config, and client layers; updates the GitHub client to support enterprise base/upload URLs and optional TLS verification skipping; adjusts config validation and testdata, expands tests, and adds Server.Reload to reload and reconcile pools at runtime. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (New / Reload)
participant Server as Server
participant Config as Config Loader/Validator
participant GHFactory as GitHub.NewClient
participant Transport as HTTP Transport
participant GHAPI as GitHub API
Caller->>Server: New(cfg) or Reload(ctx)
Server->>Config: Load & Validate()
alt invalid (SkipTLSVerify true without EnterpriseApiUrl)
Config--xServer: validation error
else valid
Config-->>Server: validated config
Server->>GHFactory: NewClient(appID, key, enterpriseURL, skipTLS)
alt skipTLS == true
GHFactory->>Transport: clone default transport, set InsecureSkipVerify & MinVersion TLS1.2
else
GHFactory->>Transport: use default transport
end
GHFactory->>GHFactory: create AppsTransport & GH client (ghinstallation)
alt enterpriseURL provided
GHFactory->>GHFactory: WithEnterpriseURLs(baseURL, uploadURL) -> set client BaseURL/UploadURL
end
GHFactory-->>Server: configured GitHub client
Server->>GHAPI: use client for API calls / installations
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/server.go (3)
278-278: Critical: Race condition on pool.config mutation.Directly mutating
pool.configwhile the pool is running creates a data race. The pool's goroutines may be reading frompool.configconcurrently (e.g., in the pool's reconciliation loop). This violates Go's memory model and can lead to undefined behavior.Consider one of these approaches:
- Add a synchronized
UpdateConfigmethod to the Pool type that uses the pool's mutex- Stop the pool, update config, and restart it
- Use atomic operations or proper synchronization
Example approach 1:
// In pool.go, add: func (p *Pool) UpdateConfig(newConfig *PoolConfig) { p.l.Lock() defer p.l.Unlock() p.config = newConfig } // Then in server.go: pool.UpdateConfig(poolConfig)
266-294: Major: GitHub client not recreated during reload.The
Reloadmethod updates pool configurations but does not recreate the GitHub client whenEnterpriseApiUrlorSkipTLSVerifysettings change in the config. Since these are the main features added by this PR, users expecting to update enterprise URLs or TLS settings via reload will find that changes don't take effect until a full server restart.After loading the new config (line 271), recreate the GitHub client and update all pools:
if err := s.config.Load(); err != nil { return fmt.Errorf("loading config: %w", err) } + + // Recreate GitHub client if enterprise settings changed + gclient, err := github.NewClient( + s.config.GitHub.AppID, + s.config.GitHub.AppPrivateKey, + s.config.GitHub.EnterpriseApiUrl, + s.config.GitHub.SkipTLSVerify, + ) + if err != nil { + return fmt.Errorf("creating github client: %w", err) + } + s.github = gclientThen ensure all pools (new and existing) use the updated client.
275-291: Major: Pools removed from config are not stopped.When a pool is removed from the config file,
Reloaddoes not stop the removed pool or remove it froms.pools. The pool will continue running indefinitely, consuming resources and potentially registering runners.Track which pools should be removed and stop them:
+ // Track current pool names + newPoolNames := make(map[string]bool) + for _, poolConfig := range s.config.Pools { + newPoolNames[poolConfig.Name] = true + } + + // Stop pools that are no longer in config + for name, pool := range s.pools { + if !newPoolNames[name] { + s.logger.Info().Msgf("Stopping removed pool %s", name) + pool.Stop() + delete(s.pools, name) + } + } + for _, poolConfig := range s.config.Pools {
🧹 Nitpick comments (6)
helper/github/client_test.go (3)
26-35: Assert skipTLS setting to cover TLS variants.Add an assertion to ensure the flag propagates:
client, err := NewClient(12345, string(key), "", true) assert.NoError(t, err) assert.NotNil(t, client) +assert.True(t, client.skipTLSVerify)
50-53: Also validate UploadURL for Enterprise.Ensure the enterprise upload base is configured:
assert.Contains(t, client.Client.BaseURL.String(), fakeEnterpriseURL) assert.Equal(t, client.transport.BaseURL, fakeEnterpriseURL) +assert.Contains(t, client.Client.UploadURL.String(), "/upload/")
104-109: Installation client: verify Enterprise UploadURL too.assert.NotNil(t, installation) // Should retain enterprise base URL assert.Contains(t, installation.BaseURL.String(), fakeEnterpriseURL) +assert.Contains(t, installation.UploadURL.String(), "/upload/")server/config.go (1)
30-35: Idiomatic naming for initialisms.Consider renaming EnterpriseApiUrl → EnterpriseAPIURL for Go style; keep YAML tag unchanged.
-type GitHubConfig struct { - ... - EnterpriseApiUrl string `yaml:"enterprise_api_url" validate:"omitempty,url"` -} +type GitHubConfig struct { + ... + EnterpriseAPIURL string `yaml:"enterprise_api_url" validate:"omitempty,url"` +}Propagate rename in references (DefaultConfig, uses) in this PR or in a follow‑up.
helper/github/client.go (2)
36-43: Set Enterprise UploadURL; WithEnterpriseURLs expects both.Compute the upload URL from the API base (GHES: /upload/api/v3/), ensure trailing slash.
- if githubEnterpriseApiUrl != "" { - ghClient, err = ghClient.WithEnterpriseURLs(githubEnterpriseApiUrl, "") + if githubEnterpriseApiUrl != "" { + base := githubEnterpriseApiUrl + if !strings.HasSuffix(base, "/") { + base += "/" + } + upload := base + if strings.Contains(base, "/api/v3/") { + upload = strings.Replace(base, "/api/v3/", "/upload/api/v3/", 1) + } + ghClient, err = ghClient.WithEnterpriseURLs(base, upload) if err != nil { return nil, err } - transport.BaseURL = githubEnterpriseApiUrl + transport.BaseURL = base }Add import:
import ( "crypto/tls" "net/http" + "strings" )
58-66: Nil‑safety and resiliency in Installation enterprise setup.Avoid potential nil deref and fall back cleanly.
- if c.Client.BaseURL != nil && c.Client.BaseURL.String() != "https://api.github.com/" { - if enterpriseClient, err := installationClient.WithEnterpriseURLs( - c.Client.BaseURL.String(), - c.Client.UploadURL.String(), - ); err == nil { + if c.Client.BaseURL != nil && c.Client.BaseURL.String() != "https://api.github.com/" { + upload := "" + if c.Client.UploadURL != nil { + upload = c.Client.UploadURL.String() + } + if enterpriseClient, err := installationClient.WithEnterpriseURLs( + c.Client.BaseURL.String(), upload, + ); err == nil { return enterpriseClient } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
helper/github/client.go(2 hunks)helper/github/client_test.go(2 hunks)server/config.go(4 hunks)server/config_test.go(1 hunks)server/server.go(7 hunks)server/testdata/config1.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/server.go (3)
helper/github/client.go (1)
NewClient(20-50)client.go (1)
NewClient(90-104)server/pool.go (1)
NewPool(61-105)
helper/github/client_test.go (1)
helper/github/client.go (2)
NewClient(20-50)Client(12-16)
server/config.go (1)
server/server.go (1)
New(43-126)
🪛 ast-grep (0.39.6)
helper/github/client.go
[warning] 23-23: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
| image: ghcr.io/hostinger/fireactions/runner:ubuntu-20.04-x64-2.310.2 | ||
| image_pull_policy: IfNotPresent | ||
| group_id: 1 | ||
| organization: hostinger |
There was a problem hiding this comment.
YAML violates validator constraints; tests will fail.
- image_pull_policy must be one of: always never ifnotpresent (case‑sensitive).
- If enforcing “skip TLS only with Enterprise,” provide a non‑empty enterprise_api_url.
Apply:
- image_pull_policy: IfNotPresent
+ image_pull_policy: ifnotpresent
@@
- image_pull_policy: IfNotPresent
+ image_pull_policy: ifnotpresentAnd set an enterprise API base:
- enterprise_api_url: "" # optional, empty = github.com, otherwise i.e. https://example.com/api/v3, or https://api.example.com
+ enterprise_api_url: "https://api.githubenterprise.example.com/api/v3/"Also applies to: 50-53
🤖 Prompt for AI Agents
In server/testdata/config1.yaml around lines 28-31 (and similarly lines 50-53),
the YAML violates validator constraints: change image_pull_policy from
"IfNotPresent" to the allowed case-sensitive value "ifnotpresent", and if you
are enforcing the “skip TLS only with Enterprise” option ensure
enterprise_api_url is set to a non-empty valid base URL (e.g.,
https://enterprise.example.com) rather than empty; update both occurrences
accordingly.
amreshh
force-pushed
the
main
branch
2 times, most recently
from
4fb14b2 to
9ef8240
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/server.go (1)
265-294: Three critical issues verified and require fixes.All three issues in the original review are confirmed:
Orphaned pools: Pools removed from config continue running. No cleanup mechanism exists.
GitHub client not reloaded: The
s.githubclient is never recreated when GitHub configuration changes, causing inconsistency between existing and newly created pools.Race condition confirmed: The
configfield in Pool struct (line 38 of pool.go) has no mutex protection. Line 278 of server.go writespool.config = poolConfigwithout synchronization, while Pool.Start() loop (line 115) concurrently reads config fields likep.config.MinRunnersandp.config.MaxRunners(lines 120, 124, 176, etc.) without locks. This creates a classic data race.All three issues require fixes before merging. The suggested fixes in the original review comment are appropriate and necessary.
♻️ Duplicate comments (3)
server/config.go (1)
33-34: Wire the custom validator to SkipTLSVerify.
skiptls_if_enterpriseis registered but never runs because the field lacks the tag. This allows insecure TLS for github.com.Apply:
type GitHubConfig struct { AppPrivateKey string `yaml:"app_private_key" validate:"required"` AppID int64 `yaml:"app_id" validate:"required"` EnterpriseApiUrl string `yaml:"enterprise_api_url" validate:"omitempty,url"` - SkipTLSVerify bool `yaml:"skip_tls_verify" validate:""` + SkipTLSVerify bool `yaml:"skip_tls_verify" validate:"skiptls_if_enterprise"` }Optionally, strengthen validation to require the API base form:
- EnterpriseApiUrl string `yaml:"enterprise_api_url" validate:"omitempty,url"` + EnterpriseApiUrl string `yaml:"enterprise_api_url" validate:"omitempty,url,endswith=/api/v3|/api/v3/"`Would you like me to add a failing test asserting that
Validate()errors whenskip_tls_verify=trueandenterprise_api_url=""?helper/github/client.go (1)
21-29: Gate InsecureSkipVerify to enterprise only and prefer TLS 1.3.Currently any
skipTLSVerify=truedisables verification even for github.com. Restrict this to enterprise URLs and bump the floor.Apply:
- baseTransport := http.DefaultTransport.(*http.Transport).Clone() - if skipTLSVerify { + baseTransport := http.DefaultTransport.(*http.Transport).Clone() + if skipTLSVerify && githubEnterpriseApiUrl != "" { if baseTransport.TLSClientConfig == nil { baseTransport.TLSClientConfig = &tls.Config{} } baseTransport.TLSClientConfig.InsecureSkipVerify = true - // Prefer TLS 1.2+ even when skipping verification. - baseTransport.TLSClientConfig.MinVersion = tls.VersionTLS12 + // Prefer TLS 1.3 even when skipping verification. + baseTransport.TLSClientConfig.MinVersion = tls.VersionTLS13 }If you need to support legacy GHE, fallback to TLS 1.2 behind a feature flag.
server/server.go (1)
49-58: Past review concerns remain unaddressed.The previous review identified that when
SkipTLSVerifyis enabled, no security warning is logged, and the feature lacks documentation. Please address the feedback from the earlier review.
🧹 Nitpick comments (5)
server/config.go (2)
61-75: Sane defaults; minor nit on doc.Defaults are fine. Consider documenting that
EnterpriseApiUrlmust be the API base (…/api/v3/) and not the site root.
107-119: Validator registration per-call; okay, but you can avoid re-registering.Safe to keep; alternatively, build a package-level validator and reuse to avoid repeated registration.
server/config_test.go (1)
17-23: Unify URL format and add negative test.
- If the intended format is the API base, assert it with a trailing slash:
"https://api.githubenterprise.example.com/api/v3/".- Add a test that
NewConfig("...")fails whenskip_tls_verify=truewith emptyenterprise_api_url.I can send a patch updating the assertion and adding the negative case. Want me to proceed?
helper/github/client_test.go (2)
61-67: Mirror the API base change in TLS and installation tests.Update enterprise URL to
.../api/v3/and assert exactBaseURL.String()to ensure correctness.Also applies to: 98-109
26-35: Consider asserting TLS gating for github.com.Add a test ensuring
skipTLSVerify=truewith empty enterprise URL does NOT setInsecureSkipVerify(once client is gated accordingly).I can add a focused test once the client change merges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
helper/github/client.go(2 hunks)helper/github/client_test.go(2 hunks)server/config.go(3 hunks)server/config_test.go(1 hunks)server/server.go(7 hunks)server/testdata/config1.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
helper/github/client_test.go (1)
helper/github/client.go (2)
NewClient(20-53)Client(12-16)
server/server.go (4)
server/config.go (1)
Config(12-23)helper/github/client.go (1)
NewClient(20-53)client.go (1)
NewClient(90-104)server/pool.go (1)
NewPool(61-105)
server/config.go (1)
server/server.go (1)
New(43-126)
🪛 ast-grep (0.39.6)
helper/github/client.go
[warning] 23-23: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (6)
server/config.go (1)
39-39: Case-sensitive policy update looks good.Validator now expects
Always Never IfNotPresent; YAML usesIfNotPresent. No action.server/testdata/config1.yaml (2)
29-31: ImagePullPolicy matches validator.
IfNotPresentaligns withoneof=Always Never IfNotPresent. No change needed.Also applies to: 51-53
19-20: No changes needed—current format is correct.Go-github's WithEnterpriseURLs automatically adds the "/api/v3/" suffix if missing, so the current config (
https://api.githubenterprise.example.com/api/v3) is already compliant and does not require a trailing slash. The suggested change is unnecessary.server/server.go (3)
37-39: LGTM! Clean refactor.Returning the function directly is more idiomatic and concise.
44-47: LGTM! Proper validation placement.Config validation before client creation ensures invalid configs are caught early.
79-79: LGTM!GitHub client properly wired into the Server struct.
| fakeEnterpriseURL := "https://api.githubenterprise.example.com/" | ||
|
|
||
| client, err := NewClient(12345, string(key), fakeEnterpriseURL, false) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, client) | ||
| assert.NotNil(t, client.transport) | ||
|
|
||
| // Ensure the Enterprise base URL was applied | ||
| assert.Contains(t, client.Client.BaseURL.String(), fakeEnterpriseURL) | ||
| assert.Equal(t, client.transport.BaseURL, fakeEnterpriseURL) | ||
| } |
There was a problem hiding this comment.
Use the API base, not host root, and assert normalized URL.
Tests pass a host root (https://api.githubenterprise.example.com/), but config and most clients expect the API base (.../api/v3/). Align to avoid false positives that would fail at runtime when hitting endpoints.
Apply:
- fakeEnterpriseURL := "https://api.githubenterprise.example.com/"
+ fakeEnterpriseURL := "https://api.githubenterprise.example.com/api/v3/"Then assert exact equality (with trailing slash) on client.Client.BaseURL.String() and client.transport.BaseURL.
📝 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.
| fakeEnterpriseURL := "https://api.githubenterprise.example.com/" | |
| client, err := NewClient(12345, string(key), fakeEnterpriseURL, false) | |
| assert.NoError(t, err) | |
| assert.NotNil(t, client) | |
| assert.NotNil(t, client.transport) | |
| // Ensure the Enterprise base URL was applied | |
| assert.Contains(t, client.Client.BaseURL.String(), fakeEnterpriseURL) | |
| assert.Equal(t, client.transport.BaseURL, fakeEnterpriseURL) | |
| } | |
| fakeEnterpriseURL := "https://api.githubenterprise.example.com/api/v3/" | |
| client, err := NewClient(12345, string(key), fakeEnterpriseURL, false) | |
| assert.NoError(t, err) | |
| assert.NotNil(t, client) | |
| assert.NotNil(t, client.transport) | |
| // Ensure the Enterprise base URL was applied | |
| assert.Contains(t, client.Client.BaseURL.String(), fakeEnterpriseURL) | |
| assert.Equal(t, client.transport.BaseURL, fakeEnterpriseURL) | |
| } |
🤖 Prompt for AI Agents
In helper/github/client_test.go around lines 43 to 53, the test currently passes
the host root URL but the code and real clients expect the API base (including
/api/v3/), which can hide runtime failures; update fakeEnterpriseURL to use the
API base (e.g. "https://api.githubenterprise.example.com/api/v3/"), then change
the assertions to check exact equality (including the trailing slash) for both
client.Client.BaseURL.String() and client.transport.BaseURL so the test verifies
normalized API base URLs precisely.
| if githubEnterpriseApiUrl != "" { | ||
| ghClient, err = ghClient.WithEnterpriseURLs(githubEnterpriseApiUrl, "") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| transport.BaseURL = githubEnterpriseApiUrl | ||
| } |
There was a problem hiding this comment.
Normalize enterprise URLs and set upload base.
WithEnterpriseURLs expects the API base (.../api/v3/) and an upload base (.../api/uploads/). Empty upload URL may break asset uploads; lack of trailing slash can break path joins.
Apply:
- if githubEnterpriseApiUrl != "" {
- ghClient, err = ghClient.WithEnterpriseURLs(githubEnterpriseApiUrl, "")
+ if githubEnterpriseApiUrl != "" {
+ base := ensureTrailingSlash(githubEnterpriseApiUrl)
+ // If base is the host root, append /api/v3/
+ if !strings.Contains(base, "/api/") {
+ base = strings.TrimRight(base, "/") + "/api/v3/"
+ }
+ upload := strings.Replace(base, "/api/v3/", "/api/uploads/", 1)
+ ghClient, err = ghClient.WithEnterpriseURLs(base, upload)
if err != nil {
return nil, err
}
- transport.BaseURL = githubEnterpriseApiUrl
+ transport.BaseURL = base
}And add helpers/imports:
+import "strings"
+
+func ensureTrailingSlash(u string) string {
+ if strings.HasSuffix(u, "/") { return u }
+ return u + "/"
+}
By default github.com is being used to register the runners, we have certain cases where github enterprise is used with a custom domain. - Option added in the config to provide custom enterprise urls - Ability added to skip tls verification in case of self signed certificates
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/server.go (1)
270-291: Reload should revalidate config and refresh GitHub client (and pools).Currently reloads only pools; changes to GitHub config aren’t applied.
if err := s.config.Load(); err != nil { return fmt.Errorf("loading config: %w", err) } + if err := s.config.Validate(); err != nil { + return fmt.Errorf("validate: %w", err) + } + + // Rebuild GitHub client in case Enterprise URL/TLS settings changed. + gclient, err := github.NewClient( + s.config.GitHub.AppID, + s.config.GitHub.AppPrivateKey, + s.config.GitHub.EnterpriseApiUrl, + s.config.GitHub.SkipTLSVerify, + ) + if err != nil { + return fmt.Errorf("creating github client: %w", err) + } + s.github = gclient + for _, poolConfig := range s.config.Pools { pool, ok := s.pools[poolConfig.Name] if ok { pool.config = poolConfig + // Ensure pools use the refreshed GitHub client. + pool.github = s.github s.logger.Info().Msgf("Pool %s reloaded", poolConfig.Name) continue } pool, err := NewPool(s.logger, poolConfig, s.github)
♻️ Duplicate comments (5)
helper/github/client_test.go (3)
43-53: Use API base, not host root; assert normalized equality (incl. transport).Tests should pass the API base (…/api/v3/) and verify exact equality.
- fakeEnterpriseURL := "https://api.githubenterprise.example.com/" + fakeEnterpriseURL := "https://api.githubenterprise.example.com/api/v3/" @@ - // Ensure the Enterprise base URL was applied - assert.Contains(t, client.Client.BaseURL.String(), fakeEnterpriseURL) - assert.Equal(t, client.transport.BaseURL, fakeEnterpriseURL) + // Ensure the Enterprise base URL was applied (exact) + assert.Equal(t, fakeEnterpriseURL, client.Client.BaseURL.String()) + assert.Equal(t, fakeEnterpriseURL, client.transport.BaseURL) + // Upload base should also be set + assert.Equal(t, "https://api.githubenterprise.example.com/api/uploads/", client.Client.UploadURL.String())
61-67: Enterprise TLS test: use API base and assert both BaseURL and UploadURL.Same normalization and stronger assertions as above.
- fakeEnterpriseURL := "https://api.githubenterprise.example.com/" + fakeEnterpriseURL := "https://api.githubenterprise.example.com/api/v3/" @@ - assert.Contains(t, client.Client.BaseURL.String(), fakeEnterpriseURL) + assert.Equal(t, fakeEnterpriseURL, client.Client.BaseURL.String()) + assert.Equal(t, "https://api.githubenterprise.example.com/api/uploads/", client.Client.UploadURL.String())
98-109: Enterprise installation: use API base and assert exact BaseURL and UploadURL.Strengthen checks to catch misconfigured enterprise upload base.
- fakeEnterpriseURL := "https://api.githubenterprise.example.com/" + fakeEnterpriseURL := "https://api.githubenterprise.example.com/api/v3/" @@ - // Should retain enterprise base URL - assert.Contains(t, installation.BaseURL.String(), fakeEnterpriseURL) + // Should retain enterprise base URL (exact) and upload base + assert.Equal(t, fakeEnterpriseURL, installation.BaseURL.String()) + assert.Equal(t, "https://api.githubenterprise.example.com/api/uploads/", installation.UploadURL.String())helper/github/client.go (2)
39-46: Normalize enterprise base and set upload base; fix transport.BaseURL.Passing a host root or empty upload URL breaks requests. Normalize to “/api/v3/” with trailing slash and set the corresponding “/api/uploads/”.
- if githubEnterpriseApiUrl != "" { - ghClient, err = ghClient.WithEnterpriseURLs(githubEnterpriseApiUrl, "") - if err != nil { - return nil, err - } - - transport.BaseURL = githubEnterpriseApiUrl - } + if githubEnterpriseApiUrl != "" { + base := ensureTrailingSlash(githubEnterpriseApiUrl) + // If host root was provided, append API base. + if !strings.Contains(strings.TrimSuffix(base, "/"), "/api/") { + base = strings.TrimRight(base, "/") + "/api/v3/" + } + upload := strings.Replace(base, "/api/v3/", "/api/uploads/", 1) + ghClient, err = ghClient.WithEnterpriseURLs(base, upload) + if err != nil { + return nil, err + } + transport.BaseURL = base + }Add imports/helpers:
import ( "crypto/tls" "net/http" + "strings" @@ ) + +func ensureTrailingSlash(u string) string { + if strings.HasSuffix(u, "/") { + return u + } + return u + "/" +}
61-71: Installation client: preserve enterprise upload base (derive if absent).Ensure UploadURL is absolute and matches “/api/uploads/”.
- if c.Client.BaseURL != nil && c.Client.BaseURL.String() != "https://api.github.com/" { - if enterpriseClient, err := installationClient.WithEnterpriseURLs( - c.Client.BaseURL.String(), - c.Client.UploadURL.String(), - ); err == nil { + if c.Client.BaseURL != nil && c.Client.BaseURL.String() != "https://api.github.com/" { + base := c.Client.BaseURL.String() + upload := "" + if c.Client.UploadURL != nil && c.Client.UploadURL.String() != "" { + upload = c.Client.UploadURL.String() + } else { + upload = strings.Replace(base, "/api/v3/", "/api/uploads/", 1) + } + if enterpriseClient, err := installationClient.WithEnterpriseURLs(base, upload); err == nil { return enterpriseClient } }
🧹 Nitpick comments (10)
helper/github/client_test.go (3)
22-24: Assert exact default API and uploads bases for GitHub.com.Use strict equality to catch normalization bugs and also assert UploadURL.
- // Should default to GitHub.com - assert.Contains(t, client.Client.BaseURL.String(), "https://api.github.com/") + // Should default to GitHub.com + assert.Equal(t, "https://api.github.com/", client.Client.BaseURL.String()) + assert.Equal(t, "https://uploads.github.com/", client.Client.UploadURL.String())
26-35: Cover TLS flag behavior.Also assert the TLS path is exercised to avoid false positives.
func TestNewClient_Success_GitHubCom_TLS(t *testing.T) { @@ client, err := NewClient(12345, string(key), "", true) assert.NoError(t, err) assert.NotNil(t, client) + assert.True(t, client.skipTLSVerify) }
81-90: Installation client on GitHub.com: also assert uploads base.Tighten assertion to cover uploads URL.
// The installation client should still have a valid BaseURL - assert.Contains(t, installation.BaseURL.String(), "https://api.github.com/") + assert.Equal(t, "https://api.github.com/", installation.BaseURL.String()) + assert.Equal(t, "https://uploads.github.com/", installation.UploadURL.String())server/config.go (1)
33-35: Validate enterprise API base shape (…/api/v3/) and trailing slash.Add a custom validator to ensure EnterpriseApiUrl ends with “/api/v3/”; avoids runtime URL mishaps.
-import ( +import ( "fmt" "os" + "strings" @@ - EnterpriseApiUrl string `yaml:"enterprise_api_url" validate:"omitempty,url"` + EnterpriseApiUrl string `yaml:"enterprise_api_url" validate:"omitempty,url,ghe_api_base"` @@ func (c *Config) Validate() error { v := validator.New() // Custom validation: SkipTLSVerify can only be true if EnterpriseApiUrl is set _ = v.RegisterValidation("skiptls_if_enterprise", func(fl validator.FieldLevel) bool { - cfg := fl.Parent().Interface().(GitHubConfig) + // Be robust to pointer/struct parents + var cfg GitHubConfig + if s, ok := fl.Parent().Interface().(GitHubConfig); ok { + cfg = s + } else if p, ok := fl.Parent().Interface().(*GitHubConfig); ok && p != nil { + cfg = *p + } else { + return true + } if cfg.SkipTLSVerify && cfg.EnterpriseApiUrl == "" { return false } return true }) + // Enterprise URL must be an API base and have a trailing slash. + _ = v.RegisterValidation("ghe_api_base", func(fl validator.FieldLevel) bool { + u := fl.Field().String() + if u == "" { + return true + } + if !strings.HasSuffix(u, "/") { + return false + } + // Require /api/v3/ segment + return strings.Contains(u, "/api/v3/") + })helper/github/client.go (2)
18-20: Doc: example should show API base, not host root.Use “https:///api/v3/” in the example to prevent misconfiguration.
-// githubEnterpriseApiUrl should be a full API base URL (e.g. https://api.githubenterprise.example.com/). +// githubEnterpriseApiUrl should be the full API base URL (e.g. https://api.githubenterprise.example.com/api/v3/).
21-29: Defense-in-depth: only allow InsecureSkipVerify for enterprise.Config validation guards this, but add a second check here to avoid misuse by external callers.
- if skipTLSVerify { + if skipTLSVerify && githubEnterpriseApiUrl != "" {server/server.go (1)
49-56: Surface a warning when SkipTLSVerify is enabled.Log a clear warning so operators notice the insecure mode.
gclient, err := github.NewClient( config.GitHub.AppID, config.GitHub.AppPrivateKey, config.GitHub.EnterpriseApiUrl, config.GitHub.SkipTLSVerify, ) @@ s := &Server{ @@ logger: &logger, } + + if config.GitHub.SkipTLSVerify { + s.logger.Warn(). + Str("enterprise_api_url", config.GitHub.EnterpriseApiUrl). + Msg("SkipTLSVerify enabled: TLS certificate verification is disabled for GitHub Enterprise. Use only for development/testing.") + }server/testdata/config1.yaml (2)
19-21: Normalize enterprise API base and reconsider SkipTLSVerify.Add a trailing slash and keep SkipTLSVerify only if truly needed (self-signed).
- enterprise_api_url: "https://api.githubenterprise.example.com/api/v3" # optional, empty = github.com, otherwise i.e. https://example.com/api/v3, or https://api.example.com + enterprise_api_url: "https://api.githubenterprise.example.com/api/v3/" # API base with trailing slash skip_tls_verify: true
49-57: Optional: fix label copy/paste to match the 4gb pool.This avoids confusion when filtering by labels.
labels: - self-hosted - - fireactions-2vcpu-2gb + - fireactions-2vcpu-4gb - fireactionsserver/config_test.go (1)
16-22: Consider adding test cases for default and edge-case scenarios.The current test only validates the happy path where
EnterpriseApiUrlis set andSkipTLSVerifyis true. Consider adding test cases for:
- Empty or unset
EnterpriseApiUrl(should default to github.com)SkipTLSVerifyset to false or unset (should default to false)- Validation rule behavior (e.g., the
skiptls_if_enter_enterpriserule mentioned in the config)This would improve test coverage and catch potential regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
helper/github/client.go(2 hunks)helper/github/client_test.go(2 hunks)server/config.go(3 hunks)server/config_test.go(1 hunks)server/server.go(7 hunks)server/testdata/config1.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/config.go (1)
server/server.go (1)
New(43-126)
helper/github/client_test.go (1)
helper/github/client.go (2)
NewClient(20-53)Client(12-16)
server/server.go (5)
helper/logger/logger.go (1)
New(11-41)server/config.go (1)
Config(12-23)helper/github/client.go (1)
NewClient(20-53)client.go (1)
NewClient(90-104)server/pool.go (1)
NewPool(61-105)
🪛 ast-grep (0.39.6)
helper/github/client.go
[warning] 23-23: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (3)
server/config.go (1)
31-35: Good: bind SkipTLSVerify to custom rule.Validation tag enforces “skip TLS only with Enterprise URL”. LGTM.
server/config_test.go (2)
12-12: LGTM!Good improvement using
t.Fatalfwith a format string for better error reporting.
21-21: ****The test data in
server/testdata/config1.yamlconfirms the URL format is intentionally without a trailing slash:"https://api.githubenterprise.example.com/api/v3". The config file's own comments provide examples (https://example.com/api/v3,https://api.example.com) that consistently exclude trailing slashes, indicating this is the expected format. The assertion on line 21 correctly matches the test data—no issues here.Likely an incorrect or invalid review comment.
- arguments added for new options github-enterprise-api-url and github-tls-skip-verify
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
install.sh (1)
392-392: Inconsistent use of export statement in parameter parsing.Line 392 exports
CONTAINERD_SNAPSHOTTER_DEVICEin the--containerd-snapshotter-device=*case, but the non-equals form (line 389) does not. This is inconsistent with how other options are handled throughout the script. For uniformity and clarity, remove the export here or add it to the space-separated form as well (though both variables are accessible in the same script regardless).--containerd-snapshotter-device=*) - export CONTAINERD_SNAPSHOTTER_DEVICE="${1#*=}" + CONTAINERD_SNAPSHOTTER_DEVICE="${1#*=}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
install.sh(16 hunks)
🔇 Additional comments (1)
install.sh (1)
345-358: Parameter parsing for new GitHub Enterprise options is well-structured.Both
--github-enterprise-api-urland--github-skip-tls-verifycorrectly handle space-separated (--flag value) and equals (--flag=value) syntax, consistent with existing option patterns.
| export FIREACTIONS_VERSION=0.2.5 | ||
| export FIRECRACKER_VERSION=1.4.1 | ||
| export KERNEL_VERSION=5.10 | ||
| export GITHUB_SKIP_TLS_VERIFY=false |
There was a problem hiding this comment.
🧩 Analysis chain
Missing initialization for GITHUB_ENTERPRISE_API_URL conflicts with documented default.
The usage text (line 21) states that --github-enterprise-api-url is optional with a default of https://api.github.com, but the variable is never initialized with this default. When users omit the flag, the config file (lines 254–255) will contain an empty enterprise_api_url: field, which contradicts the documentation.
Initialize the variable at line 11 alongside other defaults:
export GITHUB_SKIP_TLS_VERIFY=false
+ export GITHUB_ENTERPRISE_API_URL=https://api.github.comAlternatively, update the usage text to reflect that it truly defaults to empty (if downstream code handles that).
Also applies to: 21-22, 254-255
🏁 Script executed:
#!/bin/bash
# Examine install.sh to verify the review comment claims
echo "=== Checking lines around line 11 (GITHUB_SKIP_TLS_VERIFY) ==="
sed -n '1,20p' install.sh
echo -e "\n=== Checking usage text around line 21 ==="
sed -n '21,25p' install.sh
echo -e "\n=== Searching for GITHUB_ENTERPRISE_API_URL initialization ==="
grep -n "GITHUB_ENTERPRISE_API_URL" install.sh | head -20
echo -e "\n=== Checking config file generation around lines 254-255 ==="
sed -n '250,260p' install.sh
echo -e "\n=== Checking parameter parsing block around lines 345-358 ==="
sed -n '340,365p' install.sh
echo -e "\n=== Checking line 392 for export inconsistency ==="
sed -n '390,395p' install.shLength of output: 2755
Add default initialization for GITHUB_ENTERPRISE_API_URL at line 11.
The usage text documents that --github-enterprise-api-url defaults to https://api.github.com when omitted, but the variable is never initialized with this default. When users skip the flag, the config file will contain an empty enterprise_api_url: field instead of the documented default.
Add the initialization:
export GITHUB_SKIP_TLS_VERIFY=false
+ export GITHUB_ENTERPRISE_API_URL=https://api.github.com📝 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.
| export GITHUB_SKIP_TLS_VERIFY=false | |
| export GITHUB_SKIP_TLS_VERIFY=false | |
| export GITHUB_ENTERPRISE_API_URL=https://api.github.com |
🤖 Prompt for AI Agents
In install.sh around line 11, GITHUB_ENTERPRISE_API_URL is not initialized so
when --github-enterprise-api-url is omitted the config gets an empty
enterprise_api_url; initialize GITHUB_ENTERPRISE_API_URL to
https://api.github.com (exported) at or before line 11 so that if the flag is
not provided the script uses that default, and ensure later flag parsing only
overrides this variable when a non-empty value is supplied so the generated
config contains the documented default.
1 participant
By default github.com is being used to register the runners, we have certain cases where github enterprise is used with a custom domain.
Description
Related Issue(s)
#272
Checklist
Summary by CodeRabbit
New Features
Configuration
Tests