mirror of
https://github.com/discourse/discourse.git
synced 2026-08-09 21:45:25 +08:00
GitHub oneboxes and the discourse-github plugin talked to GitHub's REST
and
GraphQL API with no rate-limit awareness. On busy instances this
exhausted
GitHub's limits (60 requests/hour unauthenticated, 5000 authenticated),
and
because there was no backoff every render kept hitting GitHub and
re-failing
-- which GitHub's docs warn can get an integration banned. The recently
added PR-status onebox multiplied the number of calls and made it far
worse.
GitHub access was also fragmented: the core onebox engines used OpenURI,
the
discourse-github plugin used Octokit, and the discourse-ai bot tools
used
FinalDestination::HTTP -- three HTTP stacks, three tokens, and
inconsistent
(or entirely missing) error and rate-limit handling.
This introduces a single client, Discourse::GithubApi, that every GitHub
data-API request now flows through. It is built on Faraday with the
SSRF-safe
FinalDestination adapter and:
- authenticates per token (Bearer) and returns plain string-keyed Hashes
(get/post) or raw bodies (raw_get) -- one response shape, no
Octokit/Sawyer
- only ever sends the access token to api.github.com and
raw.githubusercontent.com, rejecting any other absolute URL, so a
user-derived path can never leak a token to an arbitrary host
- backs off on rate limits both reactively (403/429) and proactively
(when
X-RateLimit-Remaining hits 0), honouring Retry-After /
X-RateLimit-Reset,
via a shared Redis flag (GithubRateLimit) keyed per token so each
token's
budget and the shared unauthenticated/IP budget back off independently
- short-circuits while backing off without ever sleeping, so onebox
rendering
and post baking degrade to a plain link instead of blocking a request
- caches ETags and sends If-None-Match, so unchanged resources return
304s
that do not count against the rate limit
Every caller was moved onto it:
- the 6 core GitHub onebox engines, via a slimmed
Onebox::Mixins::GithubApi
adapter that keeps their public methods and translates client errors
back
to the OpenURI::HTTPError vocabulary they already rescue (engines
unchanged)
- the github_blob raw.githubusercontent.com fetch
- the discourse-github plugin (badges, linkback, permalinks, token
validator),
which no longer uses the octokit and sawyer gems (they stay in the
Gemfile for
the discourse-code-review official plugin, which still depends on them)
- the discourse-ai bot's GitHub tools (search code, diff, file content,
search files)
Also adds a GithubOneboxBackoff admin problem check that surfaces while
one of
the onebox token identities is backing off -- scoped to the tokens
resolved by
Onebox::GithubAccess (each configured github_onebox_access_tokens entry
plus the
unauthenticated client) so a backoff on the AI bot or linkback token is
not
misattributed to onebox. Its message points admins at the relevant
setting with
the {{setting:...}} link marker, which problem-check messages now expand
too.
Onebox token resolution is centralised in Onebox::GithubAccess, and the
onebox
cache TTL for transient GitHub failures is shortened so they recover
quickly.
GitHub OAuth login, theme git-clone, the inbound webhook, and the
Oneboxer
FinalDestination URL-resolution special-cases for github.com are
intentionally
out of scope -- they are different concerns, not the rate-limited data
API.
248 lines
6.4 KiB
Ruby
Vendored
248 lines
6.4 KiB
Ruby
Vendored
# frozen_string_literal: true
|
|
|
|
class ProblemCheck
|
|
class Collection
|
|
include Enumerable
|
|
|
|
def initialize(checks)
|
|
@checks = checks
|
|
end
|
|
|
|
def each(...)
|
|
checks.each(...)
|
|
end
|
|
|
|
def run_all
|
|
select(&:enabled?).each { |check| check.each_target { |t| check.new(t).run } }
|
|
end
|
|
|
|
private
|
|
|
|
attr_reader :checks
|
|
end
|
|
|
|
include ActiveSupport::Configurable
|
|
|
|
config_accessor :enabled, default: true, instance_writer: false
|
|
config_accessor :priority, default: "low", instance_writer: false
|
|
|
|
# Determines if the check should be performed at a regular interval, and if
|
|
# so how often. If left blank, the check will be performed every time the
|
|
# admin dashboard is loaded, or the data is otherwise requested.
|
|
#
|
|
config_accessor :perform_every, default: nil, instance_writer: false
|
|
|
|
# How many times the check should retry before registering a problem. Only
|
|
# works for scheduled checks.
|
|
#
|
|
config_accessor :max_retries, default: 2, instance_writer: false
|
|
|
|
# The retry delay after a failed check. Only works for scheduled checks with
|
|
# more than one retry configured.
|
|
#
|
|
config_accessor :retry_after, default: 30.seconds, instance_writer: false
|
|
|
|
# How many consecutive times the check can fail without notifying admins.
|
|
# This can be used to give some leeway for transient problems. Note that
|
|
# retries are not counted. So a check that ultimately fails after e.g. two
|
|
# retries is counted as one "blip".
|
|
#
|
|
config_accessor :max_blips, default: 0, instance_writer: false
|
|
|
|
# Indicates that the problem check is an "inline" check. This provides a
|
|
# low level construct for registering problems ad-hoc within application
|
|
# code, without having to extract the checking logic into a dedicated
|
|
# problem check.
|
|
#
|
|
config_accessor :inline, default: false, instance_writer: false
|
|
|
|
# Used to set up multiple targets for the check. For example, a check that
|
|
# operates on groups may need to specify which groups to work on.
|
|
#
|
|
config_accessor :targets, default: -> { [NO_TARGET] }, instance_writer: false
|
|
|
|
# Problem check classes need to be registered here in order to be enabled.
|
|
#
|
|
# Note: This list must come after the `config_accessor` declarations.
|
|
#
|
|
CORE_PROBLEM_CHECKS = [
|
|
ProblemCheck::BadFaviconUrl,
|
|
ProblemCheck::ContentSecurityPolicyDisabled,
|
|
ProblemCheck::EmailSendingFailures,
|
|
ProblemCheck::EmailPollingErroredRecently,
|
|
ProblemCheck::FacebookConfig,
|
|
ProblemCheck::FailingEmails,
|
|
ProblemCheck::ForceHttps,
|
|
ProblemCheck::GithubConfig,
|
|
ProblemCheck::GithubOneboxBackoff,
|
|
ProblemCheck::GoogleAnalyticsVersion,
|
|
ProblemCheck::GoogleOauth2Config,
|
|
ProblemCheck::GroupEmailCredentials,
|
|
ProblemCheck::HostNames,
|
|
ProblemCheck::ImageMagick,
|
|
ProblemCheck::MissingMailgunApiKey,
|
|
ProblemCheck::OutOfDateThemes,
|
|
ProblemCheck::PollPop3Timeout,
|
|
ProblemCheck::PollPop3AuthError,
|
|
ProblemCheck::QqMailSmtp,
|
|
ProblemCheck::RailsEnv,
|
|
ProblemCheck::Ram,
|
|
ProblemCheck::S3BackupConfig,
|
|
ProblemCheck::S3Cdn,
|
|
ProblemCheck::S3UploadConfig,
|
|
ProblemCheck::SidekiqCheck,
|
|
ProblemCheck::SubfolderEndsInSlash,
|
|
ProblemCheck::StarttlsDisabled,
|
|
ProblemCheck::TranslationOverrides,
|
|
ProblemCheck::TwitterConfig,
|
|
ProblemCheck::TwitterLogin,
|
|
ProblemCheck::UnreachableThemes,
|
|
ProblemCheck::WatchedWords,
|
|
ProblemCheck::UpcomingChangeStableOptedOut,
|
|
].freeze
|
|
|
|
# To enforce the unique constraint in Postgres <15 we need a dummy
|
|
# value, since the index considers NULLs to be distinct.
|
|
NO_TARGET = "__NULL__"
|
|
|
|
def self.[](key)
|
|
key = key.to_sym
|
|
|
|
checks.find { |c| c.identifier == key }
|
|
end
|
|
|
|
def self.checks
|
|
Collection.new(DiscoursePluginRegistry.problem_checks.concat(CORE_PROBLEM_CHECKS))
|
|
end
|
|
|
|
def self.scheduled
|
|
Collection.new(checks.select(&:scheduled?))
|
|
end
|
|
|
|
def self.realtime
|
|
Collection.new(checks.select(&:realtime?))
|
|
end
|
|
|
|
def self.identifier
|
|
name.demodulize.underscore.to_sym
|
|
end
|
|
delegate :identifier, to: :class
|
|
|
|
def self.enabled?
|
|
enabled
|
|
end
|
|
delegate :enabled?, to: :class
|
|
|
|
def self.scheduled?
|
|
perform_every.present?
|
|
end
|
|
delegate :scheduled?, to: :class
|
|
|
|
def self.realtime?
|
|
!scheduled? && !inline?
|
|
end
|
|
delegate :realtime?, to: :class
|
|
|
|
def self.inline?
|
|
inline
|
|
end
|
|
delegate :inline?, to: :class
|
|
|
|
def self.targeted?
|
|
targets.call != [ProblemCheck::NO_TARGET]
|
|
end
|
|
delegate :targeted?, to: :class
|
|
|
|
def self.each_target(&)
|
|
targets.call.each(&)
|
|
end
|
|
|
|
def self.cleanup_trackers
|
|
current_targets = targets.call
|
|
return if current_targets.empty?
|
|
|
|
ProblemCheckTracker.where(identifier:).where.not(target: current_targets).destroy_all
|
|
end
|
|
|
|
def initialize(target = NO_TARGET)
|
|
@target = target
|
|
end
|
|
|
|
attr_reader :target
|
|
|
|
def call
|
|
raise NotImplementedError
|
|
end
|
|
|
|
def run
|
|
# Never run a targeted check with NO_TARGET (stale job or default targets used by mistake).
|
|
if target == NO_TARGET && targeted?
|
|
tracker.destroy
|
|
return
|
|
end
|
|
|
|
# target is always a string when initializing this class, but the targets function
|
|
# could return IDs from the DB. Make everything string so we don't return early all the time.
|
|
if targeted? && targets.call.map(&:to_s).exclude?(target)
|
|
tracker.destroy
|
|
return
|
|
end
|
|
|
|
problem = call
|
|
|
|
yield(problem) if block_given?
|
|
|
|
next_run_at = perform_every&.from_now
|
|
|
|
if problem.blank?
|
|
tracker.no_problem!(next_run_at:)
|
|
else
|
|
tracker.problem!(
|
|
next_run_at:,
|
|
details: translation_data.merge(problem.details).merge(base_path: Discourse.base_path),
|
|
)
|
|
end
|
|
end
|
|
|
|
def tracker
|
|
ProblemCheckTracker[identifier, target]
|
|
end
|
|
|
|
def ready_to_run?
|
|
tracker.ready_to_run?
|
|
end
|
|
|
|
private
|
|
|
|
def problem(target = nil, override_key: nil, override_data: {}, details: {})
|
|
target_identifier = target.kind_of?(ActiveRecord::Base) ? target.id : target
|
|
|
|
Problem.new(
|
|
SiteSettings::LabelFormatter.expand_setting_links(
|
|
I18n.t(
|
|
override_key || translation_key,
|
|
base_path: Discourse.base_path,
|
|
**override_data.merge(
|
|
target.present? ? translation_data(target) : translation_data,
|
|
).symbolize_keys,
|
|
),
|
|
),
|
|
priority: config.priority,
|
|
identifier:,
|
|
target: target_identifier,
|
|
details:,
|
|
)
|
|
end
|
|
|
|
def no_problem
|
|
nil
|
|
end
|
|
|
|
def translation_key
|
|
"dashboard.problem.#{identifier}"
|
|
end
|
|
|
|
def translation_data(target = nil)
|
|
{}
|
|
end
|
|
end
|