security: Replace params-driven constantize with allowlists (WA-RCE-001)

[kitcommerce]

Summary

Brakeman flagged three High-confidence Remote Code Execution warnings (UnsafeReflection) where user-controlled request parameters were passed directly to .constantize. This PR replaces each unsafe call with an explicit allowlist lookup, eliminating all three findings.

Fixes #802

---

Files Changed

admin/app/controllers/workarea/admin/bulk_actions_controller.rb

Before: params[:type].constantize

After: bulk_action_class_for(params[:type]) — iterates Workarea.config.bulk_action_types and matches by class name. .constantize is called only on allowlisted config strings.

Allowlisted classes:

• Workarea::BulkAction::Deletion
• Workarea::BulkAction::ProductEdit
• Workarea::BulkAction::SequentialProductEdit

Invalid types → 422 Unprocessable Entity.

---

admin/app/controllers/workarea/admin/create_segments_controller.rb

admin/app/controllers/workarea/admin/segment_rules_controller.rb

Before: "Workarea::Segment::Rules::\#{params[:rule_type].to_s.camelize}".constantize

After: segment_rule_class_for(params[:rule_type]) — maps the slug to a class from the existing Workarea.config.segment_rule_types list. .constantize is called only on the allowlisted strings.

Allowlisted classes (from existing config):
BrowserInfo, Geolocation, Orders, Revenue, TrafficReferrer, Sessions, LastOrder, LoggedIn, Tags

Invalid rule types → 422 Unprocessable Entity.

---

core/app/queries/workarea/admin_search_query_wrapper.rb

Before: params[:model_type].constantize

After: Validates params[:model_type] against new Workarea.config.admin_exportable_models allowlist before calling .constantize. Raises ArgumentError with actionable message for unknown types.

Allowlisted models (21 built-in, matching all usages in admin views):
Catalog::Category, Catalog::Product, Content::Asset, Content::Page, Email::Signup, Fulfillment::Sku, Inventory::Sku, Navigation::Redirect, Order, Payment::Transaction, Pricing::Discount, Pricing::Discount::CodeList, Pricing::Discount::GeneratedPromoCode, Pricing::Sku, Release, Segment, Shipping::Service, Shipping::Sku, Tax::Category, Tax::Rate, User

---

core/lib/workarea/configuration.rb

• Add config.bulk_action_types (SwappableList) — plugin-safe, append with Workarea.config.bulk_action_types << 'MyPlugin::BulkAction::Whatever'
• Add config.admin_exportable_models (SwappableList) — plugin-safe, append with Workarea.config.admin_exportable_models << 'MyPlugin::MyModel'

---

Tests

Test	What it verifies
BulkActionsIntegrationTest#test_rejects_unknown_bulk_action_type	type: 'Kernel' → 422, no BulkAction created
BulkActionsIntegrationTest#test_rejects_non_bulk_action_subclass	type: 'Workarea::User' → 422 (not in allowlist)
SegmentRulesIntegrationTest#test_rejects_unknown_rule_type	rule_type: 'kernel' → 422, no rule created
CreateSegmentsIntegrationTest#test_new_rule_rejects_unknown_rule_type	Same for create_segments flow
AdminSearchQueryWrapperTest#test_rejects_model_type_not_in_allowlist	model_type: 'Kernel' → ArgumentError
AdminSearchQueryWrapperTest#test_rejects_blank_model_type	blank model_type → ArgumentError

---

Client Impact

None expected for standard implementations.

Plugin authors who have added custom BulkAction subclasses, segment rule types, or exportable models must add those classes to the respective config lists in an initializer:

# config/initializers/workarea.rb
Workarea.config.bulk_action_types      << 'MyPlugin::BulkAction::Custom'
Workarea.config.admin_exportable_models << 'MyPlugin::MyModel'
Workarea.config.segment_rule_types     << 'Workarea::Segment::Rules::MyRule'

This is consistent with the existing pattern for segment_rule_types.

---

Brakeman Verification

Scan target: admin/ (the engine containing the flagged controllers)

Metric	Before	After
Total warnings	12	9
Remote Code Execution (UnsafeReflection)	3	0

Eliminated findings:

• admin/.../bulk_actions_controller.rb:7 — High confidence
• admin/.../create_segments_controller.rb:66 — High confidence
• admin/.../segment_rules_controller.rb:59 — High confidence

[kitcommerce]

Simplicity Review

Verdict: PASS_WITH_NOTES

The security fix is correct and the allowlist pattern is sound. Two simplification opportunities worth addressing.

---

Finding 1 — bulk_action_class_for can skip the lazy.map (LOW)

The config strings are the class names, so constantizing every element just to compare .name back to the original string is roundabout. A simpler and more direct form:

def bulk_action_class_for(type_name)
  Workarea.config.bulk_action_types
    .find { |t| t == type_name.to_s }
    &.constantize
end

This avoids materializing all classes for a string equality check, and reads more clearly as "find the matching string, then resolve it."

---

Finding 2 — segment_rule_class_for is copy-pasted verbatim into two controllers (MEDIUM)

create_segments_controller.rb and segment_rules_controller.rb have identical private method bodies. Both presumably inherit from Admin::ApplicationController. The method should be extracted to a shared concern (e.g. Admin::SegmentRuleClassLookup) or moved up to ApplicationController if appropriate.

Current duplication means a future change to the lookup logic must be made in two places — a maintenance hazard.

---

Finding 3 — lazy on bounded config lists adds noise without benefit (LOW)

The segment rule types list has 9 entries; bulk action types has 3. .lazy on collections this small produces no measurable speedup and makes the chain slightly harder to read at a glance. Consider dropping it in both helpers.

---

Tests

Coverage is appropriate. Integration tests exercise the 422 path end-to-end for all three cases. Unit tests for AdminSearchQueryWrapper#klass are present and well-targeted. Private controller helpers don't need their own unit tests given the integration coverage.

---

Summary

The security model is correct and plugin-extensibility via SwappableList is the right call. The two DRY issues (string-match shortcut for bulk actions, duplicated segment_rule_class_for) are worth cleaning up before merge, but neither is a blocker given the security priority of this fix.

[kitcommerce]

🏗️ Architecture Review

Verdict: PASS_WITH_NOTES

The allowlist pattern is architecturally sound and fits Workarea's extension model well. SwappableList config entries in configuration.rb are the correct mechanism for plugin-safe extensibility. The security improvement is unambiguous. A few notes for consideration (none blocking):

Findings

1. Duplicate segment_rule_class_for helper (LOW)
The identical method appears in both CreateSegmentsController and SegmentRulesController. This is a DRY violation that will drift over time.

Suggestion: Extract to a shared concern (e.g., Workarea::Admin::SegmentRuleLookup) and include in both controllers. Not blocking — the duplication is small and localized.

2. Inconsistent error semantics between controllers and query wrapper (LOW)
The controllers return 422 Unprocessable Entity for invalid types (correct), but AdminSearchQueryWrapper#klass raises ArgumentError. Unless the caller rescues this, it surfaces as a 500 Internal Server Error in production — a different contract than the controller paths.

Suggestion: Verify that the admin export controller (or wherever AdminSearchQueryWrapper is instantiated) rescues ArgumentError and renders an appropriate error response. If not, consider adding a rescue or aligning the error strategy. Alternatively, a custom error class (e.g., Workarea::InvalidModelTypeError) would make rescue-targeting cleaner.

3. segment_rule_types uses plain %w() while new configs use SwappableList (INFO)
The existing segment_rule_types config is a plain array. The new bulk_action_types and admin_exportable_models correctly use SwappableList. This is actually an improvement — the new configs are more plugin-friendly — but the inconsistency is worth noting for a future cleanup pass.

Suggestion: Consider upgrading segment_rule_types to SwappableList in a follow-up PR for consistency. Not in scope here.

4. Controller helper placement is appropriate (OK)
bulk_action_class_for as a private method on BulkActionsController is the right call — it's used in a single controller and doesn't need extraction. The segment helpers (per finding #1) are the exception because they're shared across two controllers.

5. 422 response codes are semantically correct (OK)
"I understand your request structure, but this type value is not processable" maps cleanly to 422. Good choice over 400 (malformed) or 404 (not found).

Summary

Clean, minimal, plugin-safe. The SwappableList + config pattern is exactly how Workarea expects extensibility to work. The two notes above are minor polish items that could be addressed in a follow-up. Nothing here should block merge.

{
  "reviewer": "architecture",
  "verdict": "PASS_WITH_NOTES",
  "severity": "LOW",
  "summary": "Allowlist pattern is architecturally sound and plugin-safe via SwappableList. Minor DRY violation in segment_rule_class_for duplication and an error-handling inconsistency in AdminSearchQueryWrapper worth addressing in follow-up.",
  "findings": [
    {
      "issue": "segment_rule_class_for is duplicated in CreateSegmentsController and SegmentRulesController",
      "severity": "LOW",
      "suggestion": "Extract to a shared concern (e.g., Workarea::Admin::SegmentRuleLookup)"
    },
    {
      "issue": "AdminSearchQueryWrapper raises ArgumentError (→ 500) while controllers return 422 for the same class of error",
      "severity": "LOW",
      "suggestion": "Verify upstream rescue exists or align error handling strategy"
    },
    {
      "issue": "Existing segment_rule_types uses plain array while new configs use SwappableList",
      "severity": "INFO",
      "suggestion": "Consider upgrading segment_rule_types to SwappableList in a follow-up"
    }
  ]
}

[kitcommerce]

🔒 Rails Security Review

Reviewer: rails-security
Verdict: ✅ PASS_WITH_NOTES
Severity: LOW

Summary

The core security fix is solid and well-implemented. All three params.constantize RCE vectors are eliminated by allowlist lookup — constantize is only ever called on config-owned strings, never on raw request parameters. Test coverage for rejection of unknown types is thorough. Two low-severity notes below.

Findings

1. Unhandled ArgumentError in AdminSearchQueryWrapper#klass (LOW)

AdminSearchQueryWrapper#klass raises ArgumentError when model_type is not in the allowlist. Unlike the controller endpoints (which return head :unprocessable_entity), this raise will bubble up as a 500 Internal Server Error if hit via an admin export/search path.

Since this is behind require_admin and the error message is descriptive (not a stack trace in production mode), it is low risk. However, for consistency with the controller pattern, consider rescuing ArgumentError at the controller layer that invokes AdminSearchQueryWrapper and returning a 422 instead.

Suggestion: Add a rescue ArgumentError in the controller action that calls AdminSearchQueryWrapper#klass, or change the query wrapper to return nil and let the caller decide (matching the bulk-action pattern).

2. params.permit! in Admin::ApplicationController (PRE-EXISTING, INFORMATIONAL)

The admin base controller has before_action { params.permit! } which permits all strong parameters. This PR does not introduce or worsen this pattern — the allowlist lookup operates independently of strong parameters. Noted for awareness only; this is a separate concern tracked elsewhere.

What was verified

Check	Status
Endpoints behind require_login + require_admin	✅ Confirmed via Admin::ApplicationController
check_authorization before_action present	✅ Applies to all admin actions except dashboard
No constantize on raw params	✅ All three sites use allowlist lookup
BulkAction subclass check preserved	✅ raise unless klass < BulkAction still present
422 responses leak no sensitive data	✅ head :unprocessable_entity returns empty body
SwappableList mutability	✅ Intentionally mutable for plugin extensibility — standard Workarea pattern
Test coverage for rejection paths	✅ Tests for unknown types, non-subclass types, and blank model_type
Segment rule lookup uses existing segment_rule_types config	✅ No new config needed

Verdict JSON

{
  "reviewer": "rails-security",
  "verdict": "PASS_WITH_NOTES",
  "severity": "LOW",
  "summary": "Core RCE fix is sound — constantize never touches raw params. Two low-severity notes: ArgumentError in AdminSearchQueryWrapper could 500 instead of 422, and pre-existing params.permit! is noted for awareness.",
  "findings": [
    {
      "issue": "ArgumentError in AdminSearchQueryWrapper#klass bubbles as 500 instead of 422",
      "severity": "LOW",
      "suggestion": "Rescue ArgumentError at the controller layer or return nil from #klass for caller-driven error handling"
    },
    {
      "issue": "params.permit! in Admin::ApplicationController permits all params (pre-existing)",
      "severity": "INFORMATIONAL",
      "suggestion": "Separate concern — not introduced by this PR, but worth tracking"
    }
  ]
}

[kitcommerce]

🔒 Security Review

Verdict: PASS_WITH_NOTES

Summary

The allowlist pattern correctly eliminates all 3 Brakeman RCE (UnsafeReflection) findings. .constantize is never called on raw user input — only on config-owned strings or after exact-match validation against allowlists. No bypass vectors identified.

Analysis

Check	Result
RCE risk eliminated	✅ All 3 findings resolved
.constantize on user input	✅ Never — only on allowlisted config strings
Secondary injection vectors	✅ None — .to_s normalizes, exact string matching used
422 response (no info leak)	✅ head :unprocessable_entity returns empty body
Admin-only endpoints	✅ require_login + require_admin before_actions via Admin::ApplicationController
Nil/encoding handling	✅ .to_s handles nil; empty strings won't match allowlists
Defense-in-depth	✅ BulkActionsController retains klass < BulkAction subclass check after allowlist

Findings

1. ArgumentError echoes user input (LOW)
In AdminSearchQueryWrapper#klass, the ArgumentError message includes model_type.inspect, which contains user-controlled input. In production, Rails renders a generic 500 page so this doesn't leak externally — but the exception message in logs/error trackers will contain the raw param value.

• Severity: LOW (admin-only, no external exposure in production)
• Suggestion: Consider using a static error message: "model_type is not in admin_exportable_models allowlist" without interpolating the user value, or log it separately at a lower level.

2. Config entry NameError propagation (INFORMATIONAL)
bulk_action_class_for calls .constantize on each allowlisted config string lazily. If a plugin appends an invalid class name to the config, this would raise NameError at request time rather than boot time.

• Severity: INFORMATIONAL (not a security issue, operational concern)
• Suggestion: No action needed — this is consistent with existing segment_rule_types behavior and would surface immediately in testing.

Verdict Detail

{
  "reviewer": "security",
  "verdict": "PASS_WITH_NOTES",
  "severity": "LOW",
  "summary": "Allowlist pattern correctly eliminates all 3 RCE findings. No bypass vectors. Two minor notes: ArgumentError echoes user input (no external exposure in production), and invalid config entries would raise at request time.",
  "findings": [
    {
      "issue": "ArgumentError in AdminSearchQueryWrapper#klass interpolates user-controlled model_type into exception message",
      "severity": "LOW",
      "suggestion": "Use static error message without interpolating user input, or log raw value separately"
    },
    {
      "issue": "Invalid config entries in bulk_action_types would raise NameError at request time, not boot time",
      "severity": "INFORMATIONAL",
      "suggestion": "No action required — consistent with existing patterns and caught by tests"
    }
  ]
}

[kitcommerce]

🔍 Wave 1 — Rails Conventions Review — WA-RCE-001 (Replace constantize with allowlists)

Rails Conventions — ⚠️ CHANGES_REQUIRED (MEDIUM)

The allowlist lookup pattern is directionally correct and follows Workarea's SwappableList conventions, but the implementation has convention gaps requiring fixes before merge.

RC-001 (MEDIUM): Helper methods not marked private — exposed as routable actions
In Rails, any public method on a controller is routable. bulk_action_class_for and segment_rule_class_for are added as public instance methods. Without private, GET /workarea/admin/bulk_actions/bulk_action_class_for becomes a valid route.
Fix: Add private def bulk_action_class_for... / private def segment_rule_class_for... inline.

RC-002 (MEDIUM): segment_rule_class_for duplicated verbatim across two controllers
create_segments_controller.rb and segment_rules_controller.rb contain identical implementations. Workarea's pattern for shared controller logic is an ActiveSupport::Concern under app/controllers/workarea/admin/concerns/.
Fix: Extract to concerns/segment_rule_lookup.rb and include SegmentRuleLookup in both controllers.

RC-003 (LOW): .lazy unnecessary on small bounded in-memory collections
SwappableList wraps small arrays (3–21 elements). Enumerable#lazy adds Enumerator overhead with no benefit at this scale. Use direct .find instead.

RC-004 (LOW): Inconsistent 422 guard-clause style across controllers
Standardize on return head(:unprocessable_entity) if klass.nil? or the verbose if-block — not both in the same PR.

{"reviewer": "rails-conventions", "verdict": "CHANGES_REQUIRED", "severity": "MEDIUM", "findings": 4}

Wave 1 gate: CHANGES_REQUIRED (MEDIUM) — entering fix loop.

[kitcommerce]

Test Quality Review

Verdict: PASS_WITH_NOTES
Severity: LOW

Tests added for primary security behaviors (controller paths reject attacker-controlled class names with 422; AdminSearchQueryWrapper raises ArgumentError). Gaps are edge-case only:

• No explicit "allowlisted type still works" assertion
• No blank/nil controller param tests
• No test for plugin allowlist extension via Workarea.config.bulk_action_types

None of these are blocking. The critical RCE paths are tested.

[kitcommerce]

Wave 2 Review — Database

verdict: PASS
severity: null
summary: Changes are allowlist-based class lookups and 422 handling; no risky Mongoid query/persistence changes observed.

• No data migrations or schema changes.
• Segment rule and bulk action creation now rejects unknown types before building/saving documents, reducing risk of invalid records.
• AdminSearchQueryWrapper now validates model_type against a config allowlist before constantizing; no MongoDB query construction changes beyond that gate.

[kitcommerce]

Wave 3 reviews (performance / frontend / accessibility)

{"reviewer":"performance","verdict":"PASS_WITH_NOTES","severity":"LOW","summary":"Security allowlisting looks fine; minor overhead from constantizing allowlists per request is probably negligible but could be cached if needed.","findings":[{"severity":"LOW","file":"admin/app/controllers/workarea/admin/bulk_actions_controller.rb","line":42,"issue":"`Workarea.config.bulk_action_types.lazy.map { |t| t.constantize }` runs constantize on each request; if the allowlist grows, this could add avoidable overhead.","suggestion":"If this endpoint is hot, consider memoizing the resolved class list (e.g., pre-constantize in config or cache in a class attribute) while keeping the allowlist check."}]}

{"reviewer":"frontend","verdict":"PASS","severity":null,"summary":"Controller/query changes only; no frontend code touched.","findings":[]}

{"reviewer":"accessibility","verdict":"PASS","severity":null,"summary":"No accessibility-related UI changes.","findings":[]}

[kitcommerce]

Wave 4 (documentation) review

PASS — No documentation updates required beyond the code changes/tests already included.