security: sanitize exception messages in v1 API responses (FIX-11)#1521
security: sanitize exception messages in v1 API responses (FIX-11)#1521jjmata merged 2 commits intowe-promise:mainfrom
Conversation
Replace raw e.message/error.message interpolations in response bodies
with generic error strings, and log class+message server-side. Prevents
leaking internal exception details (stack traces, SQL fragments, record
data) to API clients.
Covers:
- API v1 accounts, categories (index/show), holdings, sync, trades,
transactions (index/show/create/update/destroy), valuations
(show/create/update): replace "Error: #{e.message}" with
"An unexpected error occurred".
- API v1 auth: device-registration rescue paths now log
"[Auth] Device registration failed: ..." and respond with
"Failed to register device".
- WebhooksController#plaid and #plaid_eu: log full error and respond
with "Invalid webhook".
- Settings::ProvidersController: generic user-facing flash alert,
detailed log line with error class + message.
Updates providers_controller_test assertion to match sanitized flash.
brin-security-scanner
Bot
added
the
contributor:verified
📝 WalkthroughWalkthroughThis PR replaces detailed exception messages returned to users with generic messages across multiple controllers and tightens device-registration validation and error handling in the API auth flow, while preserving internal logging and status codes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant AuthCtrl as Api::V1::AuthController
participant DB as Database
participant Invite as InviteCode
participant Device as MobileDeviceService
participant Token as TokenIssuer
rect rgba(200,200,255,0.5)
Client->>AuthCtrl: POST /api/v1/auth/signup (user + device params)
AuthCtrl->>DB: ActiveRecord transaction begin
AuthCtrl->>DB: create user (user.save)
DB-->>AuthCtrl: user saved
AuthCtrl->>Invite: InviteCode.claim!(code)
Invite-->>AuthCtrl: claimed
AuthCtrl->>Device: MobileDevice.upsert_device!(params)
Device-->>AuthCtrl: device record
AuthCtrl->>Token: device.issue_token!
Token-->>AuthCtrl: token_response
AuthCtrl->>DB: ActiveRecord transaction commit
AuthCtrl-->>Client: 201 Created + token_response
end
rect rgba(255,200,200,0.5)
Note over Device,Token: On Device/Token failure (ActiveRecord::RecordInvalid)
Device-->>AuthCtrl: raises RecordInvalid
AuthCtrl->>AuthCtrl: rescue RecordInvalid
AuthCtrl->>Rails.logger: log "[Auth] Device registration failed: <class> - <message>"
AuthCtrl-->>Client: 400 Bad Request { error: "Failed to register device" }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/v1/auth_controller.rb (1)
48-60:⚠️ Potential issue | 🟠 MajorMake device registration failures rollback prior auth side effects.
Line 56 and Line 377 can return 422 after callers have already committed signup/SSO mutations. In
signup, the user/family and invite claim happen before device registration; in SSO account creation, the user, invitation, and OIDC identity are committed beforeissue_mobile_tokens. Sincevalid_device_info?only checks presence andMobileDevicecan still raiseActiveRecord::RecordInvalidfor invalid attributes likedevice_type, clients can receive"Failed to register device"while the account/link was actually created. Wrap those auth mutations plus device/token issuance in one transaction, or validate device params against model constraints before any mutation.Also applies to: 213-225, 371-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/auth_controller.rb` around lines 48 - 60, The device registration can fail after user/family/invite/identity changes have been committed (see signup and issue_mobile_tokens paths), causing partial account creation; fix by wrapping the related mutations and device/token creation in a single transaction (e.g., wrap the user creation/invite claim/identity creation plus MobileDevice.upsert_device! and device.issue_token! in one ActiveRecord transaction) so any ActiveRecord::RecordInvalid from MobileDevice.upsert_device! or device.issue_token! will rollback prior changes; alternatively (or in addition) validate device params up-front using MobileDevice validations (call a validator or MobileDevice.new(device_params).valid?) before committing invites/user/identity to avoid raising after commit. Ensure changes reference InviteCode.claim!, MobileDevice.upsert_device!, device.issue_token!, signup and issue_mobile_tokens flows mentioned in the review.
🧹 Nitpick comments (1)
test/controllers/settings/providers_controller_test.rb (1)
273-274: Tighten the log expectation to cover the exception class.The comment says the log includes class and message, but the regex would still pass if
StandardError -were removed.Proposed test tightening
- Rails.logger.expects(:error).with(regexp_matches(/Failed to update provider settings.*Database error/)).once + Rails.logger.expects(:error).with(regexp_matches(/Failed to update provider settings: StandardError - Database error/)).once🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/settings/providers_controller_test.rb` around lines 273 - 274, The test currently expects Rails.logger.error to be called with /Failed to update provider settings.*Database error/ which would pass if the exception class is missing; update the Rails.logger.expects in providers_controller_test.rb to require the exception class name as well (e.g. tighten the regex to include " - ActiveRecord::RecordInvalid" or the specific exception your controller raises) so the expectation verifies both the error message and the exception class are logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/controllers/api/v1/auth_controller.rb`:
- Around line 48-60: The device registration can fail after
user/family/invite/identity changes have been committed (see signup and
issue_mobile_tokens paths), causing partial account creation; fix by wrapping
the related mutations and device/token creation in a single transaction (e.g.,
wrap the user creation/invite claim/identity creation plus
MobileDevice.upsert_device! and device.issue_token! in one ActiveRecord
transaction) so any ActiveRecord::RecordInvalid from MobileDevice.upsert_device!
or device.issue_token! will rollback prior changes; alternatively (or in
addition) validate device params up-front using MobileDevice validations (call a
validator or MobileDevice.new(device_params).valid?) before committing
invites/user/identity to avoid raising after commit. Ensure changes reference
InviteCode.claim!, MobileDevice.upsert_device!, device.issue_token!, signup and
issue_mobile_tokens flows mentioned in the review.
---
Nitpick comments:
In `@test/controllers/settings/providers_controller_test.rb`:
- Around line 273-274: The test currently expects Rails.logger.error to be
called with /Failed to update provider settings.*Database error/ which would
pass if the exception class is missing; update the Rails.logger.expects in
providers_controller_test.rb to require the exception class name as well (e.g.
tighten the regex to include " - ActiveRecord::RecordInvalid" or the specific
exception your controller raises) so the expectation verifies both the error
message and the exception class are logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3450265-34da-4df9-b530-eadbc567da2d
📒 Files selected for processing (11)
app/controllers/api/v1/accounts_controller.rbapp/controllers/api/v1/auth_controller.rbapp/controllers/api/v1/categories_controller.rbapp/controllers/api/v1/holdings_controller.rbapp/controllers/api/v1/sync_controller.rbapp/controllers/api/v1/trades_controller.rbapp/controllers/api/v1/transactions_controller.rbapp/controllers/api/v1/valuations_controller.rbapp/controllers/settings/providers_controller.rbapp/controllers/webhooks_controller.rbtest/controllers/settings/providers_controller_test.rb
Major — partial-commit on device registration failure:
- Strengthened valid_device_info? to also run MobileDevice's model
validations up-front (device_type inclusion, attribute presence), not
just a flat "are the keys present?" check. A client that sends a bad
device_type ("windows", etc.) is now rejected at the API boundary
BEFORE signup commits any user/family/invite state.
- Wrapped the signup path (user.save + InviteCode.claim + MobileDevice
upsert + token issuance) in ActiveRecord::Base.transaction. A
post-save RecordInvalid from device registration (e.g., racing
uniqueness on device_id) now rolls back the user/invite/family so
clients don't see a partial-account state.
- Rescue branch logs the exception class + message ("#{e.class} - #{e.message}")
for better postmortem debugging, matching the providers controller
pattern.
Nit:
- Tightened providers_controller_test log expectation regex to assert on
both the exception class name AND the message ("StandardError - Database
error"), so a regression that drops either still fails the test.
Tests:
- New: "should reject signup with invalid device_type before committing
any state" — POST /api/v1/auth/signup with device_type="windows"
returns 400 AND asserts no User, MobileDevice, or Doorkeeper::AccessToken
row was created.
Note on SSO path (sso_exchange → issue_mobile_tokens, lines 173/225): the
device_info in those flows comes from Rails.cache (populated by an earlier
request that already passed valid_device_info?), so the pre-validation
covers it indirectly. Wrapping the full SSO account creation (user +
invitation + OidcIdentity + issue_mobile_tokens) in one transaction would
be a meaningful architectural cleanup but is out of scope for this
error-hygiene PR — filed it as a mental note for a follow-up.
|
Thanks — addressed both points, with one caveat on scope. Major — partial-commit on device registration failure (
New test: Scope caveat: the SSO path ( Nit — test regex tightened ( All changes in commit — wolfgang 🤖 |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/controllers/api/v1/auth_controller.rb (3)
98-98: Minor: log format inconsistent with the signup branch.Line 64 logs
"#{e.class} - #{e.message}", while Line 98 and Line 395 only loge.message. Since the PR objective specifically calls out "logs exceptions including class + message for server-side debugging", aligning these two call sites with the signup format would make server-side triage consistent across all three device-registration paths.♻️ Proposed tweak
- Rails.logger.error("[Auth] Device registration failed: #{e.message}") + Rails.logger.error("[Auth] Device registration failed: #{e.class} - #{e.message}")Apply at both Line 98 (
login) and Line 395 (issue_mobile_tokens).Also applies to: 395-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/auth_controller.rb` at line 98, Update the Rails.logger.error calls in the login and issue_mobile_tokens flows to match the signup branch format by including exception class and message; locate the Rails.logger.error("[Auth] Device registration failed: #{e.message}") call inside the login method and the analogous logger in issue_mobile_tokens and change the log payload to include "#{e.class} - #{e.message}" so all three device-registration paths (signup, login, issue_mobile_tokens) consistently log exception class and message.
93-101: Consider the same transaction wrap forlogin(andissue_mobile_tokens) to avoid partial-state token revocation.
MobileDevice#issue_token!(seeapp/models/mobile_device.rb:69-88) callsrevoke_all_tokens!beforeDoorkeeper::AccessToken.create!. Ifcreate!raisesActiveRecord::RecordInvalid— which is exactly what this rescue block catches — the user's existing tokens on this device have already been revoked with no replacement issued, and the client only sees a generic 422. On login (and SSO link/create flows routed throughissue_mobile_tokenson Lines 388–397), this can effectively lock the user out of active mobile sessions on a transient failure.This is the same class of partial-commit bug the signup change addresses; wrapping device upsert + token issuance in
ActiveRecord::Base.transactionwould roll back therevoke_all_tokens!side effect on failure and keep the three code paths consistent.♻️ Example for the `login` path
- # Create device and OAuth token - begin - device = MobileDevice.upsert_device!(user, device_params) - token_response = device.issue_token! - rescue ActiveRecord::RecordInvalid => e - Rails.logger.error("[Auth] Device registration failed: #{e.message}") - render json: { error: "Failed to register device" }, status: :unprocessable_entity - return - end + # Create device and OAuth token atomically so a token-create failure + # does not leave the device with all prior tokens revoked. + token_response = nil + begin + ActiveRecord::Base.transaction do + device = MobileDevice.upsert_device!(user, device_params) + token_response = device.issue_token! + end + rescue ActiveRecord::RecordInvalid => e + Rails.logger.error("[Auth] Device registration failed: #{e.class} - #{e.message}") + render json: { error: "Failed to register device" }, status: :unprocessable_entity + return + endThe same pattern applies to
issue_mobile_tokenson Lines 388–397.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/auth_controller.rb` around lines 93 - 101, The login (and issue_mobile_tokens) flow can leave device tokens revoked if MobileDevice#issue_token! calls revoke_all_tokens! before creating a new token and the subsequent Doorkeeper::AccessToken.create! raises ActiveRecord::RecordInvalid; wrap the device upsert and token issuance in a single DB transaction to guarantee rollback on failure. Concretely, in the login action (and in the method that calls issue_mobile_tokens) surround the calls to MobileDevice.upsert_device!(...) and device.issue_token! with ActiveRecord::Base.transaction do ... end so that revoke_all_tokens! is rolled back if token creation fails; ensure you reference MobileDevice.upsert_device!, MobileDevice#issue_token!, revoke_all_tokens!, and ActiveRecord::Base.transaction when making the change.
329-334: Nit: prefer the publicerrorsenumerable over the internalerrors.errorsaccessor.
ActiveModel::Errorsis itself enumerable and yieldsActiveModel::Errorobjects, so you can skip the.errors.errorsindirection. Purely cosmetic — behavior is unchanged.♻️ Proposed tweak
- preview = MobileDevice.new(device_params) - preview.valid? - relevant_errors = preview.errors.errors.reject do |err| - err.type == :taken || err.attribute == :user - end - relevant_errors.empty? + preview = MobileDevice.new(device_params) + preview.valid? + preview.errors.none? { |err| err.type != :taken && err.attribute != :user }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/auth_controller.rb` around lines 329 - 334, Replace the internal errors.errors access with the public enumerable on ActiveModel::Errors: when building the preview (MobileDevice.new(device_params)) iterate over preview.errors (not preview.errors.errors) to filter out entries with type == :taken or attribute == :user and then check emptiness (update the use of relevant_errors to call preview.errors.reject { |err| ... } so behavior stays identical but uses the public API).test/controllers/api/v1/auth_controller_test.rb (1)
97-114: Consider also asserting the generic error body.The test covers the no-state-change contract well. A small enhancement: the neighboring test at Lines 80–95 asserts
response_data["error"], and this PR is explicitly about keeping error messages generic. Adding a body assertion here would guard against a regression wherevalid_device_info?starts leakinge.message(e.g."Device type is not included in the list") to the client.♻️ Proposed addition
assert_response :bad_request + response_data = JSON.parse(response.body) + assert_equal "Device information is required", response_data["error"] end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/api/v1/auth_controller_test.rb` around lines 97 - 114, Add an assertion that inspects the JSON response body (e.g. parse response into response_data) and verifies the error field is the generic client-facing message rather than the raw exception: after the post to "/api/v1/auth/signup" in the test "should reject signup with invalid device_type before committing any state", assert response_data["error"] is present and assert_not_match(/is not included/, response_data["error"]) (or match the same generic error string used by the neighboring test that checks response_data["error"]) to guard against leaking e.message from valid_device_info?.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/api/v1/auth_controller.rb`:
- Line 98: Update the Rails.logger.error calls in the login and
issue_mobile_tokens flows to match the signup branch format by including
exception class and message; locate the Rails.logger.error("[Auth] Device
registration failed: #{e.message}") call inside the login method and the
analogous logger in issue_mobile_tokens and change the log payload to include
"#{e.class} - #{e.message}" so all three device-registration paths (signup,
login, issue_mobile_tokens) consistently log exception class and message.
- Around line 93-101: The login (and issue_mobile_tokens) flow can leave device
tokens revoked if MobileDevice#issue_token! calls revoke_all_tokens! before
creating a new token and the subsequent Doorkeeper::AccessToken.create! raises
ActiveRecord::RecordInvalid; wrap the device upsert and token issuance in a
single DB transaction to guarantee rollback on failure. Concretely, in the login
action (and in the method that calls issue_mobile_tokens) surround the calls to
MobileDevice.upsert_device!(...) and device.issue_token! with
ActiveRecord::Base.transaction do ... end so that revoke_all_tokens! is rolled
back if token creation fails; ensure you reference MobileDevice.upsert_device!,
MobileDevice#issue_token!, revoke_all_tokens!, and
ActiveRecord::Base.transaction when making the change.
- Around line 329-334: Replace the internal errors.errors access with the public
enumerable on ActiveModel::Errors: when building the preview
(MobileDevice.new(device_params)) iterate over preview.errors (not
preview.errors.errors) to filter out entries with type == :taken or attribute ==
:user and then check emptiness (update the use of relevant_errors to call
preview.errors.reject { |err| ... } so behavior stays identical but uses the
public API).
In `@test/controllers/api/v1/auth_controller_test.rb`:
- Around line 97-114: Add an assertion that inspects the JSON response body
(e.g. parse response into response_data) and verifies the error field is the
generic client-facing message rather than the raw exception: after the post to
"/api/v1/auth/signup" in the test "should reject signup with invalid device_type
before committing any state", assert response_data["error"] is present and
assert_not_match(/is not included/, response_data["error"]) (or match the same
generic error string used by the neighboring test that checks
response_data["error"]) to guard against leaking e.message from
valid_device_info?.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad841bab-9a8b-4e4b-86b7-91c6b34720d7
📒 Files selected for processing (3)
app/controllers/api/v1/auth_controller.rbtest/controllers/api/v1/auth_controller_test.rbtest/controllers/settings/providers_controller_test.rb
github-project-automation
Bot
moved this to Todo
in Stable release train (v0.7.0) - April 2026
github-project-automation
Bot
moved this from Todo
to Done
in Stable release train (v0.7.0) - April 2026
|
🍻 |
…e-promise#1521) * fix(security): sanitize exception messages in API responses (FIX-11) Replace raw e.message/error.message interpolations in response bodies with generic error strings, and log class+message server-side. Prevents leaking internal exception details (stack traces, SQL fragments, record data) to API clients. Covers: - API v1 accounts, categories (index/show), holdings, sync, trades, transactions (index/show/create/update/destroy), valuations (show/create/update): replace "Error: #{e.message}" with "An unexpected error occurred". - API v1 auth: device-registration rescue paths now log "[Auth] Device registration failed: ..." and respond with "Failed to register device". - WebhooksController#plaid and #plaid_eu: log full error and respond with "Invalid webhook". - Settings::ProvidersController: generic user-facing flash alert, detailed log line with error class + message. Updates providers_controller_test assertion to match sanitized flash. * fix(security): address CodeRabbit review Major — partial-commit on device registration failure: - Strengthened valid_device_info? to also run MobileDevice's model validations up-front (device_type inclusion, attribute presence), not just a flat "are the keys present?" check. A client that sends a bad device_type ("windows", etc.) is now rejected at the API boundary BEFORE signup commits any user/family/invite state. - Wrapped the signup path (user.save + InviteCode.claim + MobileDevice upsert + token issuance) in ActiveRecord::Base.transaction. A post-save RecordInvalid from device registration (e.g., racing uniqueness on device_id) now rolls back the user/invite/family so clients don't see a partial-account state. - Rescue branch logs the exception class + message ("#{e.class} - #{e.message}") for better postmortem debugging, matching the providers controller pattern. Nit: - Tightened providers_controller_test log expectation regex to assert on both the exception class name AND the message ("StandardError - Database error"), so a regression that drops either still fails the test. Tests: - New: "should reject signup with invalid device_type before committing any state" — POST /api/v1/auth/signup with device_type="windows" returns 400 AND asserts no User, MobileDevice, or Doorkeeper::AccessToken row was created. Note on SSO path (sso_exchange → issue_mobile_tokens, lines 173/225): the device_info in those flows comes from Rails.cache (populated by an earlier request that already passed valid_device_info?), so the pre-validation covers it indirectly. Wrapping the full SSO account creation (user + invitation + OidcIdentity + issue_mobile_tokens) in one transaction would be a meaningful architectural cleanup but is out of scope for this error-hygiene PR — filed it as a mental note for a follow-up.
2 participants
Summary
Smallest PR of the series — a single finding addressed across several v1 controllers.
Part of the security hardening originally bundled in #1104 (closed), now split by functional area per @jjmata's feedback. Companion PRs: #1516 #1517 #1518 #1519 #1520.
Finding addressed
FIX-11 (MED, CWE-209) — Raw exception messages leaked in API error responses. Replaced
e.messagebodies with generic user-facing strings; the details are logged server-side for debugging.Files touched:
app/controllers/api/v1/{accounts,auth,categories,holdings,sync,trades,transactions,valuations}_controller.rbapp/controllers/settings/providers_controller.rb(sanitized flash + preserved logger)app/controllers/webhooks_controller.rbNote on FIX-12 (skipped)
The original #1104 also addressed FIX-12 (invite codes missing admin authorization). Upstream main already covers this with a stricter
ensure_super_adminbefore_actiononinvite_codes_controller.rb(tighter than the original patch). Skipped to preserve the current policy.Tests
test/controllers/settings/providers_controller_test.rbupdated for sanitized flash.bin/rubocopclean,bin/brakeman0 warnings.Out of scope
Related
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements