Problem/Motivation
Since SA-2017-002 it is no longer possible to manually set the ID for newly created entities.
Proposed resolution
Allow the ID to be set for newly created entities with string IDs.
Entities that do not use string IDs have further problems to overcome, see #31 and they will be handled in a different issue.
Remaining tasks
Review.
User interface changes
Nope.
API changes
Nope.
Data model changes
It is now possible to set the ID for newly created entities with string IDs.
Original issue summary
Problem/Motivation
I have custom entities in my project. With Drupal 8.2.6 I was able to make a POST through REST API. With Drupal 8.3.2 & 8.3.3 I have a problem with the field ID.
I need to be able to add the ID by myself. But the field ID should be optional in my REST call. That was the possible with Drupal 8.2.6.
In Drupal 8.3.2 & 8.3.3 I can do a POST only if the body doesn't provide field ID inside.
Is there any changes regarding user access and field ID since version 8.2.6?
Examples
This call is working only in Drupal 8.2.6
{
"id": [{"value":3}],
"name":[{"value":"Test name"}]
}
This call is working for all versions
{
"name":[{"value":"Test name"}]
}
Comment | File | Size | Author |
---|---|---|---|
#103 | 2885469-103.patch | 23.65 KB | Wim Leers |
#103 | interdiff.txt | 3.16 KB | Wim Leers |
#101 | interdiff-101.txt | 53.16 KB | amateescu |
#101 | 2885469-101.patch | 23.05 KB | amateescu |
#94 | 2885469-94.patch | 70.26 KB | amateescu |
Comments
Comment #2
remram CreditAttribution: remram commentedWith version 8.3.3 I'm getting the same message:
403 Forbidden
Comment #3
remram CreditAttribution: remram commentedComment #4
remram CreditAttribution: remram commentedComment #5
remram CreditAttribution: remram commentedThis check in class EntityAccessControlHandler and method fieldAccess is causing the problem:
I also found the security advisor ticket of this change:
Drupal Core - Critical - Access Bypass - SA-CORE-2017-002
Here is the commit on GitHub: SA-CORE-2017-002 by alexpott, xjm, larowlan, Wim Leers, samuel.morten
Is there any solution to my use case?
Comment #6
Wim LeersYou're right, this was a security hardening.
I want to make sure I fully understand your use case though. Why do you want to specify the numeric ID? This is normally auto-generated by the database; why do you want to be able to specify it when creating entities of your custom content entity type?
Comment #7
Wim LeersComment #8
tstoecklerExplicitly setting the ID during creation is an explicit feature of the Entity API and is why we converted everything that in D7 did
!empty($node->nid)
to$entity->isNew()
. For entities with integer IDs the use-case is a bit harder to justify, although it can be needed when doing migrations. But we also support (content) entities with string IDs and there it's quite necessary to set the ID explicitly.Luckily the code that is responsible already allows the UUID to be set initially, so I just moved this code around to include the ID as well.
Comment #9
Wim LeersOh, nice!
Thanks, @tstoeckler!
Comment #10
Wim LeersNote that I also referenced this issue on the (private) security.drupal.org issue tracker. I think that's how @tstoeckler found this so quickly/suddenly :)
I didn't know whether this was an intentional feature, but @tstoeckler is an Entity API maintainer and he says it is, therefore I felt it was okay to RTBC #8 :)
Comment #12
tstoecklerThis should be green.
Comment #13
remram CreditAttribution: remram commentedHi Guys and thank you for the response :)
Well, I have the current case: We need to import existing data from the old system to the Drupal project. The requirement, that I have, I have to import the old IDs from the old system. New records should take the next ID by using the default functionality from the database (auto increment).
Comment #14
remram CreditAttribution: remram commentedThank you guys. REST is working pretty nice with this patch :) great job
Comment #15
Wim LeersThis is now only testing the "update" case. Should this not also be testing the "create" case? Otherwise we may still regress.
Comment #16
Wim LeersComment #17
Wim LeersPinged @tstoeckler via IRC.
Comment #18
tstoecklerYes, #15 is correct, nicely spotted.
UserAccessControlHandlerTest
is a bit weird, though. And I'm not sure when I will get to it. So unassigning for now in the hopes that someone else may pick this up.Comment #19
tstoecklerOh, FWIW, I haven't been on IRC for ages and I don't think that's changing soon...
Comment #20
Wim LeersAnd actually, changes in
EntityAccessControlHandler
should not have test coverage inUserAccessControlhandlerTest
. We have\Drupal\KernelTests\Core\Entity\EntityAccessControlHandlerTest
for that. Perhaps we want to add a unit test for that though.Comment #21
Wim LeersPinged @tstoeckler: https://twitter.com/wimleers/status/892760208400887808.
Comment #23
Wim LeersClosely related: #2905594: Missing entity validation constraint: don't allow new entities when there is an existing one with the same ID.
Comment #24
Wim LeersComment #25
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI also stumbled upon this regression while working on the Workspaces module, which will be the first entity type with a string ID provided by core. I had a similar fix in the (related) patch from #2685749-10: Add a 'machine_name' widget for string field types with a UniqueField constraint, but I'm glad I searched before opening a new issue because @tstoeckler's code fix looks more clear than what I did.
Here's some proper test coverage for this regression.
Comment #27
tstoecklerThanks for the test, it looks great! One minor question:
Any reason you are not checking for the ID?
Comment #28
Wim Leers@amateescu++
I wrote this in #20:
@amateescu did that. Does that mean we want to revert the changes made in
UserAccessControlhandlerTest
?Comment #29
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNo good reason, but you're right, we should try to set and check the ID explicitly as well.
Nope, those changes are needed in order to make the test pass, see the failures from #8.
Comment #30
Wim LeersI think this is ready now :)
Comment #31
alexpottThis does kind of open an interesting scenario. If you can set integer IDs yourself you can easily ensure that Drupal can no longer create any entities of that type (if it is a serial field) by setting the ID to 4294967295.
I think we should only allow IDs via REST to be set when they are not auto-increment.
Comment #32
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@alexpott, that distinction seems a bit arbitrary to me and it would break at least the use case for which this issue was opened, see a detailed explanation of it in #13.
Also, what you describe was already possible before https://www.drupal.org/forum/newsletters/security-advisories-for-drupal-... , so we should maybe open a new issue to discuss it?
Comment #33
alexpott@amateescu well the migrate system existing for #13. I don't think it is beyond the realm of possibility for a site to allow anonymous comments and have REST enabled for commenting. This change going in as is makes me uneasy because of the fragility it introduces.
I'm not sure what a solution looks like but I think just opening up for stringy IDs to start with is probably a better way to go.
Comment #34
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #35
alexpottAdding more test coverage since the whole serial vs string thing was my point.
@amateescu thanks for implementing - the tests prove it works nicely.
Comment #37
alexpottOops
Comment #38
dawehnerI'm wondering whether this should be rather a check for config entities given they have a different semantic than content entities.
Comment #39
Wim Leers#38++
Comment #40
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#38/#39: not sure what you mean, can you elaborate a bit?
Comment #41
Wim Leers@amateescu: rather than reading the type of the
id
field (string or int), check if$entity instanceof ConfigEntityInterface
.Comment #42
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThen I'm not sure what that comment has to do with this issue. This is about a string ID field for content entities, like the title says...
Comment #43
Wim LeersHm, you're right. I think @dawehner and I both jumped to say this because the only entity types with string IDs in core are config entities. So I think you're right, that the patch in #37 is ready as-is.
Pinged @dawehner on IRC.
Comment #44
dawehnerI'm sorry, this makes more sense now. Thank you for the explanation!
Comment #45
xjmThis seems a minimal and fairly reasonable fix. #38 throusg #44, might make sense in a separate issue?
Since this is also a target bugfixes that targets only the regressed cases, this probably makes sense. However, since this issue is a followup to behavior deliberately added in SA-CORE-2017-002, I'd like to review and test a bit further.
Comment #46
xjmComment #47
Wim LeersComment #48
Wim Leers#38 through #44 does not need a follow-up, see #42 + #43 + #44.
Comment #49
Wim LeersComment #50
Wim LeersIn fact, I think that it was only un-RTBC'd for needing a follow-up, so back to RTBC now.
Comment #51
timmillwood+1 for RTBC.
Comment #52
timmillwood#2928215: Write EntityResourceTestBase subclasses for workspace, content_workspace, and replication_log entities depends on this.
Comment #53
larowlanPinged @xjm to clarify if
still applies
Comment #54
xjmSo we were holding back the test for this fix to avoid disclosing the specific exploits. Enough time has elapsed though that we can release the tests now.
They need a reroll but I don't think we should commit this without the coverage in the tests.
Comment #55
timmillwoodHere is a reroll off the sa-test.patch from #54, and also combined with the patch from #37. There were some conflicts between the two patches, so hopefully I resolved them successfully.
Comment #57
timmillwoodNot really sure what's going on with the EntityResourceTest breaks, but this patch fixes the EntityAccessControlHandlerTest failures.
Comment #59
timmillwoodLet's try to fix some of these failing tests.
Comment #60
BerdirIMHO, for new entities, the ID can always be set unless some custom access denies that. We only care about existing entities, where again IMHO, the ID can never be changed, that is not supported by the storage.
The initial example in OP uses an integer, not a string and there are valid reasons for supporting that.
Comment #61
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Berdir, try to convince @alexpott of that, see #31 - #34.
Comment #62
BerdirI see, missed that argument. I can see how that could be a problem and hard to protect against that.
Maybe there is a way to make it easier to override if you have that use case, without having to duplicate the whole method? Instead of returning a forbidden early, we could change the default from an allowed to a neutral in that case, so someone would need to explicitly allow access in checkFieldAccess() or a hook?
Or the implementation of it could be in checkFieldAccess(), then you can decide to not call that, but then the protection relies on all implementations calling the parent.
Comment #64
timmillwoodFixing some more test (and coding standards)
Comment #66
timmillwoodUser entities don't like random strings for the label field, so lets use random machine names.
Message entities don't have storage, so are allowed duplicate UUIDs, so lets not test that.
Comment #67
Wim Leers#54: thanks for providing that patch, @xjm!
Note that @alexpott wrote the test coverage in #54, so he should be credited. I reviewed it, as did @tstoeckler and @samuel.mortenson, so all of them should also be credited.
Comment #69
larowlanAdded those credits
Comment #70
Wim LeersReviewed, but only the changes made to the security test coverage from #54, i.e. I reviewed how it was merged, in extreme detail.
Let's use
::class
.Also: two spaces, should be one.
Also: strict inequality.
We shouldn't need this. #59 made this change, where it also reverted a few other changes (the lines starting with
$created_entity =…
and$location = …
).The reason you ended up needing to resolve the conflict this way is because this new test coverage was being added below the BC test coverage that #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available added in May 2017, but the security test coverage in #54 was created in April.
If we simply move the security test coverage to run before the BC test coverage (which is a best practice anyway), then the problem disappears :)
The only thing we then need to do is to delete the newly created entity, so that the BC layer test coverage can still run.
(I realize the
createAnotherEntity()
call was also in the patch in #54, but in a slightly different place. That itself was wrong too, it was introduced in an incorrect reroll in https://security.drupal.org/node/161938, comment 132, which I realize most of you can't see.)#64 added this, but I'd really like to avoid it.
I understand this is due to
class FeedUrlConstraint extends UniqueFieldConstraint {…}
.We can fix this in a generic way by adding a new optional parameter to
\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getNormalizedPostEntity()
.(In doing so, I noticed that
FeedResourceTestBase
extendsEntityTestResourceTestBase
instead ofEntityResourceTestBase
. I had to fix that too due to the addition of this new optional parameter.)Addressed all my remarks.
Comment #72
Wim LeersUgh, adding new optional parameters to base classes requires all subclasses to also be updated. I didn't realize that. That's annoying…
Comment #74
Wim LeersReviewed everything:
One
\n
too many. I introduced this in my previous patch, sorry ☺️This should use
$this->entityStorage
.This is very similar to
makeNormalizationInvalid()
. It's better to make that existing method a bit more powerful than adding yet another method.(This method also has a fairly strange name, so refactoring it away would solve that too.)
This could be done in a separate issue, but that's probably not worth the overhead.
Missing inheritdoc.
This comment is c/p'ed from
\Drupal\Tests\rest\Functional\EntityResource\User\UserResourceTestBase::testPatchDxForSecuritySensitiveBaseFields()
, but here we're not trying to modify ourselves, we're trying to modify another user.This doesn't run for anonymous users, yet that last line does take that into account. That last line can be simplified.
Indentation is off.
Addressed all my remarks.
Comment #77
Wim LeersOne small oversight.
Comment #79
Wim LeersComment #80
Berdirwe don't do alignment of = and it's different alignments anyway, should we remove the double space on the line we're touching
We're basically adding test coverage for a bug here, which is IMHO a bit weird, as it's not directly tied to this issue, is it?
Don't we have an issue to add uuid validation to make this a validation error instead of an exception.
why not use loadEntityByUuid()?
the $key variable is named fairly strange and not documented. Took me a while, but I guess it refers to an *entity key*, then lets name it $entity_key. I also don't really like "$key()", it's pretty random that we happen to have the same name for the keys and the method names, maybe go with $normalization[$key_field] = $this->anotherEntity->get($key_field)->toValue().
if the user was already loaded on this page then it might be in the static cache, to be extra sure, it makes sense to call resetCache() first.
not sure what our rule on documenting provider methods is.
Comment #81
Wim LeersThanks for the review!
loadEntityByUuid()
doesn't exist on\Drupal\Core\Entity\EntityStorageInterface
. It only exists on the separateentity.repository
service.loadUnchanged()
instead.Comment #83
Wim Leers#81's interdiff is obviously the wrong one. Here's the right one.
Comment #84
Wim LeersThe problem was indeed in #81's interdiff (now posted at #83). Fixed.
Comment #85
Berdir2. Right, I remember. Should we have a follow-up then to use the Unique validation constraint on uuid fields and reference that so we can eventually replace that assert-that-php-died-with-a-500-exception with a validation error?
3. I know, but why not use that service then?
4. You renamed it to $field_name now, but that's not correctl. 'label' is not a field name. It is an $entity_key, then we use the entity key info to get the actual field name.
Comment #86
Wim Leers\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase
only has one service injected for "entity" REST tests:\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::$entityStorage
. Why add more?Comment #87
Wim LeersTitle nits.
Comment #88
Berdir2. #2932009: Add a Unique constraint to uuid fields
3. Well, the point of entity.repository/that method is to simplify the code when loading an entity by uuid, it would just look like this:
Also, technically, $this->entityStorage isn't "injected", just indirectly accessed through a property that is set in the constructor. We can't actually inject anything in test classes, nor do I think it is necessary (we don't test tests). But different people have very different opinions (and strong ones) on that stuff: #2066993: Use \Drupal consistently in tests
This still leaves us with #60/62 IMHO, at the very least we need to update the issue description (because the use case explained there is *not* fixed with this patch) and have a change record that explains what exactly we allow now and provide hints on how to change that if really desired by a custom entity type, including mentioning the possible problems with it? Or maybe explore my ideas in #62, not sure.
Comment #89
Wim LeersI don't have an informed opinion about that … assigning to @alexpott for that, per #61.
Comment #90
alexpottIf the field is an integer auto-increment we shouldn't allow rest to set the ID. As explained in #31 this opens a really simple way to break a site. REST needs to treat all incoming data as untrusted.
Comment #91
Berdir@alexpott: "If the field is an integer auto-increment" is interesting. We always make int fields auto-increment in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::processIdentifierSchema() which for example user overwrites in \Drupal\user\UserStorageSchema::processIdentifierSchema() (Although user is interesting because it would still break)
But I'm fine to look into that in another issue and add an explicit way of opting-out of auto-increment that we could also check in regards to access then.
But setting to needs work then for a change record and issue summary updates to document what exactly we allow.
Comment #92
alexpottSo looking here we are still denying access on create for integer IDs as far as I can see - regardless of auto-create so my security concerns are solved whilst this restriction is in place. Nice.
Should we be passing on $which? (There are quite a few instances when we don't).
$which
has to be one of the oddest variable names around.Comment #93
Wim Leers#92:
This also needed a reroll for #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata anyway (note the changes to
\Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestTextItemNormalizerTest
, which that issue added).Comment #94
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis patch needed a reroll after #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.
Comment #95
Wim LeersThanks! 👍
At this point, so many of us touched this patch. Who is still able to RTBC this?
Comment #96
BerdirI think I can once we have the issue summary update + change record :)
Comment #97
alexpottThis the only use of $which I can find. So it's not really being used to indicate a set of entity field values - it's being used as modifier of field values. To ensure uniqueness. So $unique_value_modifier might be better? Or something along those lines.
Comment #98
Wim Leers#97: But "unique value modifier" is highly specific to this one use case. In this case, that
$which
value can indeed be used as a modifier. But for other use cases, that may not be possible. The general case looks like this:Right now, this is only being applied to
FeedResourceTestBase
, for theFeed
entity type. But there's quite a few fields lacking validation constraints, it's likely more cases of this will be encountered in the future.That is being used in the great test coverage you (Alex Pott) added (see #54, and which didn't take the
Feed
case into account yet, because its test coverage didn't exist yet at the time:So I think
$unique_value_modifier
is actually more confusing? Maybe that's just me though. I provided some extra detail in this comment to hopefully help land on the best possible solution.Comment #99
Wim LeersJust after posting #98, I realized something: this whole
$which
business is necessary because we're trying to reuse$this->getNormalizedPostEntity()
toPOST
an entity which already has been created, just to test duplicate UUID behavior.But … #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior already added logic to deal with something quite similar:
\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getModifiedEntityForPatchTesting()
is able to take an entity and replace values of fields we know to be problematic and replace them with random values. What if we do something similar here?What if we add
protected static $uniqueFieldNames
(similar toprotected static $patchProtectedFieldNames;
) and in the test coverage quoted in #98 iterate over those unique field names and generate random values for those? Wouldn't that be much simpler?Comment #100
alexpott@Wim Leers #99 that sounds great.
Comment #101
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedImplemented #99, re-wrote the issue summary and added a change record: https://www.drupal.org/node/2934705
Comment #102
Berdir$this->entity->getFieldDefinition($field_name) is a bit cleaner than this I'd say?
Would it make sense to do a $entity_type = $this->entity->getEntityType(); above, could make especially the line with the two calls quite a bit shorter and easier to read?
Is this a typo or done on purpose? :)
Nice how much shorter this got :)
Issue summary and CR looks pretty good to me.
> A solution for entity types with numeric (auto-incremented) IDs does not exist yet.
Well, there is a solution, that's to completely override the fieldAccess() method and do whatever you want. Not sure how much of that we want to mention, considering the possible security issues with that.
Comment #103
Wim Leers#101: 🎉👏 Such a short comment, such a big interdiff, much smaller patch — so much better!
All points in #102 are great nits. Since this is blocking important Workflow Initiative patches, I just fixed those nits for @amateescu :)
Comment #104
alexpott@amateescu++
@Wim Leers++
lovely to see this patch become way smaller.
Comment #105
alexpottAdding credits for reviews and reviews on the related security issue.
Comment #106
alexpottCommitted and pushed 4fbf997792 to 8.6.x and dafa5f1176 to 8.5.x. Thanks!
Comment #107
Wim LeersNot a highlight of this release, but worth mentioning in the release notes.