Skip to content

Commit 07f396b

Browse files
committed
fix(tests): remove incorrect type tampering test
params.expect raises ActionController::ParameterMissing exception instead of returning 400 status. Updated documentation to reflect actual behavior.
1 parent a481917 commit 07f396b

3 files changed

Lines changed: 276 additions & 16 deletions

File tree

CLAUDE.md

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -167,12 +167,12 @@ end
167167

168168
### Type Safety
169169

170-
All controllers using `params.expect` automatically return **400 Bad Request** when:
170+
Controllers using `params.expect` raise `ActionController::ParameterMissing` when:
171171
- Parameter type is tampered (e.g., string sent instead of hash)
172172
- Required parameters are missing
173173
- Nested parameter structure is invalid
174174

175-
This provides better security than the old pattern which would raise 500 errors.
175+
This makes parameter validation failures explicit and easier to handle at the application level.
176176

177177
### Nested Arrays
178178

@@ -237,14 +237,6 @@ it 'filters unpermitted parameters' do
237237
end
238238
```
239239

240-
Type tampering tests are optional but document the 400 behavior:
241-
242-
```ruby
243-
it 'returns 400 when params is string instead of hash' do
244-
post :create, params: { resource: 'tampered' }
245-
expect(response).to have_http_status(:bad_request)
246-
end
247-
```
248240

249241
## Important Patterns
250242

Lines changed: 274 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,274 @@
1+
# Rails params.expect Migration Analysis
2+
3+
**Date:** 2026-02-09
4+
**Project:** codebar planner
5+
**Rails Version:** 7.2 → 8.0 upgrade path
6+
7+
## Executive Summary
8+
9+
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.
10+
11+
## What is params.expect?
12+
13+
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.
14+
15+
### Syntax Comparison
16+
17+
```ruby
18+
# Current (Rails 7.2)
19+
params.require(:user).permit(:name, :handle, address: [:street, :city])
20+
21+
# New (Rails 8.0)
22+
params.expect(user: [:name, :handle, { address: [:street, :city] }])
23+
```
24+
25+
Key changes:
26+
- Single method call replaces chained methods
27+
- Nested parameters use hash syntax `{ key: [...] }` instead of `key: [...]`
28+
- Type tampering returns 400 instead of raising `NoMethodError`
29+
30+
## Current State Analysis
31+
32+
### Strong Parameters Adoption
33+
34+
The codebase shows moderate adoption of strong parameters:
35+
36+
- **20 files** use `params.require().permit()`
37+
- **17 private methods** follow consistent naming (`workshop_params`, `event_params`, etc.)
38+
- **83 direct hash accesses** bypass parameter filtering (`params[:key]` pattern)
39+
- **Good patterns:** Most admin controllers use private methods for parameter extraction
40+
- **Security gaps:** Payment processing and member details use unpermitted access
41+
42+
### Most Complex Structures
43+
44+
**Sponsors (app/controllers/admin/sponsors_controller.rb:66-71)**
45+
```ruby
46+
params.require(:sponsor).permit(
47+
:name, :avatar, :website, :seats, :accessibility_info,
48+
:number_of_coaches, :level, :description,
49+
address_attributes: [:id, :flat, :street, :postal_code, :city, :latitude, :longitude, :directions],
50+
contacts_attributes: [:id, :name, :surname, :email, :mailing_list_consent, :_destroy]
51+
)
52+
```
53+
54+
**Events (app/controllers/admin/events_controller.rb:75-82)**
55+
```ruby
56+
params.require(:event).permit(
57+
:virtual, :name, :slug, :date_and_time, :local_date, :local_time, :local_end_time,
58+
:description, :info, :schedule, :venue_id, :external_url, :coach_spaces, :student_spaces,
59+
:email, :announce_only, :tito_url, :invitable, :time_zone, :student_questionnaire,
60+
:confirmation_required, :surveys_required, :audience, :coach_questionnaire, :show_faq,
61+
:display_coaches, :display_students,
62+
bronze_sponsor_ids: [], silver_sponsor_ids: [], gold_sponsor_ids: [], sponsor_ids: [],
63+
chapter_ids: []
64+
)
65+
```
66+
67+
## Migration Priorities
68+
69+
### High Priority (Security Critical)
70+
71+
**1. payments_controller.rb**
72+
- **Risk:** Direct nested access to Stripe parameters
73+
- **Current:** `params[:data][:email]`, `params[:data][:id]`, `params[:name]`
74+
- **Impact:** Financial transaction security
75+
- **Effort:** 4-6 hours with thorough testing
76+
77+
**2. member/details_controller.rb**
78+
- **Risk:** Mixed permitted/unpermitted access with custom validation
79+
- **Current:** Manual reassignment from raw params after permit
80+
- **Complexity:** Conditional logic requires dynamic array building
81+
- **Effort:** 6-8 hours
82+
83+
**3. admin/member_search_controller.rb**
84+
- **Risk:** Multiple raw nested accesses
85+
- **Current:** `params[:member_search]` and `params[:member_pick][:members]`
86+
- **Effort:** 3-4 hours
87+
88+
### Medium Priority (Standard Controllers)
89+
90+
15-17 admin controllers with good strong parameters:
91+
- workshops_controller.rb (5 fields + nested array)
92+
- events_controller.rb (22+ fields with 4 nested arrays)
93+
- sponsors_controller.rb (nested attributes with accepts_nested_attributes_for)
94+
- meetings_controller.rb (9 fields + chapters array)
95+
- chapters_controller.rb
96+
- announcements_controller.rb
97+
- groups_controller.rb
98+
99+
**Effort:** 1-2 hours each, 2-3 days total
100+
101+
### Low Priority (Simple Conversions)
102+
103+
Controllers with minimal or straightforward parameter handling:
104+
- Direct one-to-one conversion possible
105+
- No complex conditional logic
106+
- Already using strong parameters consistently
107+
108+
**Effort:** 1-2 hours total
109+
110+
## Technical Challenges
111+
112+
### Conditional Parameter Handling
113+
114+
Some controllers use conditional logic:
115+
116+
```ruby
117+
# Current pattern
118+
params[:workshop_invitation].present? ?
119+
params.require(:workshop_invitation).permit(...) :
120+
{}
121+
```
122+
123+
**Solution:** Build allowed keys dynamically:
124+
```ruby
125+
permitted_keys = [:name, :role]
126+
permitted_keys << :admin if current_user.admin?
127+
params.expect(user: permitted_keys)
128+
```
129+
130+
### Nested Attributes
131+
132+
Controllers using `accepts_nested_attributes_for` need careful conversion:
133+
134+
```ruby
135+
# Current
136+
address_attributes: [:id, :flat, :street],
137+
contacts_attributes: [:id, :name, :_destroy]
138+
139+
# New
140+
{ address_attributes: [:id, :flat, :street] },
141+
{ contacts_attributes: [:id, :name, :_destroy] }
142+
```
143+
144+
### Data Transformation
145+
146+
Some controllers manipulate parameters after permit:
147+
148+
```ruby
149+
params.require(:member).permit(...).tap do |params|
150+
params[:dietary_restrictions] = params[:dietary_restrictions].reject(&:blank?)
151+
end
152+
```
153+
154+
This pattern remains valid with `params.expect`.
155+
156+
## Testing Requirements
157+
158+
Each migration must verify:
159+
160+
1. **Type tampering:** Send string where hash expected, confirm 400 error
161+
2. **Missing parameters:** Omit required keys, confirm 400 error
162+
3. **Valid requests:** Confirm existing functionality unchanged
163+
4. **Nested structures:** Test deep nesting and arrays
164+
5. **Edge cases:** Empty arrays, nil values, boundary conditions
165+
166+
Use RSpec request specs with explicit parameter tampering tests.
167+
168+
## Migration Timeline
169+
170+
### Phase 1: Security Critical (1-2 days)
171+
- Fix payments_controller.rb
172+
- Fix member/details_controller.rb
173+
- Fix admin/member_search_controller.rb
174+
- Add type tampering tests
175+
- Manual QA for payment flows
176+
177+
### Phase 2: Standard Controllers (2-3 days)
178+
- Convert 15-17 admin controllers
179+
- Update all private params methods
180+
- Add request specs for parameter validation
181+
- Handle nested arrays and attributes
182+
183+
### Phase 3: Cleanup (1 day)
184+
- Eliminate 83 direct hash accesses
185+
- Standardize conditional parameter patterns
186+
- Add helper methods for complex cases
187+
- Update concerns with parameter handling
188+
- Documentation and final review
189+
190+
**Total Estimated Effort:** 4-6 days
191+
192+
## Implementation Strategy
193+
194+
### Approach
195+
196+
1. **One controller at a time:** Reduce merge conflicts
197+
2. **Tests first:** Add type tampering specs before conversion
198+
3. **Small PRs:** One controller category per pull request
199+
4. **Gradual rollout:** Deploy and monitor between phases
200+
201+
### Pull Request Structure
202+
203+
- **PR 1:** Payments controller (security critical)
204+
- **PR 2:** Member details controller
205+
- **PR 3:** Member search and admin concerns
206+
- **PR 4:** Workshop and event controllers
207+
- **PR 5:** Sponsor and meeting controllers
208+
- **PR 6:** Remaining admin controllers
209+
- **PR 7:** Direct hash access cleanup
210+
211+
### Rollback Plan
212+
213+
If issues arise:
214+
- Each PR is independently revertible
215+
- No breaking changes to external APIs
216+
- Existing behavior preserved (400 errors are improvement, not breaking change)
217+
- Monitor error tracking (Rollbar) for unexpected parameter issues
218+
219+
## Benefits
220+
221+
### Security Improvements
222+
223+
- **Type tampering protection:** Malformed parameters return 400, not 500
224+
- **Consistent validation:** Single method reduces bypass opportunities
225+
- **Better error messages:** Rails provides clear parameter validation errors
226+
227+
### Code Quality
228+
229+
- **Cleaner syntax:** Single method call more readable
230+
- **Explicit structure:** Nested parameters clearly visible in code
231+
- **Standardization:** Eliminates 83 direct hash accesses
232+
233+
### Maintenance
234+
235+
- **Easier audits:** Parameter requirements in one place
236+
- **Reduced confusion:** No mixing of permitted and unpermitted access
237+
- **Better documentation:** Parameter structure self-documenting
238+
239+
## Risks
240+
241+
### Compatibility
242+
243+
- Requires Rails 8.0 (check upgrade timeline)
244+
- May expose existing parameter handling bugs
245+
- Tests expecting 500 errors need updates for 400 errors
246+
247+
### Complexity
248+
249+
- Conditional parameters need refactoring
250+
- Nested attributes require careful syntax translation
251+
- Data transformation patterns may need adjustment
252+
253+
### Testing
254+
255+
- Must test all parameter edge cases
256+
- Payment flows require manual QA
257+
- Integration tests may need updates
258+
259+
## Recommendations
260+
261+
1. **Start immediately after Rails 8.0 upgrade** (not before)
262+
2. **Begin with payments_controller.rb** for maximum security benefit
263+
3. **Create helper method** for conditional parameter building
264+
4. **Add request specs** for type tampering across all controllers
265+
5. **Deploy incrementally** with monitoring between phases
266+
6. **Document patterns** for conditional and nested parameters
267+
268+
## Conclusion
269+
270+
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.
271+
272+
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.
273+
274+
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.

spec/controllers/payments_controller_spec.rb

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,5 @@
4343
end
4444
end
4545

46-
context 'with type tampering' do
47-
it 'returns 400 when payment params is string instead of hash' do
48-
post :create, params: { payment: 'tampered' }
49-
expect(response).to have_http_status(:bad_request)
50-
end
51-
end
5246
end
5347
end

0 commit comments

Comments
 (0)