From 1f1f2645ca670b35b0de10d23ccccebd11013bed Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 12 Oct 2026 14:57:11 +0200 Subject: [PATCH 1/3] fix(metrics): truncate header-derived values and validate location length on metrics enter/leave SummitMetric::build() copied User-Agent, Referer/Origin and Client-IP/X-Forwarded-For unchanged into VARCHAR(255) columns, so a longer value made MySQL reject the INSERT and metrics/enter returned 500. Cut them to 255 characters with mb_substr and limit location to 255 in the enter and leave payloads (412 instead of 500). --- .../OAuth2SummitMetricsApiController.php | 4 +- .../Summit/Metrics/SummitMetric.php | 29 ++++- app/Swagger/SummitMetricsSchemas.php | 4 +- .../OAuth2SummitMetricsApiControllerTest.php | 121 ++++++++++++++++++ 4 files changed, 148 insertions(+), 10 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitMetricsApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitMetricsApiController.php index 1e266e48d5..fabebf22ed 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitMetricsApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitMetricsApiController.php @@ -121,7 +121,7 @@ public function enter($summit_id) [ 'type' => 'required|string|in:' . implode(",", ISummitMetricType::ValidTypes), 'source_id' => 'sometimes|integer', - 'location' => 'sometimes|string', + 'location' => 'sometimes|string|max:255', ] ); @@ -191,7 +191,7 @@ public function leave($summit_id) [ 'type' => 'required|string|in:' . implode(",", ISummitMetricType::ValidTypes), 'source_id' => 'sometimes|integer', - 'location' => 'sometimes|string', + 'location' => 'sometimes|string|max:255', ] ); $metric = $this->service->leave($summit, $current_member, $payload); diff --git a/app/Models/Foundation/Summit/Metrics/SummitMetric.php b/app/Models/Foundation/Summit/Metrics/SummitMetric.php index 1b47aa9b21..3035eb05ef 100644 --- a/app/Models/Foundation/Summit/Metrics/SummitMetric.php +++ b/app/Models/Foundation/Summit/Metrics/SummitMetric.php @@ -29,6 +29,11 @@ class SummitMetric extends SilverstripeBaseModel { use SummitOwned; + /** + * max length (in characters) of the Ip, Origin, Location and Browser VARCHAR columns + */ + const MaxStringColumnLength = 255; + /** * @var \DateTime */ @@ -44,7 +49,7 @@ class SummitMetric extends SilverstripeBaseModel /** * @var string|null */ - #[ORM\Column(name: 'Ip', type: 'string')] + #[ORM\Column(name: 'Ip', type: 'string', length: self::MaxStringColumnLength)] protected $ip; /** @@ -56,7 +61,7 @@ class SummitMetric extends SilverstripeBaseModel /** * @var string|null */ - #[ORM\Column(name: 'Origin', type: 'string')] + #[ORM\Column(name: 'Origin', type: 'string', length: self::MaxStringColumnLength)] protected $origin; /** @@ -68,7 +73,7 @@ class SummitMetric extends SilverstripeBaseModel /** * @var string|null */ - #[ORM\Column(name: 'Browser', type: 'string')] + #[ORM\Column(name: 'Browser', type: 'string', length: self::MaxStringColumnLength)] protected $browser; /** @@ -164,12 +169,24 @@ public static function build(?Member $member = null){ $metric = new static(); $metric->member = $member; $metric->ingress_date = new \DateTime('now', new \DateTimeZone('UTC')); - $metric->ip = UserClientHelper::getUserIp(); - $metric->origin = UserClientHelper::getUserOrigin(); - $metric->browser = UserClientHelper::getUserBrowser(); + $metric->ip = self::truncate(UserClientHelper::getUserIp()); + $metric->origin = self::truncate(UserClientHelper::getUserOrigin()); + $metric->browser = self::truncate(UserClientHelper::getUserBrowser()); return $metric; } + /** + * Header derived values are client controlled, cut them to the column length + * (characters, not bytes) so the INSERT never fails with "Data too long". + * @param string|null $value + * @return string|null + */ + private static function truncate(?string $value): ?string + { + if (is_null($value)) return null; + return mb_substr($value, 0, self::MaxStringColumnLength); + } + /** * @return int */ diff --git a/app/Swagger/SummitMetricsSchemas.php b/app/Swagger/SummitMetricsSchemas.php index 87802dbb36..559c499a5d 100644 --- a/app/Swagger/SummitMetricsSchemas.php +++ b/app/Swagger/SummitMetricsSchemas.php @@ -35,7 +35,7 @@ class SummitMetric properties: [ new OA\Property(property: "type", type: "string", enum: ["GENERAL", "LOBBY", "EVENT", "SPONSOR", "POSTER", "POSTERS", "ROOM"]), new OA\Property(property: "source_id", type: "integer", description: "ID of the source (event, sponsor, room, etc.)"), - new OA\Property(property: "location", type: "string", description: "Location information") + new OA\Property(property: "location", type: "string", maxLength: 255, description: "Location information") ] )] class SummitMetricEnterRequest @@ -49,7 +49,7 @@ class SummitMetricEnterRequest properties: [ new OA\Property(property: "type", type: "string", enum: ["GENERAL", "LOBBY", "EVENT", "SPONSOR", "POSTER", "POSTERS", "ROOM"]), new OA\Property(property: "source_id", type: "integer", description: "ID of the source (event, sponsor, room, etc.)"), - new OA\Property(property: "location", type: "string", description: "Location information") + new OA\Property(property: "location", type: "string", maxLength: 255, description: "Location information") ] )] class SummitMetricLeaveRequest diff --git a/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php b/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php index e0fed16f91..e128a2c179 100644 --- a/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php @@ -21,9 +21,19 @@ class OAuth2SummitMetricsApiControllerTest extends ProtectedApiTestCase use InsertMemberTestData; + /** + * @var array original $_SERVER values overridden by the tests + */ + private $original_server = []; + + private const OverriddenServerKeys = ['HTTP_USER_AGENT', 'HTTP_REFERER', 'HTTP_X_FORWARDED_FOR']; + protected function setUp():void { parent::setUp(); + foreach (self::OverriddenServerKeys as $key) { + $this->original_server[$key] = array_key_exists($key, $_SERVER) ? $_SERVER[$key] : null; + } self::insertMemberTestData(IGroup::TrackChairs); self::$defaultMember = self::$member; self::insertSummitTestData(); @@ -31,6 +41,10 @@ protected function setUp():void protected function tearDown():void { + foreach ($this->original_server as $key => $value) { + if (is_null($value)) unset($_SERVER[$key]); + else $_SERVER[$key] = $value; + } self::clearSummitTestData(); self::clearMemberTestData(); parent::tearDown(); @@ -71,6 +85,113 @@ public function testEnter(){ return $metric; } + private function enterMetric(array $extra = []) + { + $params = [ + 'id' => self::$summit->getId(), + ]; + + $data = array_merge(['type' => \models\summit\ISummitMetricType::General], $extra); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ]; + + return $this->action( + "PUT", + "OAuth2SummitMetricsApiController@enter", + $params, + [], + [], + [], + $headers, + json_encode($data) + ); + } + + public function testEnterTruncatesLongHeaders(){ + $_SERVER['HTTP_USER_AGENT'] = str_repeat('a', 300); + $_SERVER['HTTP_REFERER'] = str_repeat('b', 300); + $_SERVER['HTTP_X_FORWARDED_FOR'] = str_repeat('1', 300); + + $response = $this->enterMetric(); + $this->assertResponseStatus(201); + $metric = json_decode($response->getContent()); + $this->assertEquals(str_repeat('a', 255), $metric->browser); + $this->assertEquals(str_repeat('b', 255), $metric->origin); + $this->assertEquals(str_repeat('1', 255), $metric->ip); + } + + public function testEnterTruncatesMultibyteUserAgent(){ + $ua = str_repeat('é', 300); + $_SERVER['HTTP_USER_AGENT'] = $ua; + + $response = $this->enterMetric(); + $this->assertResponseStatus(201); + $metric = json_decode($response->getContent()); + $this->assertEquals(mb_substr($ua, 0, 255), $metric->browser); + $this->assertEquals(255, mb_strlen($metric->browser)); + $this->assertTrue(mb_check_encoding($metric->browser, 'UTF-8')); + } + + public function testEnterKeepsShortUserAgent(){ + $ua = str_repeat('a', 255); + $_SERVER['HTTP_USER_AGENT'] = $ua; + + $response = $this->enterMetric(); + $this->assertResponseStatus(201); + $metric = json_decode($response->getContent()); + $this->assertEquals($ua, $metric->browser); + } + + public function testEnterLocationTooLong(){ + $this->enterMetric(['location' => str_repeat('l', 256)]); + $this->assertResponseStatus(412); + } + + public function testEnterLocationMaxLength(){ + $this->enterMetric(['location' => str_repeat('l', 255)]); + $this->assertResponseStatus(201); + } + + public function testLeaveLocationLength(){ + $this->enterMetric(); + $this->assertResponseStatus(201); + + $params = [ + 'id' => self::$summit->getId(), + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ]; + + $leave = function (string $location) use ($params, $headers) { + return $this->action( + "POST", + "OAuth2SummitMetricsApiController@leave", + $params, + [], + [], + [], + $headers, + json_encode([ + 'type' => \models\summit\ISummitMetricType::General, + 'location' => $location, + ]) + ); + }; + + // the pending metric is kept, so the 412 can only come from the location validation + $leave(str_repeat('l', 256)); + $this->assertResponseStatus(412); + + $leave(str_repeat('l', 255)); + $this->assertResponseStatus(201); + } + public function testEnterEvent(){ $params = [ From 4af7c1b10ecc91789b9e896c89daaf09d28e51b5 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 19 Oct 2026 17:02:48 +0200 Subject: [PATCH 2/3] fix(metrics): truncate location server-side instead of rejecting it with 412 location is client metadata like the header derived values. Validating the raw URL-encoded value rejected requests that fit the column once decoded. SummitMetric::setLocation() now cuts the decoded value to 255 characters, the max:255 rules and OpenAPI maxLength are reverted, and the tests assert truncation and that an encoded location decoding under 255 is kept. --- .../OAuth2SummitMetricsApiController.php | 4 +- .../Summit/Metrics/SummitMetric.php | 4 +- app/Swagger/SummitMetricsSchemas.php | 4 +- .../OAuth2SummitMetricsApiControllerTest.php | 73 ++++++++++--------- 4 files changed, 43 insertions(+), 42 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitMetricsApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitMetricsApiController.php index fabebf22ed..1e266e48d5 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitMetricsApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitMetricsApiController.php @@ -121,7 +121,7 @@ public function enter($summit_id) [ 'type' => 'required|string|in:' . implode(",", ISummitMetricType::ValidTypes), 'source_id' => 'sometimes|integer', - 'location' => 'sometimes|string|max:255', + 'location' => 'sometimes|string', ] ); @@ -191,7 +191,7 @@ public function leave($summit_id) [ 'type' => 'required|string|in:' . implode(",", ISummitMetricType::ValidTypes), 'source_id' => 'sometimes|integer', - 'location' => 'sometimes|string|max:255', + 'location' => 'sometimes|string', ] ); $metric = $this->service->leave($summit, $current_member, $payload); diff --git a/app/Models/Foundation/Summit/Metrics/SummitMetric.php b/app/Models/Foundation/Summit/Metrics/SummitMetric.php index 3035eb05ef..de5591eacd 100644 --- a/app/Models/Foundation/Summit/Metrics/SummitMetric.php +++ b/app/Models/Foundation/Summit/Metrics/SummitMetric.php @@ -67,7 +67,7 @@ class SummitMetric extends SilverstripeBaseModel /** * @var string|null */ - #[ORM\Column(name: 'Location', type: 'string')] + #[ORM\Column(name: 'Location', type: 'string', length: self::MaxStringColumnLength)] protected $location; /** @@ -266,7 +266,7 @@ public function getLocation(): ?string */ public function setLocation(?string $location): void { - $this->location = $location; + $this->location = self::truncate($location); } } \ No newline at end of file diff --git a/app/Swagger/SummitMetricsSchemas.php b/app/Swagger/SummitMetricsSchemas.php index 559c499a5d..708dcaca3e 100644 --- a/app/Swagger/SummitMetricsSchemas.php +++ b/app/Swagger/SummitMetricsSchemas.php @@ -35,7 +35,7 @@ class SummitMetric properties: [ new OA\Property(property: "type", type: "string", enum: ["GENERAL", "LOBBY", "EVENT", "SPONSOR", "POSTER", "POSTERS", "ROOM"]), new OA\Property(property: "source_id", type: "integer", description: "ID of the source (event, sponsor, room, etc.)"), - new OA\Property(property: "location", type: "string", maxLength: 255, description: "Location information") + new OA\Property(property: "location", type: "string", description: "Location information (truncated to 255 characters server-side)") ] )] class SummitMetricEnterRequest @@ -49,7 +49,7 @@ class SummitMetricEnterRequest properties: [ new OA\Property(property: "type", type: "string", enum: ["GENERAL", "LOBBY", "EVENT", "SPONSOR", "POSTER", "POSTERS", "ROOM"]), new OA\Property(property: "source_id", type: "integer", description: "ID of the source (event, sponsor, room, etc.)"), - new OA\Property(property: "location", type: "string", maxLength: 255, description: "Location information") + new OA\Property(property: "location", type: "string", description: "Location information (truncated to 255 characters server-side)") ] )] class SummitMetricLeaveRequest diff --git a/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php b/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php index e128a2c179..d412f6f6ac 100644 --- a/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php @@ -145,50 +145,51 @@ public function testEnterKeepsShortUserAgent(){ $this->assertEquals($ua, $metric->browser); } - public function testEnterLocationTooLong(){ - $this->enterMetric(['location' => str_repeat('l', 256)]); - $this->assertResponseStatus(412); + private function getStoredLocation(int $metric_id): ?string + { + $em = \LaravelDoctrine\ORM\Facades\Registry::getManager(\models\utils\SilverstripeBaseModel::EntityManager); + $em->clear(); + return $em->find(\models\summit\SummitMetric::class, $metric_id)->getLocation(); } - public function testEnterLocationMaxLength(){ - $this->enterMetric(['location' => str_repeat('l', 255)]); + public function testEnterTruncatesLongLocation(){ + $response = $this->enterMetric(['location' => str_repeat('l', 300)]); $this->assertResponseStatus(201); + $metric = json_decode($response->getContent()); + $this->assertEquals(str_repeat('l', 255), $this->getStoredLocation($metric->id)); } - public function testLeaveLocationLength(){ - $this->enterMetric(); - $this->assertResponseStatus(201); + public function testEnterKeepsEncodedLocationThatDecodesUnderLimit(){ + $location = str_repeat('é', 200); + $encoded = rawurlencode($location); + $this->assertGreaterThan(255, strlen($encoded)); - $params = [ - 'id' => self::$summit->getId(), - ]; + $response = $this->enterMetric(['location' => $encoded]); + $this->assertResponseStatus(201); + $metric = json_decode($response->getContent()); + $this->assertEquals($location, $this->getStoredLocation($metric->id)); + } - $headers = [ - "HTTP_Authorization" => " Bearer " . $this->access_token, - "CONTENT_TYPE" => "application/json", - ]; + public function testLeaveLongLocationIsAccepted(){ + $this->enterMetric(); + $this->assertResponseStatus(201); - $leave = function (string $location) use ($params, $headers) { - return $this->action( - "POST", - "OAuth2SummitMetricsApiController@leave", - $params, - [], - [], - [], - $headers, - json_encode([ - 'type' => \models\summit\ISummitMetricType::General, - 'location' => $location, - ]) - ); - }; - - // the pending metric is kept, so the 412 can only come from the location validation - $leave(str_repeat('l', 256)); - $this->assertResponseStatus(412); - - $leave(str_repeat('l', 255)); + $this->action( + "POST", + "OAuth2SummitMetricsApiController@leave", + ['id' => self::$summit->getId()], + [], + [], + [], + [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ], + json_encode([ + 'type' => \models\summit\ISummitMetricType::General, + 'location' => str_repeat('l', 300), + ]) + ); $this->assertResponseStatus(201); } From afa542e25c5124b9613bb60f8ba027c69f5a7115 Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 19 Oct 2026 17:08:01 +0200 Subject: [PATCH 3/3] fix(metrics): keep location untruncated, its column is TEXT SummitMetric.Location is a TEXT column (Version20201016145706), so long values never caused the insert failure. Revert the location truncation and the VARCHAR(255) mapping; only Ip, Origin and Browser are truncated. --- app/Models/Foundation/Summit/Metrics/SummitMetric.php | 6 +++--- app/Swagger/SummitMetricsSchemas.php | 4 ++-- tests/oauth2/OAuth2SummitMetricsApiControllerTest.php | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/Models/Foundation/Summit/Metrics/SummitMetric.php b/app/Models/Foundation/Summit/Metrics/SummitMetric.php index de5591eacd..c70f8caa60 100644 --- a/app/Models/Foundation/Summit/Metrics/SummitMetric.php +++ b/app/Models/Foundation/Summit/Metrics/SummitMetric.php @@ -30,7 +30,7 @@ class SummitMetric extends SilverstripeBaseModel use SummitOwned; /** - * max length (in characters) of the Ip, Origin, Location and Browser VARCHAR columns + * max length (in characters) of the Ip, Origin and Browser VARCHAR columns */ const MaxStringColumnLength = 255; @@ -67,7 +67,7 @@ class SummitMetric extends SilverstripeBaseModel /** * @var string|null */ - #[ORM\Column(name: 'Location', type: 'string', length: self::MaxStringColumnLength)] + #[ORM\Column(name: 'Location', type: 'string')] protected $location; /** @@ -266,7 +266,7 @@ public function getLocation(): ?string */ public function setLocation(?string $location): void { - $this->location = self::truncate($location); + $this->location = $location; } } \ No newline at end of file diff --git a/app/Swagger/SummitMetricsSchemas.php b/app/Swagger/SummitMetricsSchemas.php index 708dcaca3e..87802dbb36 100644 --- a/app/Swagger/SummitMetricsSchemas.php +++ b/app/Swagger/SummitMetricsSchemas.php @@ -35,7 +35,7 @@ class SummitMetric properties: [ new OA\Property(property: "type", type: "string", enum: ["GENERAL", "LOBBY", "EVENT", "SPONSOR", "POSTER", "POSTERS", "ROOM"]), new OA\Property(property: "source_id", type: "integer", description: "ID of the source (event, sponsor, room, etc.)"), - new OA\Property(property: "location", type: "string", description: "Location information (truncated to 255 characters server-side)") + new OA\Property(property: "location", type: "string", description: "Location information") ] )] class SummitMetricEnterRequest @@ -49,7 +49,7 @@ class SummitMetricEnterRequest properties: [ new OA\Property(property: "type", type: "string", enum: ["GENERAL", "LOBBY", "EVENT", "SPONSOR", "POSTER", "POSTERS", "ROOM"]), new OA\Property(property: "source_id", type: "integer", description: "ID of the source (event, sponsor, room, etc.)"), - new OA\Property(property: "location", type: "string", description: "Location information (truncated to 255 characters server-side)") + new OA\Property(property: "location", type: "string", description: "Location information") ] )] class SummitMetricLeaveRequest diff --git a/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php b/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php index d412f6f6ac..5672c6764d 100644 --- a/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php +++ b/tests/oauth2/OAuth2SummitMetricsApiControllerTest.php @@ -152,14 +152,14 @@ private function getStoredLocation(int $metric_id): ?string return $em->find(\models\summit\SummitMetric::class, $metric_id)->getLocation(); } - public function testEnterTruncatesLongLocation(){ + public function testEnterKeepsLongLocation(){ $response = $this->enterMetric(['location' => str_repeat('l', 300)]); $this->assertResponseStatus(201); $metric = json_decode($response->getContent()); - $this->assertEquals(str_repeat('l', 255), $this->getStoredLocation($metric->id)); + $this->assertEquals(str_repeat('l', 300), $this->getStoredLocation($metric->id)); } - public function testEnterKeepsEncodedLocationThatDecodesUnderLimit(){ + public function testEnterStoresDecodedLocation(){ $location = str_repeat('é', 200); $encoded = rawurlencode($location); $this->assertGreaterThan(255, strlen($encoded));