diff --git a/CHANGELOG.md b/CHANGELOG.md index 151e0a2..19d1f65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,58 @@ +# Changelog + ## [Unreleased] +### Added +- Refresh token rotation with reuse detection behind `refresh_token.rotation_enabled` (default `false`): each refresh revokes the presented refresh token, and presenting an already-rotated/revoked refresh token revokes the whole token family (SEC-2) +- Paranoid mode behind `paranoid` (default `false`): unknown accounts and wrong passwords both return the generic `invalid_authentication` error with no lockable/confirmable details, preventing account enumeration (SEC-5) +- `error_response.verbose_account_state` (default `true`): set to `false` to omit the `lockable`/`confirmable` metadata blocks from error responses (SEC-7) +- `Devise::Api::Token#revoke!` and `#revoke_family!` helpers +- New `invalid_login` error (HTTP 400) returned instead of `invalid_email` when the model's `authentication_keys` do not include `:email` +- Lockable error responses now include the correctly spelled `failed_attempts` key alongside the deprecated `failed_attemps` (the misspelling will be removed in the next major release) +- `access_token`, `refresh_token` and `previous_refresh_token` are added to the host app's `filter_parameters`, and the token model filters them from `#inspect` output (GH-51) + +### Changed +- **Breaking-ish:** `POST //tokens/refresh` with an unknown refresh token now returns `invalid_refresh_token` (HTTP 400) instead of `invalid_token` (HTTP 401) +- The install generator's migration now creates **unique** indexes on `access_token` and `refresh_token`; token creation rescues `ActiveRecord::RecordNotUnique` and retries with a fresh token (SEC-4). Existing installs should add a migration: + ```ruby + remove_index :devise_api_tokens, :access_token + remove_index :devise_api_tokens, :refresh_token + add_index :devise_api_tokens, :access_token, unique: true + add_index :devise_api_tokens, :refresh_token, unique: true + ``` +- `current_devise_api_refresh_token` is now memoized in the shared controller helpers (the duplicate controller-level override was removed) +- Internal time handling standardized on `Time.current` + +### Removed +- Vestigial RBS stub (`sig/devise/api.rbs`) + +## [0.2.0] - 2024-09-27 + +- Resource lookup uses the model's `authentication_keys` instead of hardcoding `email` (#46) +- Fixed nil memoization of `current_devise_api_token` / `current_devise_api_refresh_token` (#48) +- Fixed the translation key for the unconfirmed signup message (#49) + +## [0.1.3] - 2023-08-08 + +- Fixed `AbstractController::DoubleRenderError` on refresh (#29) +- Allowed defining extra fields for sign up via `sign_up.extra_fields` (#36, #38) +- Disabled parameter wrapping in `TokensController` (#42) + +## [0.1.2] - 2023-05-30 + +- Added `sign_up.enabled` option to disable the sign up endpoint (#15) +- Fixed refresh behavior (#14) +- Fixed undefined variable error in the controller helper (#25) +- Migration template respects the configured primary/foreign key types (#23) + +## [0.1.1] - 2023-01-14 + +- Fixed invalid strategy error (#2) + +## [0.1.0] - 2023-01-14 + +- First public release: `:api` Devise module with token sign up / sign in / refresh / revoke / info endpoints (#1) + ## [0.0.0] - 2023-01-09 - Initial release diff --git a/README.md b/README.md index ad10de3..3963a29 100644 --- a/README.md +++ b/README.md @@ -90,10 +90,15 @@ Devise.setup do |config| api.refresh_token.expires_in = 1.week api.refresh_token.generator = ->(_resource_owner) { Devise.friendly_token(60) } api.refresh_token.expires_in_infinite = ->(_resource_owner) { false } + api.refresh_token.rotation_enabled = false # when true, each refresh revokes the presented refresh token and a replayed one revokes the whole token family (recommended) # Sign up api.sign_up.enabled = true - api.sign_up.extra_fields = [] + api.sign_up.extra_fields = [] # WARNING: listed fields are writable at sign up AND echoed in token/info responses - never list privileged fields like :role or :admin + + # Error responses + api.error_response.verbose_account_state = true # when false, lockable/confirmable details are omitted from error responses + api.paranoid = false # when true, unknown accounts and wrong passwords return the same generic invalid_authentication error (prevents account enumeration) # Authorization api.authorization.key = 'Authorization' @@ -125,7 +130,7 @@ end ## Routes -You can configure the tokens routes with the orginally `devise_for` method. For example: +You can configure the tokens routes with the original `devise_for` method. For example: ```ruby # config/routes.rb Rails.application.routes.draw do @@ -223,7 +228,7 @@ class Api::V1::TokensController < YourBaseController skip_before_action :verify_authenticity_token, raise: false def create - service = Devise::Api::TokensService::V2::Create.call(params: params, resource_class: Customer || resource_class) + service = Devise::Api::TokensService::V2::Create.new(params: params, resource_class: Customer).call if service.success? render json: service.success, status: :created else @@ -273,9 +278,18 @@ curl --location --request GET 'http://127.0.0.1:3000/users/tokens/info' \ --header 'Authorization: Bearer ' ``` +## Security recommendations + +- **Send tokens in the `Authorization` header.** The default `authorization.location = :both` also accepts tokens as query/body params (e.g. `GET /users/tokens/info?access_token=...`), and URLs end up in server/proxy logs, browser history and `Referer` headers. Set `api.authorization.location = :header` unless you need params support. +- **Enable refresh token rotation** (`api.refresh_token.rotation_enabled = true`). Without it a refresh token stays valid until it expires, so a stolen one can be replayed. With rotation, every refresh revokes the presented token and replaying a rotated token revokes the whole token family. +- **Enable paranoid mode** (`api.paranoid = true`) if you don't want attackers to be able to check whether an email address has an account (account enumeration). +- **Rate limit the token endpoints.** The gem does not throttle `sign_in`/`sign_up`/`refresh`; use [rack-attack](https://github.com/rack/rack-attack) or an equivalent in front of them. Devise `lockable` (if enabled) only slows per-account brute force. +- **Be careful with `sign_up.extra_fields`.** Every listed field is mass-assignable at sign up and echoed in every token/info response. +- **Token secrets in logs.** The gem automatically adds `access_token`, `refresh_token` and `previous_refresh_token` to `filter_parameters` and filters them from the token model's `#inspect`, but raw SQL logging (e.g. `log_level = :debug` in production) can still print token values — keep production SQL logging off or filtered. + ## Development -After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake rspec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. +After checking out the repo, run `bin/setup` to install dependencies. Then, run `bundle exec rake rspec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. To install this gem onto your local machine, run `bundle exec rake install`. To release a new version, update the version number in `version.rb`, and then run `bundle exec rake release`, which will create a git tag for the version, push git commits and the created tag, and push the `.gem` file to [rubygems.org](https://rubygems.org). diff --git a/app/controllers/devise/api/tokens_controller.rb b/app/controllers/devise/api/tokens_controller.rb index 876dedb..51b4021 100644 --- a/app/controllers/devise/api/tokens_controller.rb +++ b/app/controllers/devise/api/tokens_controller.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -# rubocop:disable Metrics/ClassLength module Devise module Api class TokensController < Devise.api.config.base_controller.constantize @@ -10,144 +9,94 @@ class TokensController < Devise.api.config.base_controller.constantize respond_to :json - # rubocop:disable Metrics/AbcSize def sign_up - unless Devise.api.config.sign_up.enabled - error_response = Devise::Api::Responses::ErrorResponse.new(request, error: :sign_up_disabled, - resource_class: resource_class) - - return render json: error_response.body, status: error_response.status - end + return render_error_response(error: :sign_up_disabled) unless Devise.api.config.sign_up.enabled Devise.api.config.before_sign_up.call(sign_up_params, request, resource_class) service = Devise::Api::ResourceOwnerService::SignUp.new(params: sign_up_params, resource_class: resource_class).call - - if service.success? - token = service.success - - call_devise_trackable!(token.resource_owner) - - token_response = Devise::Api::Responses::TokenResponse.new(request, token: token, action: __method__) - - Devise.api.config.after_successful_sign_up.call(token.resource_owner, token, request) - - return render json: token_response.body, status: token_response.status - end - - error_response = Devise::Api::Responses::ErrorResponse.new(request, - resource_class: resource_class, - **service.failure) - - render json: error_response.body, status: error_response.status + render_resource_owner_service_result(service, action: __method__) end - # rubocop:enable Metrics/AbcSize - # rubocop:disable Metrics/AbcSize def sign_in Devise.api.config.before_sign_in.call(sign_in_params, request, resource_class) service = Devise::Api::ResourceOwnerService::SignIn.new(params: sign_in_params, resource_class: resource_class).call - - if service.success? - token = service.success - - call_devise_trackable!(token.resource_owner) - - token_response = Devise::Api::Responses::TokenResponse.new(request, token: service.success, - action: __method__) - - Devise.api.config.after_successful_sign_in.call(token.resource_owner, token, request) - - return render json: token_response.body, status: token_response.status - end - - error_response = Devise::Api::Responses::ErrorResponse.new(request, - resource_class: resource_class, - **service.failure) - - render json: error_response.body, status: error_response.status + render_resource_owner_service_result(service, action: __method__) end - # rubocop:enable Metrics/AbcSize def info - token_response = Devise::Api::Responses::TokenResponse.new(request, token: current_devise_api_token, - action: __method__) - - render json: token_response.body, status: token_response.status + render_token_response(current_devise_api_token, action: __method__) end - # rubocop:disable Metrics/AbcSize def revoke Devise.api.config.before_revoke.call(current_devise_api_token, request) service = Devise::Api::TokensService::Revoke.new(devise_api_token: current_devise_api_token).call + return render_error_response(**service.failure) if service.failure? - if service.success? - token_response = Devise::Api::Responses::TokenResponse.new(request, token: service.success, - action: __method__) - - Devise.api.config.after_successful_revoke.call(service.success&.resource_owner, service.success, request) - - return render json: token_response.body, status: token_response.status - end - - error_response = Devise::Api::Responses::ErrorResponse.new(request, - resource_class: resource_class, - **service.failure) - - render json: error_response.body, status: error_response.status + token = service.success + Devise.api.config.after_successful_revoke.call(token&.resource_owner, token, request) + render_token_response(token, action: __method__) end - # rubocop:enable Metrics/AbcSize - # rubocop:disable Metrics/AbcSize def refresh - unless Devise.api.config.refresh_token.enabled - error_response = Devise::Api::Responses::ErrorResponse.new(request, - resource_class: resource_class, - error: :refresh_token_disabled) - - return render json: error_response.body, status: error_response.status - end + return render_error_response(error: :refresh_token_disabled) unless Devise.api.config.refresh_token.enabled + return render_error_response(error: :invalid_refresh_token) if current_devise_api_refresh_token.blank? + return handle_refresh_token_reuse if refresh_token_reused? + return render_error_response(error: :revoked_token) if current_devise_api_refresh_token.revoked? - if current_devise_api_refresh_token.blank? - error_response = Devise::Api::Responses::ErrorResponse.new(request, error: :invalid_token, - resource_class: resource_class) - - return render json: error_response.body, status: error_response.status - end - - if current_devise_api_refresh_token.revoked? - error_response = Devise::Api::Responses::ErrorResponse.new(request, error: :revoked_token, - resource_class: resource_class) + perform_refresh + end - return render json: error_response.body, status: error_response.status - end + private + def perform_refresh Devise.api.config.before_refresh.call(current_devise_api_refresh_token, request) service = Devise::Api::TokensService::Refresh.new(devise_api_token: current_devise_api_refresh_token).call + return render_error_response(**service.failure) if service.failure? - if service.success? - token_response = Devise::Api::Responses::TokenResponse.new(request, token: service.success, - action: __method__) + token = service.success + Devise.api.config.after_successful_refresh.call(token.resource_owner, token, request) + render_token_response(token, action: :refresh) + end - Devise.api.config.after_successful_refresh.call(service.success.resource_owner, service.success, request) + def render_resource_owner_service_result(service, action:) + return render_error_response(**service.failure) if service.failure? - return render json: token_response.body, status: token_response.status - end + token = service.success + call_devise_trackable!(token.resource_owner) + Devise.api.config.public_send("after_successful_#{action}").call(token.resource_owner, token, request) + render_token_response(token, action: action) + end - error_response = Devise::Api::Responses::ErrorResponse.new(request, - resource_class: resource_class, - **service.failure) + def render_token_response(token, action:) + token_response = Devise::Api::Responses::TokenResponse.new(request, token: token, action: action) + + render json: token_response.body, status: token_response.status + end + + def render_error_response(**failure) + error_response = Devise::Api::Responses::ErrorResponse.new(request, resource_class: resource_class, **failure) render json: error_response.body, status: error_response.status end - # rubocop:enable Metrics/AbcSize - private + # A revoked or already-refreshed refresh token presented again while rotation is enabled means the + # token was leaked or replayed: revoke the whole token family (OAuth2 Security BCP). + def refresh_token_reused? + Devise.api.config.refresh_token.rotation_enabled && + (current_devise_api_refresh_token.revoked? || current_devise_api_refresh_token.refreshes.exists?) + end + + def handle_refresh_token_reuse + current_devise_api_refresh_token.revoke_family! + + render_error_response(error: :revoked_token) + end def sign_up_params params.permit(*Devise.api.config.sign_up.extra_fields, *resource_class.authentication_keys, @@ -164,15 +113,6 @@ def call_devise_trackable!(resource_owner) resource_owner.update_tracked_fields!(request) end - - def current_devise_api_refresh_token - return @current_devise_api_refresh_token if defined?(@current_devise_api_refresh_token) - - token = find_devise_api_token - devise_api_token_model = Devise.api.config.base_token_model.constantize - @current_devise_api_refresh_token = devise_api_token_model.find_by(refresh_token: token) - end end end end -# rubocop:enable Metrics/ClassLength diff --git a/app/services/devise/api/resource_owner_service/authenticate.rb b/app/services/devise/api/resource_owner_service/authenticate.rb index 414efb4..4b1067f 100644 --- a/app/services/devise/api/resource_owner_service/authenticate.rb +++ b/app/services/devise/api/resource_owner_service/authenticate.rb @@ -9,7 +9,7 @@ class Authenticate < Devise::Api::BaseService def call resource = resource_class.find_for_authentication(params.slice(*resource_class.authentication_keys)) - return Failure(error: :invalid_email, record: nil) if resource.blank? + return Failure(error: resource_not_found_error, record: nil) if resource.blank? return Failure(error: :invalid_authentication, record: resource) unless authenticate!(resource) Success(resource) @@ -17,6 +17,13 @@ def call private + def resource_not_found_error + return :invalid_authentication if Devise.api.config.paranoid + return :invalid_email if resource_class.authentication_keys.map(&:to_sym).include?(:email) + + :invalid_login + end + def authenticate!(resource) resource.valid_for_authentication? do resource.valid_password?(params[:password]) diff --git a/app/services/devise/api/tokens_service/create.rb b/app/services/devise/api/tokens_service/create.rb index bed7a75..3e27c43 100644 --- a/app/services/devise/api/tokens_service/create.rb +++ b/app/services/devise/api/tokens_service/create.rb @@ -4,6 +4,10 @@ module Devise module Api module TokensService class Create < Devise::Api::BaseService + # Retries after ActiveRecord::RecordNotUnique when two concurrent requests win the + # application-level uniqueness check with the same generated token (see unique DB indexes) + MAX_TOKEN_GENERATION_ATTEMPTS = 3 + option :resource_owner option :previous_refresh_token, type: Types::String | Types::Nil, default: proc { nil } @@ -18,11 +22,20 @@ def call private def create_devise_api_token - devise_api_token = resource_owner.access_tokens.new(params) + attempts = 0 + + begin + devise_api_token = resource_owner.access_tokens.new(params) + + return Success(devise_api_token) if devise_api_token.save - return Success(devise_api_token) if devise_api_token.save + Failure(error: :devise_api_token_create_error, record: devise_api_token) + rescue ::ActiveRecord::RecordNotUnique + attempts += 1 + retry if attempts < MAX_TOKEN_GENERATION_ATTEMPTS - Failure(error: :devise_api_token_create_error, record: devise_api_token) + raise + end end def params diff --git a/app/services/devise/api/tokens_service/refresh.rb b/app/services/devise/api/tokens_service/refresh.rb index f61cf99..995387f 100644 --- a/app/services/devise/api/tokens_service/refresh.rb +++ b/app/services/devise/api/tokens_service/refresh.rb @@ -9,9 +9,9 @@ class Refresh < Devise::Api::BaseService def call return Failure(error: :expired_refresh_token) if devise_api_token.refresh_token_expired? + return create_devise_api_token unless Devise.api.config.refresh_token.rotation_enabled - devise_api_token = yield create_devise_api_token - Success(devise_api_token) + create_devise_api_token_with_rotation end private @@ -20,6 +20,21 @@ def create_devise_api_token Devise::Api::TokensService::Create.new(resource_owner: resource_owner, previous_refresh_token: devise_api_token.refresh_token).call end + + # Mints the replacement token and revokes the presented refresh token atomically, so a + # rotated refresh token can never be replayed (its reuse triggers family revocation upstream) + def create_devise_api_token_with_rotation + result = nil + + devise_api_token.class.transaction do + result = create_devise_api_token + raise ::ActiveRecord::Rollback if result.failure? + + devise_api_token.revoke! + end + + result + end end end end diff --git a/app/services/devise/api/tokens_service/revoke.rb b/app/services/devise/api/tokens_service/revoke.rb index 6a9f3ae..ff33653 100644 --- a/app/services/devise/api/tokens_service/revoke.rb +++ b/app/services/devise/api/tokens_service/revoke.rb @@ -9,7 +9,7 @@ class Revoke < Devise::Api::BaseService def call return Success(devise_api_token) if devise_api_token.blank? return Success(devise_api_token) if devise_api_token.revoked? || devise_api_token.expired? - return Success(devise_api_token) if devise_api_token.update(revoked_at: Time.zone.now) + return Success(devise_api_token) if devise_api_token.update(revoked_at: Time.current) Failure(error: :devise_api_token_revoke_error, record: devise_api_token) end diff --git a/config/locales/en.yml b/config/locales/en.yml index ceaea79..8b46a27 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -11,6 +11,7 @@ en: sign_up_disabled: "Sign up is disabled for this application" invalid_refresh_token: "Refresh token is invalid" invalid_email: "Email is invalid" + invalid_login: "Login credentials are invalid" invalid_resource_owner: "Resource owner is invalid" resource_owner_create_error: "Resource owner could not be created" devise_api_token_create_error: "Token could not be created" diff --git a/docs/README.md b/docs/README.md index 784fb1a..d5839f2 100644 --- a/docs/README.md +++ b/docs/README.md @@ -25,7 +25,7 @@ Internal documentation for contributors and AI coding agents. These documents de ## Ground rules for AI-driven development in this repo 1. **Read [architecture.md](architecture.md) first.** It explains the two invariants that shape everything: the single `Devise.api.config` global, and the string-based `base_token_model` / `base_controller` indirection (`constantize` at use sites — never hardcode `Devise::Api::Token` or the controller class in library code). -2. **Behavioral changes need request specs.** Real coverage lives in `spec/requests/`; service specs are currently placeholders (see [testing.md](testing.md)). +2. **Behavioral changes need request specs.** End-to-end coverage lives in `spec/requests/`; service specs (`spec/services/`) assert the monad contracts (see [testing.md](testing.md)). 3. **Error types are public API.** Adding/renaming a symbol in `ErrorResponse::ERROR_TYPES` requires a locale entry in `config/locales/en.yml`, a status mapping, and an entry in [api-reference.md](api-reference.md). 4. **Schema changes touch three places:** the generator template (`lib/devise/api/generators/templates/migration.rb.erb`), the dummy app (`spec/dummy/db/migrate` + `spec/dummy/db/schema.rb`), and [data-model.md](data-model.md). Host apps upgrade via new migrations, so also consider an upgrade path. 5. **Run `bundle exec rake` before finishing** — it runs RSpec and RuboCop, exactly what CI runs. diff --git a/docs/analysis/known-issues.md b/docs/analysis/known-issues.md index 9ae29ef..029cf0e 100644 --- a/docs/analysis/known-issues.md +++ b/docs/analysis/known-issues.md @@ -1,60 +1,56 @@ # Known Issues & Code-Quality Findings -Working backlog from a full-codebase review (`main` @ `bd49310`, v0.2.0). Ordered by user impact. Security-relevant items live in [security-review.md](security-review.md) and are only cross-referenced here. Check this list before "fixing" surprising code — some quirks are shipped public API. +Working backlog from a full-codebase review (`main` @ `bd49310`, v0.2.0), updated after the 2026-08 fix pass. Ordered by user impact. Security-relevant items live in [security-review.md](security-review.md) and are only cross-referenced here. Check this list before "fixing" surprising code — some quirks are shipped public API. -## Bugs / API warts +## Open -### KI-1 · `failed_attemps` typo is public API -`ErrorResponse#devise_lockable_info` (`lib/devise/api/responses/error_response.rb:85`) emits the misspelled key `failed_attemps`. Clients may already depend on it. Fix path: emit **both** `failed_attempts` and the deprecated misspelling for one minor version, changelog it, drop the typo at the next breaking release. - -### KI-2 · `invalid_refresh_token` error type is unreachable -Declared in `ERROR_TYPES`, mapped to 400, has a locale string — but no code path ever produces it (the refresh action returns `invalid_token` for unknown refresh tokens). Either use it there (more precise; mildly breaking for clients matching on `error`) or remove it. +### KI-8 · `refresh_token.expires_in` is not snapshotted per row +Access-token TTL is copied into the row (`expires_in` column); refresh-token TTL is computed from *live config* at check time, so changing the config re-times every existing token (see [data-model.md](../data-model.md)). At minimum keep documented; ideally add a `refresh_token_expires_in` column for symmetry. -### KI-3 · `invalid_email` hardcodes "email" while lookup uses `authentication_keys` -Since PR #46, `Authenticate` finds resources via `resource_class.authentication_keys` (which may be `username`, etc.), but a miss still returns `error: :invalid_email` / "Email is invalid". Misleading for non-email authentication keys. A generic `invalid_login` (with alias period) would fit better. +### KI-14 · Non-default configuration is untested (mostly resolved) +`spec/requests/configuration_overrides_spec.rb` covers `authorization.location = :header`/`:params` exclusively, `sign_up.enabled = false`, `refresh_token.enabled = false` and `sign_up.extra_fields` end-to-end; `spec/requests/refresh_token_rotation_spec.rb` and `spec/requests/paranoid_mode_spec.rb` cover `rotation_enabled`, `paranoid` and `verbose_account_state`; `spec/devise/api/token_spec.rb` covers `expires_in_infinite` procs and custom generators; `spec/devise/api/configuration_spec.rb` covers overrides on fresh instances. Still untested: custom `authorization.key`/`scheme`/`params_key` and `base_token_model`/`base_controller` overrides. -### KI-4 · Duplicate, divergent `current_devise_api_refresh_token` -Defined twice: memoized in `TokensController` (`app/controllers/devise/api/tokens_controller.rb:168`) and unmemoized in `Controllers::Helpers` (`lib/devise/api/controllers/helpers.rb:34`). The helper version hits the DB on every call and the two can drift. Consolidate into the helper (memoized, mirroring `current_devise_api_token`) and delete the controller override. +## Resolved -## Dead / vestigial code +### KI-1 · ~~`failed_attemps` typo is public API~~ (resolved 2026-08) +`ErrorResponse#devise_lockable_info` now emits both the canonical `failed_attempts` and the deprecated misspelled `failed_attemps` (kept for backward compatibility). Drop the typo at the next major release; changelogged. -### KI-5 · ~~Dead method in `TokensService::Create`~~ (resolved) -`#authenticate_service` was never called and referenced `params` / `resource_class`, which didn't exist on this service — it would have `NameError`d if invoked. Copy-paste leftover; deleted as part of the coverage push. +### KI-2 · ~~`invalid_refresh_token` error type is unreachable~~ (resolved 2026-08) +The `refresh` action now returns `invalid_refresh_token` (400) for missing/unknown refresh tokens instead of the generic `invalid_token` (401). Breaking-ish for clients matching on the error symbol; changelogged. -### KI-6 · RBS stub -`sig/devise/api.rbs` declares only the `VERSION` constant. Either flesh out signatures or drop the `sig/` directory to avoid signaling type support that doesn't exist. +### KI-3 · ~~`invalid_email` hardcodes "email" while lookup uses `authentication_keys`~~ (resolved 2026-08) +`Authenticate` now returns `invalid_email` only when `:email` is one of the model's `authentication_keys`, and a new generic `invalid_login` (400, "Login credentials are invalid") otherwise. With `paranoid` enabled, both collapse into `invalid_authentication`. -## Consistency / hygiene +### KI-4 · ~~Duplicate, divergent `current_devise_api_refresh_token`~~ (resolved 2026-08) +Consolidated into `Controllers::Helpers` with memoization mirroring `current_devise_api_token`; the controller-level override was deleted. -### KI-7 · Mixed time sources -`Token#expired?` / `#refresh_token_expired?` use `Time.now.utc`; `TokensService::Revoke` stamps `Time.zone.now`. Harmless while columns are UTC datetimes, but standardize on `Time.current` for Rails idiom. +### KI-5 · ~~Dead method in `TokensService::Create`~~ (resolved) +`#authenticate_service` was never called and referenced `params` / `resource_class`, which didn't exist on this service — it would have `NameError`d if invoked. Copy-paste leftover; deleted as part of the coverage push. -### KI-8 · `refresh_token.expires_in` is not snapshotted per row -Access-token TTL is copied into the row (`expires_in` column); refresh-token TTL is computed from *live config* at check time, so changing the config re-times every existing token (see [data-model.md](../data-model.md)). At minimum keep documented; ideally add a `refresh_token_expires_in` column for symmetry. +### KI-6 · ~~RBS stub~~ (resolved 2026-08) +`sig/devise/api.rbs` declared only the `VERSION` constant; the `sig/` directory was removed. -### KI-9 · CHANGELOG stale -Only records `0.0.0` while the gem is at `0.2.0` with substantive releases in between (git history has the real story). Backfill before the next release; releases without changelog entries should fail review. +### KI-7 · ~~Mixed time sources~~ (resolved 2026-08) +Standardized on `Time.current` (`Token#expired?`/`#refresh_token_expired?`/`#revoke!`, `TokensService::Revoke`). -### KI-10 · Controller carries rubocop-disable scar tissue -`TokensController` disables `Metrics/ClassLength` and `Metrics/AbcSize` around every action; each action repeats the same error-render boilerplate. Extracting a private `render_error(error, record: nil)` / `render_token(token, action:)` pair would drop the disables and shrink the class substantially. +### KI-9 · ~~CHANGELOG stale~~ (resolved 2026-08) +Backfilled 0.1.0 → 0.2.0 from git history plus an Unreleased section for the current pass. Releases without changelog entries should fail review. -### KI-11 · README nits -"orginally" typo in the Routes section; `rake rspec` in the Development section should read `bundle exec rake rspec`; the service example calls `Create.call(...)` but `BaseService` defines no class-level `.call` (instances are `new(...).call`). +### KI-10 · ~~Controller carries rubocop-disable scar tissue~~ (resolved 2026-08) +`TokensController` now uses private `render_token_response` / `render_error_response` / `render_resource_owner_service_result` / `perform_refresh` helpers; all `rubocop:disable` comments are gone. -## Test-coverage gaps (feeds the "add more tests" milestone) +### KI-11 · ~~README nits~~ (resolved 2026-08) +Fixed the "orginally" typo, the `rake rspec` command, and the service example (`.new(...).call` instead of the non-existent class-level `.call`). ### KI-12 · ~~Service specs are placeholders~~ (resolved) All six `spec/services/**` files now assert the monad contracts (`Success`/`Failure` per branch, per [services.md](../services.md)), including the failure paths unreachable through the HTTP API (`:invalid_resource_owner`, `:devise_api_token_create_error`, `:devise_api_token_revoke_error`, sign-up transaction rollback). ### KI-13 · ~~No generator specs~~ (resolved) -`rails g devise_api:install` is covered by `spec/devise/api/generators/install_generator_spec.rb` (migration template rendering with the current Active Record version, locale copy, migration numbering). - -### KI-14 · Non-default configuration is untested (mostly resolved) -`spec/requests/configuration_overrides_spec.rb` covers `authorization.location = :header`/`:params` exclusively, `sign_up.enabled = false`, `refresh_token.enabled = false` and `sign_up.extra_fields` end-to-end; `spec/devise/api/token_spec.rb` covers `expires_in_infinite` procs and custom generators; `spec/devise/api/configuration_spec.rb` covers overrides on fresh instances; callback invocation was already asserted in `spec/requests/tokens_spec.rb`. Still untested: custom `authorization.key`/`scheme`/`params_key` and `base_token_model`/`base_controller` overrides. +`rails g devise_api:install` is covered by `spec/devise/api/generators/install_generator_spec.rb` (migration template rendering with the current Active Record version, unique token indexes, locale copy, migration numbering). ## Cross-references into security review -- Non-unique token indexes → [SEC-4](security-review.md) -- Plaintext token storage → [SEC-1](security-review.md) -- No refresh rotation → [SEC-2](security-review.md) -- Default `:both` token location + GET `info` → [SEC-3](security-review.md) +- Non-unique token indexes → [SEC-4](security-review.md) (resolved) +- Plaintext token storage → [SEC-1](security-review.md) (open; log filtering shipped) +- No refresh rotation → [SEC-2](security-review.md) (resolved, opt-in `rotation_enabled`) +- Default `:both` token location + GET `info` → [SEC-3](security-review.md) (documented; default unchanged) diff --git a/docs/analysis/security-review.md b/docs/analysis/security-review.md index 3e72e10..50e60ab 100644 --- a/docs/analysis/security-review.md +++ b/docs/analysis/security-review.md @@ -1,6 +1,6 @@ # Security Review -Static review of the codebase as of `main` @ `bd49310` (v0.2.0, 2026-08). This is the working document for the planned "security hardening" milestone: each finding has an ID, severity, and remediation sketch. When a finding is fixed, move it to the *Resolved* section with the PR reference. +Static review of the codebase as of `main` @ `bd49310` (v0.2.0, 2026-08), updated after the 2026-08 hardening pass. This is the working document for the "security hardening" milestone: each finding has an ID, severity, and remediation sketch. When a finding is fixed, move it to the *Resolved* section with the PR reference. Severity scale: **High** = practical account/session compromise under a realistic threat model; **Medium** = meaningful weakening of the security posture; **Low** = defense-in-depth / hardening; **Info** = document-and-accept candidates. @@ -11,39 +11,21 @@ Severity scale: **High** = practical account/session compromise under a realisti **Remediation:** store a digest (e.g. `SHA256`) and look up by digest; return the raw token only once at creation. Needs a migration path (dual-read window or forced re-login) and is a breaking change for host apps that query tokens directly — consider a `hash_token_secrets` config flag defaulting on in the next minor. This also resolves SEC-6. -### SEC-2 · Medium · No refresh-token rotation invalidation or reuse detection -`TokensService::Refresh` mints a new token but leaves the presented refresh token fully usable until its own TTL (`docs/data-model.md#refresh-semantics-important`). A stolen refresh token can be replayed repeatedly, in parallel with the legitimate client, with no signal. The `previous_refresh_token` chain already stores exactly the data needed for reuse detection but nothing consumes it. - -**Remediation (OAuth2 Security BCP pattern):** on refresh, revoke the presented token row; on presentation of an already-used refresh token (a row that has `refreshes.any?` or is revoked-by-rotation), revoke the whole chain ("token family") and force re-authentication. Could ship behind `refresh_token.rotation_enabled` for backward compatibility. +**Partial mitigation shipped (2026-08):** token values are now filtered from request logs (`filter_parameters` via the engine initializer) and from `Token#inspect` (`filter_attributes`), addressing GH-51. Raw SQL logging can still print values; digest storage remains the real fix. ### SEC-3 · Medium · Tokens accepted in URL params by default `authorization.location` defaults to `:both`, and `info` is a GET route — so `GET /users/tokens/info?access_token=…` is a documented usage. Query-string tokens end up in server/proxy/CDN access logs, browser history, and potentially `Referer` headers. -**Remediation:** change the default to `:header` (breaking-ish; needs changelog callout), or at minimum document the risk prominently and exclude the params path from examples. Keep `:params`/`:both` as opt-in. - -### SEC-4 · Low · Token uniqueness not enforced by the database -The migration template indexes `access_token` / `refresh_token` but **not uniquely**; uniqueness relies on an AR validation plus a check-then-insert loop (`generate_uniq_*`), which races under concurrency. A duplicate token would make `find_by(access_token:)` return an arbitrary row — i.e. one user's token could resolve to another user's session in the pathological case. - -**Remediation:** `unique: true` on both indexes (new migration for existing installs + template change), rescue `ActiveRecord::RecordNotUnique` with a regenerate-and-retry in `TokensService::Create`. - -### SEC-5 · Low · Account enumeration via differentiated errors -`Authenticate` returns `:invalid_email` (400, "Email is invalid") when no account exists vs `:invalid_authentication` (401) when the password is wrong — a clean oracle for enumerating registered emails. Sign-up validation errors ("Email has already been taken") enumerate too. - -**Remediation:** optionally collapse both to `invalid_authentication` behind a `paranoid`-style config flag (mirror Devise's `config.paranoid`), defaulting off to preserve current API behavior. +**Remediation:** change the default to `:header` (breaking; needs a major-version changelog callout). **Interim (shipped 2026-08):** the README "Security recommendations" section now tells host apps to set `api.authorization.location = :header`, and token params are filtered from request logs. `:params`/`:both` remain opt-in-by-default until the next breaking release. ### SEC-6 · Low · Token lookup is not constant-time `find_by(access_token: token)` compares via DB index. With 60-char `friendly_token` entropy the timing side channel is not practically exploitable, noted for completeness. Hashing tokens (SEC-1) makes this moot. -### SEC-7 · Low · Lockable error payload aids brute-force pacing -`ErrorResponse#devise_lockable_info` exposes `max_attempts`, `failed_attemps` (sic), `locked_at`, `unlock_at` to the unauthenticated caller — an attacker learns exactly how many guesses remain and when to resume. - -**Remediation:** gate the lockable/confirmable detail blocks behind a config flag (e.g. `error_response.verbose_account_state`, default true for compatibility, recommended false). - ### SEC-8 · Info · No rate limiting -The gem relies entirely on Devise `lockable` (if enabled) to slow credential stuffing; `sign_in`/`sign_up`/`refresh` are otherwise unthrottled. Out of scope to implement in-gem, but the README/docs should recommend `rack-attack` (or equivalent) on the token endpoints. +The gem relies entirely on Devise `lockable` (if enabled) to slow credential stuffing; `sign_in`/`sign_up`/`refresh` are otherwise unthrottled. Out of scope to implement in-gem. The README "Security recommendations" section now recommends `rack-attack` (or equivalent) on the token endpoints; keeping open as Info in case in-gem throttling hooks are ever wanted. ### SEC-9 · Info · `sign_up.extra_fields` is a mass-assignment and disclosure lever -Fields listed there are both *writable at sign-up* and *echoed in every token/info response* (`TokenResponse#default_resource_owner`). A host app adding `:role` or `:admin` here creates a privilege-escalation hole. Needs a loud documentation warning (config docs + README). +Fields listed there are both *writable at sign-up* and *echoed in every token/info response* (`TokenResponse#default_resource_owner`). A host app adding `:role` or `:admin` here creates a privilege-escalation hole. Warnings shipped 2026-08 in the README (config example + "Security recommendations"); keeping open as Info because the sharp edge itself remains. ### SEC-10 · Info · Deliberate CSRF skip `skip_before_action :verify_authenticity_token` is correct for bearer-token endpoints; note that accepting tokens from params (`SEC-3`) is what keeps CSRF relevant — cookie-less bearer auth in the header is not CSRF-able. @@ -58,4 +40,14 @@ Fields listed there are both *writable at sign-up* and *echoed in every token/in ## Resolved -*(empty — move findings here with PR links as they land)* +### SEC-2 · Medium · No refresh-token rotation invalidation or reuse detection — *resolved 2026-08 (opt-in)* +`TokensService::Refresh` minted a new token but left the presented refresh token fully usable until its own TTL. Fixed with the OAuth2 Security BCP pattern behind `refresh_token.rotation_enabled` (default `false` for backward compatibility): each refresh revokes the presented token (same transaction as the mint), and presenting a rotated/revoked refresh token again revokes the whole token family (`Token#revoke_family!`) and returns `revoked_token`. Recommended `true` in the README; consider defaulting on at the next breaking release. Covered by `spec/requests/refresh_token_rotation_spec.rb` and `spec/services/tokens_service/refresh_spec.rb`. + +### SEC-4 · Low · Token uniqueness not enforced by the database — *resolved 2026-08* +The migration template now creates `unique: true` indexes on `access_token` and `refresh_token` (existing installs: add the migration listed in the CHANGELOG), and `TokensService::Create` rescues `ActiveRecord::RecordNotUnique` with a regenerate-and-retry (3 attempts). Covered by `spec/devise/api/token_spec.rb` ("database uniqueness") and `spec/services/tokens_service/create_spec.rb`. + +### SEC-5 · Low · Account enumeration via differentiated errors — *resolved 2026-08 (opt-in)* +`paranoid` config flag (default `false`, mirroring Devise's `config.paranoid`): unknown accounts, wrong passwords, and locked/unconfirmed accounts all return the same generic `invalid_authentication` (401) with no lockable/confirmable details. Covered by `spec/requests/paranoid_mode_spec.rb`. + +### SEC-7 · Low · Lockable error payload aids brute-force pacing — *resolved 2026-08 (opt-in)* +`error_response.verbose_account_state` config flag (default `true` for compatibility, recommended `false`): when disabled, the `lockable`/`confirmable` metadata blocks (`max_attempts`, `failed_attempts`, `locked_at`, `unlock_at`, …) are omitted from error responses. `paranoid` implies it. Covered by `spec/requests/paranoid_mode_spec.rb`. diff --git a/docs/api-reference.md b/docs/api-reference.md index ac4b8f3..8b3c93a 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -12,6 +12,8 @@ All endpoints are drawn by `devise_for :` for any model with the `:api` m Tokens are sent per `authorization` config (default `:both`): `Authorization: Bearer ` header **or** `access_token` query/body param (params win over header). The **same extraction** is used for access and refresh tokens — `refresh` expects the *refresh* token in the same slot. +With `refresh_token.rotation_enabled` (default off), a successful `refresh` also revokes the presented refresh token, and presenting a rotated/revoked refresh token again revokes its whole token family and returns `revoked_token` (reuse detection). + ## Success payloads Built by `Devise::Api::Responses::TokenResponse` (`lib/devise/api/responses/token_response.rb`). @@ -48,13 +50,14 @@ Built by `Devise::Api::Responses::ErrorResponse` (`lib/devise/api/responses/erro { "error": "", "error_description": ["human readable message(s)"], - "lockable": { "locked": true, "max_attempts": 5, "failed_attemps": 5, "locked_at": "...", "unlock_at": "..." }, + "lockable": { "locked": true, "max_attempts": 5, "failed_attempts": 5, "failed_attemps": 5, "locked_at": "...", "unlock_at": "..." }, "confirmable": { "confirmed": false, "confirmation_sent_at": "..." } } ``` -- `lockable` / `confirmable` blocks appear **only** for `invalid_authentication` errors on models with those Devise modules (`compact` removes them otherwise). Note the `failed_attemps` key is a shipped typo — treat as public API until a deliberate breaking change (see [analysis/known-issues.md](analysis/known-issues.md)). -- `error_description` comes from `record.errors.full_messages` when a record with validation errors is attached; otherwise from `config/locales/en.yml` under `devise.api.error_response.`. +- `lockable` / `confirmable` blocks appear **only** for `invalid_authentication` errors on models with those Devise modules (`compact` removes them otherwise), and only while `error_response.verbose_account_state` is `true` (the default) and `paranoid` is `false` — see [configuration.md](configuration.md). +- `failed_attempts` is the canonical key; `failed_attemps` is the original shipped typo, kept for backward compatibility until the next major release (see [analysis/known-issues.md](analysis/known-issues.md)). +- `error_description` comes from `record.errors.full_messages` when a record with validation errors is attached; otherwise from `config/locales/en.yml` under `devise.api.error_response.`. With `paranoid` enabled, `invalid_authentication` always uses the generic message (no locked/unconfirmed specialization). ## Error catalog @@ -62,13 +65,14 @@ Source of truth: `ErrorResponse::ERROR_TYPES` + `#status`. Every symbol must hav | `error` | HTTP status | Raised by / when | |---|---|---| -| `invalid_authentication` | 401 | `Authenticate` — bad password, locked, or unconfirmed account (description specializes per module state) | -| `invalid_token` | 401 | helpers `authenticate_devise_api_token!` (no/unknown access token); `refresh` when refresh token unknown | +| `invalid_authentication` | 401 | `Authenticate` — bad password, locked, or unconfirmed account (description specializes per module state unless `paranoid`); with `paranoid` enabled, also returned when no account matches | +| `invalid_token` | 401 | helpers `authenticate_devise_api_token!` (no/unknown access token) | | `expired_token` | 401 | helpers — access token past `expires_in` | | `expired_refresh_token` | 401 | `TokensService::Refresh` — refresh token past `refresh_token.expires_in` | -| `revoked_token` | 401 | helpers / `refresh` — token has `revoked_at` | -| `invalid_email` | 400 | `Authenticate` — no resource found for the given authentication keys | -| `invalid_refresh_token` | 400 | (declared; mapped to 400) | +| `revoked_token` | 401 | helpers / `refresh` — token has `revoked_at`; also the reuse-detection response when `refresh_token.rotation_enabled` revokes a token family | +| `invalid_email` | 400 | `Authenticate` — no resource found and `:email` is one of the model's `authentication_keys` (and `paranoid` is off) | +| `invalid_login` | 400 | `Authenticate` — no resource found and the model authenticates by non-email keys (and `paranoid` is off) | +| `invalid_refresh_token` | 400 | `refresh` action — no/unknown refresh token presented | | `refresh_token_disabled` | 400 | `refresh` action when `refresh_token.enabled` is false | | `sign_up_disabled` | 400 | `sign_up` action when `sign_up.enabled` is false | | `invalid_resource_owner` | 400 | `TokensService::Create` — owner doesn't respond to `access_tokens` (model lacks `:api`) | diff --git a/docs/configuration.md b/docs/configuration.md index e2624a5..848131b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -29,6 +29,7 @@ Settings are read **at use time**, never cached at boot — changing them (e.g. | `expires_in` | `1.week` | Duration | `Token#refresh_token_expired?` — computed from `created_at`, **not stored per-row** (a config change retroactively affects existing tokens) | | `expires_in_infinite` | `proc { \|owner\| false }` | proc → bool | `Token#refresh_token_expired?` | | `generator` | `proc { \|owner\| Devise.friendly_token(60) }` | proc → String | `Token.generate_uniq_refresh_token` | +| `rotation_enabled` | `false` | bool | `TokensService::Refresh` (revokes the presented token in the same transaction as minting the new one) and the `refresh` action's reuse detection (a rotated/revoked refresh token presented again triggers `Token#revoke_family!`) | ## `sign_up` @@ -37,6 +38,18 @@ Settings are read **at use time**, never cached at boot — changing them (e.g. | `enabled` | `true` | `sign_up` action gate (→ `sign_up_disabled` error) | | `extra_fields` | `[]` | permitted sign-up params **and** extra keys in the `resource_owner` response object (both directions!) | +## `error_response` + +| Setting | Default | Consumed by | +|---|---|---| +| `verbose_account_state` | `true` | `ErrorResponse` — when `false`, the `lockable`/`confirmable` metadata blocks are omitted from error bodies (the locked/unconfirmed `error_description` specialization is kept) | + +## `paranoid` + +| Setting | Default | Consumed by | +|---|---|---| +| `paranoid` | `false` | `Authenticate` (unknown account returns `invalid_authentication` instead of `invalid_email`/`invalid_login`) and `ErrorResponse` (always the generic description, never lockable/confirmable details). Mirrors Devise's `config.paranoid`: makes existent and non-existent accounts indistinguishable to callers. | + ## `authorization` | Setting | Default | Consumed by | diff --git a/docs/data-model.md b/docs/data-model.md index 626d8a1..d81890c 100644 --- a/docs/data-model.md +++ b/docs/data-model.md @@ -9,14 +9,14 @@ From the generator template (`lib/devise/api/generators/templates/migration.rb.e | Column | Type | Null | Index | Notes | |---|---|---|---|---| | `resource_owner_type` / `resource_owner_id` | string / fk | no | composite | polymorphic owner | -| `access_token` | string | no | yes (⚠ not unique) | opaque token, stored **in plaintext** | -| `refresh_token` | string | yes | yes (⚠ not unique) | `nil` when refresh disabled | +| `access_token` | string | no | yes, **unique** | opaque token, stored **in plaintext** | +| `refresh_token` | string | yes | yes, **unique** | `nil` when refresh disabled | | `expires_in` | integer | no | — | access-token TTL in seconds, snapshotted at creation | | `revoked_at` | datetime | yes | — | non-nil ⇒ revoked | | `previous_refresh_token` | string | yes | yes | links a token to the refresh token it was minted from | | `created_at` / `updated_at` | datetime | no | — | expiry math is based on `created_at` | -Uniqueness is enforced only by ActiveRecord validations + generate-and-retry loops (`generate_uniq_access_token` / `generate_uniq_refresh_token`), **not** by unique DB indexes — see [analysis/known-issues.md](analysis/known-issues.md). +Uniqueness is enforced in three layers: unique DB indexes on `access_token`/`refresh_token` (the backstop), ActiveRecord uniqueness validations, and generate-and-retry loops (`generate_uniq_access_token` / `generate_uniq_refresh_token`). If a concurrent insert still hits the index, `TokensService::Create` rescues `ActiveRecord::RecordNotUnique` and retries with a fresh token (up to 3 attempts). Installs created before the unique indexes shipped should add them via migration (see CHANGELOG). ## Entity relationships @@ -47,25 +47,31 @@ stateDiagram-v2 note right of refreshable refresh mints a NEW row with previous_refresh_token = old refresh_token. - The old row is NOT revoked or deleted. + Default: the old row is NOT revoked or deleted. + With rotation_enabled: the old row is revoked. end note refreshable --> [*] ``` Predicates on `Token`: -- `expired?` — `Time.now.utc > created_at + expires_in.seconds`, unless `access_token.expires_in_infinite.(owner)`. The per-row `expires_in` snapshot means config changes only affect *new* tokens. -- `refresh_token_expired?` — `Time.now.utc > created_at + Devise.api.config.refresh_token.expires_in.seconds`, unless infinite. **Not** snapshotted — reads current config, so changing `refresh_token.expires_in` retroactively re-times existing tokens. +- `expired?` — `Time.current > created_at + expires_in.seconds`, unless `access_token.expires_in_infinite.(owner)`. The per-row `expires_in` snapshot means config changes only affect *new* tokens. +- `refresh_token_expired?` — `Time.current > created_at + Devise.api.config.refresh_token.expires_in.seconds`, unless infinite. **Not** snapshotted — reads current config, so changing `refresh_token.expires_in` retroactively re-times existing tokens. - `revoked?` — `revoked_at.present?`. `active?` = not expired and not revoked. +Mutators: `revoke!` (stamps `revoked_at`, idempotent) and `revoke_family!` (walks to the chain root via `previous_refresh`, then revokes the root and every descendant via `refreshes` — used by refresh-token reuse detection). + +The model also filters `access_token`/`refresh_token`/`previous_refresh_token` from `#inspect` via `filter_attributes`, and the engine adds the same keys to the host app's `filter_parameters` (request-log redaction). Raw SQL logging can still print token values. + ## Refresh semantics (important) `TokensService::Refresh`: -1. Controller finds the row by `refresh_token` column; rejects unknown (`invalid_token`) or revoked (`revoked_token`) tokens. -2. Service rejects if `refresh_token_expired?` (`expired_refresh_token`). -3. Otherwise mints a **new** token row (`previous_refresh_token` = the presented refresh token) and returns it. -4. The old row keeps its state: its access token stays usable until its own expiry, and its refresh token **can be presented again** until the refresh-token TTL passes. There is no rotation/invalidations-on-reuse — a deliberate current behavior with security implications, tracked in [analysis/security-review.md](analysis/security-review.md). +1. Controller finds the row by `refresh_token` column; rejects unknown (`invalid_refresh_token`, 400) or revoked (`revoked_token`, 401) tokens. +2. With `refresh_token.rotation_enabled`, a presented token that is revoked **or already has `refreshes`** is treated as reuse: the whole family is revoked (`revoke_family!`) and `revoked_token` is returned. +3. Service rejects if `refresh_token_expired?` (`expired_refresh_token`). +4. Otherwise mints a **new** token row (`previous_refresh_token` = the presented refresh token) and returns it. With rotation enabled, the presented token is revoked in the same transaction. +5. **Without rotation (the default)** the old row keeps its state: its access token stays usable until its own expiry, and its refresh token **can be presented again** until the refresh-token TTL passes — see [analysis/security-review.md](analysis/security-review.md) (SEC-2, resolved via the opt-in flag). Revoking (`TokensService::Revoke`) only stamps `revoked_at` on the *presented access token's row* — not the whole chain, and not other sessions. diff --git a/docs/development.md b/docs/development.md index 0135889..0c76462 100644 --- a/docs/development.md +++ b/docs/development.md @@ -18,7 +18,7 @@ bundle exec rubocop # lint only (or: bundle exec rubocop -a for saf bundle exec rubocop --config .rubocop.yml --parallel # exactly what CI runs ``` -Style highlights (`.rubocop.yml`): target Ruby 2.7, single quotes, 120-char lines, `Style/Documentation` off, method/ABC limits at 30 (the controller already carries targeted `rubocop:disable` comments for `Metrics/AbcSize` — prefer refactoring over adding more disables). +Style highlights (`.rubocop.yml`): target Ruby 2.7, single quotes, 120-char lines, `Style/Documentation` off, method/ABC limits at 30. The codebase currently has no `rubocop:disable` comments — prefer refactoring (extracted helpers, constants) over adding them. ## Repo conventions @@ -32,7 +32,6 @@ Style highlights (`.rubocop.yml`): target Ruby 2.7, single quotes, 120-char line - Version constant: `lib/devise/api/version.rb` (currently `0.2.0`). SemVer intent; still pre-1.0 so minor bumps may break. - `CHANGELOG.md` exists but has not been maintained past the initial release — update it as part of any release work. - Release flow (maintainer): bump `version.rb` → update CHANGELOG → `bundle exec rake release` (tags, pushes, publishes to rubygems.org). Gem files are `git ls-files` minus `bin|test|spec|features` (see gemspec) — nothing in `spec/dummy` ships. -- `sig/devise/api.rbs` is a stub — RBS is not actually maintained. ## CI diff --git a/docs/services.md b/docs/services.md index 880d518..f1aa128 100644 --- a/docs/services.md +++ b/docs/services.md @@ -34,7 +34,7 @@ graph LR ### `Authenticate` - **Inputs:** `params: Types::Hash`, `resource_class: Types::Class` - **Logic:** `find_for_authentication(params.slice(*authentication_keys))` → `valid_for_authentication? { valid_password? }` (increments `failed_attempts` for lockable) → `active_for_authentication?` (fails for locked/unconfirmed). -- **Success:** the resource owner. **Failures:** `:invalid_email` (no record), `:invalid_authentication` (record attached). +- **Success:** the resource owner. **Failures:** no record → `:invalid_email` when `:email` is an authentication key, `:invalid_login` otherwise, or `:invalid_authentication` when `paranoid` is enabled (all with `record: nil`); wrong password / inactive account → `:invalid_authentication` (record attached). ### `SignIn` - **Inputs:** `params`, `resource_class` @@ -50,17 +50,17 @@ graph LR ### `Create` - **Inputs:** `resource_owner` (untyped), `previous_refresh_token: String | Nil = nil` -- **Logic:** guards `resource_owner.respond_to?(:access_tokens)` → builds row with generated unique access/refresh tokens, `expires_in` snapshot from config, `previous_refresh_token` passthrough. +- **Logic:** guards `resource_owner.respond_to?(:access_tokens)` → builds row with generated unique access/refresh tokens, `expires_in` snapshot from config, `previous_refresh_token` passthrough. Rescues `ActiveRecord::RecordNotUnique` from the unique token indexes and retries with freshly generated tokens (`MAX_TOKEN_GENERATION_ATTEMPTS = 3`), then re-raises. - **Success:** the token. **Failures:** `:invalid_resource_owner`, `:devise_api_token_create_error`. ### `Refresh` - **Inputs:** `devise_api_token` (typed `Types.Instance()` — resolved at class load), `resource_owner` (defaults to the token's owner) -- **Logic:** reject if `refresh_token_expired?` → `Create` with `previous_refresh_token` set. Does **not** revoke the old token. +- **Logic:** reject if `refresh_token_expired?` → `Create` with `previous_refresh_token` set. With `refresh_token.rotation_enabled`, the new token is minted and the presented token revoked in one transaction (a `Create` failure rolls back and leaves the presented token untouched); otherwise the old token is **not** revoked. - **Failures:** `:expired_refresh_token` or propagated. ### `Revoke` - **Inputs:** `devise_api_token` (optional — may be `nil`) -- **Logic:** blank token → `Success(nil)`; already revoked/expired → `Success(token)` (idempotent); else stamp `revoked_at = Time.zone.now`. +- **Logic:** blank token → `Success(nil)`; already revoked/expired → `Success(token)` (idempotent); else stamp `revoked_at = Time.current`. - **Failures:** `:devise_api_token_revoke_error`. ## Conventions for new services @@ -69,4 +69,4 @@ graph LR 2. Return `Success(value)` / `Failure(error: :symbol, record: model_or_nil)` — never raise for expected outcomes. 3. New error symbols require: `ERROR_TYPES` entry, status mapping in `ErrorResponse#status`, locale string, api-reference row. 4. Compose via do-notation (`yield`), not manual `if service.success?` nesting. -5. Cover behavior with request specs (service unit specs are currently placeholders — improving this is a known gap). +5. Cover behavior with both request specs and service unit specs asserting the monad contract (see `spec/services/**`). diff --git a/docs/testing.md b/docs/testing.md index 37575fd..0fb0b6b 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -45,12 +45,15 @@ Note the traits work by **backdating `created_at`** because all expiry math deri | Endpoints for a bare model (no trackable/lockable/confirmable) | `spec/requests/admin_user_tokens_spec.rb` | ✅ | | `authenticate_devise_api_token!` on a host controller | `spec/requests/authentication_spec.rb` (via dummy `HomeController`) | ✅ | | Non-default config (disabled sign_up/refresh, extra_fields, `:header`/`:params`-only location, revoke failure) | `spec/requests/configuration_overrides_spec.rb` | ✅ | +| Refresh-token rotation + family-revocation reuse detection (`rotation_enabled`) | `spec/requests/refresh_token_rotation_spec.rb` | ✅ | +| Enumeration hardening (`paranoid`, `error_response.verbose_account_state`) | `spec/requests/paranoid_mode_spec.rb` | ✅ | +| Engine initializer (`filter_parameters`) | `spec/devise/api/engine_spec.rb` | ✅ | | Default + customized routes (`controllers:`, `path:`, `path_names:` overrides) | `spec/routing/*.rb` | ✅ | | Config defaults + overrides on fresh instances | `spec/devise/api/configuration_spec.rb` | ✅ | | Response classes (incl. locked/unconfirmed/bare-model variants, disabled refresh, extra_fields) | `spec/devise/api/responses/*_spec.rb` | ✅ | | **Service objects** (monad contract: `Success`/`Failure` per branch) | `spec/services/**` | ✅ | -| Token model (`active?`, expiry incl. `expires_in_infinite`, generator collision retry, conditional validations) | `spec/devise/api/token_spec.rb` | ✅ | -| Controller helpers (extraction rescue, invalid location `ArgumentError`, unmemoized refresh-token lookup) | `spec/devise/api/controllers/helpers_spec.rb` | ✅ | +| Token model (`active?`, expiry incl. `expires_in_infinite`, generator collision retry, conditional validations, `revoke!`/`revoke_family!`, unique-index backstop, `#inspect` redaction) | `spec/devise/api/token_spec.rb` | ✅ | +| Controller helpers (extraction rescue, invalid location `ArgumentError`, memoized refresh-token lookup) | `spec/devise/api/controllers/helpers_spec.rb` | ✅ | | Generator (migration template, locale copy) | `spec/devise/api/generators/install_generator_spec.rb` | ✅ | ## Conventions for new specs diff --git a/lib/devise/api/configuration.rb b/lib/devise/api/configuration.rb index 424d2ea..5a12b4a 100644 --- a/lib/devise/api/configuration.rb +++ b/lib/devise/api/configuration.rb @@ -18,6 +18,7 @@ class Configuration setting :expires_in, default: 1.week, reader: true setting :generator, default: proc { |_resource_owner| ::Devise.friendly_token(60) }, reader: true setting :expires_in_infinite, default: proc { |_resource_owner| false }, reader: true + setting :rotation_enabled, default: false, reader: true end setting :sign_up, reader: true do @@ -32,6 +33,12 @@ class Configuration setting :params_key, default: 'access_token', reader: true end + setting :error_response, reader: true do + setting :verbose_account_state, default: true, reader: true + end + + setting :paranoid, default: false, reader: true + setting :base_token_model, default: 'Devise::Api::Token', reader: true setting :base_controller, default: '::DeviseController', reader: true diff --git a/lib/devise/api/controllers/helpers.rb b/lib/devise/api/controllers/helpers.rb index 65a2ae1..26be1eb 100644 --- a/lib/devise/api/controllers/helpers.rb +++ b/lib/devise/api/controllers/helpers.rb @@ -32,9 +32,11 @@ def authenticate_devise_api_token! end def current_devise_api_refresh_token - token = find_devise_api_token + return @current_devise_api_refresh_token if defined?(@current_devise_api_refresh_token) - Devise.api.config.base_token_model.constantize.find_by(refresh_token: token) + token = find_devise_api_token + devise_api_token_model = Devise.api.config.base_token_model.constantize + @current_devise_api_refresh_token = devise_api_token_model.find_by(refresh_token: token) end def current_devise_api_token diff --git a/lib/devise/api/generators/templates/migration.rb.erb b/lib/devise/api/generators/templates/migration.rb.erb index 67161ea..7f44d58 100644 --- a/lib/devise/api/generators/templates/migration.rb.erb +++ b/lib/devise/api/generators/templates/migration.rb.erb @@ -7,8 +7,8 @@ class CreateDeviseApiTables < ActiveRecord::Migration<%= migration_version %> create_table :devise_api_tokens, id: primary_key_type do |t| t.belongs_to :resource_owner, null: false, polymorphic: true, index: true, type: foreign_key_type - t.string :access_token, null: false, index: true - t.string :refresh_token, null: true, index: true + t.string :access_token, null: false, index: { unique: true } + t.string :refresh_token, null: true, index: { unique: true } t.integer :expires_in, null: false t.datetime :revoked_at, null: true t.string :previous_refresh_token, null: true, index: true diff --git a/lib/devise/api/rails/engine.rb b/lib/devise/api/rails/engine.rb index 95e5b22..a707910 100644 --- a/lib/devise/api/rails/engine.rb +++ b/lib/devise/api/rails/engine.rb @@ -5,6 +5,12 @@ module Api module Rails class Engine < ::Rails::Engine isolate_namespace Devise::Api + + # Keep raw token secrets out of the host app's request logs (tokens can arrive as + # query/body params when authorization.location is :params or :both) + initializer 'devise.api.filter_parameters' do |app| + app.config.filter_parameters |= %i[access_token refresh_token previous_refresh_token] + end end end end diff --git a/lib/devise/api/responses/error_response.rb b/lib/devise/api/responses/error_response.rb index 3fb192a..64a5ba2 100644 --- a/lib/devise/api/responses/error_response.rb +++ b/lib/devise/api/responses/error_response.rb @@ -15,6 +15,7 @@ class ErrorResponse sign_up_disabled invalid_refresh_token invalid_email + invalid_login invalid_resource_owner resource_owner_create_error devise_api_token_create_error @@ -22,6 +23,15 @@ class ErrorResponse invalid_authentication ].freeze + UNAUTHORIZED_ERRORS = %i[ + invalid_token expired_token expired_refresh_token revoked_token invalid_authentication + ].freeze + + BAD_REQUEST_ERRORS = %i[ + invalid_email invalid_login invalid_refresh_token refresh_token_disabled sign_up_disabled + invalid_resource_owner + ].freeze + ERROR_TYPES.each do |error_type| method_name = error_type.end_with?('_error') ? error_type : "#{error_type}_error" @@ -55,33 +65,34 @@ def status private - # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def error_description return [I18n.t("devise.api.error_response.#{error}")] if record.blank? - if invalid_authentication_error? && devise_lockable_info.present? && record.access_locked? - return [I18n.t('devise.api.error_response.lockable.locked')] - end - if invalid_authentication_error? && devise_confirmable_info.present? && !record.confirmed? - return [I18n.t('devise.api.error_response.confirmable.unconfirmed')] - end - return [I18n.t('devise.api.error_response.invalid_authentication')] if invalid_authentication_error? + return invalid_authentication_description if invalid_authentication_error? record.errors.full_messages end - # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity - def devise_lockable_info - unless resource_class.present? && - resource_class.supported_devise_modules.lockable? && - invalid_authentication_error? - return nil + def invalid_authentication_description + unless Devise.api.config.paranoid + return [I18n.t('devise.api.error_response.lockable.locked')] if lockable_record? && record.access_locked? + if confirmable_record? && !record.confirmed? + return [I18n.t('devise.api.error_response.confirmable.unconfirmed')] + end end + [I18n.t('devise.api.error_response.invalid_authentication')] + end + + def devise_lockable_info + return nil unless verbose_account_state? && lockable_record? && invalid_authentication_error? + unlock_at = record.access_locked? ? record.locked_at + ::Devise.unlock_in : nil { locked: record.access_locked?, max_attempts: ::Devise.maximum_attempts, + failed_attempts: record.failed_attempts, + # Deprecated misspelling kept for backward compatibility; will be removed in the next major release failed_attemps: record.failed_attempts, locked_at: record.locked_at, unlock_at: unlock_at @@ -89,11 +100,7 @@ def devise_lockable_info end def devise_confirmable_info - unless resource_class.present? && - resource_class.supported_devise_modules.confirmable? && - invalid_authentication_error? - return nil - end + return nil unless verbose_account_state? && confirmable_record? && invalid_authentication_error? { confirmed: record.confirmed?, @@ -101,20 +108,24 @@ def devise_confirmable_info }.compact end + def lockable_record? + record.present? && resource_class.present? && resource_class.supported_devise_modules.lockable? + end + + def confirmable_record? + record.present? && resource_class.present? && resource_class.supported_devise_modules.confirmable? + end + + def verbose_account_state? + !Devise.api.config.paranoid && Devise.api.config.error_response.verbose_account_state + end + def unauthorized_status? - invalid_token_error? || - expired_token_error? || - expired_refresh_token_error? || - revoked_token_error? || - invalid_authentication_error? + UNAUTHORIZED_ERRORS.include?(error) end def bad_request_status? - invalid_email_error? || - invalid_refresh_token_error? || - refresh_token_disabled_error? || - sign_up_disabled_error? || - invalid_resource_owner_error? + BAD_REQUEST_ERRORS.include?(error) end end end diff --git a/lib/devise/api/token.rb b/lib/devise/api/token.rb index 27a5ac2..c0dfc2e 100644 --- a/lib/devise/api/token.rb +++ b/lib/devise/api/token.rb @@ -7,6 +7,10 @@ module Api class Token < ::ActiveRecord::Base self.table_name = 'devise_api_tokens' + # Redact raw token secrets from #inspect / pretty_print output (see also the + # devise.api.filter_parameters engine initializer for request-log filtering) + self.filter_attributes += %i[access_token refresh_token previous_refresh_token] + # associations belongs_to :resource_owner, polymorphic: true, @@ -36,6 +40,21 @@ def revoked? revoked_at.present? end + def revoke! + return self if revoked? + + update!(revoked_at: Time.current) + self + end + + # Revokes every token in this token's refresh chain (ancestors and descendants). Used for + # refresh-token reuse detection when refresh_token.rotation_enabled is on. + def revoke_family! + transaction do + family_tokens.each(&:revoke!) + end + end + def active? !inactive? end @@ -47,13 +66,13 @@ def inactive? def expired? return false if Devise.api.config.access_token.expires_in_infinite.call(resource_owner) - !!(expires_in && Time.now.utc > expires_at) + !!(expires_in && Time.current > expires_at) end def refresh_token_expired? return false if Devise.api.config.refresh_token.expires_in_infinite.call(resource_owner) - Time.now.utc > refresh_token_expires_at + Time.current > refresh_token_expires_at end def self.generate_uniq_access_token(resource_owner) @@ -83,6 +102,19 @@ def expires_at def refresh_token_expires_at created_at + Devise.api.config.refresh_token.expires_in.seconds end + + def family_tokens + root = self + root = root.previous_refresh while root.previous_refresh.present? + + tokens = [] + queue = [root] + while (token = queue.shift) + tokens << token + queue.concat(token.refreshes.to_a) + end + tokens + end end end end diff --git a/sig/devise/api.rbs b/sig/devise/api.rbs deleted file mode 100644 index 34cc2b5..0000000 --- a/sig/devise/api.rbs +++ /dev/null @@ -1,6 +0,0 @@ -module Devise - module Api - VERSION: String - # See the writing guide of rbs: https://github.com/ruby/rbs#guides - end -end diff --git a/spec/devise/api/configuration_spec.rb b/spec/devise/api/configuration_spec.rb index a67d4a2..a6b43a6 100644 --- a/spec/devise/api/configuration_spec.rb +++ b/spec/devise/api/configuration_spec.rb @@ -40,6 +40,10 @@ expect(config.refresh_token.expires_in_infinite.call).to eq false end + it 'rotation_enabled is false' do + expect(config.refresh_token.rotation_enabled).to eq false + end + it 'generator returns a token string with using Devise.friendly_token' do allow(Devise).to receive(:friendly_token).with(60).and_return('token') expect(config.refresh_token.generator.call).to eq 'token' @@ -48,6 +52,18 @@ end end + context 'error_response' do + it 'verbose_account_state is true' do + expect(config.error_response.verbose_account_state).to eq true + end + end + + context 'paranoid' do + it 'is false' do + expect(config.paranoid).to eq false + end + end + context 'sign_up' do it 'enabled is true' do expect(config.sign_up.enabled).to eq true @@ -131,6 +147,9 @@ config.config.sign_up.enabled = false config.config.sign_up.extra_fields = [:name] config.config.authorization.location = :header + config.config.refresh_token.rotation_enabled = true + config.config.error_response.verbose_account_state = false + config.config.paranoid = true end it 'reflects the overridden values' do @@ -140,6 +159,9 @@ expect(config.sign_up.enabled).to eq false expect(config.sign_up.extra_fields).to eq [:name] expect(config.authorization.location).to eq :header + expect(config.refresh_token.rotation_enabled).to eq true + expect(config.error_response.verbose_account_state).to eq false + expect(config.paranoid).to eq true end it 'does not affect other instances' do @@ -149,6 +171,9 @@ expect(other_config.refresh_token.enabled).to eq true expect(other_config.sign_up.enabled).to eq true expect(other_config.authorization.location).to eq :both + expect(other_config.refresh_token.rotation_enabled).to eq false + expect(other_config.error_response.verbose_account_state).to eq true + expect(other_config.paranoid).to eq false end end end diff --git a/spec/devise/api/controllers/helpers_spec.rb b/spec/devise/api/controllers/helpers_spec.rb index 496a94a..6ef827e 100644 --- a/spec/devise/api/controllers/helpers_spec.rb +++ b/spec/devise/api/controllers/helpers_spec.rb @@ -23,6 +23,12 @@ it 'returns the token record' do expect(host.current_devise_api_refresh_token).to eq(devise_api_token) end + + it 'memoizes the lookup' do + 2.times { host.current_devise_api_refresh_token } + + expect(host).to have_received(:find_devise_api_token).once + end end context 'when no token can be extracted' do @@ -33,6 +39,12 @@ it 'returns nil' do expect(host.current_devise_api_refresh_token).to be_nil end + + it 'memoizes the nil result' do + 2.times { host.current_devise_api_refresh_token } + + expect(host).to have_received(:find_devise_api_token).once + end end end diff --git a/spec/devise/api/engine_spec.rb b/spec/devise/api/engine_spec.rb new file mode 100644 index 0000000..a6ba91a --- /dev/null +++ b/spec/devise/api/engine_spec.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Devise::Api::Rails::Engine do + it 'adds the token secrets to the host application filter parameters' do + expect(::Rails.application.config.filter_parameters) + .to include(:access_token, :refresh_token, :previous_refresh_token) + end +end diff --git a/spec/devise/api/generators/install_generator_spec.rb b/spec/devise/api/generators/install_generator_spec.rb index 54c88c5..7de10e5 100644 --- a/spec/devise/api/generators/install_generator_spec.rb +++ b/spec/devise/api/generators/install_generator_spec.rb @@ -37,6 +37,13 @@ it 'creates the devise api tokens table' do expect(File.read(migration_paths.first)).to include('create_table :devise_api_tokens') end + + it 'adds unique indexes for the token secrets' do + migration = File.read(migration_paths.first) + + expect(migration).to include('t.string :access_token, null: false, index: { unique: true }') + expect(migration).to include('t.string :refresh_token, null: true, index: { unique: true }') + end end context 'locale file' do diff --git a/spec/devise/api/responses/error_response_spec.rb b/spec/devise/api/responses/error_response_spec.rb index 38276a5..75b28f6 100644 --- a/spec/devise/api/responses/error_response_spec.rb +++ b/spec/devise/api/responses/error_response_spec.rb @@ -14,6 +14,7 @@ sign_up_disabled invalid_refresh_token invalid_email + invalid_login invalid_resource_owner resource_owner_create_error devise_api_token_create_error @@ -31,6 +32,7 @@ expect(described_class.new(nil, error: :sign_up_disabled)).to respond_to(:sign_up_disabled_error?) expect(described_class.new(nil, error: :invalid_refresh_token)).to respond_to(:invalid_refresh_token_error?) expect(described_class.new(nil, error: :invalid_email)).to respond_to(:invalid_email_error?) + expect(described_class.new(nil, error: :invalid_login)).to respond_to(:invalid_login_error?) expect(described_class.new(nil, error: :invalid_resource_owner)).to respond_to(:invalid_resource_owner_error?) expect(described_class.new(nil, error: :resource_owner_create_error)).to respond_to(:resource_owner_create_error?) expect(described_class.new(nil, @@ -203,6 +205,27 @@ end end + context 'invalid login error response' do + let(:error_response) { described_class.new(nil, error: :invalid_login) } + + it 'has a status of 400' do + expect(error_response.status).to eq :bad_request + end + + it 'has a body with an error and error description' do + allow(I18n).to receive(:t) + .with('devise.api.error_response.invalid_login') + .and_return('Invalid login') + + expect(error_response.body).to eq( + error: :invalid_login, + error_description: ['Invalid login'] + ) + + expect(I18n).to have_received(:t).with('devise.api.error_response.invalid_login') + end + end + context 'invalid resource owner error response' do let(:error_response) { described_class.new(nil, error: :invalid_resource_owner) } @@ -326,6 +349,7 @@ lockable: { locked: false, max_attempts: ::Devise.maximum_attempts, + failed_attempts: 0, failed_attemps: 0 } ) @@ -351,6 +375,7 @@ lockable: { locked: true, max_attempts: ::Devise.maximum_attempts, + failed_attempts: record.failed_attempts, failed_attemps: record.failed_attempts, locked_at: record.locked_at, unlock_at: record.locked_at + ::Devise.unlock_in diff --git a/spec/devise/api/token_spec.rb b/spec/devise/api/token_spec.rb index d9e4e31..ea618cd 100644 --- a/spec/devise/api/token_spec.rb +++ b/spec/devise/api/token_spec.rb @@ -94,6 +94,83 @@ end end + describe '#revoke!' do + context 'when the token is not revoked' do + let(:devise_api_token) { create(:devise_api_token) } + + it 'stamps revoked_at' do + expect(devise_api_token.revoke!).to eq devise_api_token + expect(devise_api_token.reload.revoked?).to eq true + end + end + + context 'when the token is already revoked' do + let(:devise_api_token) { create(:devise_api_token, :revoked) } + + it 'keeps the original revoked_at' do + original_revoked_at = devise_api_token.revoked_at + + devise_api_token.revoke! + + expect(devise_api_token.reload.revoked_at).to be_within(1.second).of(original_revoked_at) + end + end + end + + describe '#revoke_family!' do + let(:user) { create(:user) } + let!(:root_token) { create(:devise_api_token, resource_owner: user) } + let!(:middle_token) do + create(:devise_api_token, resource_owner: user, previous_refresh_token: root_token.refresh_token) + end + let!(:leaf_token) do + create(:devise_api_token, resource_owner: user, previous_refresh_token: middle_token.refresh_token) + end + let!(:unrelated_token) { create(:devise_api_token, resource_owner: user) } + + it 'revokes the whole refresh chain from any member' do + middle_token.revoke_family! + + expect(root_token.reload.revoked?).to eq true + expect(middle_token.reload.revoked?).to eq true + expect(leaf_token.reload.revoked?).to eq true + end + + it 'does not touch tokens outside the family' do + middle_token.revoke_family! + + expect(unrelated_token.reload.revoked?).to eq false + end + end + + describe 'database uniqueness of token secrets' do + let(:devise_api_token) { create(:devise_api_token) } + + it 'rejects a duplicate access token even when validations are bypassed' do + duplicate = build(:devise_api_token, resource_owner: devise_api_token.resource_owner, + access_token: devise_api_token.access_token) + + expect { duplicate.save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique) + end + + it 'rejects a duplicate refresh token even when validations are bypassed' do + duplicate = build(:devise_api_token, resource_owner: devise_api_token.resource_owner, + refresh_token: devise_api_token.refresh_token) + + expect { duplicate.save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique) + end + end + + describe 'secret redaction' do + let(:devise_api_token) { create(:devise_api_token) } + + it 'filters token secrets out of #inspect' do + expect(devise_api_token.inspect).to include('[FILTERED]') + expect(devise_api_token.inspect).not_to include(devise_api_token.access_token) + expect(devise_api_token.inspect).not_to include(devise_api_token.refresh_token) + end + end + describe '.generate_uniq_access_token' do context 'when the first generated token is already taken' do let(:user) { create(:user) } diff --git a/spec/dummy/db/migrate/20260825000003_add_unique_token_indexes_to_devise_api_tokens.rb b/spec/dummy/db/migrate/20260825000003_add_unique_token_indexes_to_devise_api_tokens.rb new file mode 100644 index 0000000..ad1d896 --- /dev/null +++ b/spec/dummy/db/migrate/20260825000003_add_unique_token_indexes_to_devise_api_tokens.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddUniqueTokenIndexesToDeviseApiTokens < ActiveRecord::Migration[7.0] + def change + remove_index :devise_api_tokens, :access_token + remove_index :devise_api_tokens, :refresh_token + add_index :devise_api_tokens, :access_token, unique: true + add_index :devise_api_tokens, :refresh_token, unique: true + end +end diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb index 67b89b5..03e3dd0 100644 --- a/spec/dummy/db/schema.rb +++ b/spec/dummy/db/schema.rb @@ -12,7 +12,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 20_260_825_000_002) do +ActiveRecord::Schema[7.0].define(version: 20_260_825_000_003) do create_table 'admin_users', force: :cascade do |t| t.string 'email', default: '', null: false t.string 'encrypted_password', default: '', null: false @@ -31,9 +31,9 @@ t.string 'previous_refresh_token' t.datetime 'created_at', null: false t.datetime 'updated_at', null: false - t.index ['access_token'], name: 'index_devise_api_tokens_on_access_token' + t.index ['access_token'], name: 'index_devise_api_tokens_on_access_token', unique: true t.index ['previous_refresh_token'], name: 'index_devise_api_tokens_on_previous_refresh_token' - t.index ['refresh_token'], name: 'index_devise_api_tokens_on_refresh_token' + t.index ['refresh_token'], name: 'index_devise_api_tokens_on_refresh_token', unique: true t.index %w[resource_owner_type resource_owner_id], name: 'index_devise_api_tokens_on_resource_owner' end diff --git a/spec/requests/paranoid_mode_spec.rb b/spec/requests/paranoid_mode_spec.rb new file mode 100644 index 0000000..ecd9b41 --- /dev/null +++ b/spec/requests/paranoid_mode_spec.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# Covers the account-enumeration hardening flags: paranoid (SEC-5) and +# error_response.verbose_account_state (SEC-7). +RSpec.describe Devise::Api::TokensController, type: :request do + describe 'POST /users/tokens/sign_in' do + context 'when paranoid mode is enabled' do + around do |example| + original = Devise.api.config.paranoid + Devise.api.config.paranoid = true + example.run + ensure + Devise.api.config.paranoid = original + end + + context 'and the account does not exist' do + before do + post sign_in_user_tokens_path, params: { email: 'unknown@development.com', password: 'pass123456' }, + as: :json + end + + it 'returns http unauthorized instead of bad request' do + expect(response).to have_http_status(:unauthorized) + end + + it 'returns the generic invalid authentication error' do + expect(parsed_body.error).to eq 'invalid_authentication' + expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.invalid_authentication')]) + end + end + + context 'and the password is wrong' do + let(:user) { create(:user, password: 'pass123456') } + + before do + user.confirm + + post sign_in_user_tokens_path, params: { email: user.email, password: 'wrong password' }, as: :json + end + + it 'returns the same response shape as a missing account' do + expect(response).to have_http_status(:unauthorized) + expect(parsed_body.error).to eq 'invalid_authentication' + expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.invalid_authentication')]) + end + + it 'does not expose lockable or confirmable state' do + expect(parsed_body.lockable).to be_nil + expect(parsed_body.confirmable).to be_nil + end + end + + context 'and the account is locked' do + let(:user) { create(:user, password: 'pass123456') } + + before do + user.confirm + user.lock_access! + + post sign_in_user_tokens_path, params: { email: user.email, password: 'pass123456' }, as: :json + end + + it 'returns the generic invalid authentication error without account state' do + expect(response).to have_http_status(:unauthorized) + expect(parsed_body.error).to eq 'invalid_authentication' + expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.invalid_authentication')]) + expect(parsed_body.lockable).to be_nil + expect(parsed_body.confirmable).to be_nil + end + end + end + + context 'when verbose account state is disabled' do + around do |example| + original = Devise.api.config.error_response.verbose_account_state + Devise.api.config.error_response.verbose_account_state = false + example.run + ensure + Devise.api.config.error_response.verbose_account_state = original + end + + context 'and the password is wrong' do + let(:user) { create(:user, password: 'pass123456') } + + before do + user.confirm + + post sign_in_user_tokens_path, params: { email: user.email, password: 'wrong password' }, as: :json + end + + it 'omits the lockable and confirmable blocks' do + expect(response).to have_http_status(:unauthorized) + expect(parsed_body.error).to eq 'invalid_authentication' + expect(parsed_body.lockable).to be_nil + expect(parsed_body.confirmable).to be_nil + end + end + + context 'and the account is locked' do + let(:user) { create(:user, password: 'pass123456') } + + before do + user.confirm + user.lock_access! + + post sign_in_user_tokens_path, params: { email: user.email, password: 'pass123456' }, as: :json + end + + it 'keeps the locked error description but omits the lockable metadata' do + expect(response).to have_http_status(:unauthorized) + expect(parsed_body.error).to eq 'invalid_authentication' + expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.lockable.locked')]) + expect(parsed_body.lockable).to be_nil + expect(parsed_body.confirmable).to be_nil + end + end + end + end +end diff --git a/spec/requests/refresh_token_rotation_spec.rb b/spec/requests/refresh_token_rotation_spec.rb new file mode 100644 index 0000000..83e0be5 --- /dev/null +++ b/spec/requests/refresh_token_rotation_spec.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# Covers refresh_token.rotation_enabled (SEC-2): rotating out the presented refresh token and +# revoking the whole token family when a rotated refresh token is replayed. +RSpec.describe Devise::Api::TokensController, type: :request do + describe 'POST /users/tokens/refresh' do + let(:user) { create(:user) } + let(:devise_api_token) { create(:devise_api_token, resource_owner: user) } + + context 'when rotation is disabled (default)' do + before do + post refresh_user_tokens_path, headers: authentication_headers_for(user, devise_api_token, :refresh_token), + as: :json + end + + it 'keeps the presented refresh token usable' do + expect(devise_api_token.reload.revoked?).to eq false + + post refresh_user_tokens_path, headers: authentication_headers_for(user, devise_api_token, :refresh_token), + as: :json + + expect(response).to have_http_status(:success) + end + end + + context 'when rotation is enabled' do + around do |example| + original = Devise.api.config.refresh_token.rotation_enabled + Devise.api.config.refresh_token.rotation_enabled = true + example.run + ensure + Devise.api.config.refresh_token.rotation_enabled = original + end + + context 'and the refresh token is presented for the first time' do + before do + post refresh_user_tokens_path, headers: authentication_headers_for(user, devise_api_token, :refresh_token), + as: :json + end + + it 'returns http success with a new token' do + expect(response).to have_http_status(:success) + expect(parsed_body.token).to be_present + expect(parsed_body.refresh_token).not_to eq(devise_api_token.refresh_token) + end + + it 'revokes the presented refresh token' do + expect(devise_api_token.reload.revoked?).to eq true + end + + it 'links the new token to the presented one' do + expect(devise_api_token.refreshes.count).to eq(1) + end + end + + context 'and a rotated refresh token is replayed' do + before do + post refresh_user_tokens_path, headers: authentication_headers_for(user, devise_api_token, :refresh_token), + as: :json + post refresh_user_tokens_path, headers: authentication_headers_for(user, devise_api_token, :refresh_token), + as: :json + end + + it 'returns http unauthorized' do + expect(response).to have_http_status(:unauthorized) + end + + it 'returns a revoked token error response' do + expect(parsed_body.error).to eq 'revoked_token' + expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.revoked_token')]) + end + + it 'revokes the whole token family' do + expect(Devise::Api::Token.count).to eq(2) + expect(Devise::Api::Token.all).to all(be_revoked) + end + end + + context 'and an unrevoked refresh token that was already refreshed before rotation is replayed' do + let!(:successor) do + create(:devise_api_token, resource_owner: user, previous_refresh_token: devise_api_token.refresh_token) + end + + before do + post refresh_user_tokens_path, headers: authentication_headers_for(user, devise_api_token, :refresh_token), + as: :json + end + + it 'returns http unauthorized with a revoked token error' do + expect(response).to have_http_status(:unauthorized) + expect(parsed_body.error).to eq 'revoked_token' + end + + it 'revokes the whole token family' do + expect(devise_api_token.reload.revoked?).to eq true + expect(successor.reload.revoked?).to eq true + end + end + end + end +end diff --git a/spec/requests/tokens_spec.rb b/spec/requests/tokens_spec.rb index 55ac2e4..2f2319b 100644 --- a/spec/requests/tokens_spec.rb +++ b/spec/requests/tokens_spec.rb @@ -188,6 +188,7 @@ expect(parsed_body.lockable).to be_present expect(parsed_body.lockable.locked).to eq true expect(parsed_body.lockable.max_attempts).to eq Devise.maximum_attempts + expect(parsed_body.lockable.failed_attempts).to eq user.reload.failed_attempts expect(parsed_body.lockable.failed_attemps).to be_present expect(parsed_body.lockable.locked_at.to_date).to eq user.locked_at.to_date end @@ -251,6 +252,7 @@ expect(parsed_body.lockable).to be_present expect(parsed_body.lockable.locked).to eq false expect(parsed_body.lockable.max_attempts).to eq Devise.maximum_attempts + expect(parsed_body.lockable.failed_attempts).to eq 1 expect(parsed_body.lockable.failed_attemps).to eq 1 end @@ -493,13 +495,13 @@ as: :json end - it 'returns http unauthorized' do - expect(response).to have_http_status(:unauthorized) + it 'returns http bad request' do + expect(response).to have_http_status(:bad_request) end it 'returns an error response' do - expect(parsed_body.error).to eq 'invalid_token' - expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.invalid_token')]) + expect(parsed_body.error).to eq 'invalid_refresh_token' + expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.invalid_refresh_token')]) end it 'does not refresh the token' do @@ -515,13 +517,13 @@ post refresh_user_tokens_path(access_token: devise_api_token.refresh_token), as: :json end - it 'returns http unauthorized' do - expect(response).to have_http_status(:unauthorized) + it 'returns http bad request' do + expect(response).to have_http_status(:bad_request) end it 'returns an error response' do - expect(parsed_body.error).to eq 'invalid_token' - expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.invalid_token')]) + expect(parsed_body.error).to eq 'invalid_refresh_token' + expect(parsed_body.error_description).to eq([I18n.t('devise.api.error_response.invalid_refresh_token')]) end it 'does not refresh the token' do diff --git a/spec/services/resource_owner_service/authenticate_spec.rb b/spec/services/resource_owner_service/authenticate_spec.rb index 16468ab..ad1bdb0 100644 --- a/spec/services/resource_owner_service/authenticate_spec.rb +++ b/spec/services/resource_owner_service/authenticate_spec.rb @@ -19,6 +19,36 @@ end end + context 'when no resource owner matches non-email authentication keys' do + let(:params) { { name: 'unknown', password: 'pass123456' } } + + before do + allow(User).to receive(:authentication_keys).and_return([:name]) + end + + it 'returns a generic invalid login failure instead of invalid email' do + expect(result).to be_failure + expect(result.failure).to eq(error: :invalid_login, record: nil) + end + end + + context 'when paranoid mode is enabled and no resource owner matches' do + let(:params) { { email: 'unknown@development.com', password: 'pass123456' } } + + around do |example| + original = Devise.api.config.paranoid + Devise.api.config.paranoid = true + example.run + ensure + Devise.api.config.paranoid = original + end + + it 'returns the same failure as a wrong password' do + expect(result).to be_failure + expect(result.failure).to eq(error: :invalid_authentication, record: nil) + end + end + context 'when the password is wrong' do let(:user) { create(:user) } let(:params) { { email: user.email, password: 'wrong password' } } diff --git a/spec/services/tokens_service/create_spec.rb b/spec/services/tokens_service/create_spec.rb index 8a31aa6..1ab198c 100644 --- a/spec/services/tokens_service/create_spec.rb +++ b/spec/services/tokens_service/create_spec.rb @@ -49,6 +49,34 @@ end end + context 'when the database rejects a duplicate token' do + let(:resource_owner) { create(:user) } + let(:existing_token) { create(:devise_api_token, resource_owner: resource_owner) } + + before do + # Simulate the check-then-insert race: the generator returns an already-taken token first, + # the application-level uniqueness validation is bypassed and the unique index rejects it + allow_any_instance_of(Devise::Api::Token).to receive(:valid?).and_return(true) + end + + it 'retries with a freshly generated token' do + allow(Devise::Api::Token).to receive(:generate_uniq_access_token) + .and_return(existing_token.access_token, SecureRandom.hex(32)) + + expect(result).to be_success + expect(result.success).to be_persisted + expect(result.success.access_token).not_to eq(existing_token.access_token) + end + + it 'gives up and raises after exhausting the retries' do + allow(Devise::Api::Token).to receive(:generate_uniq_access_token).and_return(existing_token.access_token) + + expect { result }.to raise_error(ActiveRecord::RecordNotUnique) + expect(Devise::Api::Token).to have_received(:generate_uniq_access_token) + .exactly(described_class::MAX_TOKEN_GENERATION_ATTEMPTS).times + end + end + context 'when the token cannot be saved' do let(:resource_owner) { create(:user) } diff --git a/spec/services/tokens_service/refresh_spec.rb b/spec/services/tokens_service/refresh_spec.rb index 278d402..20501fa 100644 --- a/spec/services/tokens_service/refresh_spec.rb +++ b/spec/services/tokens_service/refresh_spec.rb @@ -54,5 +54,44 @@ expect(result.failure[:error]).to eq(:devise_api_token_create_error) end end + + context 'when rotation is enabled' do + around do |example| + original = Devise.api.config.refresh_token.rotation_enabled + Devise.api.config.refresh_token.rotation_enabled = true + example.run + ensure + Devise.api.config.refresh_token.rotation_enabled = original + end + + context 'and the refresh token is valid' do + let(:devise_api_token) { create(:devise_api_token) } + + it 'returns a success with a new token' do + expect(result).to be_success + expect(result.success.previous_refresh_token).to eq(devise_api_token.refresh_token) + end + + it 'revokes the presented token' do + result + + expect(devise_api_token.reload.revoked?).to eq true + end + end + + context 'and the token creation fails' do + let(:devise_api_token) { create(:devise_api_token) } + + before do + allow(Devise.api.config.access_token).to receive(:expires_in).and_return(nil) + end + + it 'returns a failure and keeps the presented token unrevoked' do + expect(result).to be_failure + expect(result.failure[:error]).to eq(:devise_api_token_create_error) + expect(devise_api_token.reload.revoked?).to eq false + end + end + end end end