From 2894acd79d0a346c8f5909054a4c712a057805d7 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Tue, 27 Oct 2026 17:05:50 +0000 Subject: [PATCH] fix(hub): throttle GitHub App setup authorization Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../concerns/security_rate_limiting.rb | 1 + .../github_app_setup_controller.rb | 1 + hub/docs/github-app-setup.md | 2 + hub/docs/security-rate-limits.md | 3 + .../github_app_setup_rate_limits_test.rb | 99 +++++++++++++++++++ 5 files changed, 106 insertions(+) create mode 100644 hub/test/integration/github_app_setup_rate_limits_test.rb diff --git a/hub/app/controllers/concerns/security_rate_limiting.rb b/hub/app/controllers/concerns/security_rate_limiting.rb index 27cc6e82..74ec2bce 100644 --- a/hub/app/controllers/concerns/security_rate_limiting.rb +++ b/hub/app/controllers/concerns/security_rate_limiting.rb @@ -4,6 +4,7 @@ module SecurityRateLimiting include TrustedRequestSource BUDGETS = { + github_app_setup: [ 10, 1.minute ], oauth_initiation: [ 60, 5.minutes ], oauth_callback: [ 60, 5.minutes ], comments: [ 60, 1.minute ], diff --git a/hub/app/controllers/github_app_setup_controller.rb b/hub/app/controllers/github_app_setup_controller.rb index 8dab14a2..9d38b533 100644 --- a/hub/app/controllers/github_app_setup_controller.rb +++ b/hub/app/controllers/github_app_setup_controller.rb @@ -3,6 +3,7 @@ class GithubAppSetupController < SuperglueController allow_unauthenticated_access before_action :disable_for_local_mode before_action :resume_session + before_action -> { enforce_security_rate_limit(:github_app_setup, identity: trusted_request_ip) }, only: :create rescue_from GithubAppSetup::Error, with: :setup_not_found rescue_from GithubClient::Error, with: :setup_not_found diff --git a/hub/docs/github-app-setup.md b/hub/docs/github-app-setup.md index 396888ea..e0ee6df4 100644 --- a/hub/docs/github-app-setup.md +++ b/hub/docs/github-app-setup.md @@ -60,6 +60,8 @@ With a private organization App, connecting a public repository lets every organ The setup request expires after 15 minutes and can be used once. +Setup authorization allows 10 attempts per minute per trusted source IP, including rejected secrets and attempts from different browser Sessions. Further attempts return HTTP 429 with retry guidance. See [Security rate limits](security-rate-limits.md) for shared storage and proxy requirements. + After setup, enroll the person administering Hub at `/operator/enrollment` using the bootstrap secret and a fresh GitHub sign-in. App creation does not automatically designate its owner or the first signed-in person as a Hub operator. Existing configured deployments use the same route without resetting their App. See [Security audit](security-audit.md#enroll-an-operator). The App requests: diff --git a/hub/docs/security-rate-limits.md b/hub/docs/security-rate-limits.md index 4456a73e..925d7651 100644 --- a/hub/docs/security-rate-limits.md +++ b/hub/docs/security-rate-limits.md @@ -6,6 +6,7 @@ These fixed budgets are product policy, not measured capacity or configurable de | Feature | Allowance | Counted identity | | --- | --- | --- | +| GitHub App setup authorization, including rejected secrets | 10 per minute | Trusted source IP | | GitHub OAuth initiation, including enrollment and fresh verification | 60 per 5 minutes | Trusted source IP | | GitHub OAuth callback | 60 per 5 minutes, separately from initiation | Trusted source IP | | Comment creation, replies, edits, deletion, reactions, and resolution | 60 per minute | Hub user | @@ -23,6 +24,8 @@ The request path uses Rails's rate limiter with one explicit feature scope. Prod IP budgets use the same trusted-proxy boundary as the [security audit](security-audit.md). Hub uses the socket peer unless Rails's configured trusted proxies include it. The HTTPS edge must replace untrusted forwarding metadata with a verified client chain, and backend ingress and proxy trust must match the intended forwarders. Local socket tests do not establish deployed TLS or proxy enforcement. +Setup authorization attempts share their IP allowance across browser Sessions. Opening the setup page and continuing an authorized setup through its callback and installation do not spend this allowance. A throttled attempt cannot authorize setup, even with the correct bootstrap secret. Keep using a long, generated bootstrap secret; throttling does not replace secret strength. + Fresh-verification POST requests spend the existing initiation allowance before challenge lookup, including unknown or deleted targets. Passive reads do not spend it. Local HTTP 429 responses remain in ordinary request logs only; they never write security-audit records. HTTP 429 includes `Retry-After`, private/no-store caching, and localized retry guidance. The delay is conservatively the full window in seconds, not a claim about the precise remaining lifetime. JSON responses include `code: "rate_limited"`, `errors`, and `retryAfter`. Browser forms show the explanation; review UI retains data and drafts and shows it in an alert. Canvas actions generate their own retry wording from the validated delay instead of forwarding server prose. Canvas review shows guidance even with the Hub sidebar closed. Neither surface automatically retries a rejected mutation. diff --git a/hub/test/integration/github_app_setup_rate_limits_test.rb b/hub/test/integration/github_app_setup_rate_limits_test.rb new file mode 100644 index 00000000..7ca7ae14 --- /dev/null +++ b/hub/test/integration/github_app_setup_rate_limits_test.rb @@ -0,0 +1,99 @@ +require "test_helper" +require_relative "../support/security_rate_limit_store" + +class GithubAppSetupRateLimitsTest < ActionDispatch::IntegrationTest + include SecurityRateLimitStore + + setup do + use_security_rate_limit_store + travel_to Time.current.change(usec: 0) + @previous_bootstrap_secret = ENV["REPLICA_BOOTSTRAP_SECRET"] + ENV["REPLICA_BOOTSTRAP_SECRET"] = github_app_configurations.bootstrap_secret + end + + teardown do + ENV["REPLICA_BOOTSTRAP_SECRET"] = @previous_bootstrap_secret + restore_security_rate_limit_store + travel_back + end + + test "ten rejected secrets exhaust the budget across browser session resets" do + 10.times do + reset! + post setup_path, params: { bootstrap_secret: "wrong" }, as: :json + assert_response :not_found + end + + reset! + assert_no_difference("SecurityAuditEvent.count") do + post setup_path, params: { bootstrap_secret: "wrong" }, as: :json + assert_security_throttle(60) + end + end + + test "an exhausted budget blocks the correct secret without authorizing or auditing until expiry" do + started = Time.current + 10.times do + post setup_path, params: { bootstrap_secret: "wrong" }, as: :json + assert_response :not_found + end + + assert_no_difference("SecurityAuditEvent.count") do + post setup_path, params: { bootstrap_secret: github_app_configurations.bootstrap_secret }, as: :json + assert_security_throttle(60) + end + get setup_path, as: :json + assert_response :success + assert_includes response.body, "github_app_setup/show" + assert_not_includes response.body, "github_app_setup/register" + + travel_to started + 59.seconds + post setup_path, params: { bootstrap_secret: github_app_configurations.bootstrap_secret }, as: :json + assert_security_throttle(60) + + travel_to started + 1.minute + assert_difference("SecurityAuditEvent.where(action: 'github_app_setup.authorize', outcome: 'succeeded').count") do + post setup_path, params: { bootstrap_secret: github_app_configurations.bootstrap_secret }, as: :json + assert_redirected_to setup_path + end + get setup_path, as: :json + assert_response :success + assert_includes response.body, "github_app_setup/register" + end + + test "untrusted forwarded addresses cannot split a peer budget while another peer has its own" do + 10.times do |index| + post setup_path, params: { bootstrap_secret: "wrong" }, headers: { + "REMOTE_ADDR" => "203.0.113.10", "X-Forwarded-For" => "198.51.100.#{index + 1}" + }, as: :json + assert_response :not_found + end + + post setup_path, params: { bootstrap_secret: "wrong" }, headers: { + "REMOTE_ADDR" => "203.0.113.10", "X-Forwarded-For" => "198.51.100.200" + }, as: :json + assert_security_throttle(60) + + post setup_path, params: { bootstrap_secret: "wrong" }, + headers: { "REMOTE_ADDR" => "203.0.113.11" }, as: :json + assert_response :not_found + end + + test "setup reads callback and installation requests do not spend the authorization budget" do + 10.times do + get setup_path, as: :json + assert_response :success + get callback_setup_path, params: { code: "unused", state: "invalid" }, as: :json + assert_response :not_found + get install_setup_path, params: { installation_id: 456 }, as: :json + assert_response :not_found + end + + 10.times do + post setup_path, params: { bootstrap_secret: "wrong" }, as: :json + assert_response :not_found + end + post setup_path, params: { bootstrap_secret: "wrong" }, as: :json + assert_security_throttle(60) + end +end