mirror of
https://github.com/we-promise/sure.git
synced 2026-08-05 16:42:18 +00:00
* feat(snaptrade): replace device-flow OAuth with authorization-code + PKCE flow Squashed from 16 commits on snaptrade-oauth-apps for a clean rebase onto current upstream/main ahead of opening a PR. * fix(snaptrade): address PR #2747 review feedback on OAuth PKCE flow - Remove unreachable dead-code guard in import_latest_snaptrade_data - Guard apply_oauth_tokens! against a malformed payload missing access_token - Wrap token endpoint network errors in ApiError and retry like data calls - Remove unused Provider::Snaptrade#revoke_token! instance method - Preserve return_to/accountable_type through the SnapTrade portal callback so the account-linking flow no longer drops users back to accounts_path - Show the real absolute OAuth callback URL in self-hosted setup instructions - Refresh brakeman.ignore fingerprint for the connect redirect after the return_to/accountable_type params were added Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y8SCCmKX6RphB5E73WSUQQ * fix(snaptrade): don't retry non-idempotent OAuth/API requests CodeRabbit flagged that Provider::Snaptrade retried OAuth token exchanges/refreshes and all API POST/DELETE calls (get_connection_url, delete_connection) after timeouts/connection failures. If the response is lost after SnapTrade already consumed a single-use auth code, rotated the refresh token, or applied a POST/DELETE, replaying the request either fails with invalid_grant on a token that actually succeeded, or risks duplicate side effects. Retries are now limited to GET requests; OAuth token requests and non-GET API calls translate a network failure straight into an ApiError without replay. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NrrGkgSBEqhjjBmmH1fcXL * fix(snaptrade): stop querying non-deterministically encrypted token via empty-string compare CodeRabbit flagged that the syncable scope's where.not(oauth_access_token: [nil, ""]) re-encrypts "" with a random IV on every query, so the "" comparison can never match a stored ciphertext and is a silent no-op. No code path ever persists oauth_access_token as "" (only nil or a real token via apply_oauth_tokens!), so the exclusion is unnecessary -- narrowed the scope to a plain NULL check, which encryption handles transparently since nil is never encrypted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NrrGkgSBEqhjjBmmH1fcXL --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
202 lines
9.2 KiB
Plaintext
202 lines
9.2 KiB
Plaintext
{
|
|
"ignored_warnings": [
|
|
{
|
|
"warning_type": "Redirect",
|
|
"warning_code": 18,
|
|
"fingerprint": "6d0cf62d5751106cd05dd3b4622beacaba9bdf91aaf798c2003a2a65e6bb903c",
|
|
"check_name": "Redirect",
|
|
"message": "Possible unprotected redirect",
|
|
"file": "app/controllers/snaptrade_items_controller.rb",
|
|
"line": 36,
|
|
"link": "https://brakemanscanner.org/docs/warning_types/redirect/",
|
|
"code": "redirect_to(Current.family.snaptrade_items.find(params[:id]).connection_portal_url(:redirect_url => callback_snaptrade_items_url(:item_id => Current.family.snaptrade_items.find(params[:id]).id, :return_to => params[:return_to], :accountable_type => params[:accountable_type])), :allow_other_host => true)",
|
|
"render_path": null,
|
|
"location": {
|
|
"type": "method",
|
|
"class": "SnaptradeItemsController",
|
|
"method": "connect"
|
|
},
|
|
"user_input": "Current.family.snaptrade_items.find(params[:id]).connection_portal_url(:redirect_url => callback_snaptrade_items_url(:item_id => Current.family.snaptrade_items.find(params[:id]).id, :return_to => params[:return_to], :accountable_type => params[:accountable_type]))",
|
|
"confidence": "Weak",
|
|
"cwe_id": [
|
|
601
|
|
],
|
|
"note": "Intentional redirect to SnapTrade's external OAuth portal for brokerage connection. return_to/accountable_type are only used to route the user back to setup_accounts after the portal callback and are echoed verbatim, not used to build the redirect target itself."
|
|
},
|
|
{
|
|
"warning_type": "Redirect",
|
|
"warning_code": 18,
|
|
"fingerprint": "fc14dec97ce908f7483a8eb1f3aa6ea38f4369b8f9fe656e089c91a91c01a125",
|
|
"check_name": "Redirect",
|
|
"message": "Possible unprotected redirect",
|
|
"file": "app/controllers/snaptrade_items_controller.rb",
|
|
"line": 201,
|
|
"link": "https://brakemanscanner.org/docs/warning_types/redirect/",
|
|
"code": "redirect_to(Provider::Snaptrade.authorize_url(:redirect_uri => oauth_callback_snaptrade_items_url, :state => SecureRandom.hex(32), :code_challenge => Provider::Snaptrade.generate_pkce[:challenge]), :allow_other_host => true)",
|
|
"render_path": null,
|
|
"location": {
|
|
"type": "method",
|
|
"class": "SnaptradeItemsController",
|
|
"method": "oauth_authorize"
|
|
},
|
|
"user_input": "Provider::Snaptrade.authorize_url(:redirect_uri => oauth_callback_snaptrade_items_url, :state => SecureRandom.hex(32), :code_challenge => Provider::Snaptrade.generate_pkce[:challenge])",
|
|
"confidence": "Weak",
|
|
"cwe_id": [
|
|
601
|
|
],
|
|
"note": "Intentional redirect to SnapTrade's external OAuth authorize endpoint. URL is built entirely from Provider::Snaptrade::AUTHORIZE_URL (hardcoded constant), server-side oauth_client_id config, the internal oauth_callback_snaptrade_items_url named route, and locally generated SecureRandom state/PKCE values -- no request params are interpolated into the redirect target."
|
|
},
|
|
{
|
|
"warning_type": "Redirect",
|
|
"warning_code": 18,
|
|
"fingerprint": "723b1970ca6bf16ea0c2c1afa0c00d3c54854a16568d6cb933e497947565d9ab",
|
|
"check_name": "Redirect",
|
|
"message": "Possible unprotected redirect",
|
|
"file": "app/controllers/family_exports_controller.rb",
|
|
"line": 30,
|
|
"link": "https://brakemanscanner.org/docs/warning_types/redirect/",
|
|
"code": "redirect_to(Current.family.family_exports.find(params[:id]).export_file, :allow_other_host => true)",
|
|
"render_path": null,
|
|
"location": {
|
|
"type": "method",
|
|
"class": "FamilyExportsController",
|
|
"method": "download"
|
|
},
|
|
"user_input": "Current.family.family_exports.find(params[:id]).export_file",
|
|
"confidence": "Weak",
|
|
"cwe_id": [
|
|
601
|
|
],
|
|
"note": ""
|
|
},
|
|
{
|
|
"warning_type": "Mass Assignment",
|
|
"warning_code": 105,
|
|
"fingerprint": "81c63f2c375da309440b9308df3ae9d1fcbada7043a26919898b08f3a38b29f6",
|
|
"check_name": "PermitAttributes",
|
|
"message": "Potentially dangerous key allowed for mass assignment",
|
|
"file": "app/controllers/api/v1/trades_controller.rb",
|
|
"line": 159,
|
|
"link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
|
|
"code": "params.require(:trade).permit(:account_id, :date, :qty, :price, :currency, :security_id, :ticker, :manual_ticker, :investment_activity_label, :category_id, :fee, :type, :amount, :transfer_account_id)",
|
|
"render_path": null,
|
|
"location": {
|
|
"type": "method",
|
|
"class": "Api::V1::TradesController",
|
|
"method": "trade_params"
|
|
},
|
|
"user_input": ":account_id",
|
|
"confidence": "High",
|
|
"cwe_id": [
|
|
915
|
|
],
|
|
"note": "account_id validated in create action (line 42: family.accounts.visible.find ensures family membership; lines 44-48: supports_trades? check); security_id validated via Security.find (line 290); transfer_account_id validated in Transfer::Creator where family.accounts.find scopes both source and destination accounts to the family (app/models/transfer/creator.rb lines 4-5); type validated in build_create_form_params against allowed values: buy, sell, dividend, deposit, withdrawal, interest (line 217)"
|
|
},
|
|
{
|
|
"warning_type": "Mass Assignment",
|
|
"warning_code": 105,
|
|
"fingerprint": "aaccd8db0be34afdc88e5af08d91ae2e8b7765dfea2f3fc6e1c37db0adc7b991",
|
|
"check_name": "PermitAttributes",
|
|
"message": "Potentially dangerous key allowed for mass assignment",
|
|
"file": "app/controllers/invitations_controller.rb",
|
|
"line": 58,
|
|
"link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
|
|
"code": "params.require(:invitation).permit(:email, :role)",
|
|
"render_path": null,
|
|
"location": {
|
|
"type": "method",
|
|
"class": "InvitationsController",
|
|
"method": "invitation_params"
|
|
},
|
|
"user_input": ":role",
|
|
"confidence": "Medium",
|
|
"cwe_id": [
|
|
915
|
|
],
|
|
"note": ""
|
|
},
|
|
{
|
|
"warning_type": "Mass Assignment",
|
|
"warning_code": 105,
|
|
"fingerprint": "01a88a0a17848e70999c17f6438a636b00e01da39a2c0aa0c46f20f0685c7202",
|
|
"check_name": "PermitAttributes",
|
|
"message": "Potentially dangerous key allowed for mass assignment",
|
|
"file": "app/controllers/admin/users_controller.rb",
|
|
"line": 35,
|
|
"link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
|
|
"code": "params.require(:user).permit(:role)",
|
|
"render_path": null,
|
|
"location": {
|
|
"type": "method",
|
|
"class": "Admin::UsersController",
|
|
"method": "user_params"
|
|
},
|
|
"user_input": ":role",
|
|
"confidence": "Medium",
|
|
"cwe_id": [
|
|
915
|
|
],
|
|
"note": "Protected by Pundit authorization - UserPolicy requires super_admin and prevents users from changing their own role"
|
|
},
|
|
{
|
|
"warning_type": "Dangerous Eval",
|
|
"warning_code": 13,
|
|
"fingerprint": "ad8e31fe9321feba741d551654a6fb0c0c7cfe8abee81822e6143404817bff5b",
|
|
"check_name": "Evaluation",
|
|
"message": "Dynamic string evaluated as code",
|
|
"file": "app/helpers/styled_form_builder.rb",
|
|
"line": 10,
|
|
"link": "https://brakemanscanner.org/docs/warning_types/dangerous_eval/",
|
|
"code": "class_eval(\" def #{selector}(method, options = {})\\n form_options = options.slice(:label, :label_tooltip, :inline, :container_class, :required)\\n html_options = options.except(:label, :label_tooltip, :inline, :container_class)\\n\\n build_field(method, form_options, html_options) do |merged_options|\\n super(method, merged_options)\\n end\\n end\\n\", \"app/helpers/styled_form_builder.rb\", (10 + 1))",
|
|
"render_path": null,
|
|
"location": {
|
|
"type": "method",
|
|
"class": "StyledFormBuilder",
|
|
"method": null
|
|
},
|
|
"user_input": null,
|
|
"confidence": "Weak",
|
|
"cwe_id": [
|
|
913,
|
|
95
|
|
],
|
|
"note": "Uses similar pattern to Rails internal form builder"
|
|
},
|
|
{
|
|
"warning_type": "Dynamic Render Path",
|
|
"warning_code": 15,
|
|
"fingerprint": "fb6f7abeabc405d6882ffd41dbe8016403ef39307a5c6b4cd7b18adfaf0c24bf",
|
|
"check_name": "Render",
|
|
"message": "Render path contains parameter value",
|
|
"file": "app/views/import/configurations/show.html.erb",
|
|
"line": 34,
|
|
"link": "https://brakemanscanner.org/docs/warning_types/dynamic_render_path/",
|
|
"code": "render(partial => permitted_import_configuration_path(Current.family.imports.find(params[:import_id])), { :locals => ({ :import => Current.family.imports.find(params[:import_id]) }) })",
|
|
"render_path": [
|
|
{
|
|
"type": "controller",
|
|
"class": "Import::ConfigurationsController",
|
|
"method": "show",
|
|
"line": 7,
|
|
"file": "app/controllers/import/configurations_controller.rb",
|
|
"rendered": {
|
|
"name": "import/configurations/show",
|
|
"file": "app/views/import/configurations/show.html.erb"
|
|
}
|
|
}
|
|
],
|
|
"location": {
|
|
"type": "template",
|
|
"template": "import/configurations/show"
|
|
},
|
|
"user_input": "params[:import_id]",
|
|
"confidence": "Weak",
|
|
"cwe_id": [
|
|
22
|
|
],
|
|
"note": ""
|
|
}
|
|
],
|
|
"brakeman_version": "7.1.0"
|
|
}
|