align Encryptable and HasEncryptedEmail behavior - encrypt on set, decrypt on access, prevent double encryption

paperscissors · paperscissors · commit 55566bc416fc · 2025-07-29T15:13:45.000-04:00
diff --git a/src/Encryptable.php b/src/Encryptable.php
@@ -24,56 +24,35 @@ public function getEncryptableAttributes(): array
      */
     protected static function bootEncryptable()
     {
-        static::saving(function ($model) {
-            $model->encryptAttributes();
-        });
-
-        static::retrieved(function ($model) {
-            $model->decryptAttributes();
-        });
+        // Since we now encrypt immediately on set, we don't need saving event
+        // Since we decrypt on access without modifying state, we don't need retrieved event
         
-        // Also decrypt when models are created from arrays (like from relationships)
-        static::creating(function ($model) {
-            // Don't decrypt on creating, just mark that we need to check later
-        });
+        // Keep this minimal - encryption happens on __set(), decryption on __get/getAttribute
     }
 
     /**
      * Encrypt attributes marked as encryptable.
+     * DEPRECATED: This method is no longer used since encryption happens immediately in __set().
      *
      * @return void
      */
     protected function encryptAttributes()
     {
-        $databaseHasLimits = DB::connection()->getDriverName() === 'pgsql';
-        
-        foreach ($this->getEncryptableAttributes() as $attribute) {
-            if (isset($this->attributes[$attribute]) && ! empty($this->attributes[$attribute])) {
-                // Only encrypt if the value is not already encrypted
-                if (!$this->isValueEncrypted($this->attributes[$attribute])) {
-                    // Use the field-specific encryption that handles character limits
-                    $this->attributes[$attribute] = $this->encryptValueForStorage($this->attributes[$attribute], $databaseHasLimits);
-                }
-            }
-        }
+        // This method is deprecated and no longer used.
+        // Encryption now happens immediately in __set() method, aligning with HasEncryptedEmail behavior.
     }
 
     /**
      * Decrypt attributes marked as encryptable.
+     * DEPRECATED: This method is no longer used since we decrypt on access without modifying state.
      *
      * @return void
      */
     protected function decryptAttributes()
     {
-        foreach ($this->getEncryptableAttributes() as $attribute) {
-            if (isset($this->attributes[$attribute]) && ! empty($this->attributes[$attribute])) {
-                try {
-                    $this->attributes[$attribute] = $this->decryptValue($this->attributes[$attribute]);
-                } catch (\Exception $e) {
-                    // If the value can't be decrypted, leave it as is
-                }
-            }
-        }
+        // This method is deprecated and no longer used.
+        // Decryption now happens on-demand in __get() and getAttribute() without modifying $this->attributes
+        // This prevents double encryption issues and aligns with HasEncryptedEmail behavior
     }
 
     /**
@@ -134,8 +113,7 @@ protected function decryptValue($value)
     
     /**
      * Dynamically retrieve attributes.
-     * This ensures attributes are decrypted when accessed via notifications
-     * and other systems that might bypass the standard attribute getters.
+     * Returns decrypted values without modifying internal state.
      *
      * @param  string  $key
      * @return mixed
@@ -147,21 +125,24 @@ public function __get($key)
             return parent::__get($key);
         }
 
-        // If the attribute exists and is encrypted, ensure it's decrypted
+        // If the attribute exists and is encrypted, return decrypted value
         $encryptableAttributes = $this->getEncryptableAttributes();
         
         if (in_array($key, $encryptableAttributes) && isset($this->attributes[$key])) {
             if (is_string($this->attributes[$key]) && !empty($this->attributes[$key])) {
                 // Check if the value appears to be encrypted
                 if ($this->isValueEncrypted($this->attributes[$key])) {
                     try {
+                        // Return decrypted value directly - DO NOT modify $this->attributes
                         return $this->decryptValue($this->attributes[$key]);
                     } catch (\Exception $e) {
                         // If decryption fails, return the original value
                         return $this->attributes[$key];
                     }
                 }
             }
+            // If not encrypted, return as-is
+            return $this->attributes[$key];
         }
         
         // Default behavior if not an encrypted field
@@ -170,7 +151,7 @@ public function __get($key)
     
     /**
      * Dynamically set attributes.
-     * Ensures attributes are stored as plain text when set, to be encrypted on save.
+     * Encrypts immediately when set, like HasEncryptedEmail trait.
      *
      * @param  string  $key
      * @param  mixed  $value
@@ -183,28 +164,52 @@ public function __set($key, $value)
             return parent::__set($key, $value);
         }
         
-        // Set the attribute as normal - do NOT encrypt here
-        // Encryption should only happen on save via the saving event
+        $encryptableAttributes = $this->getEncryptableAttributes();
+        
+        if (in_array($key, $encryptableAttributes)) {
+            if (!empty($value)) {
+                // Check if value is already encrypted to avoid double-encryption
+                if (!$this->isValueEncrypted($value)) {
+                    // Encrypt the value immediately
+                    $databaseHasLimits = DB::connection()->getDriverName() === 'pgsql';
+                    $this->attributes[$key] = $this->encryptValueForStorage($value, $databaseHasLimits);
+                } else {
+                    // Already encrypted, use as is
+                    $this->attributes[$key] = $value;
+                }
+            } else {
+                $this->attributes[$key] = null;
+            }
+            return;
+        }
+        
+        // For non-encryptable attributes, use default behavior
         parent::__set($key, $value);
     }
     
     /**
      * Convert the model's attributes to an array.
-     * Ensures all attributes are properly decrypted.
+     * Ensures encrypted attributes are decrypted by going through getAttribute().
      *
      * @return array
      */
     public function attributesToArray()
     {
-        // Make sure all encryptable attributes are decrypted
-        $this->decryptAttributes();
+        $attributes = parent::attributesToArray();
         
-        return parent::attributesToArray();
+        // For encryptable attributes, make sure we get the decrypted values
+        foreach ($this->getEncryptableAttributes() as $key) {
+            if (array_key_exists($key, $attributes)) {
+                $attributes[$key] = $this->getAttribute($key);
+            }
+        }
+        
+        return $attributes;
     }
     
     /**
      * Get an attribute from the model.
-     * Ensures encrypted attributes are properly decrypted.
+     * Returns decrypted values without modifying internal state.
      *
      * @param  string  $key
      * @return mixed
@@ -223,10 +228,8 @@ public function getAttribute($key)
             if (is_string($this->attributes[$key]) && !empty($this->attributes[$key])) {
                 if ($this->isValueEncrypted($this->attributes[$key])) {
                     try {
-                        $decrypted = $this->decryptValue($this->attributes[$key]);
-                        // Store the decrypted value back to avoid repeated decryption
-                        $this->attributes[$key] = $decrypted;
-                        return $decrypted;
+                        // Return decrypted value directly - DO NOT modify $this->attributes
+                        return $this->decryptValue($this->attributes[$key]);
                     } catch (\Exception $e) {
                         // If decryption fails, return the attribute as-is
                         return $this->attributes[$key];
@@ -244,6 +247,7 @@ public function getAttribute($key)
     /**
      * Check if a value appears to be encrypted.
      * Detects both Laravel standard encryption (AES-256-CBC) and compact encryption formats.
+     * Enhanced with bulletproof double encryption protection.
      *
      * @param  string  $value
      * @return bool
@@ -256,37 +260,55 @@ protected function isValueEncrypted($value)
         
         // Check for compact encryption format (starts with 'c:')
         if (strpos($value, 'c:') === 0) {
-            return true;
+            // Validate compact format: c:base64.base64.base64
+            $parts = explode('.', substr($value, 2));
+            if (count($parts) === 3) {
+                // All parts should be valid base64
+                foreach ($parts as $part) {
+                    if (base64_decode($part, true) === false) {
+                        return false;
+                    }
+                }
+                return true;
+            }
+            return false;
         }
         
         // Check for Laravel standard encryption format (base64 encoded JSON with iv, value, mac)
-        // Laravel AES-256-CBC encryption produces base64 strings that decode to JSON
-        if (strlen($value) > 50 && base64_decode($value, true) !== false) {
-            $decoded = base64_decode($value, true);
-            if ($decoded !== false) {
-                $json = json_decode($decoded, true);
-                // Laravel encryption has 'iv', 'value', and 'mac' keys
-                // May also have additional keys like 'tag' in newer versions
-                if (is_array($json) && 
-                    isset($json['iv'], $json['value'], $json['mac'])) {
-                    return true;
-                }
-            }
+        // Must be long enough to contain encryption data
+        if (strlen($value) < 100) {
+            return false;
         }
         
-        // Additional pattern check for Laravel encryption that starts with 'eyJ'
-        // (base64 encoding of '{"' which is common for Laravel encryption JSON)
-        if (preg_match('/^eyJ[A-Za-z0-9+\/]+=*$/', $value)) {
-            $decoded = base64_decode($value, true);
-            if ($decoded !== false) {
-                $json = json_decode($decoded, true);
-                if (is_array($json) && 
-                    isset($json['iv'], $json['value'], $json['mac'])) {
-                    return true;
-                }
-            }
+        // Must be valid base64
+        $decoded = base64_decode($value, true);
+        if ($decoded === false) {
+            return false;
+        }
+        
+        // Must be valid JSON
+        $json = json_decode($decoded, true);
+        if (!is_array($json)) {
+            return false;
+        }
+        
+        // Must have required Laravel encryption keys
+        if (!isset($json['iv'], $json['value'], $json['mac'])) {
+            return false;
+        }
+        
+        // All components should be base64 strings
+        if (!is_string($json['iv']) || !is_string($json['value']) || !is_string($json['mac'])) {
+            return false;
+        }
+        
+        // Validate that components are valid base64
+        if (base64_decode($json['iv'], true) === false || 
+            base64_decode($json['value'], true) === false ||
+            base64_decode($json['mac'], true) === false) {
+            return false;
         }
         
-        return false;
+        return true;
     }
 }
diff --git a/tests/Unit/EncryptableTest.php b/tests/Unit/EncryptableTest.php
@@ -434,7 +434,7 @@ public function testDoubleEncryptionDecryption()
         $this->assertNotEquals($originalValue, $rawData->address);
     }
 
-    public function testSingleEncryptionOnSaveOnly()
+    public function testImmediateEncryptionOnSet()
     {
         // Create a test table
         $this->app['db']->connection()->getSchemaBuilder()->create('test_models', function ($table) {
@@ -447,34 +447,39 @@ public function testSingleEncryptionOnSaveOnly()
         // Create a new model
         $model = new TestEncryptableModel();
         
-        // Set values - these should NOT be encrypted yet
+        // Set values - these should be encrypted IMMEDIATELY
         $model->email = 'test@example.com';
         $model->phone = '123-456-7890';
         
-        // Check that the values are still plain text before save
+        // Check that accessing the values returns the original plain text
         $this->assertEquals('test@example.com', $model->email);
         $this->assertEquals('123-456-7890', $model->phone);
         
-        // Check the raw attributes are also plain text
-        $this->assertEquals('test@example.com', $model->getAttributes()['email']);
-        $this->assertEquals('123-456-7890', $model->getAttributes()['phone']);
+        // Check that the raw attributes are actually encrypted
+        $rawAttributes = $model->getAttributes();
+        $this->assertNotEquals('test@example.com', $rawAttributes['email']);
+        $this->assertNotEquals('123-456-7890', $rawAttributes['phone']);
         
-        // Now save the model - this should encrypt the values
+        // Verify they are detectable as encrypted
+        $reflection = new \ReflectionClass($model);
+        $method = $reflection->getMethod('isValueEncrypted');
+        $method->setAccessible(true);
+        $this->assertTrue($method->invokeArgs($model, [$rawAttributes['email']]));
+        $this->assertTrue($method->invokeArgs($model, [$rawAttributes['phone']]));
+        
+        // Now save the model - values should already be encrypted, no additional encryption
         $model->save();
         
         // After save, when accessed, they should still return the original values
         $this->assertEquals('test@example.com', $model->email);
         $this->assertEquals('123-456-7890', $model->phone);
         
-        // But the raw database should be encrypted
+        // Database should contain the same encrypted values as in memory
         $rawData = $this->app['db']->connection()->table('test_models')->where('id', $model->id)->first();
         $this->assertNotEquals('test@example.com', $rawData->email);
         $this->assertNotEquals('123-456-7890', $rawData->phone);
         
-        // And they should be detectable as encrypted
-        $reflection = new \ReflectionClass($model);
-        $method = $reflection->getMethod('isValueEncrypted');
-        $method->setAccessible(true);
+        // Values should still be encrypted in the same way
         $this->assertTrue($method->invokeArgs($model, [$rawData->email]));
         $this->assertTrue($method->invokeArgs($model, [$rawData->phone]));
         
@@ -483,6 +488,59 @@ public function testSingleEncryptionOnSaveOnly()
         $this->assertEquals('test@example.com', $freshModel->email);
         $this->assertEquals('123-456-7890', $freshModel->phone);
     }
+
+    public function testNoDoubleEncryptionOnMultipleSaves()
+    {
+        // Create a test table
+        $this->app['db']->connection()->getSchemaBuilder()->create('test_models', function ($table) {
+            $table->increments('id');
+            $table->text('email')->nullable();
+            $table->text('phone')->nullable();  
+            $table->timestamps();
+        });
+
+        // Create and save a model
+        $model = new TestEncryptableModel();
+        $model->email = 'test@example.com';
+        $model->phone = '123-456-7890';
+        $model->save();
+
+        // Get the encrypted values after first save
+        $firstRawData = $this->app['db']->connection()->table('test_models')->where('id', $model->id)->first();
+        $firstEncryptedEmail = $firstRawData->email;
+        $firstEncryptedPhone = $firstRawData->phone;
+
+        // Access the values (which should decrypt them for display)
+        $this->assertEquals('test@example.com', $model->email);
+        $this->assertEquals('123-456-7890', $model->phone);
+
+        // Save again - this should NOT cause re-encryption
+        $model->save();
+
+        // Get the encrypted values after second save
+        $secondRawData = $this->app['db']->connection()->table('test_models')->where('id', $model->id)->first();
+        
+        // The encrypted values should be identical (no double encryption)
+        $this->assertEquals($firstEncryptedEmail, $secondRawData->email);
+        $this->assertEquals($firstEncryptedPhone, $secondRawData->phone);
+
+        // Values should still decrypt correctly
+        $this->assertEquals('test@example.com', $model->email);
+        $this->assertEquals('123-456-7890', $model->phone);
+
+        // Try setting the same values again and saving
+        $model->email = 'test@example.com';
+        $model->phone = '123-456-7890';
+        $model->save();
+
+        // Should still be properly encrypted and decryptable
+        $thirdRawData = $this->app['db']->connection()->table('test_models')->where('id', $model->id)->first();
+        $this->assertNotEquals('test@example.com', $thirdRawData->email);
+        $this->assertNotEquals('123-456-7890', $thirdRawData->phone);
+        
+        $this->assertEquals('test@example.com', $model->email);
+        $this->assertEquals('123-456-7890', $model->phone);
+    }
 }
 
 class TestEncryptableModel extends Model
