feat: [PPT-2454] sanitize attributes

.ameba.yml

@@ -63,3 +63,19 @@ Style/MultilineStringLiteral:
 
 Metrics/CyclomaticComplexity:
   Enabled: false
+
+Lint/WhitespaceAroundMacroExpression:
+  Enabled: false
+
+Lint/SpecEqWithBoolOrNilLiteral:
+  Excluded:
+    - "spec/**"
+
+Style/VerboseNilType:
+  Enabled: false
+
+Lint/UnusedRescueVariable:
+  Enabled: false
+
+Style/MultilineCurlyBlock:
+  Enabled: false

shard.override.yml

@@ -5,4 +5,7 @@ dependencies:
   retriable:
     github: Sija/retriable.cr
   pg-orm:
-    github: spider-gazelle/pg-orm
+    github: spider-gazelle/pg-orm
+  sanitize:
+    github: straight-shoota/sanitize
+    branch: master

shard.yml

@@ -6,6 +6,7 @@ dependencies:
   # Data validation library
   active-model:
     github: spider-gazelle/active-model
+    version: ~> 4.4
 
   # Data validation library
   CrystalEmail:

spec/asset_category_spec.cr

@@ -8,6 +8,16 @@ module PlaceOS::Model
   describe AssetCategory do
     test_round_trip(AssetCategory)
 
+    it "preserves a JSON string stored in the description field" do
+      json_description = %({"resource_type":"locker_banks","created_at":1765438626035})
+      asset_category = Generator.asset_category
+      asset_category.description = json_description
+      asset_category.save!
+
+      reloaded = AssetCategory.find!(asset_category.id)
+      reloaded.description.should eq json_description
+    end
+
     it "saves an Asset" do
       asset_category = Generator.asset_category.save!
 

spec/booking_spec.cr

@@ -742,4 +742,28 @@ module PlaceOS::Model
     deleted_booking.should_not be_nil
     deleted_booking.not_nil!.deleted.should be_true
   end
+
+  it "sanitizes extension_data string values before saving", tags: "extension_data_sanitization" do
+    tenant_id = Generator.tenant.id
+    user_email = "test@place.tech"
+
+    booking = Booking.new(
+      booking_type: "desk",
+      asset_ids: ["desk-1"],
+      booking_start: 1.hour.from_now.to_unix,
+      booking_end: 2.hours.from_now.to_unix,
+      user_email: PlaceOS::Model::Email.new(user_email),
+      user_name: "Test User",
+      booked_by_email: PlaceOS::Model::Email.new(user_email),
+      booked_by_name: "Test User",
+      tenant_id: tenant_id,
+      booked_by_id: "user-1234",
+      history: [] of Booking::History
+    )
+    booking.change_extension_data(JSON::Any.new({"note" => JSON::Any.new("<script>alert('xss')</script>hello")}))
+    booking.save!
+
+    saved = Booking.find!(booking.id.not_nil!)
+    saved.extension_data["note"].as_s.should eq "hello"
+  end
 end

src/placeos-models/alert.cr

@@ -21,8 +21,8 @@ module PlaceOS::Model
       CUSTOM
     end
 
-    attribute name : String, es_subfield: "keyword"
-    attribute description : String = ""
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    attribute description : String = "", sanitize: :common
     attribute enabled : Bool = true, es_subfield: "keyword"
     attribute any_match : Bool = false, es_subfield: "keyword"
 

src/placeos-models/alert_dashboard.cr

@@ -7,8 +7,8 @@ module PlaceOS::Model
 
     table :alert_dashboard
 
-    attribute name : String, es_subfield: "keyword"
-    attribute description : String = ""
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    attribute description : String = "", sanitize: :common
     attribute enabled : Bool = true
 
     # Association

src/placeos-models/api_key.cr

@@ -10,8 +10,8 @@ module PlaceOS::Model
 
     table :api_key
 
-    attribute name : String, es_subfield: "keyword"
-    attribute description : String = ""
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    attribute description : String = "", sanitize: :common
 
     attribute scopes : Array(UserJWT::Scope) = [UserJWT::Scope::PUBLIC], converter: PlaceOS::Model::DBArrConverter(PlaceOS::Model::UserJWT::Scope), es_type: "keyword"
 
@@ -78,7 +78,7 @@ module PlaceOS::Model
 
     @[JSON::Field(ignore: true)]
     getter x_api_key : String? do
-      return nil if self.persisted?
+      return if self.persisted?
       "#{self.safe_id}.#{self.secret}"
     end
 

src/placeos-models/asset.cr

@@ -10,22 +10,22 @@ module PlaceOS::Model
 
     attribute identifier : String?, es_type: "keyword"
     attribute serial_number : String?
-    attribute other_data : JSON::Any?
+    attribute other_data : JSON::Any?, sanitize: :common
     attribute barcode : String?
 
-    attribute name : String?
+    attribute name : String?, sanitize: :text
     attribute client_ids : JSON::Any? # {floorsense_id: "", other_id: ""} etc
     attribute map_id : String?
     attribute bookable : Bool = true
     attribute accessible : Bool = false
     attribute zones : Array(String) = [] of String, es_type: "keyword"
     attribute place_groups : Array(String) = [] of String, es_type: "keyword"
-    attribute assigned_to : String?   # email
-    attribute assigned_name : String? # name of user
+    attribute assigned_to : String?                    # email
+    attribute assigned_name : String?, sanitize: :text # name of user
     # queryable with AND and OR operators
-    attribute features : Array(String) = [] of String, es_type: "keyword"
+    attribute features : Array(String) = [] of String, sanitize: :text, es_type: "keyword"
     attribute images : Array(String) = [] of String, es_type: "keyword"
-    attribute notes : String? # email
+    attribute notes : String?, sanitize: :common # email
     attribute security_system_groups : Array(String) = [] of String, es_type: "keyword"
 
     # attribute parent_id : String? # nested resource like lockers and locker banks

src/placeos-models/asset_category.cr

@@ -8,7 +8,11 @@ module PlaceOS::Model
     table :asset_category
 
     # i.e. a tablet
-    attribute name : String, es_subfield: "keyword"
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    # NOTE: intentionally not sanitized — this field is used to hold raw JSON
+    # strings (e.g. `{"resource_type":"locker_banks",...}`) and an HTML
+    # sanitizer would mangle characters such as `"` and `&`. See the
+    # "preserves a JSON string stored in the description field" spec.
     attribute description : String?
     attribute hidden : Bool = false, es_subfield: "keyword"
 

src/placeos-models/asset_purchase_order.cr

@@ -7,9 +7,9 @@ module PlaceOS::Model
 
     table :asset_purchase_order
 
-    attribute purchase_order_number : String, es_type: "keyword"
-    attribute invoice_number : String?
-    attribute supplier_details : JSON::Any?
+    attribute purchase_order_number : String, sanitize: :text, es_type: "keyword"
+    attribute invoice_number : String?, sanitize: :text
+    attribute supplier_details : JSON::Any?, sanitize: :common
     attribute purchase_date : Int64?
 
     attribute unit_price : Int64?

src/placeos-models/asset_type.cr

@@ -8,10 +8,10 @@ module PlaceOS::Model
 
     table :asset_type
 
-    attribute name : String, es_subfield: "keyword"
-    attribute brand : String
-    attribute description : String?
-    attribute model_number : String?
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    attribute brand : String, sanitize: :text
+    attribute description : String?, sanitize: :common
+    attribute model_number : String?, sanitize: :text
     attribute images : Array(String)? = [] of String
 
     belongs_to AssetCategory, foreign_key: "category_id", association_name: "category"

src/placeos-models/authority.cr

@@ -13,8 +13,8 @@ module PlaceOS::Model
 
     table :authority
 
-    attribute name : String, es_subfield: "keyword"
-    attribute description : String = ""
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    attribute description : String = "", sanitize: :common
     attribute domain : String
 
     # TODO: feature request: autogenerate login url
@@ -76,7 +76,7 @@ module PlaceOS::Model
     # Locates an authority by email domain
     def self.find_by_email(email : String) : Authority?
       parts = email.split('@', 2)
-      return nil unless parts.size == 2
+      return unless parts.size == 2
       search_domain = parts[1].downcase
       Authority.where("email_domains @> ARRAY['#{search_domain}']").first?
     end

src/placeos-models/base/model.cr

@@ -200,7 +200,7 @@ module Time::EpochConverterOptional
 
   def self.from_json(value : JSON::PullParser) : Time?
     str = value.read_raw
-    return nil unless str
+    return unless str
     if (val = str.to_i?)
       Time.unix(val)
     else

src/placeos-models/booking.cr

@@ -44,39 +44,39 @@ module PlaceOS::Model
       DECLINED
     end
 
-    attribute booking_type : String
+    attribute booking_type : String, sanitize: :text
     attribute booking_start : Int64
     attribute booking_end : Int64
     attribute timezone : String?
     attribute asset_id : String
     attribute user_id : String?
     attribute user_email : PlaceOS::Model::Email, format: "email", converter: PlaceOS::Model::EmailConverter
-    attribute user_name : String
+    attribute user_name : String, sanitize: :text
     attribute zones : Array(String) = -> { [] of String }
     # used to hold information relating to the state of the booking process
-    attribute process_state : String?
+    attribute process_state : String?, sanitize: :text
     attribute last_changed : Int64?
     attribute approved : Bool = false
     attribute approved_at : Int64?
     attribute rejected : Bool = false
     attribute rejected_at : Int64?
     attribute approver_id : String?
-    attribute approver_name : String?
+    attribute approver_name : String?, sanitize: :text
     attribute approver_email : String?, format: "email"
-    attribute department : String?
-    attribute title : String?
+    attribute department : String?, sanitize: :text
+    attribute title : String?, sanitize: :text
     attribute checked_in : Bool = false
     attribute checked_in_at : Int64?
     attribute checked_out_at : Int64?
-    attribute description : String?
+    attribute description : String?, sanitize: :common
     attribute deleted : Bool = false
     attribute deleted_at : Int64?
     attribute booked_by_email : PlaceOS::Model::Email, format: "email", converter: PlaceOS::Model::EmailConverter
-    attribute booked_by_name : String
+    attribute booked_by_name : String, sanitize: :text
     # if we want to record the system that performed the bookings
     # (kiosk, mobile, swipe etc)
-    attribute booked_from : String?
-    attribute extension_data : JSON::Any = JSON::Any.new(Hash(String, JSON::Any).new)
+    attribute booked_from : String?, sanitize: :text
+    attribute extension_data : JSON::Any = JSON::Any.new(Hash(String, JSON::Any).new), sanitize: :common
     attribute history : Array(History) = [] of History, converter: PlaceOS::Model::DBArrConverter(PlaceOS::Model::Booking::History)
 
     attribute email_digest : String?, ignore_deserialize: true
@@ -678,12 +678,12 @@ module PlaceOS::Model
     end
 
     private def get_children
-      return nil unless parent?
+      return unless parent?
       Booking.where(parent_id: id).to_a
     end
 
     private def get_parent
-      return nil if parent?
+      return if parent?
       Booking.where(id: parent_id).to_a[0]
     end
 

src/placeos-models/booking_instance.cr

@@ -22,7 +22,7 @@ module PlaceOS::Model
     attribute deleted : Bool = false
     attribute deleted_at : Int64?
 
-    attribute extension_data : JSON::Any? = nil
+    attribute extension_data : JSON::Any? = nil, sanitize: :common
     attribute history : Array(History) = [] of History, converter: PlaceOS::Model::DBArrConverter(PlaceOS::Model::Booking::History)
 
     # property so we can set this if we've already fetched the parent

src/placeos-models/broker.cr

@@ -17,8 +17,8 @@ module PlaceOS::Model
 
     attribute auth_type : AuthType = AuthType::UserPassword, converter: Enum::ValueConverter(PlaceOS::Model::Broker::AuthType)
 
-    attribute name : String, es_subfield: "keyword"
-    attribute description : String = ""
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    attribute description : String = "", sanitize: :common
 
     attribute host : String
     attribute port : Int32 = 1883 # Default MQTT port for non-tls connections

src/placeos-models/chat.cr

@@ -6,7 +6,7 @@ module PlaceOS::Model
     include PlaceOS::Model::Timestamps
     table :chats
 
-    attribute summary : String
+    attribute summary : String, sanitize: :common
 
     belongs_to User, foreign_key: "user_id", association_name: "user", presence: true
     belongs_to ControlSystem, foreign_key: "system_id", presence: true

src/placeos-models/chat_message.cr

@@ -23,7 +23,7 @@ module PlaceOS::Model
 
     attribute chat_id : String
     attribute role : Role = Role::User, converter: Enum::ValueConverter(PlaceOS::Model::ChatMessage::Role), es_type: "integer"
-    attribute content : String? = nil
+    attribute content : String? = nil, sanitize: :common
     attribute tokens : Int32 = 0
 
     # When `role` is `Function`, then this should contain the name of the function whose response is in the `content`.

src/placeos-models/control_system.cr

@@ -20,18 +20,18 @@ module PlaceOS::Model
 
     table :sys
 
-    attribute name : String, es_subfield: "keyword"
-    attribute description : String = ""
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    attribute description : String = "", sanitize: :common
 
     # Room search meta-data
     # Building + Level are both filtered using zones
-    attribute features : Set(String) = -> { Set(String).new }
+    attribute features : Set(String) = -> { Set(String).new }, sanitize: :text
     attribute email : Email?, converter: PlaceOS::Model::EmailConverter
     attribute bookable : Bool = false
     attribute public : Bool = false
-    attribute display_name : String?
-    attribute code : String?
-    attribute type : String?
+    attribute display_name : String?, sanitize: :text
+    attribute code : String?, sanitize: :text
+    attribute type : String?, sanitize: :text
     attribute capacity : Int32 = 0
     attribute map_id : String?
     attribute approval : Bool = false

src/placeos-models/converter/json_string.cr

@@ -49,7 +49,7 @@ module Enum::ValueConverter(T)
   end
 
   def self.to_json(val : T | Nil)
-    return nil if val.nil?
+    return if val.nil?
     previous_def
   end
 

src/placeos-models/doorkeeper_application.cr

@@ -9,7 +9,7 @@ module PlaceOS::Model
   class DoorkeeperApplication < ModelWithAutoKey
     table :oauth_applications
 
-    attribute name : String, es_subfield: "keyword"
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
     attribute secret : String
     attribute scopes : String = "public"
     attribute owner_id : String, es_type: "keyword"

src/placeos-models/driver.cr

@@ -12,8 +12,8 @@ module PlaceOS::Model
 
     table :driver
 
-    attribute name : String, es_subfield: "keyword"
-    attribute description : String = ""
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
+    attribute description : String = "", sanitize: :common
     attribute json_schema : JSON::Any = JSON::Any.new({} of String => JSON::Any), converter: JSON::Any::StringConverter, es_type: "text"
 
     attribute default_uri : String?

src/placeos-models/edge.cr

@@ -8,9 +8,9 @@ module PlaceOS::Model
 
     table :edge
 
-    attribute name : String, es_subfield: "keyword"
+    attribute name : String, sanitize: :text, es_subfield: "keyword"
 
-    attribute description : String = ""
+    attribute description : String = "", sanitize: :common
 
     attribute api_key_id : String, mass_assignment: false