From 5e2e1a856eb1e65272706a8cb30f28311cc153f9 Mon Sep 17 00:00:00 2001 From: Stephen von Takach Date: Tue, 20 Oct 2026 11:17:03 +1100 Subject: [PATCH 1/2] fix: unified change history, stateless MCP specs and stale linked bookings Co-Authored-By: Claude Opus 5.5 --- shard.lock | 6 ++-- .../bookings_extension_data_sqli_spec.cr | 2 +- spec/controllers/events_spec.cr | 10 +++--- spec/controllers/helpers/api_spec_helper.cr | 2 +- spec/controllers/history_spec.cr | 31 +++++++++++++++++++ spec/controllers/mcp_spec.cr | 3 +- src/config.cr | 14 ++++----- src/controllers/application.cr | 31 +++++++++++++++++++ src/controllers/events.cr | 11 ++++--- src/controllers/utilities/current-user.cr | 4 +++ 10 files changed, 91 insertions(+), 23 deletions(-) create mode 100644 spec/controllers/history_spec.cr diff --git a/shard.lock b/shard.lock index 2717e242..c931644c 100644 --- a/shard.lock +++ b/shard.lock @@ -11,7 +11,7 @@ shards: active-model: git: https://github.com/spider-gazelle/active-model.git - version: 4.5.0 + version: 4.7.0 ameba: git: https://github.com/veelenga/ameba.git @@ -139,7 +139,7 @@ shards: pg-orm: git: https://github.com/spider-gazelle/pg-orm.git - version: 2.4.1 + version: 2.5.1 place_calendar: git: https://github.com/placeos/calendar.git @@ -151,7 +151,7 @@ shards: placeos-models: git: https://github.com/placeos/models.git - version: 9.119.6 + version: 9.122.1 pool: git: https://github.com/ysbaddaden/pool.git diff --git a/spec/controllers/bookings_extension_data_sqli_spec.cr b/spec/controllers/bookings_extension_data_sqli_spec.cr index 6d9af97e..b5065834 100644 --- a/spec/controllers/bookings_extension_data_sqli_spec.cr +++ b/spec/controllers/bookings_extension_data_sqli_spec.cr @@ -48,7 +48,7 @@ describe "Bookings#index extension_data SQL injection", tags: ["security", "sqli JSON.parse(injected.body).as_a.should be_empty # a classic tautology payload is likewise inert - tautology = client.get("#{BOOKINGS_BASE}?#{period}&extension_data={foo:x%27 OR %271%27=%271}", headers: headers) + tautology = client.get("#{BOOKINGS_BASE}?#{period}&extension_data={foo:x%27%20OR%20%271%27=%271}", headers: headers) tautology.status_code.should eq(200) JSON.parse(tautology.body).as_a.should be_empty end diff --git a/spec/controllers/events_spec.cr b/spec/controllers/events_spec.cr index 518adf1c..05d4a6fc 100644 --- a/spec/controllers/events_spec.cr +++ b/spec/controllers/events_spec.cr @@ -1264,13 +1264,13 @@ describe Events, tags: ["event"] do # Create history records for the event History.create!( - type: "event", + resource_type: "event", resource_id: event.event_id, action: "created", changed_fields: [] of String ) History.create!( - type: "event", + resource_type: "event", resource_id: event.event_id, action: "updated", changed_fields: ["event_start", "event_end"] @@ -1309,8 +1309,8 @@ describe Events, tags: ["event"] do ) # Create history for both events - History.create!(type: "event", resource_id: event1.event_id, action: "created", changed_fields: [] of String) - History.create!(type: "event", resource_id: event2.event_id, action: "created", changed_fields: [] of String) + History.create!(resource_type: "event", resource_id: event1.event_id, action: "created", changed_fields: [] of String) + History.create!(resource_type: "event", resource_id: event2.event_id, action: "created", changed_fields: [] of String) # Query history filtered by system_id response = client.get( @@ -1352,7 +1352,7 @@ describe Events, tags: ["event"] do ) # Create history using ical_uid as resource_id (as might happen with some calendar providers) - History.create!(type: "event", resource_id: event.ical_uid, action: "updated", changed_fields: ["ext_data.notes"]) + History.create!(resource_type: "event", resource_id: event.ical_uid, action: "updated", changed_fields: ["ext_data.notes"]) response = client.get( "#{EVENTS_BASE}/history?period_start=#{event_start - 60}&period_end=#{event_end + 60}", diff --git a/spec/controllers/helpers/api_spec_helper.cr b/spec/controllers/helpers/api_spec_helper.cr index e915f41d..2baf35e3 100644 --- a/spec/controllers/helpers/api_spec_helper.cr +++ b/spec/controllers/helpers/api_spec_helper.cr @@ -319,7 +319,7 @@ end # the shared spec users, so specs that create groups clear them before each example. def clear_group_tables [ - PlaceOS::Model::GroupHistory, + PlaceOS::Model::History, PlaceOS::Model::GroupInvitation, PlaceOS::Model::GroupZone, PlaceOS::Model::GroupUser, diff --git a/spec/controllers/history_spec.cr b/spec/controllers/history_spec.cr new file mode 100644 index 00000000..e6b5a720 --- /dev/null +++ b/spec/controllers/history_spec.cr @@ -0,0 +1,31 @@ +require "../spec_helper" +require "./helpers/booking_helper" + +describe "change history" do + client = AC::SpecHelper.client + + it "attributes changes to the client of the request" do + History.clear + headers = Mock::Headers.office365_guest + headers["X-App-Name"] = "workplace" + headers["X-App-Build"] = "abc123" + headers["User-Agent"] = "Spec Browser" + + starting = 10.minutes.from_now.to_unix + ending = 30.minutes.from_now.to_unix + response = client.post(BOOKINGS_BASE, headers: headers, + body: %({"asset_id":"desk-history","booking_start":#{starting},"booking_end":#{ending},"booking_type":"desk"})) + response.status_code.should eq(201) + booking_id = JSON.parse(response.body)["id"].to_s + + entry = History.for_resource(Booking.table_name, booking_id).to_a.first + entry.action.should eq "create" + metadata = entry.metadata + metadata.app_name.should eq "workplace" + metadata.app_build.should eq "abc123" + metadata.user_agent.should eq "Spec Browser" + metadata.request_id.should eq response.headers["X-Request-ID"] + + History.current_metadata.should be_nil + end +end diff --git a/spec/controllers/mcp_spec.cr b/spec/controllers/mcp_spec.cr index f686463f..2b88cfd1 100644 --- a/spec/controllers/mcp_spec.cr +++ b/spec/controllers/mcp_spec.cr @@ -20,12 +20,11 @@ describe App::MCP do initialize_params = {"protocolVersion" => JSON::Any.new("2025-11-25")} no_params = {} of String => JSON::Any - # establishes a session, returning the headers to use for it + # initializes the stateless server, returning the headers to use for later requests open_session = ->(credentials : HTTP::Headers) { headers = mcp_headers.call(credentials) response = rpc.call(headers, "initialize", initialize_params) response.status_code.should eq 200 - headers["Mcp-Session-Id"] = response.headers["Mcp-Session-Id"] headers } call_tool = ->(headers : HTTP::Headers, name : String, arguments : Hash(String, JSON::Any)) { diff --git a/src/config.cr b/src/config.cr index 4188a7df..2d948cce 100644 --- a/src/config.cr +++ b/src/config.cr @@ -27,16 +27,16 @@ require "action-controller/server" # Execution contexts (the Crystal runtime default since 1.21). # The bookings index route can return large payloads, so it runs the whole -# request in a dedicated "bookings" context. Unbound routes offload their -# response serialisation to the shared response context. +# request in a dedicated "bookings" context. +# +# Responses of unbound routes are not offloaded: transaction-wrapped routes +# (booking and event create / update) serialise their response before the +# transaction commits, and an offloaded fiber reads on another connection that +# can't see those writes. `offload_responses` is a compile time macro, so it +# can't be toggled by an environment variable. ActionController::ExecutionContext.define "bookings" ActionController::ExecutionContext.parallelism "bookings", ENV["BOOKINGS_WORKERS"]?.try(&.to_i) || 4 -if (response_parallelism = ENV["OFFLOAD_WORKERS"]?.try(&.to_i)) && response_parallelism > 0 - ActionController::ExecutionContext.offload_responses - ActionController::ExecutionContext.response_parallelism = response_parallelism -end - # Filter out sensitive params that shouldn't be logged filter_params = ["password", "bearer_token"] keeps_headers = ["X-Request-ID"] diff --git a/src/controllers/application.cr b/src/controllers/application.cr index f4ae22bc..bea7d5d0 100644 --- a/src/controllers/application.cr +++ b/src/controllers/application.cr @@ -68,6 +68,37 @@ abstract class Application < ActionController::Base response.headers["X-Request-ID"] = request_id end + # Attributes model changes made during the request in their change + # history. The metadata is only built when a change is recorded, after + # the request has been authenticated. + @[AC::Route::Filter(:around_action)] + def with_history_metadata(&) + provider = ::PlaceOS::Model::History::MetadataProvider.new { history_metadata(request, @current_user) } + ::PlaceOS::Model::History.with_metadata(provider) { yield } + end + + # Who made a change and from where, see `PlaceOS::Model::History::Metadata` + def history_metadata(request : HTTP::Request, current_user : ::PlaceOS::Model::User?) : ::PlaceOS::Model::History::Metadata + headers = request.headers + metadata = ::PlaceOS::Model::History::Metadata.new( + api_key_id: api_key.try(&.id), + api_key_name: api_key.try(&.name), + app_id: history_header(headers, "X-App-Id"), + app_name: history_header(headers, "X-App-Name"), + app_date: history_header(headers, "X-App-Date"), + app_build: history_header(headers, "X-App-Build"), + user_agent: history_header(headers, "User-Agent"), + ip: client_ip, + request_id: @request_id, + ) + current_user ? metadata.with_user(current_user) : metadata + end + + # Client supplied header, limited in length as it is stored with each change + private def history_header(headers : HTTP::Headers, name : String) : String? + headers[name]?.presence.try(&.[0, 256]) + end + # ============================ # JWT Scope Check # ============================ diff --git a/src/controllers/events.cr b/src/controllers/events.cr index ea2532be..da59ab53 100644 --- a/src/controllers/events.cr +++ b/src/controllers/events.cr @@ -398,8 +398,11 @@ class Events < Application # Collect event IDs from metadata event_ids = metadatas.flat_map { |meta| [meta.event_id, meta.ical_uid] }.uniq! - # Query history for these event IDs - History.where({:type => "event", :resource_id => event_ids}).to_a + # Query history of these events and their metadata, newest first + History + .where(resource_type: ["event", EventMetadata.table_name], resource_id: event_ids) + .order(created_at: :desc, id: :desc) + .to_a end protected def can_create?(user_email : String, host_email : String, attendees : Array(String)) : Bool @@ -1525,8 +1528,8 @@ class Events < Application end private def record_event_history(event_id : String, action : String, changed_fields : Array(String) = [] of String) - History.create!( - type: "event", + History.record_change( + resource_type: "event", resource_id: event_id, action: action, changed_fields: changed_fields diff --git a/src/controllers/utilities/current-user.cr b/src/controllers/utilities/current-user.cr index 9a9d7f01..603ab373 100644 --- a/src/controllers/utilities/current-user.cr +++ b/src/controllers/utilities/current-user.cr @@ -10,6 +10,9 @@ module Utils::CurrentUser @user_token : ::UserJWT? + # The API key used to authenticate the request, if any + getter api_key : ::PlaceOS::Model::ApiKey? = nil + # Parses, and validates JWT if present. # Throws Error::MissingBearer and JWT::Error. def authorize! @@ -24,6 +27,7 @@ module Utils::CurrentUser raise Error::Unauthorized.new "API key has expired" if api_key.expired? user_token = api_key.build_jwt Log.context.set(api_key_id: api_key.id, api_key_name: api_key.name) + @api_key = api_key @current_user = ::PlaceOS::Model::User.find(user_token.id) return @user_token = user_token rescue e : Error::Unauthorized From eae8d6b8d2be76e48e794bfcd5a994482cfdfc62 Mon Sep 17 00:00:00 2001 From: Stephen von Takach Date: Tue, 20 Oct 2026 11:20:08 +1100 Subject: [PATCH 2/2] fix(spec): unique asset ids for generated bookings Co-Authored-By: Claude Opus 5.5 --- spec/controllers/helpers/booking_helper.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/helpers/booking_helper.cr b/spec/controllers/helpers/booking_helper.cr index 46a14601..b63c190e 100644 --- a/spec/controllers/helpers/booking_helper.cr +++ b/spec/controllers/helpers/booking_helper.cr @@ -7,7 +7,7 @@ module BookingsHelper def create_booking(tenant_id : Int64, user_email : String, zones : Array(String) = random_zones, event_id = nil, booking_type = "desk", parent_id = nil, asset_id = nil) user_name = Faker::Internet.user_name - asset_id = asset_id.presence || "asset-#{Random.new.rand(500)}" + asset_id = asset_id.presence || "asset-#{UUID.random}" Booking.create!( tenant_id: tenant_id, parent_id: parent_id,