Skip to content

Commit

Permalink
Merge pull request from GHSA-vr2x-7687-h6qv
Browse files Browse the repository at this point in the history
  • Loading branch information
@soyuka
soyuka committed Feb 27, 2023
1 parent 47e9d55 commit dcc4733
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 62 deletions.
9 changes: 9 additions & 0 deletions features/authorization/deny.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,12 @@ Feature: Authorization checking
Then the response status code should be 200
And the response should contain "ownerOnlyProperty"
And the JSON node "ownerOnlyProperty" should be equal to the string "updated"

Scenario: A user retrieves a resource with an admin only viewable property
When I add "Accept" header equal to "application/json"
And I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg=="
And I send a "GET" request to "/secured_dummies"
Then the response status code should be 200
And the response should contain "ownerOnlyProperty"


3 changes: 1 addition & 2 deletions src/Hal/Serializer/CollectionNormalizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ protected function getPaginationData(iterable $object, array $context = []): arr
[$paginator, $paginated, $currentPage, $itemsPerPage, $lastPage, $pageTotalItems, $totalItems] = $this->getPaginationConfig($object, $context);
$parsed = IriHelper::parseIri($context['uri'] ?? '/', $this->pageParameterName);

$metadata = $this->resourceMetadataFactory->create($context['resource_class'] ?? '');
$operation = $metadata->getOperation($context['operation_name'] ?? null);
$operation = $context['operation'] ?? $this->getOperation($context);
$urlGenerationStrategy = $operation->getUrlGenerationStrategy();

$data = [
Expand Down
80 changes: 25 additions & 55 deletions src/Hydra/Serializer/CollectionNormalizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,68 +19,68 @@
use ApiPlatform\JsonLd\ContextBuilderInterface;
use ApiPlatform\JsonLd\Serializer\JsonLdContextTrait;
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
use ApiPlatform\Serializer\ContextTrait;
use ApiPlatform\Serializer\AbstractCollectionNormalizer;
use ApiPlatform\State\Pagination\PaginatorInterface;
use ApiPlatform\State\Pagination\PartialPaginatorInterface;
use Symfony\Component\Serializer\Normalizer\CacheableSupportsMethodInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareTrait;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
use Symfony\Component\Serializer\Serializer;

/**
* This normalizer handles collections.
*
* @author Kevin Dunglas <dunglas@gmail.com>
* @author Samuel ROZE <samuel.roze@gmail.com>
*/
final class CollectionNormalizer implements NormalizerInterface, NormalizerAwareInterface, CacheableSupportsMethodInterface
final class CollectionNormalizer extends AbstractCollectionNormalizer
{
use ContextTrait;
use JsonLdContextTrait;
use NormalizerAwareTrait;

public const FORMAT = 'jsonld';
public const IRI_ONLY = 'iri_only';
private array $defaultContext = [
self::IRI_ONLY => false,
];

public function __construct(private readonly ContextBuilderInterface $contextBuilder, private readonly ResourceClassResolverInterface $resourceClassResolver, private readonly IriConverterInterface $iriConverter, private readonly ?ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory = null, array $defaultContext = [])
public function __construct(private readonly ContextBuilderInterface $contextBuilder, ResourceClassResolverInterface $resourceClassResolver, private readonly IriConverterInterface $iriConverter, private readonly ?ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory = null, array $defaultContext = [])
{
$this->defaultContext = array_merge($this->defaultContext, $defaultContext);

if ($this->resourceMetadataCollectionFactory) {
trigger_deprecation('api-platform/core', '3.0', sprintf('Injecting "%s" within "%s" is not needed anymore and this dependency will be removed in 4.0.', ResourceMetadataCollectionFactoryInterface::class, self::class));
}
}

/**
* {@inheritdoc}
*/
public function supportsNormalization(mixed $data, string $format = null, array $context = []): bool
{
return self::FORMAT === $format && is_iterable($data);
parent::__construct($resourceClassResolver, '');
}

/**
* {@inheritdoc}
*
* @param iterable $object
* Gets the pagination data.
*/
public function normalize(mixed $object, string $format = null, array $context = []): array|string|int|float|bool|\ArrayObject|null
protected function getPaginationData(iterable $object, array $context = []): array
{
if (!isset($context['resource_class']) || isset($context['api_sub_level'])) {
return $this->normalizeRawCollection($object, $format, $context);
}

$resourceClass = $this->resourceClassResolver->getResourceClass($object, $context['resource_class']);
$context = $this->initContext($resourceClass, $context);
$context['api_collection_sub_level'] = true;
$data = $this->addJsonLdContext($this->contextBuilder, $resourceClass, $context);
$data['@id'] = $this->iriConverter->getIriFromResource($resourceClass, UrlGeneratorInterface::ABS_PATH, $context['operation'] ?? null, $context);
$data['@type'] = 'hydra:Collection';

if ($object instanceof PaginatorInterface) {
$data['hydra:totalItems'] = $object->getTotalItems();
}

if (\is_array($object) || ($object instanceof \Countable && !$object instanceof PartialPaginatorInterface)) {
$data['hydra:totalItems'] = \count($object);
}

return $data;
}

/**
* Gets items data.
*/
protected function getItemsData(iterable $object, string $format = null, array $context = []): array
{
$data = [];
$data['hydra:member'] = [];

$iriOnly = $context[self::IRI_ONLY] ?? $this->defaultContext[self::IRI_ONLY];

if (($operation = $context['operation'] ?? null) && method_exists($operation, 'getItemUriTemplate')) {
Expand Down Expand Up @@ -108,36 +108,6 @@ public function normalize(mixed $object, string $format = null, array $context =
}
}

if ($object instanceof PaginatorInterface) {
$data['hydra:totalItems'] = $object->getTotalItems();
}

if (\is_array($object) || ($object instanceof \Countable && !$object instanceof PartialPaginatorInterface)) {
$data['hydra:totalItems'] = \count($object);
}

return $data;
}

public function hasCacheableSupportsMethod(): bool
{
return true;
}

/**
* Normalizes a raw collection (not API resources).
*/
protected function normalizeRawCollection(iterable $object, string $format = null, array $context = []): array|\ArrayObject
{
if (\is_array($object) && !$object && ($context[Serializer::EMPTY_ARRAY_AS_OBJECT] ?? false)) {
return new \ArrayObject();
}

$data = [];
foreach ($object as $index => $obj) {
$data[$index] = $this->normalizer->normalize($obj, $format, $context);
}

return $data;
}
}
5 changes: 1 addition & 4 deletions src/JsonApi/Serializer/CollectionNormalizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

use ApiPlatform\Api\ResourceClassResolverInterface;
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
use ApiPlatform\Metadata\Resource\ResourceMetadataCollection;
use ApiPlatform\Serializer\AbstractCollectionNormalizer;
use ApiPlatform\Util\IriHelper;
use Symfony\Component\Serializer\Exception\UnexpectedValueException;
Expand Down Expand Up @@ -44,9 +43,7 @@ protected function getPaginationData($object, array $context = []): array
[$paginator, $paginated, $currentPage, $itemsPerPage, $lastPage, $pageTotalItems, $totalItems] = $this->getPaginationConfig($object, $context);
$parsed = IriHelper::parseIri($context['uri'] ?? '/', $this->pageParameterName);

/** @var ResourceMetadataCollection */
$metadata = $this->resourceMetadataFactory->create($context['resource_class'] ?? '');
$operation = $metadata->getOperation($context['operation_name'] ?? null);
$operation = $context['operation'] ?? $this->getOperation($context);
$urlGenerationStrategy = $operation->getUrlGenerationStrategy();

$data = [
Expand Down
9 changes: 9 additions & 0 deletions src/Serializer/AbstractCollectionNormalizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
namespace ApiPlatform\Serializer;

use ApiPlatform\Api\ResourceClassResolverInterface;
use ApiPlatform\Metadata\Operation;
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
use ApiPlatform\State\Pagination\PaginatorInterface;
use ApiPlatform\State\Pagination\PartialPaginatorInterface;
Expand Down Expand Up @@ -89,6 +90,7 @@ public function normalize(mixed $object, string $format = null, array $context =

unset($context['operation']);
unset($context['operation_type'], $context['operation_name']);

$itemsData = $this->getItemsData($object, $format, $context);

return array_merge_recursive($data, $paginationData, $itemsData);
Expand Down Expand Up @@ -137,6 +139,13 @@ protected function getPaginationConfig(iterable $object, array $context = []): a
return [$paginator, $paginated, $currentPage, $itemsPerPage, $lastPage, $pageTotalItems, $totalItems];
}

protected function getOperation(array $context = []): Operation
{
$metadata = $this->resourceMetadataFactory->create($context['resource_class'] ?? '');

return $metadata->getOperation($context['operation_name'] ?? null);
}

/**
* Gets the pagination data.
*/
Expand Down
6 changes: 6 additions & 0 deletions src/Serializer/AbstractItemNormalizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use ApiPlatform\Exception\InvalidArgumentException;
use ApiPlatform\Exception\ItemNotFoundException;
use ApiPlatform\Metadata\ApiProperty;
use ApiPlatform\Metadata\CollectionOperationInterface;
use ApiPlatform\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
use ApiPlatform\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
Expand Down Expand Up @@ -114,6 +115,11 @@ public function normalize(mixed $object, string $format = null, array $context =
return $this->serializer->normalize($object, $format, $context);
}

if (isset($context['operation']) && $context['operation'] instanceof CollectionOperationInterface) {
unset($context['operation']);
unset($context['iri']);
}

if ($this->resourceClassResolver->isResourceClass($resourceClass)) {
$context = $this->initContext($resourceClass, $context);
}
Expand Down
12 changes: 11 additions & 1 deletion src/Serializer/CacheKeyTrait.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@

namespace ApiPlatform\Serializer;

/**
* Used to override Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::getCacheKey which is private
* We need the cache_key in JsonApi and Hal before it is computed in Symfony.
*
* @see https://github.com/symfony/symfony/blob/49b6ab853d81e941736a1af67845efa3401e7278/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php#L723 which isn't protected
*/
trait CacheKeyTrait
{
private function getCacheKey(?string $format, array $context): string|bool
Expand All @@ -21,10 +27,14 @@ private function getCacheKey(?string $format, array $context): string|bool
unset($context[$key]);
}
unset($context[self::EXCLUDE_FROM_CACHE_KEY]);
unset($context[self::OBJECT_TO_POPULATE]);
unset($context['cache_key']); // avoid artificially different keys

try {
return md5($format.serialize($context));
return hash('xxh128', $format.serialize([
'context' => $context,
'ignored' => $context[self::IGNORED_ATTRIBUTES] ?? $this->defaultContext[self::IGNORED_ATTRIBUTES],
]));
} catch (\Exception) {
// The context cannot be serialized, skip the cache
return false;
Expand Down
5 changes: 5 additions & 0 deletions src/Serializer/SerializerContextBuilder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use ApiPlatform\Util\RequestAttributesExtractor;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Serializer\Encoder\CsvEncoder;
use Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer;

/**
* {@inheritdoc}
Expand Down Expand Up @@ -77,6 +78,10 @@ public function createFromRequest(Request $request, bool $normalization, array $
}
}

// to keep the cache computation smaller, we have "operation_name" and "iri" anyways
$context[AbstractObjectNormalizer::EXCLUDE_FROM_CACHE_KEY][] = 'root_operation';
$context[AbstractObjectNormalizer::EXCLUDE_FROM_CACHE_KEY][] = 'operation';

return $context;
}
}

8 comments on commit dcc4733

@DanielRuf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Toflar @soyuka does this affect only API Platform 2.7.0 and newer?

Because the version range at GHSA-vr2x-7687-h6qv (which says < 2.7.10) may produce false positives since this would theoretically include all versions down to 1.0 / the first version.

Also this line says, that only specific branches are affected:

This issue impacts the 2.7, 3.0 and 3.1 branches. Upgrade to v2.7.10, v3.0.12 or v3.1.3.

https://github.com/api-platform/core/blob/main/CHANGELOG.md#270-alpha1
https://github.com/api-platform/core/releases/tag/v2.7.0

Unfortunately there is no good overview in the documentation, in which version some feature was added:
https://api-platform.com/docs/core/security/
https://api-platform.com/docs/schema-generator/configuration/#adding-a-custom-attribute-or-modifying-a-generated-attribute

@Toflar
Copy link
Contributor

@Toflar Toflar commented on dcc4733 Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to speak for @soyuka or any APIP maintainer so you might want to wait for an official response.
But here's my reasoning: I think < 2.7.10 was used because this is the lowest version currently still being maintained. In other words, nobody cares about the exact lower versions that are affected as these are all unmaintained anyway and might even contain other security vulnerabilites. If you care about your API being protected, you need to update to a maintained version anway :) So why even bother?

@DanielRuf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Toflar because in this case here GitHub as CNA (CVE Numbering Authority) would also set the CPE (Common Platform Enumeration) value to < 2.7.10 so every security scanner would wrongly report this CVE in older versions. Which is basically a false positive. Many do no full research and then have a problem because such false positives lead to wrong assumptions.

A lower end for the ranges (and a correct CPE) is always recommended so security scanners can properly detect and flag this.

If you care about your API being protected, you need to update to a maintained version anway :) So why even bother?

It's more about correct CVE identification. Also regarding unmaintained / outdated versions: in most cases you can tell (by checking the technical details) if a project is really affected by something or if a CVE is mitigated (by design) / poses no actual risk.

Unfortunately it is not the first case that wrong CPE entries derived from GitHub Advisories cause false positives and thus lead to other problems (due to the wrong information).
If we want to do it the right way, then we need exact and correct version ranges with lower limits.

@Toflar
Copy link
Contributor

@Toflar Toflar commented on dcc4733 Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look, the situation presents itself rather straight-forward:
A lower bound is only helpful, if it's precisely the version where a vulnerability was introduced. Otherwise, you would potentially report a given version range as being safe even though it isn't. These false-positives would be much worse than what we have now.

And in order to find out which version was the one introducing the vulnerability, you have two options:

a) either you know exactly
b) or you have to find out

I guess we can safely assume that a) was not the case here, otherwise the lower bound would've been set accordingly.
So we're left with b).
Finding out would mean you have to go back in time and write tests for all those lower versions. The tests might even need to be different in 2.6, 2.5, 2.4 etc. because the code changed, CI changed, tools used changed etc.
It needs a lot of time. Time that people usually are not ready to invest in unmaintained software.
That being said, I'm fairly sure that the report would be adjusted if you want to be the one doing that investigation to find out the precise lower bound.

Anyway, I won't be adding anything else to this discussion. I'm not arguing against the proper lower bound. You asked for a reason and I tried to give you a potential answer to your question because you mentioned me :) As I said, you may want to wait for a response from a maintainer. I can't change anything about it anyway.

@soyuka
Copy link
Member Author

@soyuka soyuka commented on dcc4733 Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @DanielRuf,

Our reasoning is that, even if this issue was introduced in 2.7 (which we aren't sure at 100% as it also depends on the version of some of your vendors), we don't want people to stay on non-maintained versions. It is okay if this issue gets detected on < 2.7.0. Today, it is really hard to try and find exactly when this was introduced as you'd have to also take into consideration the matrix of symfony dependencies this bug relates to. With PHP 7.x not being supported anymore (security-wise) it is a security issue to be on < 2.7. Indeed, 2.7 is the minimal version to have security updates on this project.

Anyways, the feature for security attributes was introduced at 6e3a259 so I can add the > 2.6 constraint if you think it's better?

/edit: boundary for 2.6 added.

@DanielRuf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyways, the feature for security attributes was introduced at 6e3a259 so I can add the > 2.6 constraint if you think it's better?

/edit: boundary for 2.6 added.

Thank you very much. Let's see then if GitHub as CNA updates the CPE entries.

@DanielRuf
Copy link

@DanielRuf DanielRuf commented on dcc4733 Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soyuka one last small question: shouldn't it be >= 2.6.0 as this includes 2.6.0?

6e3a259
https://github.com/api-platform/core/releases/tag/v2.6.0

@soyuka
Copy link
Member Author

@soyuka soyuka commented on dcc4733 Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielRuf fixed thanks

Please sign in to comment.