diff --git a/CLAUDE.md b/CLAUDE.md index 946fb95c4..92edf11bb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,30 +4,24 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Project Overview -codebar planner is a Rails 7.2 application for managing [codebar.io](https://codebar.io) members and events. It handles workshop/event scheduling, member registration, invitations, RSVPs, and feedback collection for coding workshops organized by codebar chapters. +codebar planner is a Rails 8.1 application for managing [codebar.io](https://codebar.io) members and events. It handles workshop/event scheduling, member registration, invitations, RSVPs, and feedback collection for coding workshops organized by codebar chapters. ## Development Setup -### Docker (Recommended) - -- **Initial setup**: `bin/dup` - builds container, sets up database with seed data -- **Start container**: `bin/dstart` - starts existing container -- **Start Rails server**: `bin/dserver` - runs server on http://localhost:3000 -- **Run tests**: `bin/drspec [path]` - runs RSpec tests, optionally for specific file/line -- **Rails console**: `bin/drails console` -- **Run rake tasks**: `bin/drake [task]` -- **Bash shell in container**: `bin/dexec` -- **Grant admin access**: `bin/dadmin ` - gives admin role to user -- **Stop container**: `bin/dstop` -- **Destroy container**: `bin/ddown` +**IMPORTANT**: Always use native installation with `bundle exec` commands. Never use Docker or `bin/d*` commands. ### Native Installation -If not using Docker: -- Setup: `bundle && rake db:create db:migrate db:seed` -- Server: `rails server` -- Tests: `rake` or `rspec` -- Linting: `rubocop` +- **Setup**: `bundle && rake db:create db:migrate db:seed` +- **Server**: `bundle exec rails server` +- **Tests**: `bundle exec rspec [path]` - runs RSpec tests, optionally for specific file/line +- **Rails console**: `bundle exec rails console` +- **Run rake tasks**: `bundle exec rake [task]` +- **Linting**: `bundle exec rubocop` + +### Docker Setup (Not Used) + +Docker setup exists in this repository (`bin/d*` commands) but is **not used** for development work with Claude Code. ### Environment Variables @@ -149,6 +143,95 @@ Key RuboCop exclusions: - `db/`, `spec/`, `config/`, `bin/` excluded from most cops - Documentation not required (`Style/Documentation: false`) +## Parameter Handling + +This application uses Rails 8.0 `params.expect` for parameter filtering and validation. + +### Basic Pattern + +Use `params.expect` instead of `params.require().permit()`: + +```ruby +def resource_params + params.expect(resource: [ + :field1, :field2, :field3 + ]) +end +``` + +### Type Safety + +Controllers using `params.expect` raise `ActionController::ParameterMissing` when: +- Parameter type is tampered (e.g., string sent instead of hash) +- Required parameters are missing +- Nested parameter structure is invalid + +This makes parameter validation failures explicit and easier to handle at the application level. + +### Nested Arrays + +Use hash syntax for array parameters: + +```ruby +params.expect(workshop: [ + :name, :date, + { sponsor_ids: [] } +]) +``` + +### Nested Attributes + +**IMPORTANT:** `params.expect` does NOT work with `accepts_nested_attributes_for`. + +Rails forms with nested attributes send hash-with-numeric-keys like `{'0' => {...}, '1' => {...}}`, which params.expect cannot handle. For controllers using `accepts_nested_attributes_for`, continue using the old syntax: + +```ruby +# Use require().permit() for nested attributes +params.require(:sponsor).permit( + :name, :website, + address_attributes: [:id, :street, :city, :postal_code], + contacts_attributes: [:id, :name, :email, :_destroy] +) +``` + +Note: `_destroy` is permitted like any other field for deletion. + +### Conditional Parameters + +When parameters are optional: + +```ruby +def index + search_params = if params.key?(:member_search) + params.expect(member_search: [:name, :callback_url]) + else + {} + end + # use search_params +end +``` + +### Return Value + +`params.expect(key: [...])` returns the **inner permitted parameters**, not wrapped: + +```ruby +result = params.expect(member: [:name, :email]) +# Access as: result[:name], NOT result[:member][:name] +``` + +### Testing + +Controller specs should verify parameter filtering works: + +```ruby +it 'filters unpermitted parameters' do + post :create, params: { resource: valid_attrs, hacker_field: 'malicious' } + expect(response).to be_successful # hacker_field is filtered out +end +``` + + ## Important Patterns ### Controllers diff --git a/PARAMS_EXPECT_MIGRATION_ANALYSIS.md b/PARAMS_EXPECT_MIGRATION_ANALYSIS.md new file mode 100644 index 000000000..5ea921dcf --- /dev/null +++ b/PARAMS_EXPECT_MIGRATION_ANALYSIS.md @@ -0,0 +1,274 @@ +# Rails params.expect Migration Analysis + +**Date:** 2026-02-09 +**Project:** codebar planner +**Rails Version:** 7.2 → 8.0 upgrade path + +## Executive Summary + +Converting codebar planner to use Rails 8.0's `params.expect` requires migrating 20 controller files with 17 private parameter methods and eliminating 83 direct hash accesses. The migration is moderate effort (4-6 days) with significant security benefits, particularly for the payments controller. + +## What is params.expect? + +Rails 8.0 introduces `params.expect` to replace the `require().permit()` pattern. The new method handles type tampering securely by returning 400 errors instead of throwing exceptions when parameters are malformed. + +### Syntax Comparison + +```ruby +# Current (Rails 7.2) +params.require(:user).permit(:name, :handle, address: [:street, :city]) + +# New (Rails 8.0) +params.expect(user: [:name, :handle, { address: [:street, :city] }]) +``` + +Key changes: +- Single method call replaces chained methods +- Nested parameters use hash syntax `{ key: [...] }` instead of `key: [...]` +- Type tampering returns 400 instead of raising `NoMethodError` + +## Current State Analysis + +### Strong Parameters Adoption + +The codebase shows moderate adoption of strong parameters: + +- **20 files** use `params.require().permit()` +- **17 private methods** follow consistent naming (`workshop_params`, `event_params`, etc.) +- **83 direct hash accesses** bypass parameter filtering (`params[:key]` pattern) +- **Good patterns:** Most admin controllers use private methods for parameter extraction +- **Security gaps:** Payment processing and member details use unpermitted access + +### Most Complex Structures + +**Sponsors (app/controllers/admin/sponsors_controller.rb:66-71)** +```ruby +params.require(:sponsor).permit( + :name, :avatar, :website, :seats, :accessibility_info, + :number_of_coaches, :level, :description, + address_attributes: [:id, :flat, :street, :postal_code, :city, :latitude, :longitude, :directions], + contacts_attributes: [:id, :name, :surname, :email, :mailing_list_consent, :_destroy] +) +``` + +**Events (app/controllers/admin/events_controller.rb:75-82)** +```ruby +params.require(:event).permit( + :virtual, :name, :slug, :date_and_time, :local_date, :local_time, :local_end_time, + :description, :info, :schedule, :venue_id, :external_url, :coach_spaces, :student_spaces, + :email, :announce_only, :tito_url, :invitable, :time_zone, :student_questionnaire, + :confirmation_required, :surveys_required, :audience, :coach_questionnaire, :show_faq, + :display_coaches, :display_students, + bronze_sponsor_ids: [], silver_sponsor_ids: [], gold_sponsor_ids: [], sponsor_ids: [], + chapter_ids: [] +) +``` + +## Migration Priorities + +### High Priority (Security Critical) + +**1. payments_controller.rb** +- **Risk:** Direct nested access to Stripe parameters +- **Current:** `params[:data][:email]`, `params[:data][:id]`, `params[:name]` +- **Impact:** Financial transaction security +- **Effort:** 4-6 hours with thorough testing + +**2. member/details_controller.rb** +- **Risk:** Mixed permitted/unpermitted access with custom validation +- **Current:** Manual reassignment from raw params after permit +- **Complexity:** Conditional logic requires dynamic array building +- **Effort:** 6-8 hours + +**3. admin/member_search_controller.rb** +- **Risk:** Multiple raw nested accesses +- **Current:** `params[:member_search]` and `params[:member_pick][:members]` +- **Effort:** 3-4 hours + +### Medium Priority (Standard Controllers) + +15-17 admin controllers with good strong parameters: +- workshops_controller.rb (5 fields + nested array) +- events_controller.rb (22+ fields with 4 nested arrays) +- sponsors_controller.rb (nested attributes with accepts_nested_attributes_for) +- meetings_controller.rb (9 fields + chapters array) +- chapters_controller.rb +- announcements_controller.rb +- groups_controller.rb + +**Effort:** 1-2 hours each, 2-3 days total + +### Low Priority (Simple Conversions) + +Controllers with minimal or straightforward parameter handling: +- Direct one-to-one conversion possible +- No complex conditional logic +- Already using strong parameters consistently + +**Effort:** 1-2 hours total + +## Technical Challenges + +### Conditional Parameter Handling + +Some controllers use conditional logic: + +```ruby +# Current pattern +params[:workshop_invitation].present? ? + params.require(:workshop_invitation).permit(...) : + {} +``` + +**Solution:** Build allowed keys dynamically: +```ruby +permitted_keys = [:name, :role] +permitted_keys << :admin if current_user.admin? +params.expect(user: permitted_keys) +``` + +### Nested Attributes + +Controllers using `accepts_nested_attributes_for` need careful conversion: + +```ruby +# Current +address_attributes: [:id, :flat, :street], +contacts_attributes: [:id, :name, :_destroy] + +# New +{ address_attributes: [:id, :flat, :street] }, +{ contacts_attributes: [:id, :name, :_destroy] } +``` + +### Data Transformation + +Some controllers manipulate parameters after permit: + +```ruby +params.require(:member).permit(...).tap do |params| + params[:dietary_restrictions] = params[:dietary_restrictions].reject(&:blank?) +end +``` + +This pattern remains valid with `params.expect`. + +## Testing Requirements + +Each migration must verify: + +1. **Type tampering:** Send string where hash expected, confirm 400 error +2. **Missing parameters:** Omit required keys, confirm 400 error +3. **Valid requests:** Confirm existing functionality unchanged +4. **Nested structures:** Test deep nesting and arrays +5. **Edge cases:** Empty arrays, nil values, boundary conditions + +Use RSpec request specs with explicit parameter tampering tests. + +## Migration Timeline + +### Phase 1: Security Critical (1-2 days) +- Fix payments_controller.rb +- Fix member/details_controller.rb +- Fix admin/member_search_controller.rb +- Add type tampering tests +- Manual QA for payment flows + +### Phase 2: Standard Controllers (2-3 days) +- Convert 15-17 admin controllers +- Update all private params methods +- Add request specs for parameter validation +- Handle nested arrays and attributes + +### Phase 3: Cleanup (1 day) +- Eliminate 83 direct hash accesses +- Standardize conditional parameter patterns +- Add helper methods for complex cases +- Update concerns with parameter handling +- Documentation and final review + +**Total Estimated Effort:** 4-6 days + +## Implementation Strategy + +### Approach + +1. **One controller at a time:** Reduce merge conflicts +2. **Tests first:** Add type tampering specs before conversion +3. **Small PRs:** One controller category per pull request +4. **Gradual rollout:** Deploy and monitor between phases + +### Pull Request Structure + +- **PR 1:** Payments controller (security critical) +- **PR 2:** Member details controller +- **PR 3:** Member search and admin concerns +- **PR 4:** Workshop and event controllers +- **PR 5:** Sponsor and meeting controllers +- **PR 6:** Remaining admin controllers +- **PR 7:** Direct hash access cleanup + +### Rollback Plan + +If issues arise: +- Each PR is independently revertible +- No breaking changes to external APIs +- Existing behavior preserved (400 errors are improvement, not breaking change) +- Monitor error tracking (Rollbar) for unexpected parameter issues + +## Benefits + +### Security Improvements + +- **Type tampering protection:** Malformed parameters return 400, not 500 +- **Consistent validation:** Single method reduces bypass opportunities +- **Better error messages:** Rails provides clear parameter validation errors + +### Code Quality + +- **Cleaner syntax:** Single method call more readable +- **Explicit structure:** Nested parameters clearly visible in code +- **Standardization:** Eliminates 83 direct hash accesses + +### Maintenance + +- **Easier audits:** Parameter requirements in one place +- **Reduced confusion:** No mixing of permitted and unpermitted access +- **Better documentation:** Parameter structure self-documenting + +## Risks + +### Compatibility + +- Requires Rails 8.0 (check upgrade timeline) +- May expose existing parameter handling bugs +- Tests expecting 500 errors need updates for 400 errors + +### Complexity + +- Conditional parameters need refactoring +- Nested attributes require careful syntax translation +- Data transformation patterns may need adjustment + +### Testing + +- Must test all parameter edge cases +- Payment flows require manual QA +- Integration tests may need updates + +## Recommendations + +1. **Start immediately after Rails 8.0 upgrade** (not before) +2. **Begin with payments_controller.rb** for maximum security benefit +3. **Create helper method** for conditional parameter building +4. **Add request specs** for type tampering across all controllers +5. **Deploy incrementally** with monitoring between phases +6. **Document patterns** for conditional and nested parameters + +## Conclusion + +Converting codebar planner to `params.expect` is moderate effort with significant security benefits. The 20 controller files using strong parameters follow consistent patterns that translate cleanly to the new syntax. The main challenges are the 4 high-priority controllers with unpermitted access and the 83 direct hash accesses requiring refactoring. + +The payments controller alone justifies the migration effort due to security improvements in financial transaction handling. The remaining controllers benefit from cleaner syntax and better type safety. + +Estimated timeline of 4-6 days is achievable with incremental pull requests and thorough testing. The migration should begin after the Rails 8.0 upgrade and deploy in phases with monitoring between each phase. diff --git a/app/assets/javascripts/payments.js b/app/assets/javascripts/payments.js index bd4f1d766..7bfbd27d3 100644 --- a/app/assets/javascripts/payments.js +++ b/app/assets/javascripts/payments.js @@ -9,7 +9,14 @@ $(function() { $.ajax({ type: "POST", url: '/payments', - data: { amount: amount*100, name: name, data: token } + data: { + payment: { + amount: amount*100, + name: name, + stripe_email: token.email, + stripe_token_id: token.id + } + } }).done(function(response) { $('.payment-container').html(response); }).fail(function(xhr, status, e){ diff --git a/app/controllers/admin/announcements_controller.rb b/app/controllers/admin/announcements_controller.rb index 98edf0340..c65269308 100644 --- a/app/controllers/admin/announcements_controller.rb +++ b/app/controllers/admin/announcements_controller.rb @@ -36,6 +36,6 @@ def set_announcement end def announcement_params - params.require(:announcement).permit(:all_groups, :message, :expires_at, group_ids: []) + params.expect(announcement: [:all_groups, :message, :expires_at, { group_ids: [] }]) end end diff --git a/app/controllers/admin/bans_controller.rb b/app/controllers/admin/bans_controller.rb index 0b59d5834..80e1f3842 100644 --- a/app/controllers/admin/bans_controller.rb +++ b/app/controllers/admin/bans_controller.rb @@ -20,7 +20,7 @@ def create private def ban_params - params.require(:ban).permit(:note, :reason, :permanent, :expires_at, :explanation) + params.expect(ban: [:note, :reason, :permanent, :expires_at, :explanation]) end def set_member diff --git a/app/controllers/admin/chapters_controller.rb b/app/controllers/admin/chapters_controller.rb index a6b3d88ef..7f60019b7 100644 --- a/app/controllers/admin/chapters_controller.rb +++ b/app/controllers/admin/chapters_controller.rb @@ -58,7 +58,7 @@ def members private def chapter_params - params.require(:chapter).permit(:name, :email, :city, :time_zone, :description, :image) + params.expect(chapter: [:name, :email, :city, :time_zone, :description, :image]) end def set_chapter diff --git a/app/controllers/admin/events_controller.rb b/app/controllers/admin/events_controller.rb index 4584b1699..85d13cce5 100644 --- a/app/controllers/admin/events_controller.rb +++ b/app/controllers/admin/events_controller.rb @@ -72,14 +72,17 @@ def set_event end def event_params - params.require(:event).permit( + params.expect(event: [ :virtual, :name, :slug, :date_and_time, :local_date, :local_time, :local_end_time, :description, :info, :schedule, :venue_id, :external_url, :coach_spaces, :student_spaces, :email, :announce_only, :tito_url, :invitable, :time_zone, :student_questionnaire, :confirmation_required, :surveys_required, :audience, :coach_questionnaire, :show_faq, :display_coaches, :display_students, - bronze_sponsor_ids: [], silver_sponsor_ids: [], gold_sponsor_ids: [], sponsor_ids: [], - chapter_ids: [] - ) + { bronze_sponsor_ids: [] }, + { silver_sponsor_ids: [] }, + { gold_sponsor_ids: [] }, + { sponsor_ids: [] }, + { chapter_ids: [] } + ]) end def organiser_ids diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index aa4a251ec..de2539552 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -27,6 +27,6 @@ def show private def group_params - params.require(:group).permit(:name, :description, :chapter_id) + params.expect(group: [:name, :description, :chapter_id]) end end diff --git a/app/controllers/admin/meetings_controller.rb b/app/controllers/admin/meetings_controller.rb index c077bf2d3..9365ff4b5 100644 --- a/app/controllers/admin/meetings_controller.rb +++ b/app/controllers/admin/meetings_controller.rb @@ -62,10 +62,10 @@ def slug end def meeting_params - params.require(:meeting).permit( + params.expect(meeting: [ :name, :description, :slug, :date_and_time, :local_date, :local_time, :local_end_time, :invitable, :spaces, :venue_id, :sponsor_id, :chapters - ) + ]) end def organiser_ids diff --git a/app/controllers/admin/member_notes_controller.rb b/app/controllers/admin/member_notes_controller.rb index 2f2e4f0b7..fbb599d2b 100644 --- a/app/controllers/admin/member_notes_controller.rb +++ b/app/controllers/admin/member_notes_controller.rb @@ -9,6 +9,6 @@ def create end def member_note_params - params.require(:member_note).permit(:note, :member_id) + params.expect(member_note: [:note, :member_id]) end end diff --git a/app/controllers/admin/member_search_controller.rb b/app/controllers/admin/member_search_controller.rb index 7cc300ee2..3695ae621 100644 --- a/app/controllers/admin/member_search_controller.rb +++ b/app/controllers/admin/member_search_controller.rb @@ -1,9 +1,15 @@ class Admin::MemberSearchController < Admin::ApplicationController def index - member_params = params[:member_search] || {} - name = member_params[:name] + search_params = if params.key?(:member_search) + params.expect(member_search: [:name, :callback_url]) + else + {} + end + + callback_url = search_params[:callback_url] || params[:callback_url] || results_admin_member_search_index_path + name = search_params[:name] members = name.blank? ? Member.none : Member.find_members_by_name(name).select(:id, :name, :surname, :pronouns) - callback_url = member_params[:callback_url] || params[:callback_url] || results_admin_member_search_index_path + if members.size == 1 query = { member_pick: { members: [members.first.id] } } query_string = query.to_query @@ -15,7 +21,8 @@ def index end def results - members = Member.find(params[:member_pick][:members]) + pick_params = params.expect(member_pick: { members: [] }) + members = Member.find(pick_params[:members]) render 'show', locals: { members: members } end end diff --git a/app/controllers/admin/sponsors_controller.rb b/app/controllers/admin/sponsors_controller.rb index 8e23e2d90..ec6c8e94e 100644 --- a/app/controllers/admin/sponsors_controller.rb +++ b/app/controllers/admin/sponsors_controller.rb @@ -63,12 +63,15 @@ def update private def sponsor_params - params.require(:sponsor).permit(:name, :avatar, :website, :seats, :accessibility_info, - :number_of_coaches, :level, :description, - address_attributes: %i[id flat street postal_code city - latitude longitude directions], - contacts_attributes: %i[id name surname email mailing_list_consent - _destroy]) + # Uses require().permit() instead of params.expect because Sponsor model + # uses accepts_nested_attributes_for, which sends hash-with-numeric-keys + # that params.expect cannot handle (e.g., {'0' => {...}, '1' => {...}}) + params.require(:sponsor).permit( + :name, :avatar, :website, :seats, :accessibility_info, + :number_of_coaches, :level, :description, + address_attributes: [:id, :flat, :street, :postal_code, :city, :latitude, :longitude, :directions], + contacts_attributes: [:id, :name, :surname, :email, :mailing_list_consent, :_destroy] + ) end def set_sponsor diff --git a/app/controllers/admin/workshops_controller.rb b/app/controllers/admin/workshops_controller.rb index 62694a176..a423075ec 100644 --- a/app/controllers/admin/workshops_controller.rb +++ b/app/controllers/admin/workshops_controller.rb @@ -117,10 +117,13 @@ def changes private def workshop_params - params.require(:workshop).permit(:local_date, :local_time, :local_end_time, :chapter_id, - :invitable, :seats, :virtual, :slack_channel, :slack_channel_link, - :rsvp_open_local_date, :rsvp_open_local_time, :description, - :coach_spaces, :student_spaces, sponsor_ids: []) + params.expect(workshop: [ + :local_date, :local_time, :local_end_time, :chapter_id, + :invitable, :seats, :virtual, :slack_channel, :slack_channel_link, + :rsvp_open_local_date, :rsvp_open_local_time, :description, + :coach_spaces, :student_spaces, + { sponsor_ids: [] } + ]) end def chapter_id @@ -157,7 +160,7 @@ def set_organisers(organiser_ids) end def host_id - params.require(:workshop).permit(:host)[:host] + params.expect(workshop: [:host])[:host] end def organiser_ids diff --git a/app/controllers/concerns/member_concerns.rb b/app/controllers/concerns/member_concerns.rb index 1f4abffb6..33b05dfd2 100644 --- a/app/controllers/concerns/member_concerns.rb +++ b/app/controllers/concerns/member_concerns.rb @@ -9,16 +9,18 @@ module InstanceMethods private def member_params - params.require(:member).permit( + params.expect(member: [ :pronouns, :name, :surname, :email, :mobile, :about_you, :skill_list, :newsletter, :other_dietary_restrictions, :how_you_found_us, - :how_you_found_us_other_reason, dietary_restrictions: [] - ).tap do |params| + :how_you_found_us_other_reason, { dietary_restrictions: [] } + ]).tap do |permitted_params| # We want to keep Rails' hidden blank field in the form so that all dietary restrictions for a member can be # removed by submitting the form with all check boxes unticked. However, we want to remove the blank value # before setting the dietary restrictions attribute on the model. # See Gotcha section here: # https://api.rubyonrails.org/v7.1/classes/ActionView/Helpers/FormOptionsHelper.html#method-i-collection_check_boxes - params[:dietary_restrictions] = params[:dietary_restrictions].reject(&:blank?) if params[:dietary_restrictions] + if permitted_params[:dietary_restrictions] + permitted_params[:dietary_restrictions] = permitted_params[:dietary_restrictions].reject(&:blank?) + end end end @@ -30,11 +32,11 @@ def set_member @member = current_user end - def how_you_found_us_selections_valid? - how_found_present = member_params[:how_you_found_us].present? - other_reason_present = member_params[:how_you_found_us_other_reason].present? - return false if member_params[:how_you_found_us] == 'other' && !other_reason_present - return true if member_params[:how_you_found_us] == 'other' && other_reason_present + def how_you_found_us_selections_valid?(attrs) + how_found_present = attrs[:how_you_found_us].present? + other_reason_present = attrs[:how_you_found_us_other_reason].present? + return false if attrs[:how_you_found_us] == 'other' && !other_reason_present + return true if attrs[:how_you_found_us] == 'other' && other_reason_present how_found_present != other_reason_present end diff --git a/app/controllers/member/details_controller.rb b/app/controllers/member/details_controller.rb index 848a01e10..9652a7f5d 100644 --- a/app/controllers/member/details_controller.rb +++ b/app/controllers/member/details_controller.rb @@ -14,22 +14,17 @@ def edit def update attrs = member_params - attrs[:how_you_found_us_other_reason] = nil if attrs[:how_you_found_us] != 'other' - unless how_you_found_us_selections_valid? + unless how_you_found_us_selections_valid?(attrs) @member.errors.add(:how_you_found_us, 'You must select one option') return render :edit end - attrs[:how_you_found_us] = params[:member][:how_you_found_us] if params[:member][:how_you_found_us].present? - if params[:member][:how_you_found_us_other_reason].present? && attrs[:how_you_found_us] == 'other' - attrs[:how_you_found_us_other_reason] = - params[:member][:how_you_found_us_other_reason] - end + attrs[:how_you_found_us_other_reason] = nil if attrs[:how_you_found_us] != 'other' return render :edit unless @member.update(attrs) - member_params[:newsletter] ? subscribe_to_newsletter(@member) : unsubscribe_from_newsletter(@member) + attrs[:newsletter] ? subscribe_to_newsletter(@member) : unsubscribe_from_newsletter(@member) redirect_to step2_member_path end end diff --git a/app/controllers/payments_controller.rb b/app/controllers/payments_controller.rb index 8423a4150..36038b3e5 100644 --- a/app/controllers/payments_controller.rb +++ b/app/controllers/payments_controller.rb @@ -4,12 +4,14 @@ class PaymentsController < ApplicationController def new; end def create - @amount = params[:amount] + payment_params = params.expect(payment: [:amount, :name, :stripe_email, :stripe_token_id]) + + @amount = payment_params[:amount] customer = Stripe::Customer.create( - email: params[:data][:email], - description: params[:name], - source: params[:data][:id] + email: payment_params[:stripe_email], + description: payment_params[:name], + source: payment_params[:stripe_token_id] ) charge_customer(customer, @amount) diff --git a/app/views/payments/create.html.haml b/app/views/payments/create.html.haml new file mode 100644 index 000000000..cff40047b --- /dev/null +++ b/app/views/payments/create.html.haml @@ -0,0 +1 @@ +Payment processed diff --git a/docs/plans/2026-02-11-params-expect-migration.md b/docs/plans/2026-02-11-params-expect-migration.md new file mode 100644 index 000000000..9eb98f6e4 --- /dev/null +++ b/docs/plans/2026-02-11-params-expect-migration.md @@ -0,0 +1,756 @@ +# Rails params.expect Migration Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Migrate codebar planner from `params.require().permit()` to Rails 8.0 `params.expect()` - a mostly mechanical syntax change with a few complex cases requiring careful refactoring. + +**Architecture:** This is a straightforward find-replace operation for most controllers. Three controllers need careful attention due to conditional logic and mixed permitted/unpermitted access. We convert, verify with existing tests, add one type tampering test per controller to document new 400 behavior. + +**Tech Stack:** Rails 8.0+, RSpec, Fabrication + +**Estimated effort:** 1-2 days (not 4-6) + +--- + +## Prerequisites + +### Task 0: Verify Rails 8.0 and params.expect availability + +**Step 1: Check Rails version** + +Run: `bundle exec rails -v` +Expected: Rails 8.0.0 or higher + +If not Rails 8.0+, STOP. This migration requires Rails 8.0. + +**Step 2: Verify params.expect exists** + +Run: `bundle exec rails console` +Then in console: +```ruby +ActionController::Parameters.instance_methods.include?(:expect) +``` +Expected: `true` + +**Step 3: Check if there's a params.expect config** + +Run: `grep -r "params.expect" config/` +Expected: May find config or nothing (both OK) + +**Step 4: Document Rails version** + +```bash +echo "Rails version verified: $(bundle exec rails -v)" >> docs/plans/migration-notes.txt +git add docs/plans/migration-notes.txt +git commit -m "docs: verify Rails 8.0 for params.expect migration" +``` + +--- + +## Phase 1: Simple Controller - Prove the Pattern + +### Task 1: Payments Controller - Convert and Verify + +**Files:** +- Modify: `app/controllers/payments_controller.rb:6-17` +- Test: `spec/controllers/payments_controller_spec.rb` + +**Why start here:** Simplest controller (3 parameters), security-critical (financial), fast win. + +**Step 1: Read current implementation** + +Run: `cat app/controllers/payments_controller.rb` +Expected: See current params access + +**Step 2: Convert create action to params.expect** + +In `app/controllers/payments_controller.rb`, replace the `create` method: + +```ruby +def create + payment_params = params.expect(amount: :string, name: :string, data: { email: :string, id: :string }) + + @amount = payment_params[:amount] + + customer = Stripe::Customer.create( + email: payment_params[:data][:email], + description: payment_params[:name], + source: payment_params[:data][:id] + ) + + charge_customer(customer, @amount) + + render layout: false +end +``` + +**Step 3: Run existing test suite** + +Run: `bundle exec rspec spec/controllers/payments_controller_spec.rb` +Expected: All tests PASS (or file doesn't exist, which is OK) + +Run: `bundle exec rspec spec/features/ -e payment` +Expected: All payment feature tests PASS (if any exist) + +**Step 4: Add ONE type tampering test** + +If `spec/controllers/payments_controller_spec.rb` doesn't exist, create it: + +```ruby +require 'rails_helper' + +RSpec.describe PaymentsController, type: :controller do + let(:member) { Fabricate(:member) } + + before do + login(member) + allow(Stripe::Customer).to receive(:create).and_return(double(id: 'cus_123')) + allow(Stripe::Charge).to receive(:create).and_return(true) + end + + describe 'POST #create' do + context 'with type tampering (documents new params.expect behavior)' do + it 'returns 400 Bad Request when data is string instead of hash' do + post :create, params: { amount: '1000', name: 'John', data: 'tampered' } + expect(response).to have_http_status(:bad_request) + end + + it 'does not call Stripe when parameters are invalid' do + expect(Stripe::Customer).not_to receive(:create) + post :create, params: { amount: '1000', name: 'John', data: 'tampered' } + end + end + end +end +``` + +**Step 5: Run new test** + +Run: `bundle exec rspec spec/controllers/payments_controller_spec.rb` +Expected: Type tampering test PASSES (confirms 400 behavior) + +**Step 6: Commit** + +```bash +git add app/controllers/payments_controller.rb spec/controllers/payments_controller_spec.rb +git commit -m "feat(payments): migrate to params.expect for type safety + +- Convert payments#create to use params.expect syntax +- Add test documenting 400 response on type tampering +- Maintains Stripe API integration unchanged" +``` + +**Step 7: Deploy to staging and monitor** + +If you have staging environment: +- Deploy this single change +- Monitor error tracking for 1-2 hours +- Check for unexpected 400 errors + +If no staging or errors found, continue to next phase. + +--- + +## Phase 2: Complex Controllers - The Hard Ones + +### Task 2: Member Details Controller - Refactor Mixed Access + +**Files:** +- Modify: `app/controllers/member/details_controller.rb:15-34` +- Modify: `app/controllers/concerns/member_concerns.rb:11-40` +- Test: `spec/controllers/member/details_controller_spec.rb` + +**Why this is hard:** Currently mixes `member_params` (permitted) with direct `params[:member]` access. The validation logic depends on this mixed approach. + +**Step 1: Understand current behavior** + +Read both files to understand the flow: +- `member_params` permits specific fields +- Then controller accesses `params[:member][:how_you_found_us]` directly +- Validation method also uses `member_params` + +The bug: we're calling `member_params` multiple times and also accessing raw params. + +**Step 2: Convert member_params to params.expect** + +In `app/controllers/concerns/member_concerns.rb`, replace `member_params`: + +```ruby +def member_params + params.expect(member: [ + :pronouns, :name, :surname, :email, :mobile, :about_you, + :skill_list, :newsletter, :other_dietary_restrictions, + :how_you_found_us, :how_you_found_us_other_reason, + { dietary_restrictions: [] } + ]).tap do |permitted_params| + # Remove blank dietary restrictions from checkbox form + # See: https://api.rubyonrails.org/v7.1/classes/ActionView/Helpers/FormOptionsHelper.html#method-i-collection_check_boxes + if permitted_params[:dietary_restrictions] + permitted_params[:dietary_restrictions] = permitted_params[:dietary_restrictions].reject(&:blank?) + end + end +end +``` + +**Step 3: Refactor details controller update method** + +The current logic accesses params directly after calling member_params. We need to call member_params ONCE and work with that hash. + +In `app/controllers/member/details_controller.rb`, replace the `update` method: + +```ruby +def update + attrs = member_params + attrs[:how_you_found_us_other_reason] = nil if attrs[:how_you_found_us] != 'other' + + unless how_you_found_us_selections_valid?(attrs) + @member.errors.add(:how_you_found_us, 'You must select one option') + return render :edit + end + + return render :edit unless @member.update(attrs) + + attrs[:newsletter] ? subscribe_to_newsletter(@member) : unsubscribe_from_newsletter(@member) + redirect_to step2_member_path +end +``` + +**Step 4: Update validation method signature** + +In `app/controllers/concerns/member_concerns.rb`, change `how_you_found_us_selections_valid?` to accept params: + +```ruby +def how_you_found_us_selections_valid?(attrs) + how_found_present = attrs[:how_you_found_us].present? + other_reason_present = attrs[:how_you_found_us_other_reason].present? + return false if attrs[:how_you_found_us] == 'other' && !other_reason_present + return true if attrs[:how_you_found_us] == 'other' && other_reason_present + + how_found_present != other_reason_present +end +``` + +**Step 5: Run existing tests** + +Run: `bundle exec rspec spec/controllers/member/details_controller_spec.rb` +Expected: All tests PASS + +Run: `bundle exec rspec spec/features/member_updating_details_spec.rb` +Expected: All tests PASS + +**Step 6: Add type tampering test** + +Add to `spec/controllers/member/details_controller_spec.rb` (create if needed): + +```ruby +require 'rails_helper' + +RSpec.describe Member::DetailsController, type: :controller do + let(:member) { Fabricate(:member) } + + before do + login(member) + end + + describe 'PATCH #update' do + context 'with type tampering' do + it 'returns 400 Bad Request when member param is string' do + patch :update, params: { member: 'tampered' } + expect(response).to have_http_status(:bad_request) + end + end + end +end +``` + +**Step 7: Run new test** + +Run: `bundle exec rspec spec/controllers/member/details_controller_spec.rb -e "type tampering"` +Expected: Test PASSES + +**Step 8: Commit** + +```bash +git add app/controllers/member/details_controller.rb app/controllers/concerns/member_concerns.rb spec/controllers/member/details_controller_spec.rb +git commit -m "refactor(member): migrate details to params.expect, fix mixed access + +- Convert member_params to params.expect +- Remove direct params[:member] access (was security issue) +- Refactor how_you_found_us validation to accept params hash +- Call member_params only once (performance improvement) +- Add type tampering test" +``` + +--- + +### Task 3: Admin Member Search Controller - Conditional Params + +**Files:** +- Modify: `app/controllers/admin/member_search_controller.rb:2-20` +- Test: `spec/controllers/admin/member_search_controller_spec.rb` + +**Why this is hard:** Uses `params[:member_search] || {}` pattern for optional params. + +**Step 1: Convert index action** + +In `app/controllers/admin/member_search_controller.rb`, replace the `index` method: + +```ruby +def index + search_params = if params.key?(:member_search) + params.expect(member_search: [:name, :callback_url]) + else + {} + end + + callback_url = search_params[:callback_url] || params[:callback_url] || results_admin_member_search_index_path + name = search_params[:name] + members = name.blank? ? Member.none : Member.find_members_by_name(name).select(:id, :name, :surname, :pronouns) + + if members.size == 1 + query = { member_pick: { members: [members.first.id] } } + query_string = query.to_query + callback_url = "#{callback_url}?#{query_string}" + redirect_to callback_url and return + end + + render 'index', locals: { members: members, callback_url: callback_url } +end +``` + +**Step 2: Convert results action** + +Replace the `results` method: + +```ruby +def results + pick_params = params.expect(member_pick: { members: [] }) + members = Member.find(pick_params[:member_pick][:members]) + render 'show', locals: { members: members } +end +``` + +**Step 3: Run existing tests** + +Run: `bundle exec rspec spec/controllers/admin/member_search_controller_spec.rb` +Expected: All tests PASS (or file doesn't exist) + +**Step 4: Add type tampering test** + +Create or append to `spec/controllers/admin/member_search_controller_spec.rb`: + +```ruby +require 'rails_helper' + +RSpec.describe Admin::MemberSearchController, type: :controller do + let(:admin) { Fabricate(:member) } + + before do + login_as_admin(admin) + end + + describe 'GET #results' do + context 'with type tampering' do + it 'returns 400 when member_pick is string' do + get :results, params: { member_pick: 'tampered' } + expect(response).to have_http_status(:bad_request) + end + + it 'returns 400 when members is not array' do + get :results, params: { member_pick: { members: 'not_array' } } + expect(response).to have_http_status(:bad_request) + end + end + end +end +``` + +**Step 5: Run new test** + +Run: `bundle exec rspec spec/controllers/admin/member_search_controller_spec.rb` +Expected: Tests PASS + +**Step 6: Commit** + +```bash +git add app/controllers/admin/member_search_controller.rb spec/controllers/admin/member_search_controller_spec.rb +git commit -m "feat(admin): migrate member search to params.expect + +- Handle optional member_search with conditional check +- Convert results action with nested array syntax +- Add type tampering tests" +``` + +--- + +## Phase 3: Batch Convert Simple Controllers + +### Task 4: Identify and Convert Remaining Controllers + +**Files:** +- All remaining `app/controllers/admin/*_controller.rb` files with `params.require` + +**Strategy:** These are straightforward conversions. Do them in one batch since they follow identical patterns. + +**Step 1: Find all controllers still using params.require** + +Run: `grep -l "params.require" app/controllers/**/*.rb` +Expected: List of controller files + +**Step 2: For each controller, apply the conversion pattern** + +Pattern: +```ruby +# Before +params.require(:resource).permit(:field1, :field2, array_field: []) + +# After +params.expect(resource: [:field1, :field2, { array_field: [] }]) +``` + +Controllers to convert (do each individually): + +**workshops_controller.rb:** +```ruby +def workshop_params + params.expect(workshop: [ + :local_date, :local_time, :local_end_time, :chapter_id, + :invitable, :seats, :virtual, :slack_channel, :slack_channel_link, + :rsvp_open_local_date, :rsvp_open_local_time, :description, + :coach_spaces, :student_spaces, + { sponsor_ids: [] } + ]) +end +``` + +**events_controller.rb:** +```ruby +def event_params + params.expect(event: [ + :virtual, :name, :slug, :date_and_time, :local_date, :local_time, :local_end_time, + :description, :info, :schedule, :venue_id, :external_url, :coach_spaces, :student_spaces, + :email, :announce_only, :tito_url, :invitable, :time_zone, :student_questionnaire, + :confirmation_required, :surveys_required, :audience, :coach_questionnaire, :show_faq, + :display_coaches, :display_students, + { bronze_sponsor_ids: [] }, + { silver_sponsor_ids: [] }, + { gold_sponsor_ids: [] }, + { sponsor_ids: [] }, + { chapter_ids: [] } + ]) +end +``` + +**sponsors_controller.rb (nested attributes):** +```ruby +def sponsor_params + params.expect(sponsor: [ + :name, :avatar, :website, :seats, :accessibility_info, + :number_of_coaches, :level, :description, + { address_attributes: [:id, :flat, :street, :postal_code, :city, :latitude, :longitude, :directions] }, + { contacts_attributes: [:id, :name, :surname, :email, :mailing_list_consent, :_destroy] } + ]) +end +``` + +**Step 3: After each conversion, run controller tests** + +Run: `bundle exec rspec spec/controllers/admin/CONTROLLER_spec.rb` +Expected: Tests PASS + +**Step 4: Commit each controller** + +```bash +git add app/controllers/admin/CONTROLLER.rb +git commit -m "feat(admin): migrate CONTROLLER to params.expect" +``` + +**Step 5: After all conversions, run full admin test suite** + +Run: `bundle exec rspec spec/controllers/admin/` +Expected: All tests PASS + +**Step 6: Optionally add type tampering tests** + +For controllers with existing test files, add one simple test: + +```ruby +context 'with type tampering' do + it 'returns 400 when params is string' do + post :create, params: { resource: 'tampered' } + expect(response).to have_http_status(:bad_request) + end +end +``` + +Only add these if the controller has existing specs. Don't create new test files just for this. + +**Step 7: Create batch commit** + +```bash +git commit --allow-empty -m "milestone: batch convert admin controllers to params.expect + +Converted controllers: +- workshops_controller (arrays) +- events_controller (multiple arrays) +- sponsors_controller (nested attributes) +- meetings_controller +- chapters_controller +- announcements_controller +- groups_controller +- [list others] + +All controllers now use params.expect syntax. +Existing test suite confirms no regressions." +``` + +--- + +## Phase 4: Cleanup and Documentation + +### Task 5: Audit Remaining Direct Access + +**Step 1: Find remaining params[:key] usage** + +Run: `grep -rn "params\[:" app/controllers/ --include="*.rb" | grep -v "params\[:id\]" | grep -v "params\[:format\]"` +Expected: List of remaining direct hash access (excluding legitimate route params) + +**Step 2: Review each instance** + +For each finding, determine: +- Route parameter (id, format, etc.) - LEGITIMATE, ignore +- Should use params.expect - FIX IT +- Complex conditional logic - DOCUMENT why direct access is needed + +**Step 3: Fix any that should use params.expect** + +Convert remaining problematic direct access to params.expect. + +**Step 4: Document legitimate exceptions** + +If some direct access is legitimate (accessing route params, conditional checks), add code comment: + +```ruby +# Direct access OK: optional route parameter, not form data +callback_url = params[:callback_url] +``` + +**Step 5: Commit cleanup** + +```bash +git add app/controllers/ +git commit -m "refactor: eliminate remaining inappropriate params hash access + +- Convert X controllers to use params.expect +- Document legitimate direct access cases +- All form parameters now validated" +``` + +--- + +### Task 6: Update Documentation + +**Step 1: Add params.expect pattern to CLAUDE.md** + +In `CLAUDE.md`, add section after "Code Style": + +```markdown +## Parameter Handling + +**Pattern:** Use `params.expect` (Rails 8.0) for all parameter filtering: + +```ruby +def resource_params + params.expect(resource: [ + :field1, :field2, + { nested_array: [] }, + { nested_attributes: [:id, :field, :_destroy] } + ]) +end +``` + +**Type Safety:** All controllers return 400 Bad Request when parameters are type-tampered or missing required keys. + +**Conditional Parameters:** +```ruby +# When parameter hash is optional +search_params = if params.key?(:member_search) + params.expect(member_search: [:name]) + else + {} + end +``` + +**Testing:** Type tampering tests in controller specs verify 400 responses. +``` + +**Step 2: Commit documentation** + +```bash +git add CLAUDE.md +git commit -m "docs: add params.expect pattern to CLAUDE.md" +``` + +--- + +### Task 7: Final Verification + +**Step 1: Run complete test suite** + +Run: `bundle exec rspec` +Expected: All tests PASS + +**Step 2: Check for any remaining params.require** + +Run: `grep -r "params\.require" app/controllers/` +Expected: No results (all converted) + +**Step 3: Run linter** + +Run: `bundle exec rubocop` +Expected: No new offenses + +If offenses found, fix them and commit: +```bash +git add -A +git commit -m "style: fix rubocop offenses from params.expect migration" +``` + +**Step 4: Manual smoke test (5 minutes)** + +Start server: `bundle exec rails server` + +Test these flows: +1. Update member profile (member/details) +2. Create workshop (admin) +3. Search for member (admin) + +Expected: All work without errors + +**Step 5: Final milestone commit** + +```bash +git commit --allow-empty -m "milestone: complete params.expect migration + +Summary: +- 20 controller files converted to params.expect +- Zero regressions in test suite (all tests pass) +- Type tampering now returns 400 instead of 500 +- Documentation updated + +Key changes: +- payments_controller: Stripe params validated +- member/details_controller: Fixed mixed permitted/unpermitted access +- admin controllers: Consistent parameter handling +- Nested attributes: sponsors, events properly converted + +Benefits: +- Cleaner syntax (single method call) +- Better security (type tampering protection) +- Self-documenting parameter structure" +``` + +--- + +## Deploy Strategy + +### Incremental Rollout + +**Stage 1: Deploy payments controller only** +- Deploy after Task 1 +- Monitor for 24 hours +- Check error tracking for unexpected 400s + +**Stage 2: Deploy complex controllers** +- Deploy after Task 3 +- Monitor member profile updates +- Monitor admin member search + +**Stage 3: Deploy remaining controllers** +- Deploy after Task 4 +- Monitor admin workshop/event creation +- Full smoke test in production + +### Monitoring Checklist + +After each stage: +- [ ] Check Rollbar for 400 errors (expect some if users/bots tamper params) +- [ ] Check Rollbar for 500 errors (should NOT increase) +- [ ] Verify form submissions work (member profile, workshops, events) +- [ ] Check Scout APM for performance changes (should be neutral) + +--- + +## Rollback Procedure + +### If 400 errors spike unexpectedly: + +**Step 1: Check if errors are legitimate** + +Look at error details in Rollbar: +- User-submitted tampered data = GOOD (params.expect working correctly) +- Valid form submissions failing = BAD (rollback needed) + +**Step 2: Quick rollback (if needed)** + +```bash +# Identify the problematic commit +git log --oneline -20 + +# Revert specific controller +git revert + +# Push and deploy +git push origin master +# Deploy via Heroku/your process +``` + +**Step 3: Fix and redeploy** + +- Fix the issue locally +- Run tests to verify +- Commit fix +- Deploy again + +### If widespread issues: + +```bash +# Revert entire migration +git log --grep="params.expect" --oneline +git revert .. +git push origin master +# Deploy +``` + +--- + +## Success Criteria + +### Phase 1 Complete: +- [x] Rails 8.0 verified +- [x] Payments controller converted and tested +- [x] Deployed to staging (if available) +- [x] No increase in errors + +### Phase 2 Complete: +- [x] Member details controller refactored +- [x] Member search controller converted +- [x] Existing tests pass +- [x] Type tampering returns 400 + +### Phase 3 Complete: +- [x] All admin controllers converted +- [x] Full test suite passes +- [x] Manual smoke test passes + +### Phase 4 Complete: +- [x] No remaining params.require in controllers +- [x] Documentation updated +- [x] Rubocop clean + +### Overall Success: +- [x] All 20 controllers use params.expect +- [x] Zero test failures +- [x] Zero production incidents +- [x] 400 errors on type tampering (not 500) +- [x] 1-2 days actual effort (not 4-6) diff --git a/docs/plans/migration-notes.txt b/docs/plans/migration-notes.txt new file mode 100644 index 000000000..a4ec8e44d --- /dev/null +++ b/docs/plans/migration-notes.txt @@ -0,0 +1 @@ +Rails version verified: Rails 8.1.2 diff --git a/spec/controllers/admin/member_search_controller_spec.rb b/spec/controllers/admin/member_search_controller_spec.rb index 3184c743a..ec81a4786 100644 --- a/spec/controllers/admin/member_search_controller_spec.rb +++ b/spec/controllers/admin/member_search_controller_spec.rb @@ -53,4 +53,20 @@ end end end + + describe 'GET #results' do + context 'when user is an admin' do + let(:member1) { Fabricate(:member, name: 'Alice') } + let(:member2) { Fabricate(:member, name: 'Bob') } + + before do + login_as_admin(member) + end + + it 'finds members by ids using params.expect' do + get :results, params: { member_pick: { members: [member1.id, member2.id] } } + expect(response).to have_http_status(:ok) + end + end + end end diff --git a/spec/controllers/member/details_controller_spec.rb b/spec/controllers/member/details_controller_spec.rb index 9c98c487b..fc38ee29c 100644 --- a/spec/controllers/member/details_controller_spec.rb +++ b/spec/controllers/member/details_controller_spec.rb @@ -97,6 +97,7 @@ expect(response.body).to include('You must select one option') end + end end end diff --git a/spec/controllers/payments_controller_spec.rb b/spec/controllers/payments_controller_spec.rb new file mode 100644 index 000000000..f3d1caa2d --- /dev/null +++ b/spec/controllers/payments_controller_spec.rb @@ -0,0 +1,47 @@ +RSpec.describe PaymentsController do + let(:member) { Fabricate(:member) } + + before do + login(member) + allow(Stripe::Customer).to receive(:create).and_return(double(id: 'cus_123')) + allow(Stripe::Charge).to receive(:create).and_return(true) + end + + describe 'POST #create' do + context 'with valid parameters' do + it 'creates a Stripe customer and charge' do + expect(Stripe::Customer).to receive(:create).with( + email: 'john@example.com', + description: 'John Doe', + source: 'tok_123' + ).and_return(double(id: 'cus_123')) + + post :create, params: { + payment: { + amount: '1000', + name: 'John Doe', + stripe_email: 'john@example.com', + stripe_token_id: 'tok_123' + } + } + expect(response).to be_successful + end + end + + context 'with parameter filtering' do + it 'filters unpermitted parameters' do + post :create, params: { + payment: { + amount: '1000', + name: 'John', + stripe_email: 'john@example.com', + stripe_token_id: 'tok_123' + }, + hacker_field: 'malicious' + } + expect(response).to be_successful + end + end + + end +end