Skip to content

Raise on blank issuer in resolve_issuer#299

Merged
55728 merged 1 commit into
doorkeeper-gem:masterfrom
55728:fix/issue-298-blank-issuer-guard
Jun 3, 2026
Merged

Raise on blank issuer in resolve_issuer#299
55728 merged 1 commit into
doorkeeper-gem:masterfrom
55728:fix/issue-298-blank-issuer-guard

Conversation

@55728

@55728 55728 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #298

Problem

Since v1.10.0, Doorkeeper::OpenidConnect.resolve_issuer unified issuer dispatch and calls arity-2 issuer blocks with (resource_owner, application).

In the discovery context both arguments are nil. An existing arity-2 block that relied on the v1.9.0 behavior — where DiscoveryController called issuer.call(request) and the block received the request as its first argument — now receives nil and can return nil.

resolve_issuer then silently coerced that nil to "" via to_s. The result was a discovery document advertising an empty issuer, while the ID token iss claim still carried the configured value. OIDC clients reject this mismatch, so the failure surfaced far from its cause (a broken callback) instead of at the misconfiguration.

This is the scenario reported in #298.

Change

resolve_issuer now raises Errors::InvalidConfiguration when the resolved issuer is blank, mirroring the existing signing_key handling (signing_key must resolve to at least one key). The guard covers both the static-value and callable paths, so any misconfigured issuer (a nil block result, an empty string, or whitespace) now fails loudly instead of shipping a broken discovery document.

The issuer argument unification itself is intentional and unchanged — this PR only adds a fail-fast guard on the resolved value.

Migration

For blocks that previously relied on the discovery-context request:

# arity-3 block — works in both discovery and token contexts
issuer do |resource_owner, application, request|
  request&.base_url || "https://default.example.com"
end

or, for a constant issuer:

issuer "https://example.com"

Files

File Change
lib/doorkeeper/openid_connect.rb resolve_issuer computes the value, then raises InvalidConfiguration if blank?. Covers static + callable paths.
config/locales/en.yml New issuer_not_configured message, following the signing_key_not_configured precedent.
spec/lib/openid_connect_spec.rb Specs for the blank guard: an arity-2 block returning nil in the discovery context (the reported case), and a blank static value.
spec/controllers/discovery_controller_spec.rb Seven partial-config blocks (testing protocol / discovery_url_options / end-session) now set a valid issuer. They previously relied on the silent-"" behavior.
spec/support/doorkeeper_configuration.rb The shared configure_doorkeeper helper now sets an issuer (fixes the EC/HMAC as_jws_token examples in one place).

Notes

  • blank? (not empty?) is used so a whitespace-only issuer is also rejected.
  • The pre-existing IdToken#as_json test that stubs issuer: nil to assert iss is omitted is unaffected — it stubs the instance method and never reaches resolve_issuer.

Testing

  • Targeted specs (openid_connect_spec, id_token_spec, discovery_controller_spec): 85 examples, 0 failures
  • Full suite: 287 examples, 0 failures
  • RuboCop: no offenses on changed files

Since v1.10.0, `resolve_issuer` dispatches arity-2 issuer blocks with
`(resource_owner, application)`. In the discovery context both are nil,
so an existing arity-2 block that relied on the v1.9.0 behavior (where
DiscoveryController passed the request as the first argument) can return
nil. `resolve_issuer` then silently coerced that to `""` via `to_s`,
producing a discovery document whose `issuer` was empty while the ID
token `iss` was the configured value -- OIDC clients reject the mismatch.

Guard against this by raising `Errors::InvalidConfiguration` when the
resolved issuer is blank, mirroring the existing `signing_key` handling.
This applies to both the static and callable paths, so a misconfigured
issuer now fails loudly instead of shipping a broken discovery document.

Specs that reconfigured OpenID Connect without an issuer (and only
asserted unrelated fields) relied on the old silent-"" behavior; they
now set a valid issuer.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes Doorkeeper::OpenidConnect.resolve_issuer fail fast when the configured issuer resolves to a blank value, preventing discovery documents from advertising an empty issuer and surfacing misconfigured issuer callbacks closer to the source of the problem (Fixes #298).

Changes:

  • Add a blank? guard in resolve_issuer that raises Errors::InvalidConfiguration when the resolved issuer is blank (static or callable).
  • Add an I18n error message for the new invalid-issuer configuration error.
  • Update and extend specs to cover the new behavior and ensure existing discovery specs explicitly configure a valid issuer where needed.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/doorkeeper/openid_connect.rb Computes issuer value (callable/static) and raises InvalidConfiguration when it resolves blank.
config/locales/en.yml Adds issuer_not_configured translation string used by the new guard.
spec/lib/openid_connect_spec.rb Adds test coverage for raising on blank issuer from callable and static configurations.
spec/controllers/discovery_controller_spec.rb Updates partial-config contexts to set a non-blank issuer to avoid new configuration error.
spec/support/doorkeeper_configuration.rb Ensures shared spec helper configures an issuer so unrelated signing-algorithm specs don’t fail.
CHANGELOG.md Documents the behavior change and links it to #298 / #299.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nbulaj nbulaj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⚒️

@55728

55728 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review! 👀✨🛠️

@55728 55728 merged commit 5620cfe into doorkeeper-gem:master Jun 3, 2026
30 checks passed
@55728 55728 deleted the fix/issue-298-blank-issuer-guard branch June 3, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v1.10.0 issuer arity-2 dispatch can break existing discovery issuer blocks

3 participants