Rescue bike attribute assignment instead of 500ing (#4036)

A form filling bot POSTs `/bikes` with every field set to `"1"`.
`frame_material: "1"` isn't a valid enum value, and assigning it raised
`ArgumentError` — [1,441 notices in 7
days](https://app.honeybadger.io/projects/35931/faults/133165943), plus
[a second
fault](https://app.honeybadger.io/projects/35931/faults/133168206) on
the redirect back, where `embed_extended` rebuilds the form from the
same stored b_param and raises again.

- **`BikeServices::Builder.build` assigns the bike's attributes one at a
time and rescues**, adding an error on whichever attribute the model
refused. That's the whole fix — nothing about enums specifically, so
anything else assignment can raise on lands the same way. Assigning
individually is what keeps the rest of the attributes on the bike, so
the re-rendered form still holds what was entered.
- The bike is then invalid through the existing `bike.errors` →
`b_param.bike_errors` path, which every entry point already handles:
`/bikes` redirects to the form, the embed flashes the message, and the
registration flow returns to step 2.

Specs cover the two faulted paths plus the registration flow, which
permits no enum a bot can poison (`cycle_type` is `friendly_find`ed,
`status` is gated by `BParam#status`) but still builds the bike from
whatever the registration holds.

Author: sethherr

app/services/bike_services/builder.rb

@@ -10,7 +10,13 @@ def build(b_param, new_attrs = nil)
       bike = Bike.new(cycle_type: "bike")
       # passed_organization is assigned unless b_param has an organization
       passed_organization = new_attrs.delete(:organization)
-      bike.attributes = b_param.safe_bike_attrs(new_attrs)
+      # One at a time, so a value the model refuses to take (an out of range enum, say)
+      # becomes an error on the bike rather than a 500, and the rest still land
+      b_param.safe_bike_attrs(new_attrs).each do |key, value|
+        bike.attributes = {key => value}
+      rescue ArgumentError
+        bike.errors.add(key, "is not valid")
+      end
 
       bike.address_record&.bike = bike # Kinda gross, but gotta get it both ways!
 

spec/requests/bikes/create_request_spec.rb

@@ -229,6 +229,18 @@ def expect_created_stolen_bike(bike_params: nil, stolen_params: {})
       end
     end
   end
+  context "invalid frame_material" do
+    # Form filling bots submit "1" for every field - the enum assignment used to raise
+    let(:bike_params) { basic_bike_params.merge(frame_material: "1") }
+    it "renders the error" do
+      expect {
+        post base_url, params: {bike: bike_params}
+      }.to change(Bike, :count).by(0)
+      b_param = BParam.last
+      expect(b_param.bike_errors).to eq(["Frame material is not valid"])
+      expect(response).to redirect_to(new_bike_url(b_param_token: b_param.id_token))
+    end
+  end
   context "no existing b_param, impounded" do
     let(:bike_params) { basic_bike_params }
     context "impound_record" do

spec/requests/organizations_request_spec.rb

@@ -231,6 +231,18 @@
         expect(bike.status).to eq "status_stolen"
         expect(bike.owner_email).to eq(b_param_attrs[:bike][:owner_email])
       end
+      context "with an invalid enum value" do
+        let(:b_param_attrs) do
+          {bike: {owner_email: "someemail@stuff.com", frame_material: "1",
+                  creation_organization_id: current_organization.id.to_s}}
+        end
+        it "renders" do
+          get "#{base_url}/#{current_organization.id}/embed_extended?b_param_id_token=#{b_param.id_token}"
+          expect(response.code).to eq("200")
+          expect(response).to render_template(:embed_extended)
+          expect(assigns(:bike).frame_material).to be_blank
+        end
+      end
     end
   end
 

spec/requests/register_request_spec.rb

@@ -702,6 +702,24 @@ def address_street_field
         end
       end
 
+      # This flow permits no enum a bot could poison - cycle_type is friendly_found and
+      # status is gated by BParam#status - but it creates the bike from whatever the
+      # registration already holds, so a bad value can still arrive at Builder
+      context "registration holding an invalid enum value" do
+        let(:b_param) do
+          BParam.create(origin: "register_flow",
+            params: {bike: {owner_email:, manufacturer_id: "Trek", frame_material: "1"}}.as_json)
+        end
+
+        it "sends them back to step 2 with the error, rather than raising" do
+          expect {
+            patch base_url, params: {b_param_token: b_param.id_token, bike: bike_details}
+          }.to_not change(Bike, :count)
+          expect(flash[:error]).to match(/frame material/i)
+          expect(response).to redirect_to register_path(b_param_token: b_param.id_token, step: 2)
+        end
+      end
+
       context "registering to their own address" do
         let(:owner_email) { current_user.email }
 

spec/services/bike_services/builder_spec.rb

@@ -127,6 +127,16 @@
       end
     end
 
+    context "with an invalid enum value" do
+      let(:b_param_params) { {bike: {frame_material: "1", handlebar_type: "9", frame_model: "Cool model"}} }
+      it "adds an error rather than raising" do
+        expect(bike.errors.full_messages).to eq(["Frame material is not valid", "Handlebar type is not valid"])
+        expect(bike.frame_material).to be_blank
+        expect(bike.handlebar_type).to be_blank
+        expect(bike.frame_model).to eq "Cool model"
+      end
+    end
+
     context "with organization" do
       let(:new_attrs) { {organization:} }