[Fix] Site .env: stop escaping double quotes in values that never needed quoting (#1228)

* fix: allow quoted values in .env variables

* fix: tighten CR coverage and fix coderabbit bug

Author: RichardAnderson

app/Helpers/EnvParser.php

@@ -68,6 +68,7 @@ public static function parse(string $raw): array
             if ($wasDoubleQuoted) {
                 $value = preg_replace_callback('/\\\\(.)/s', fn (array $matches): string => match ($matches[1]) {
                     'n' => "\n",
+                    'r' => "\r",
                     '"' => '"',
                     '\\' => '\\',
                     default => $matches[0],
@@ -169,18 +170,31 @@ public static function stringify(array $variables): string
                 continue;
             }
 
-            $needsQuotes = str_contains($value, "\n") ||
-                str_contains($value, ' ') ||
-                str_contains($value, '"') ||
-                str_contains($value, "'") ||
-                str_contains($value, '#');
+            $needsQuotes = preg_match('/\s/', $value) === 1
+                || str_contains($value, '#')
+                || str_starts_with($value, '"')
+                || str_starts_with($value, "'");
 
-            if ($needsQuotes) {
-                $escapedValue = str_replace(['\\', "\n", '"'], ['\\\\', '\\n', '\\"'], $value);
-                $lines[] = "{$key}=\"{$escapedValue}\"";
-            } else {
+            if (! $needsQuotes) {
                 $lines[] = "{$key}={$value}";
+
+                continue;
             }
+
+            if (
+                str_contains($value, '"')
+                && ! str_contains($value, "'")
+                && ! str_contains($value, '\\')
+                && ! str_contains($value, "\n")
+                && ! str_contains($value, "\r")
+            ) {
+                $lines[] = "{$key}='{$value}'";
+
+                continue;
+            }
+
+            $escapedValue = str_replace(['\\', "\n", "\r", '"'], ['\\\\', '\\n', '\\r', '\\"'], $value);
+            $lines[] = "{$key}=\"{$escapedValue}\"";
         }
 
         return implode("\n", $lines);

tests/Feature/ApplicationTest.php

@@ -546,7 +546,7 @@ function vitoPestFeatureApplicationTestAssertBroadcastsDeploymentCreated(callabl
 });
 
 test('update env file with variables', function () {
-    SSH::fake();
+    $ssh = SSH::fake();
 
     $this->actingAs($this->user);
 
@@ -558,10 +558,16 @@ function vitoPestFeatureApplicationTestAssertBroadcastsDeploymentCreated(callabl
             ['key' => 'APP_ENV', 'value' => 'production'],
             ['key' => 'APP_DEBUG', 'value' => 'false'],
             ['key' => 'DB_PASSWORD', 'value' => 'secret123'],
+            ['key' => 'VITE_PAYMENT_METHODS_MOLLIE', 'value' => '["ideal","paybybank","bancontact"]'],
         ],
     ])
         ->assertSessionDoesntHaveErrors();
 
+    $this->assertStringContainsString(
+        'VITE_PAYMENT_METHODS_MOLLIE=["ideal","paybybank","bancontact"]',
+        $ssh->getUploadedContent()
+    );
+
     $this->site->refresh();
 
     expect(data_get($this->site->type_data, 'env_path'))->toEqual($this->site->path.'/.env');

tests/Unit/EnvParserTest.php

@@ -116,6 +116,87 @@
     expect($result)->toEqual('MULTILINE="line1\nline2"');
 });
 
+test('stringify leaves a value containing double quotes unquoted when nothing forces quoting', function () {
+    $variables = [
+        ['key' => 'VITE_PAYMENT_METHODS_MOLLIE', 'value' => '["ideal","paybybank","bancontact"]'],
+    ];
+    $result = EnvParser::stringify($variables);
+
+    expect($result)->toEqual('VITE_PAYMENT_METHODS_MOLLIE=["ideal","paybybank","bancontact"]');
+});
+
+/**
+ * @return array<string, array<int, string>>
+ */
+dataset('jsonArrayOnDiskProvider', function () {
+    return [
+        'bare' => ['VITE_PAYMENT_METHODS_MOLLIE=["ideal","paybybank","bancontact"]'],
+        'single quoted' => ["VITE_PAYMENT_METHODS_MOLLIE='[\"ideal\",\"paybybank\",\"bancontact\"]'"],
+        'already escaped' => ['VITE_PAYMENT_METHODS_MOLLIE="[\\"ideal\\",\\"paybybank\\",\\"bancontact\\"]"'],
+    ];
+});
+
+test('saving rewrites every on disk form of a json array value as valid json', function (string $raw) {
+    $parsed = EnvParser::parse($raw);
+
+    expect($parsed)->toHaveCount(1);
+    expect($parsed[0]['value'])->toEqual('["ideal","paybybank","bancontact"]');
+    expect(EnvParser::stringify($parsed))->toEqual('VITE_PAYMENT_METHODS_MOLLIE=["ideal","paybybank","bancontact"]');
+})->with('jsonArrayOnDiskProvider');
+
+test('stringify prefers single quotes over escaping double quotes', function () {
+    $variables = [
+        ['key' => 'K', 'value' => 'he said "hi"'],
+    ];
+    $result = EnvParser::stringify($variables);
+
+    expect($result)->toEqual('K=\'he said "hi"\'');
+});
+
+test('stringify keeps a mid string double quote unquoted', function () {
+    $result = EnvParser::stringify([['key' => 'K', 'value' => 'x"y']]);
+
+    expect($result)->toEqual('K=x"y');
+});
+
+test('stringify quotes values containing a hash', function () {
+    $result = EnvParser::stringify([['key' => 'K', 'value' => '#fff']]);
+
+    expect($result)->toEqual('K="#fff"');
+});
+
+test('stringify quotes a value that starts with a double quote', function () {
+    $result = EnvParser::stringify([['key' => 'K', 'value' => '"hello"']]);
+
+    expect($result)->toEqual('K=\'"hello"\'');
+});
+
+test('stringify quotes a value that starts with a single quote', function () {
+    $result = EnvParser::stringify([['key' => 'K', 'value' => "'hello'"]]);
+
+    expect($result)->toEqual('K="\'hello\'"');
+});
+
+test('stringify escapes carriage returns rather than writing them raw', function () {
+    $result = EnvParser::stringify([['key' => 'K', 'value' => "a\rb"]]);
+
+    expect($result)->toEqual('K="a\rb"');
+    expect(EnvParser::parse($result)[0]['value'])->toEqual("a\rb");
+});
+
+test('stringify does not single quote a carriage return value', function () {
+    $result = EnvParser::stringify([['key' => 'K', 'value' => "a\rb\"c"]]);
+
+    expect($result)->toEqual('K="a\rb\"c"');
+    expect(EnvParser::parse($result)[0]['value'])->toEqual("a\rb\"c");
+});
+
+test('stringify escapes when both quote styles are present', function () {
+    $result = EnvParser::stringify([['key' => 'K', 'value' => 'it\'s "quoted"']]);
+
+    expect($result)->toEqual('K="it\'s \\"quoted\\""');
+});
+
 test('stringify skips empty keys', function () {
     $variables = [
         ['key' => '', 'value' => 'empty'],
@@ -155,6 +236,13 @@
         'empty' => [''],
         'base64 padding' => ['base64:abc=123='],
         'backslash and quote' => ['C:\dir "x"'],
+        'json array' => ['["ideal","paybybank","bancontact"]'],
+        'leading double quote' => ['"hello"'],
+        'leading single quote' => ["'hello'"],
+        'tab' => ["a\tb"],
+        'trailing tab' => ["a\t"],
+        'carriage return' => ["a\rb"],
+        'carriage return with double quote' => ["a\rb\"c"],
     ];
 });