Never serialize password fields by default

I like this. Should we document this somewhere? Also $field = $object->getParent(); is definitely not needed (unless getParent has side effects but I doubt). And actually return NULL; is the same as nothing so an empty function body with a comment will just do :)

How does this overlap with field level permissions? See #2029855: Missing access control for user base fields? Which would then deny view access to the password for everyone?

Access happens in EntityResource, not on the serializer, so this will obviously fail yes, but does it also fail when accessing through GET user/1 ?

I guess we need to do something like this?

Not sure what we need for DataFieldRow instead, will need a test.

I'm not sure, but if it's implemented in the serializer, then I think it should be in ContentEntityNormalizer::normalize()/denormalize(), there is no need for a custom password normalizer, all we need is the view/edit access permission?

Note that even if we decide that password is so special to get a custom serializer that never prints out the data, we still need to solve the general access problem. The same view will also display the user e-mail and all other data that is private/protected.

Adding access to the serializer means that it suddenly needs to have a concept of a user, so that you can serialize in the context of a given user. And forcing access checks there will likely mess up the existing kernel tests that have no current user :)

Fixing tags.

Yes, I agree with berdir I think (or at least favour berdirs side of the argument against this). I would not really like to see this kind of coupling to parts of our Serializer, if possible.

Ok, in that case, I think the issue should be moved to rest.module, also updated the issue title a bit. Issue summary should be updated to explain the remaining problem and possible solutions.

I was able to reproduce this on a fresh 8.0.x update. I had filed the issue originally as 8.0.0-beta2.

I think that serialization or rest module should handle this. I don't think we need a secure string type, as we can remove the property during normalization, which would save an API change.

- Added steps to reproduce.

Um. Nope.

Edit: I will write more when I can write words fitting polite conversation. You are free to read #2364647: [sechole] Remove blacklist mode from Filter:XSS to understand why I can't.

I agree this should be on serialization module, because else you could still somehow get to a serializer that gives you that field.

i.e. custom web service.

And things like that happen, we really want Secure-by-default and fool-proof.

    $normalized = $this->serializer->normalize($account);

As long as its possible to do that, I think that is a problem.

Why is password a text field anyway and not its own subtype of a text field that can be treated differently?

Edit:

The access problem for serialized values is still another problem, but we should split this off to another critical.

I suggested a secure data type in the non-public issue, but wasn't sure if that would fly in beta. I think we could get away with it, but it wouldn't be our best effort.

+1 to making it secure by default.

Split field access issue into #2365319: Entity normalization should check field access to avoid leaking data.

Putting this issue back to the scope of the Issue Summary and re-titling.

RTBC +1 for #1 - no real RTBC though obviously ...

Reasoning as discussed in IRC:

We should never _ever_ serialize password fields IMHO. If you want to really provide the password hash via a Web Service, you will have to write your own Normalizer, which is possible and set a higher priority.

The chances of a password leaking out due to a bug somewhere are too high - even if the user might have access to see such data in the first place.

Manually tested patch #31. JSON output from the rest export view below. Pass becomes null.

[{"uid":[{"value":"1"}],"uuid":[{"value":"034f34f1-9d2f-4836-8600-b274fb7c457b"}],"langcode":[{"value":"en"}],"preferred_langcode":[{"value":"en"}],"preferred_admin_langcode":[{"value":""}],"name":[{"value":"admin"}],"pass":[null],"mail":[{"value":"admin@example.com"}],"signature":[{"value":null}],"signature_format":[{"value":null}],"timezone":[{"value":"America\/New_York"}],"status":[{"value":"1"}],"created":[{"value":"1414532010"}],"changed":[{"value":"1414532132"}],"access":[{"value":"1414552568"}],"login":[{"value":"1414532132"}],"init":[{"value":"admin@example.com"}],"roles":[{"target_id":"authenticated"},{"target_id":"administrator"}],"user_picture":[{"target_id":null,"display":null,"description":null,"alt":null,"title":null,"width":null,"height":null}]}]

Pass null means that it will be submitted and overwritten when sending this back again with PATCH?

How is that supposed to work anyway? (considering the protection we have in the UI where you need to know th the old password)

I don't really get why we shouldn't use field access for this, but Ok...

So #2365319: Entity normalization should check field access to avoid leaking data is the way I would prefer to see this go. I think that makes this issue redundant now?

Returning NULL for a key like that is not really useful. This approach of adding another normalizer is relatively fragile, and not flexible at all. As any override of the normalizer would be a global site-wide thing.

Had a chat with Fabianx. If we are going down this route of enforcing the password never gets shown in normalized data, I would prefer to see this on the entity normalizer level, rather than the field level.

This will give people more flexibility should they want to change this behaviour. As well as just removing the pass key from the normalized data - which IMO, is much better than always having a NULL value.

My reasoning for still wanting a password field that is always excluded from any entities this is added to is:

Custom users might want to have their own account entity or need a password hash somewhere else.

Hardcoding 'pass' is not a good idea IMHO as its a type and such deserves a special type of security.

Btw. is that "field" always loaded on the $account entity? I hope not ...

So does dpm($account) show the ->password hash too? I assume not ...

The reason we are having this discussion is because of the move from internal properties that had been seldom exposed to proper fields.

It is valid to be able to access the pass field on an account via getValue() because you may need to confirm that a password needs to be rehashed. I do not think it is valid to expose this to an API or via print_r() or var_dump().

@Fabianx: The current_user service is protected, but print_r() or var_dump() on a user entity loaded via user_load(). This is the same behavior as previous versions of Drupal. The User entity would need to implement __toString(), right?

@damiankloip: The tests in #31 still should apply to #38. I don't think the tests need to be different?

I've updated the issue summary to clarify that there is discussion on approach.

Edit: I agree with @berdir and @damiankloip that the property name is not useful and is confusing to be just null. It should be removed from the normalized entity entirely. I don't think that this would be a problem when submitting back via PATCH because it should be handled by validation, right? Right? Oh god....

The Entity Normalizer approach is not secure by default without but would require adding the same UserNormalizer class to hal module as well with a higher priority. This also means any other format that is added would NOT be secure by default unless they did would need to do similarly.

I attached a patch with the UserNormalizer duplicated into hal module and with patch #31's tests to demonstrate how this approach is not a viable solution even though it would pass tests.

I prefer the field normalization approach instead.

Edit: fix my incorrect statements.

I prefer the field normalization approach instead.

You are not in a much better position if you go for the field normalization approach. I guess you mean the return NULL thing? You would still have to add another normalizer for the password field to hal too. The implementations for normalization for hal is just too different that this will always be the case.

So I think maybe the best compromise could be that we get a password datatype, and check for that in the entity normalizer when iterating fields. This way we can avoid empty properties all the time, make sure passwords are never normalized, and avoid more normalizers?

The problem we have is that typed data has no notion of a public or protected/private property. Yes, access does this to an extent but will not always satisfy, like this case. In an ideal world, we could somehow ask the entity/typed data for "public" properties, then check access.

Hmm. Maybe just the one would just be fine.

So I think maybe the best compromise could be that we get a password datatype, and check for that in the entity normalizer when iterating fields. This way we can avoid empty properties all the time, make sure passwords are never normalized, and avoid more normalizers?

I am fine with that :)

Which earlier patches are these based from? Interdiff doesn't seem like the last one :)

+++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
@@ -34,6 +34,10 @@ class ComplexDataNormalizer extends NormalizerBase {
+      if (method_exists($field, 'getFieldDefinition') && ($definition = $field->getFieldDefinition()) && $definition->getType() == 'password') {

Out of interest, when would getFieldDefinition not exist?

+++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
@@ -90,7 +90,13 @@ public function normalize($entity, $format = NULL, array $context = array()) {
-      if (in_array($field->getFieldDefinition()->getName(), $exclude)) {
...
+      if (in_array($field_name, $exclude)) {
...
+      $field = $entity->get($field_name);
+      if (method_exists($field, 'getFieldDefinition') && ($definition = $field->getFieldDefinition()) && $definition->getType() == 'password') {
+        // Never normalize password items.

I don't think it is a good idea to overwrite the $field variable here.

In case its not a password item, this would continue the rest of the function with a different variable.

---

Can we use something like:

if (in_array($field_name, $exclude) || $this->isPasswordField($entity, $field_name)) {

instead?

---

Tests looks great, this would be RTBC - except for the above ...

RTBC, great work!

1. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -90,7 +90,12 @@ public function normalize($entity, $format = NULL, array $context = array()) {
   +      if (($definition = $field->getFieldDefinition()) && $definition->getType() == 'password') {
   

   If we are checking that the is a field definition. It would fail the first time when getFieldDefinition()->getName(); is called anyway.

2. +++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
   @@ -34,6 +34,10 @@ class ComplexDataNormalizer extends NormalizerBase {
   +      if (method_exists($field, 'getFieldDefinition') && ($definition = $field->getFieldDefinition()) && $definition->getType() == 'password') {
   

   This lives on FieldItemInterface. We can just check that instead?

3. +++ b/core/modules/serialization/src/Tests/EntitySerializationTest.php
   @@ -105,6 +108,16 @@ public function testNormalize() {
   +    $this->assertTrue(empty($normalized['pass']));
   

   Can we assert the array key does not exist instead? This would pass fine for the behaviour we didn't want of adding keys with NULL values too.

Also, looks like ComplexDataNormalizer was the only class I didn't add a unit test for ( I was sure I did...)! I will create a follow up for that.

Not a part of this issue, but it would still be nice if typed data had a notion of public/private properties instead... Then we could just check that arbitrarily.

Back to RTBC

Yes, that looks better now. Thanks larowlan!

1. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -90,7 +90,13 @@ public function normalize($entity, $format = NULL, array $context = array()) {
   +      if ($definition->getType() == 'password') {
   

   The check here.

2. +++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
   @@ -34,6 +34,10 @@ class ComplexDataNormalizer extends NormalizerBase {
   +      if ($field instanceof FieldItemListInterface && $field->getFieldDefinition()->getType() == 'password') {
   

   is a bit different from the check here.

Also the solution here doesn't seem to match either option in the issue summary.

Issue summary says the lack of normalization would be enforced either at the field level or the entity level, but instead we're enforcing it in the Normalizer classes themselves - so any added normalizer will also have to have this check no?

If we could have the check in NormalizerBase that'd be better, but not having it at this level at all seems optimal. (Note I've been loosely following this issue but didn't re-read it before reviewing the patch - it might be explained in comments, but it's definitely not in the issue summary).

Catch, yes, see some of the earlier comments. We discussed earlier in the issue not wanting this type of logic coupled to the serializer but this seemed to get a bad response. This was the compromise, as I definitely didn't want to see null values everywhere.

Field access checking (see related issue) can mostly solve this by itself. There was then the argument that passwords should never be shown in serialised form, regardless of access.

As I mentioned previously, I would like to see some solution at the typed data level. Maybe something that indicates public or protected properties.

Hopefully that sums it up! Thoughts?

Hmm, so to check I've got this right:

If #2365319: Entity normalization should check field access to avoid leaking data landed first, the security issue would be fixed because there'd never be read access to passwords. However if read access was enabled somehow then they'd still get serialized.

Then this issue just ensures that passwords never get serialized regardless of access?

Basically, yes. I originally just wanted that fix over this one as it is way more generic. I think the argument on this thread was (on my phone, can't be bothered to read everything) that user 1 could still get this serialized...

No, user 1 is not relevant, view access to the password is always denied, for everyone.

So yes, #67 is correct.

And I still don't really get the approach here. Sure, the password might be the most critical piece of that, but even with this patch, such a view still exposes *tons* of private information like e-mail address and also all configurable fields despite their field access implementations, which is a very common thing to have, especially on users. That's still a critical security issue. So we still need to patch views/rest to check access on all fields (or fix #2365319: Entity normalization should check field access to avoid leaking data), and if we do that, then the password field would also never be shown.

Hey, I didn't say that, just summarising :) I think we are agreeing here. I would rather just see the patch from #2365319: Entity normalization should check field access to avoid leaking data get in tbh.

And yeah, I didn't realise field perms were locked down as well as they are already. So I would say we should not use the approach at all in light of that. Checking field access should be all we need now.

A password still is really special and therefore we should do everything to protect it.

If there is a bug in the access functionality due to some weird circumstance, I would still want that password to not be able to go out ...

An email is sensitive - yes, but hacks of password databases still are the most sensitive ones ...

Also an added password type makes still a lot of sense.

If we have this as a fallback for when access fails, I'd personally go for something similar to earlier patches on this issue where you end up with password => null. If that only happens when access fails, then it seems fine to me.

Then we'd not have to make the change individually in each normalizer. In irc damiankloip suggested a PasswordItemNormalizer or similar.

Also I'm tempted to mark this postponed on #2365319: Entity normalization should check field access to avoid leaking data and bump it down to major, but not quite doing that yet.

Yes, if this was purely a fallback (which I agree it should be), I would be good with the NULL value approach. So I think #2365319: Entity normalization should check field access to avoid leaking data should stay as critical, get that in, then this provides the fallback protection.

I think larowlans patch from #31 is the one we want in that case. Which uses a PasswordFieldItemNormalizer, and adds the Password field type. works for me

Yes I liked #31 while following the issue then saw #61 RTBC and couldn't figure out why the change (hence the CNW). Agreed on the combination of that patch and the other issue.

Then this issue just ensures that passwords never get serialized regardless of access?

I don't get why we would do that. If I serialize the user entity containing the hashed password, I'd expect the hashed password to be in there. What if that's information that I want to send over to some service, e.g. to some database for storing it? Public webservice endpoints are not the only use case for a serialization system, so please do not pretend it is and break serialization for it.

#2365319: Entity normalization should check field access to avoid leaking data seems to be the right thing to do and would be covering this anyway.

#76: That is exactly what this is about:

To ensure you need to do extra work (e.g. changing the serializer) to be able to export such sensitive data.

Just because you can do something, does not need to mean that we allow you to do so easily.

You should know what you are doing and the default is "secure".

ok, this patch is based on #31.

I removed the PasswordFieldItemNormalizer in favour of a more generic NullNormalizer. I think this makes sense, as we can just pass the class/interface into the constructor instead. This is then a better generic component that can be used with the serializer.

Also moved the test assertions into their own method, added an assertion for the fallback (now that field access takes care of the common case), and a quick unit test for the NullNormalizer, just to keep parity with other normalizer test coverage.

Patch should take into account field access and serialize password hash, so test needs work for authorized

+++ b/core/modules/serialization/src/Normalizer/NullNormalizer.php
@@ -0,0 +1,32 @@
+  public function normalize($object, $format = NULL, array $context = array()) {
+    return NULL;

It's not clear how to make password hash "transportable with that approach.

We added field level access checking, see #2365319: Entity normalization should check field access to avoid leaking data.

The idea is that you need to do some work if you want passwords to be 'transportable'. Is that what you mean?

@damiankloip yes, exactly

Here is a test only patch for changes in EntitySerializationTest, we're expecting one fail for that last assertion.

That is the expected fail.

1. +++ b/core/modules/serialization/src/Normalizer/NullNormalizer.php
   @@ -0,0 +1,32 @@
   +  /**
   +   * Constructs a NullNormalizer object.
   +   *
   +   * @param string|array $supported_interface_of_class
   +   *   The supported interface(s) or class(es).
   +   */
   +  public function __construct($supported_interface_of_class) {
   +    $this->supportedInterfaceOrClass = $supported_interface_of_class;
   +  }
   

   nit - do we even need to overwrite the constructor?

   Or is the constructor in NormalizerBase marked abstract or not existent?

2. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/NullNormalizerTest.php
   @@ -0,0 +1,58 @@
   +  /**
   +   * @covers ::supportsNormalization
   +   */
   

   nit - I think we need to @covers ::__construct too, to get 100% test coverage for the class.

   I am unsure however how core handles that and if 100% test coverage is a goal at all for unit testable classes.

Overall it would be nice if we could document somewhere that this value is NULL'ed always due to security reasons, but no idea where.

The patch looks great!

RTBC +1 (from my side), no real RTBC, because andypost had some outstanding feedback - I think.

Wouldn't you just set a default password yourself, and that will get passed into the values that are used to denormalize? Then access will take over. If more is needed than that, like providing a default password. This seems like something the user entity needs control of?

Passed patch is #78, failed patch is #82. #82 is just the tests extracted from the patch in #78.

RE #86:

1. Yes, we need to overwrite this. This is the point of the generic NullNormalizer :) The base class does not do this for us. I don't think we want to move it to there either, as pretty much every case would need a new class anyway.
2. Added that to the @covers for supportsNormalization

@andypost, so the answer is, if you want to start actually sending passwords around and you know what you're doing, you can just remove this NullNormalizer service for the password from the container.

Sorry, ignore #89 patch, I added a bunch of files!!

Oh snap! Now #2388707: UserAccessControlHandler has wrong $explicit_check_fields name for the password field when checking field access is in, this just got harder to verify, as that now works as expected! :)

Maybe we need to test this with another entity that is not user, but has a password field.

Updated the tests to work with the recent changes to Drupal 8's core access controls. I created a test support module that implements hook_entity_field_access_alter() to grant the test user access to the password field.

I also cleaned up some comments and added test messages.

Thanks for working on this issue, forgot about this!

using the alter hook seems fine to me. Just a couple of things:

1. +++ b/core/modules/serialization/tests/modules/entity_serialization_test/entity_serialization_test.module
   @@ -0,0 +1,21 @@
   + * Implements hook_entity_field_access_alter().  Overrides some default access
   + * control to support testing.
   

   We can't have multiple lines here like this. Let's just keep 'Implements ...' there, adn move the next comment to the next line but one.

2. +++ b/core/modules/serialization/tests/modules/entity_serialization_test/entity_serialization_test.module
   @@ -0,0 +1,21 @@
   +  // Override default access control from UserAccessControlHandler to allow access to 'pass' field for our test user
   

   for the test user

Fixed #97.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/PasswordItem.php
@@ -0,0 +1,22 @@
+ *   default_formatter = "string"
...
+ */
+class PasswordItem extends StringItem {}

It feels a little bit odd that we render the password hashes, even this issue is about not doing it. Can we implement some NullFormatter which is used by default here?

Just remove it. default_widget is also wrong, string can definitely not be used to edit a password field.

I opened the related issue #2418119: REST user updates bypass tightened user account change validation. What does this issue mean regarding support of changing passwords via REST?

Yes, that is correct. This just determines the reading of that field, which is done when we normalize. denormalization will still be fine.

RTBC, looks great now!

Still need an issue summary update.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes.

[Edit for accuracy - i did not push :)]

Looks like the commit did not happen... right?
There is no commit comment in this issue queue, and I looked at a current checkout and I don't think the patch is applied.

You push my patch, I'll push yours! :D

Committed and pushed to 8.0.x. Thanks!