Fix #653: incorrectly strips quotes from string values that resemble YAML booleans #654

leinardi · 2025-03-03T16:04:31Z

Issue #, if available:
#653

Description of changes:
Ensure ambiguous YAML scalars (ON, OFF, YES, NO, TRUE, FALSE) remain quoted to prevent unintended type conversion in CloudFormation templates. Includes tests to verify correct handling in Parameters and Properties.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ues that resemble YAML booleans

leinardi · 2025-03-05T08:03:08Z

Hi @ericzbeard, thanks so much for merging this fix! Do you have an ETA for the next release of Rain? We're looking forward to it so we can use Rain in our CI without needing complex workarounds 🙂

leinardi · 2025-03-06T08:33:50Z

Hi @ericzbeard, I actually found an issue with the changes from this PR: now it seems to have the opposite issue, as in it's converting proper boolean values like yes and no into strings "yes" and "no".
~~I'm already working on a fix, I'll make a PR as soon as possible. Please avoid making new releases until then.~~

leinardi · 2025-03-06T09:49:16Z

@ericzbeard I’ve looked deeper into this, and after analyzing how yaml.v3 works, I can confirm that this behavior is not a bug, but rather an intentional aspect of YAML 1.2 compliance. In YAML 1.2, only true and false are recognized as valid boolean values, while yes, no, on, and off are always treated as strings. Since rain relies on yaml.v3, it follows YAML 1.2 behavior by design, meaning that booleans should be represented as true and false only.

Why this PR's changes should be kept & released

Before this fix, ambiguous scalars like on, off, yes, and no were incorrectly converted to booleans, with no way to enforce them as strings, since CloudFormation is not YAML 1.2 compliant.
With this fix, they are now consistently treated as strings, preventing unwanted type conversion and ensuring correctness in CloudFormation.
This is not a limitation of rain, but an intentional aspect of YAML 1.2, which yaml.v3 correctly adheres to. Since rain is built on yaml.v3, it must also be compliant with YAML 1.2, meaning only true and false should be used for booleans.
This should be explicitly stated in the documentation and the changelog to make it clear to users that yes, no, on, and off will always be treated as strings.

Suggested Changelog Entry

Breaking Change: rain now fully adheres to YAML 1.2 for booleans, meaning only true and false are valid values. The values yes, no, on, and off will always be treated as strings. If these were previously used as booleans, update them to true or false accordingly.

leinardi · 2025-03-06T10:49:22Z

Before fix (string to boolean conversion, no workaround):

  Param1:            ->  Param1:
    Default: "ON"    ->    Default: ON    # no workaround
  Param2:            ->  Param2:
    Default: "OFF"   ->    Default: OFF   # no workaround
  Param3:            ->  Param3:
    Default: "Yes"   ->    Default: Yes   # no workaround
  Param4:            ->  Param4:
    Default: "No"    ->    Default: No    # no workaround
  Param5:            ->  Param5:
    Default: "True"  ->    Default: "True"
  Param6:            ->  Param6:
    Default: "False" ->    Default: "False"
  Param7:            ->  Param7:
    Default: Maybe   ->    Default: Maybe
  Param8:            ->  Param8:
    Default: ON      ->    Default: ON
  Param9:            ->  Param9:
    Default: OFF     ->    Default: OFF
  Param10:           ->  Param10:
    Default: Yes     ->    Default: Yes
  Param11:           ->  Param11:
    Default: No      ->    Default: No
  Param12:           ->  Param12:
    Default: True    ->    Default: True
  Param13:           ->  Param13:
    Default: False   ->    Default: False

After fix (boolean to string conversion, workaround: just use true/false for booleans, as required by YAML 1.2):

  Param1:            ->  Param1:
    Default: "ON"    ->    Default: "ON"
  Param2:            ->  Param2:
    Default: "OFF"   ->    Default: "OFF"
  Param3:            ->  Param3:
    Default: "Yes"   ->    Default: "Yes"
  Param4:            ->  Param4:
    Default: "No"    ->    Default: "No"
  Param5:            ->  Param5:
    Default: "True"  ->    Default: "True"
  Param6:            ->  Param6:
    Default: "False" ->    Default: "False"
  Param7:            ->  Param7:
    Default: Maybe   ->    Default: Maybe
  Param8:            ->  Param8:
    Default: ON      ->    Default: "ON"    # replace with True
  Param9:            ->  Param9:
    Default: OFF     ->    Default: "OFF"   # replace with False
  Param10:           ->  Param10:
    Default: Yes     ->    Default: "Yes"   # replace with True
  Param11:           ->  Param11:
    Default: No      ->    Default: "No"    # replace with False
  Param12:           ->  Param12:
    Default: True    ->    Default: True
  Param13:           ->  Param13:
    Default: False   ->    Default: False

ericzbeard · 2025-03-06T21:43:52Z

Thanks for the detailed write-up on this issue. I'm somewhat concerned about shipping something that's technically a breaking change in a minor version release. But I imagine that in most (all?) cases where this mattered, people had probably adopted the flag to change the node styles across the entire template.

ericzbeard · 2025-03-06T22:05:49Z

Can we put this new behavior behind a new node-style?

leinardi · 2025-03-07T09:50:15Z

Yeah sure, something like this?

Index: cft/format/transform.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/cft/format/transform.go b/cft/format/transform.go
--- a/cft/format/transform.go	(revision 471caf008e4562aa5f8b62e01a544f3f685db8cc)
+++ b/cft/format/transform.go	(date 1741340107190)
@@ -119,14 +119,16 @@
 		if n.Kind == yaml.ScalarNode {
 			n.Style = yaml.DoubleQuotedStyle
 		}
-	case "":
-		// Default style for consistent formatting:
-		// Force double quotes on ambiguous scalar strings.
+	case "strict-booleans":
+		// Enforce YAML 1.2 behavior: Only `true` and `false` are booleans, all others are strings
 		if n.Kind == yaml.ScalarNode && n.Tag == "!!str" && isAmbiguousScalar(n.Value) {
 			n.Style = yaml.DoubleQuotedStyle
 		} else {
 			n.Style = 0
 		}
+	case "":
+		// Default style for consistent formatting
+		n.Style = 0
 	default:
 		panic("invalid --node-style: " + NodeStyle)
 	}
Index: cft/format/format_test.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/cft/format/format_test.go b/cft/format/format_test.go
--- a/cft/format/format_test.go	(revision 471caf008e4562aa5f8b62e01a544f3f685db8cc)
+++ b/cft/format/format_test.go	(date 1741340931613)
@@ -801,75 +801,86 @@
 	}
 }
 
-func TestAmbiguousScalarOn(t *testing.T) {
-	input := `
-Resources:
-  MyResource:
-    Type: AWS::RDS::DBClusterParameterGroup
-    Properties:
-      Parameters:
-        require_secure_transport: "ON"
-`
-	// We expect the ambiguous value "ON" to remain quoted in the output.
-	expected := `Resources:
-  MyResource:
-    Type: AWS::RDS::DBClusterParameterGroup
-    Properties:
-      Parameters:
-        require_secure_transport: "ON"
-`
-	template, err := parse.String(input)
-	if err != nil {
-		t.Fatal(err)
-	}
+func TestAmbiguousScalars_StrictBooleans_On(t *testing.T) {
+	// Save the original style
+	originalStyle := format.NodeStyle
+	// Restore after test completes
+	defer func() {
+		format.NodeStyle = originalStyle
+	}()
 
-	actual := format.String(template, format.Options{Unsorted: true})
-	if d := cmp.Diff(strings.TrimSpace(expected), strings.TrimSpace(actual)); d != "" {
-		t.Fatalf("Diff: %s", d)
-	}
-}
+	// Override for this test only
+	format.NodeStyle = "strict-booleans"
 
-func TestAmbiguousScalarsInParameters(t *testing.T) {
 	input := `
-Parameters:
-  Param1:
-    Default: "ON"
-  Param2:
-    Default: "OFF"
-  Param3:
-    Default: "Yes"
-  Param4:
-    Default: "No"
-  Param5:
-    Default: "True"
-  Param6:
-    Default: "False"
-  Param7:
-    Default: "Maybe"
+Param1:
+  Default: "ON"
+Param2:
+  Default: "OFF"
+Param3:
+  Default: "Yes"
+Param4:
+  Default: "No"
+Param5:
+  Default: "True"
+Param6:
+  Default: "False"
+Param7:
+  Default: Maybe
+Param8:
+  Default: ON
+Param9:
+  Default: OFF
+Param10:
+  Default: Yes
+Param11:
+  Default: No
+Param12:
+  Default: True
+Param13:
+  Default: False
 `
 	// The ambiguous values (Param1 through Param6) should be rendered with quotes,
 	// while a non-ambiguous value (Param7) may be unquoted.
-	expected := `Parameters:
-  Param1:
-    Default: "ON"
+	expected := `
+Param1:
+  Default: "ON"
 
-  Param2:
-    Default: "OFF"
+Param2:
+  Default: "OFF"
 
-  Param3:
-    Default: "Yes"
+Param3:
+  Default: "Yes"
 
-  Param4:
-    Default: "No"
+Param4:
+  Default: "No"
 
-  Param5:
-    Default: "True"
+Param5:
+  Default: "True"
 
-  Param6:
-    Default: "False"
+Param6:
+  Default: "False"
 
-  Param7:
-    Default: Maybe
+Param7:
+  Default: Maybe
+
+Param8:
+  Default: "ON"
+
+Param9:
+  Default: "OFF"
+
+Param10:
+  Default: "Yes"
+
+Param11:
+  Default: "No"
+
+Param12:
+  Default: True
+
+Param13:
+  Default: False
 `
 	template, err := parse.String(input)
 	if err != nil {
@@ -882,18 +893,76 @@
 	}
 }
 
-func TestNonAmbiguousScalar(t *testing.T) {
+func TestAmbiguousScalars_StrictBooleans_Off(t *testing.T) {
 	input := `
-Resources:
-  MyResource:
-    Properties:
-      example_value: "OnX"
+Param1:
+  Default: "ON"
+Param2:
+  Default: "OFF"
+Param3:
+  Default: "Yes"
+Param4:
+  Default: "No"
+Param5:
+  Default: "True"
+Param6:
+  Default: "False"
+Param7:
+  Default: Maybe
+Param8:
+  Default: ON
+Param9:
+  Default: OFF
+Param10:
+  Default: Yes
+Param11:
+  Default: No
+Param12:
+  Default: True
+Param13:
+  Default: False
 `
-	// Since "OnX" is not an ambiguous token, it can be rendered without quotes.
-	expected := `Resources:
-  MyResource:
-    Properties:
-      example_value: OnX
+	// The ambiguous values (Param1 through Param6) should be rendered with quotes,
+	// while a non-ambiguous value (Param7) may be unquoted.
+	expected := `
+Param1:
+  Default: ON
+
+Param2:
+  Default: OFF
+
+Param3:
+  Default: Yes
+
+Param4:
+  Default: No
+
+Param5:
+  Default: "True"
+
+Param6:
+  Default: "False"
+
+Param7:
+  Default: Maybe
+
+Param8:
+  Default: ON
+
+Param9:
+  Default: OFF
+
+Param10:
+  Default: Yes
+
+Param11:
+  Default: No
+
+Param12:
+  Default: True
+
+Param13:
+  Default: False
 `
 	template, err := parse.String(input)
 	if err != nil {

Let me know if I should create a PR with these changes.

r-heimann · 2025-03-08T09:18:49Z

Related to #319

Fix aws-cloudformation#653: incorrectly strips quotes from string val…

4d348fa

…ues that resemble YAML booleans

leinardi mentioned this pull request Mar 3, 2025

Rain incorrectly strips quotes from string values that resemble YAML booleans, altering data types in CloudFormation templates #653

Closed

ericzbeard approved these changes Mar 4, 2025

View reviewed changes

ericzbeard merged commit 471caf0 into aws-cloudformation:main Mar 4, 2025
1 check passed

leinardi mentioned this pull request Mar 5, 2025

String type characters are converted to Boolean type characters #480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #653: incorrectly strips quotes from string values that resemble YAML booleans #654

Fix #653: incorrectly strips quotes from string values that resemble YAML booleans #654

leinardi commented Mar 3, 2025 •

edited

Loading

leinardi commented Mar 5, 2025

leinardi commented Mar 6, 2025 •

edited

Loading

leinardi commented Mar 6, 2025 •

edited

Loading

leinardi commented Mar 6, 2025 •

edited

Loading

ericzbeard commented Mar 6, 2025

ericzbeard commented Mar 6, 2025

leinardi commented Mar 7, 2025 •

edited

Loading

r-heimann commented Mar 8, 2025

Fix #653: incorrectly strips quotes from string values that resemble YAML booleans #654

Fix #653: incorrectly strips quotes from string values that resemble YAML booleans #654

Conversation

leinardi commented Mar 3, 2025 • edited Loading

leinardi commented Mar 5, 2025

leinardi commented Mar 6, 2025 • edited Loading

leinardi commented Mar 6, 2025 • edited Loading

Why this PR's changes should be kept & released

Suggested Changelog Entry

leinardi commented Mar 6, 2025 • edited Loading

ericzbeard commented Mar 6, 2025

ericzbeard commented Mar 6, 2025

leinardi commented Mar 7, 2025 • edited Loading

r-heimann commented Mar 8, 2025

leinardi commented Mar 3, 2025 •

edited

Loading

leinardi commented Mar 6, 2025 •

edited

Loading

leinardi commented Mar 6, 2025 •

edited

Loading

leinardi commented Mar 6, 2025 •

edited

Loading

leinardi commented Mar 7, 2025 •

edited

Loading