Skip to content

Fix: non-confidential apps should not generate a client secret#96

Merged
nbulaj merged 4 commits into
masterfrom
copilot/fix-non-confidential-client-secret
Mar 12, 2026
Merged

Fix: non-confidential apps should not generate a client secret#96
nbulaj merged 4 commits into
masterfrom
copilot/fix-non-confidential-client-secret

Conversation

Copilot AI commented Mar 11, 2026

Copy link
Copy Markdown
Contributor
  • Fix: non-confidential apps should not generate a client secret
  • Fix CI: pin rspec-rails to ~> 6.0 to fix uninitialized constant ActiveSupport::LoggerThreadSafeLevel::Logger (latest rspec-rails 7.x requires Rails 7.1+, but CI tests Rails 6.x and 7.0)
  • Re-add spec/models/doorkeeper/application_mixin_spec.rb with tests for non-confidential secret behavior
  • Update .gitignore to track the spec file
Original prompt

This section details on the original issue you should resolve

<issue_title>Non-confidential client still generates a secret</issue_title>
<issue_description>Hello,

For public/non-confidential applications, a secret still gets generated in the application_mixin.rb file. During the before_validation lifecycle method, generate_secret executes and always creates one.

This is in contrast to what the standard doorkeeper gem does. It will first check if the application is confidential. If it isn't, then no secret is generated.

Is this an intentional difference between the two gems?</issue_description>

<agent_instructions>Analyze and fix the issue</agent_instructions>

Comments on the Issue (you are @copilot in this section)


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Co-authored-by: nbulaj <1443426+nbulaj@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix secret generation for non-confidential clients Fix: non-confidential apps should not generate a client secret Mar 11, 2026
@nbulaj

nbulaj commented Mar 12, 2026

Copy link
Copy Markdown
Member

Hey @copilot can we add tests as well?

Copilot AI commented Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

Hey @copilot can we add tests as well?

Added tests in spec/models/doorkeeper/application_mixin_spec.rb (commit 5b71114) covering:

  • Secret generation on create: confidential apps generate a secret; non-confidential (public) apps don't
  • Secret validation: confidential apps require a secret; non-confidential apps are valid without one
  • secret_required? method: returns true for confidential apps and false for non-confidential ones

Also updated .gitignore to allow committing this custom spec file while keeping the doorkeeper-generated spec files (copied by the Rake task) ignored.

@nbulaj nbulaj marked this pull request as ready for review March 12, 2026 06:42
@nbulaj nbulaj force-pushed the copilot/fix-non-confidential-client-secret branch from 5b71114 to ba600ca Compare March 12, 2026 06:44
@nbulaj

nbulaj commented Mar 12, 2026

Copy link
Copy Markdown
Member

@copilot tests are failing in CI with

An error occurred while loading ./spec/lib/oauth/token_response_spec.rb.
Failure/Error: require "rails"

NameError:
  uninitialized constant ActiveSupport::LoggerThreadSafeLevel::Logger
# ./spec/dummy/config/application.rb:3:in `require'
# ./spec/dummy/config/application.rb:3:in `<top (required)>'
# ./spec/dummy/config/environment.rb:2:in `require'
# ./spec/dummy/config/environment.rb:2:in `<top (required)>'

Check and fix them

Copilot AI commented Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

@copilot tests are failing in CI with

...

Fixed in commit d95c76b. The CI failure was caused by the unpinned rspec-rails pulling in version 7.x, which requires Rails 7.1+, but the CI matrix tests against Rails 6.x and 7.0. Pinned rspec-rails back to ~> 6.0 (allowing any 6.x version, compatible with Rails 6.0 through 7.x).

@nbulaj nbulaj force-pushed the copilot/fix-non-confidential-client-secret branch from d95c76b to 95838a3 Compare March 12, 2026 08:05
@nbulaj nbulaj merged commit 28ad721 into master Mar 12, 2026
25 of 81 checks passed
@stevetsanders

Copy link
Copy Markdown

Thanks!

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.

Non-confidential client still generates a secret

3 participants