OpenID Connect config_for can cause runtime error

Discovered after !130688 (merged) was merged since this MR introduces a boot-time call to a method that gets OAuth configuration for GitHub provider.

The actual bug was introduced by a community contribution a year ago in !95308 (merged). However, this would only be discovered if an instance has both openid_connect and config is attempted to be retrieved for a different provider. In this case, we're trying to get github but my GDK instance has openid_connect configured also.

Code path:

  • From !130688 (merged), https://gitlab.com/gitlab-org/gitlab/-/blob/6291f4223e8801cd0cfafbf04cf351cf0f523a76/lib/gitlab/github_import/attachments_downloader.rb#L15 calls Gitlab::GithubImport::MarkdownText.github_url
  • Gitlab::GithubImport::MarkdownText.github_url calls Gitlab::Auth::OAuth::Provider.config_for
  • Gitlab::Auth::OAuth::Provider.config_for iterates over providers and checks whether a name matches. If it doesn't match by name there's an OR conditional added in !95308 (merged) that calls provider.args.name. If the requested provider name doesn't exist, but openid_connect does, then it will hit the provider.args.name call and most providers don't specify an args name. Only if configuring more than one provider of the same type per the description in !95308 (merged).

The code in config_for needs to be improved to fail safely, and we also need a test that would have caught this. But I also recommend we move the constant from https://gitlab.com/gitlab-org/gitlab/-/blob/6291f4223e8801cd0cfafbf04cf351cf0f523a76/lib/gitlab/github_import/attachments_downloader.rb#L15 to a method so it's only evaluated if needed at runtime if someone uses the GitHub importer, and not on boot.

Assignee Loading
Time tracking Loading