diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 2eafddb4be..199c8ffeb5 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -82,6 +82,7 @@ jobs: - { name: "SpeakerSubmitterPublishedFilter", filter: "tests/ProcessSpeakersEmailRequestJobTest.php tests/ProcessSubmittersEmailRequestJobTest.php tests/SpeakerRepositoryTest.php tests/SubmitterRepositoryTest.php tests/SpeakerServiceOriginalFilterTest.php tests/SubmitterServiceOriginalFilterTest.php" } - { name: "SpeakerBulkEmailChunkResume", filter: "tests/SpeakerServiceBulkSendChunkingTest.php tests/SpeakerServiceResumeSendEmailsTest.php tests/ProcessSpeakersEmailRequestJobResumeTest.php tests/ProcessSpeakersEmailRequestJobFailedHookTest.php" } - { name: "AttendeeBulkEmailChunkResume", filter: "tests/AttendeeServiceBulkSendChunkingTest.php tests/AttendeeServiceResumeSendEmailsTest.php tests/ProcessAttendeesEmailRequestJobResumeTest.php tests/ProcessAttendeesEmailRequestJobFailedHookTest.php tests/SummitAttendeeAnnouncementEmailTest.php tests/DoctrineSummitAttendeeRepositoryTest.php tests/SummitExtraQuestionsMemoizationTest.php" } + - { name: "FilterParserEncoding", filter: "tests/FilterParserEncodingTest.php" } - { name: "SummitJsonGenerator", filter: "tests/SummitJsonGeneratorCommandTest.php" } - { name: "Repositories", filter: "tests/Repositories/" } - { name: "Services", filter: "tests/Unit/Services/" } diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php index c0c2a5c12a..419d54c60c 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php @@ -615,102 +615,105 @@ public function getAttendeesBySummit($summit_id) 'has_manager' => ['=='], ]; - if (Request::has('filter')) { - $filter = FilterParser::parse(Request::get('filter'), $filterRules); - } + // the pre-parse below must also run inside processRequest, so a FilterParserException answers 412 + return $this->processRequest(function () use ($summit, $filterRules, $filter) { + if (Request::has('filter')) { + $filter = FilterParser::parse(Request::get('filter'), $filterRules); + } - if (is_null($filter)) $filter = new Filter(); + if (is_null($filter)) $filter = new Filter(); - $params = []; + $params = []; - if(!is_null($filter)) { - $votingDateFilter = $filter->getFilter('presentation_votes_date'); - if ($votingDateFilter != null) { - $params['begin_attendee_voting_period_date'] = $votingDateFilter[0]->getValue(); - if (count($votingDateFilter) > 1) { - $params['end_attendee_voting_period_date'] = $votingDateFilter[1]->getValue(); + if(!is_null($filter)) { + $votingDateFilter = $filter->getFilter('presentation_votes_date'); + if ($votingDateFilter != null) { + $params['begin_attendee_voting_period_date'] = $votingDateFilter[0]->getValue(); + if (count($votingDateFilter) > 1) { + $params['end_attendee_voting_period_date'] = $votingDateFilter[1]->getValue(); + } + } + $trackGroupFilter = $filter->getFilter('presentation_votes_track_group_id'); + if ($trackGroupFilter != null) { + $params['presentation_votes_track_group_id'] = $trackGroupFilter[0]->getValue(); } } - $trackGroupFilter = $filter->getFilter('presentation_votes_track_group_id'); - if ($trackGroupFilter != null) { - $params['presentation_votes_track_group_id'] = $trackGroupFilter[0]->getValue(); - } - } - return $this->_getAll( - function () use($filterRules){ - return $filterRules; - }, - function () { - return [ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'company' => 'sometimes|string', - 'has_company' => ['sometimes', new Boolean()], - 'email' => 'sometimes|string', - 'external_order_id' => 'sometimes|string', - 'external_attendee_id' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'ticket_type' => 'sometimes|string', - 'badge_type' => 'sometimes|string', - 'features' => 'sometimes|string', - 'access_levels' => 'sometimes|string', - 'status' => 'sometimes|string', - 'has_member' => 'sometimes|required|string|in:true,false', - 'has_tickets'=> 'sometimes|required|string|in:true,false', - 'has_virtual_checkin'=> 'sometimes|required|string|in:true,false', - 'has_checkin'=> 'sometimes|required|string|in:true,false', - 'tickets_count' => 'sometimes|integer', - 'presentation_votes_date' => 'sometimes|date_format:U|epoch_seconds', - 'presentation_votes_count' => 'sometimes|integer', - 'presentation_votes_track_group_id' => 'sometimes|integer', - 'ticket_type_id' => 'sometimes|integer', - 'badge_type_id' => 'sometimes|integer', - 'features_id'=> 'sometimes|integer', - 'access_levels_id' => 'sometimes|integer', - 'summit_hall_checked_in_date' => 'sometimes|date_format:U|epoch_seconds', - 'tags' => 'sometimes|string', - 'tags_id' => 'sometimes|integer', - 'notes' => 'sometimes|string', - 'has_notes' => ['sometimes', new Boolean()], - 'has_manager' => ['sometimes', new Boolean()], - ]; - }, - function () { - return [ - 'first_name', - 'last_name', - 'email', - 'full_name', - 'company', - 'id', - 'external_order_id', - 'member_id', - 'status', - 'full_name', - 'presentation_votes_count', - 'summit_hall_checked_in_date', - 'tickets_count', - 'has_notes', - ]; - }, - function ($filter) use ($summit) { - if ($filter instanceof Filter) { - $filter->addFilterCondition(FilterElement::makeEqual('summit_id', $summit->getId())); - } - return $filter; - }, - function () { - return SerializerRegistry::SerializerType_Private; - }, - null, - null, - null, - $params - ); + return $this->_getAll( + function () use($filterRules){ + return $filterRules; + }, + function () { + return [ + 'id' => 'sometimes|integer', + 'not_id' => 'sometimes|integer', + 'first_name' => 'sometimes|string', + 'last_name' => 'sometimes|string', + 'full_name' => 'sometimes|string', + 'company' => 'sometimes|string', + 'has_company' => ['sometimes', new Boolean()], + 'email' => 'sometimes|string', + 'external_order_id' => 'sometimes|string', + 'external_attendee_id' => 'sometimes|string', + 'member_id' => 'sometimes|integer', + 'ticket_type' => 'sometimes|string', + 'badge_type' => 'sometimes|string', + 'features' => 'sometimes|string', + 'access_levels' => 'sometimes|string', + 'status' => 'sometimes|string', + 'has_member' => 'sometimes|required|string|in:true,false', + 'has_tickets'=> 'sometimes|required|string|in:true,false', + 'has_virtual_checkin'=> 'sometimes|required|string|in:true,false', + 'has_checkin'=> 'sometimes|required|string|in:true,false', + 'tickets_count' => 'sometimes|integer', + 'presentation_votes_date' => 'sometimes|date_format:U|epoch_seconds', + 'presentation_votes_count' => 'sometimes|integer', + 'presentation_votes_track_group_id' => 'sometimes|integer', + 'ticket_type_id' => 'sometimes|integer', + 'badge_type_id' => 'sometimes|integer', + 'features_id'=> 'sometimes|integer', + 'access_levels_id' => 'sometimes|integer', + 'summit_hall_checked_in_date' => 'sometimes|date_format:U|epoch_seconds', + 'tags' => 'sometimes|string', + 'tags_id' => 'sometimes|integer', + 'notes' => 'sometimes|string', + 'has_notes' => ['sometimes', new Boolean()], + 'has_manager' => ['sometimes', new Boolean()], + ]; + }, + function () { + return [ + 'first_name', + 'last_name', + 'email', + 'full_name', + 'company', + 'id', + 'external_order_id', + 'member_id', + 'status', + 'full_name', + 'presentation_votes_count', + 'summit_hall_checked_in_date', + 'tickets_count', + 'has_notes', + ]; + }, + function ($filter) use ($summit) { + if ($filter instanceof Filter) { + $filter->addFilterCondition(FilterElement::makeEqual('summit_id', $summit->getId())); + } + return $filter; + }, + function () { + return SerializerRegistry::SerializerType_Private; + }, + null, + null, + null, + $params + );; + }); } #[OA\Get( diff --git a/app/Http/Utils/Filters/FilterParser.php b/app/Http/Utils/Filters/FilterParser.php index 7181775ee7..8b8f8e5eff 100644 --- a/app/Http/Utils/Filters/FilterParser.php +++ b/app/Http/Utils/Filters/FilterParser.php @@ -47,6 +47,10 @@ public static function parse($filters, $allowed_fields = []) foreach ($filters as $filter) // parse Main Filters ( 1st grade ) { + // the and(...)/or(...) regex below is not end-anchored, so bytes after the closing + // parenthesis would be dropped silently instead of reaching filterExpresion + if (!mb_check_encoding($filter, 'UTF-8')) + throw new FilterParserException("Invalid filter format: the filter is not valid UTF-8."); // check main operator $main_op_matches = null; @@ -149,6 +153,12 @@ public static function parse($filters, $allowed_fields = []) public static function filterExpresion(string $exp) { + // the messages below echo the expression, so it has to be valid UTF-8 before anything else + // (JsonResponse cannot encode it otherwise); this also has to run before any preg_match with + // the u modifier, which returns false and not 0 on invalid UTF-8 + if (!mb_check_encoding($exp, 'UTF-8')) + throw new FilterParserException("Invalid filter format: the filter is not valid UTF-8."); + Log::debug(sprintf("FilterParser::filterExpresion %s", $exp)); if (!preg_match('/\[\]|\(\)|>=|<=|<>|==|=\@|\@\@|<|>/i', $exp, $matches)) throw new FilterParserException(sprintf("Invalid filter format %s (should be [:FIELD_NAME:OPERAND:VALUE]).", $exp)); @@ -156,7 +166,17 @@ public static function filterExpresion(string $exp) $op = $matches[0]; $operands = explode($op, $exp, 2); $field = strtolower(trim($operands[0])); + // second urldecode on purpose: a literal + travels as %2B and is encoded again by the HTTP client $value = str_replace(['\\,','\\;'], [',',';'], urldecode($operands[1])); + + // never echo the value: it may not be valid UTF-8 + if (!mb_check_encoding($value, 'UTF-8')) + throw new FilterParserException(sprintf("Filter by field %s has a value that is not valid UTF-8.", $field)); + + // characters outside the BMP (emoji) can not be compared against our utf8mb3/latin1 columns + if (preg_match('/[\x{10000}-\x{10FFFF}]/u', $value)) + throw new FilterParserException(sprintf("Filter by field %s has a value with unsupported characters.", $field)); + Log::debug(sprintf("FilterParser::filterExpresion field %s op %s value %s", $field, $op, json_encode($value))); return [$field, $op, $value]; } diff --git a/tests/FilterParserEncodingTest.php b/tests/FilterParserEncodingTest.php new file mode 100644 index 0000000000..1f006a268c --- /dev/null +++ b/tests/FilterParserEncodingTest.php @@ -0,0 +1,105 @@ + ['=@', '=='], + 'first_name' => ['=@', '=='], + ]; + + private static function parseValue(string $expression): string + { + $filter = FilterParser::parse($expression, self::RULES); + $elements = $filter->getFilter('title'); + return $elements[0]->getValue(); + } + + public function testEncodedPlusIsKept(): void + { + $this->assertSame('C++', self::parseValue('title==C%2B%2B')); + } + + public function testNonAsciiBmpTextIsKept(): void + { + $this->assertSame('José', self::parseValue('title==Jos%C3%A9')); + $this->assertSame('€', self::parseValue('title==%E2%82%AC')); + $this->assertSame('日本', self::parseValue('title==%E6%97%A5%E6%9C%AC')); + } + + public function testInvalidUtf8ValueIsRejected(): void + { + $this->expectException(FilterParserException::class); + FilterParser::parse('title=@%DA', self::RULES); + } + + public function testInvalidUtf8ValueIsRejectedWithEqualOperator(): void + { + $this->expectException(FilterParserException::class); + FilterParser::parse('title==%CA', self::RULES); + } + + public function testEmojiValueIsRejected(): void + { + $this->expectException(FilterParserException::class); + FilterParser::parse('title=@' . "\u{1F600}", self::RULES); + } + + public function testEncodedEmojiValueIsRejected(): void + { + $this->expectException(FilterParserException::class); + FilterParser::parse('title=@%F0%9F%98%80', self::RULES); + } + + public function testInvalidUtf8FieldIsRejectedWithSafeMessage(): void + { + try { + FilterParser::parse("\xDA==x", self::RULES); + $this->fail('FilterParserException expected'); + } catch (FilterParserException $ex) { + $this->assertNotFalse(json_encode(['message' => $ex->getMessage()])); + } + } + + public function testInvalidUtf8ExpressionWithoutOperatorIsRejectedWithSafeMessage(): void + { + try { + FilterParser::parse("\xDA", self::RULES); + $this->fail('FilterParserException expected'); + } catch (FilterParserException $ex) { + $this->assertNotFalse(json_encode(['message' => $ex->getMessage()])); + } + } + + public function testInvalidUtf8OutsideMainOperatorWrapperIsRejected(): void + { + // the and(...) wrapper regex is not end-anchored, so the trailing byte is not part of + // the expression that filterExpresion validates + $this->expectException(FilterParserException::class); + FilterParser::parse("and(title==ok)\xDA", self::RULES); + } +} diff --git a/tests/oauth2/OAuth2AttendeesApiTest.php b/tests/oauth2/OAuth2AttendeesApiTest.php index 59db39ae51..f9cacccc05 100644 --- a/tests/oauth2/OAuth2AttendeesApiTest.php +++ b/tests/oauth2/OAuth2AttendeesApiTest.php @@ -72,6 +72,45 @@ public function testGetAttendees(){ $this->assertTrue(!is_null($attendees)); } + /** + * @param string $filter the filter as PHP hands it over, after its own decode of the query string + */ + private function getAttendeesWithFilter(string $filter) + { + $params = [ + 'id' => self::$summit->getId(), + 'page' => 1, + 'per_page' => 10, + 'filter' => $filter, + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + return $this->action( + "GET", + "OAuth2SummitAttendeesApiController@getAttendeesBySummit", + $params, + [], + [], + [], + $headers + ); + } + + public function testGetAttendeesFilterFieldNotAllowed(){ + $this->getAttendeesWithFilter('nope==x'); + $this->assertResponseStatus(412); + } + + public function testGetAttendeesFilterInvalidUtf8Value(){ + // first_name=@%25DA on the wire + $this->getAttendeesWithFilter('first_name=@%DA'); + $this->assertResponseStatus(412); + } + public function testGetAttendeesCSV(){ $params = [ diff --git a/tests/oauth2/OAuth2MembersApiTest.php b/tests/oauth2/OAuth2MembersApiTest.php index a9353a23f8..8583d58b3a 100644 --- a/tests/oauth2/OAuth2MembersApiTest.php +++ b/tests/oauth2/OAuth2MembersApiTest.php @@ -101,6 +101,59 @@ public function testGetMemberByFullName() $this->assertResponseStatus(200); } + /** + * @param string|array $filter the filter as PHP hands it over, after its own decode of the query string + */ + private function getMembersWithFilter($filter) + { + $headers = ["HTTP_Authorization" => " Bearer " . $this->access_token]; + return $this->action( + "GET", + "OAuth2MembersApiController@getAll", + ['filter' => $filter, 'per_page' => 5], + [], + [], + [], + $headers + ); + } + + public function testGetMembersFilterEncodedPlus() + { + // C++ as openstack-uicore-foundation sends it: %2B, encoded again on the wire + $this->getMembersWithFilter('first_name=@C%2B%2B'); + $this->assertResponseStatus(200); + } + + public function testGetMembersFilterInvalidUtf8Value() + { + // first_name=@%25DA on the wire + $this->getMembersWithFilter('first_name=@%DA'); + $this->assertResponseStatus(412); + } + + public function testGetMembersFilterEmojiValue() + { + $this->getMembersWithFilter("first_name=@\u{1F600}"); + $this->assertResponseStatus(412); + } + + public function testGetMembersFilterInvalidUtf8Field() + { + // filter=%DA==x on the wire + $response = $this->getMembersWithFilter("\xDA==x"); + $this->assertResponseStatus(412); + $this->assertNotNull(json_decode($response->getContent())); + } + + public function testGetMembersFilterInvalidUtf8WithoutOperator() + { + // filter=%DA on the wire + $response = $this->getMembersWithFilter("\xDA"); + $this->assertResponseStatus(412); + $this->assertNotNull(json_decode($response->getContent())); + } + public function testGetMembersEmpty() { diff --git a/tests/oauth2/OAuth2SummitEventsApiTest.php b/tests/oauth2/OAuth2SummitEventsApiTest.php index 7c72067c0e..b65a1e243b 100644 --- a/tests/oauth2/OAuth2SummitEventsApiTest.php +++ b/tests/oauth2/OAuth2SummitEventsApiTest.php @@ -1210,6 +1210,46 @@ public function testCurrentSummitEventsBySummitType() $this->assertTrue(!is_null($events)); } + /** + * @param string $filter the filter as PHP hands it over, after its own decode of the query string + */ + private function getScheduledEventsWithFilter(string $filter) + { + $params = [ + 'id' => self::$summit->getId(), + 'filter' => $filter, + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + return $this->action + ( + "GET", + "OAuth2SummitEventsApiController@getScheduledEvents", + $params, + [], + [], + [], + $headers + ); + } + + public function testGetScheduledEventsFilterInvalidUtf8Value() + { + // title=@%25DA on the wire + $this->getScheduledEventsWithFilter('title=@%DA'); + $this->assertResponseStatus(412); + } + + public function testGetScheduledEventsFilterEmojiValue() + { + $this->getScheduledEventsWithFilter("title=@\u{1F600}"); + $this->assertResponseStatus(412); + } + public function testCurrentSummitPublishedEventsBySummitType() { $this->markTestSkipped('Skipped test: needs review');