From 952ce0285ba76268b0615530b09e43d086feb4c2 Mon Sep 17 00:00:00 2001
From: mleku <me@mleku.dev>
Date: Tue, 2 Dec 2025 11:53:52 +0000
Subject: [PATCH] Validate ISO-8601 duration format for max_expiry_duration

Added validation to reject invalid max_expiry_duration formats in policy configs, ensuring compliance with ISO-8601 standards. Updated the `New` function to fail fast on invalid inputs and included detailed error messages for better clarity. Comprehensive tests were added to verify both valid and invalid scenarios.

bump tag to build binary with update
---
 pkg/policy/new_fields_test.go | 88 +++++++++++++++++++++++++++++++++++
 pkg/policy/policy.go          | 12 ++++-
 pkg/version/version           |  2 +-
 3 files changed, 99 insertions(+), 3 deletions(-)

diff --git a/pkg/policy/new_fields_test.go b/pkg/policy/new_fields_test.go
index a048a19..4703347 100644
--- a/pkg/policy/new_fields_test.go
+++ b/pkg/policy/new_fields_test.go
@@ -1071,6 +1071,94 @@ func TestNewFieldsInGlobalRule(t *testing.T) {
 	}
 }
 
+// =============================================================================
+// New() Validation Tests - Ensures invalid configs fail at load time
+// =============================================================================
+
+// TestNewRejectsInvalidMaxExpiryDuration verifies that New() fails fast when
+// given an invalid max_expiry_duration format like "T10M" instead of "PT10M".
+// This prevents silent failures where constraints are ignored.
+func TestNewRejectsInvalidMaxExpiryDuration(t *testing.T) {
+	tests := []struct {
+		name        string
+		json        string
+		expectError bool
+		errorMatch  string
+	}{
+		{
+			name: "valid PT10M format accepted",
+			json: `{
+				"rules": {
+					"4": {"max_expiry_duration": "PT10M"}
+				}
+			}`,
+			expectError: false,
+		},
+		{
+			name: "invalid T10M format (missing P prefix) rejected",
+			json: `{
+				"rules": {
+					"4": {"max_expiry_duration": "T10M"}
+				}
+			}`,
+			expectError: true,
+			errorMatch:  "max_expiry_duration",
+		},
+		{
+			name: "invalid 10M format (missing PT prefix) rejected",
+			json: `{
+				"rules": {
+					"4": {"max_expiry_duration": "10M"}
+				}
+			}`,
+			expectError: true,
+			errorMatch:  "max_expiry_duration",
+		},
+		{
+			name: "valid P7D format accepted",
+			json: `{
+				"rules": {
+					"1": {"max_expiry_duration": "P7D"}
+				}
+			}`,
+			expectError: false,
+		},
+		{
+			name: "invalid 7D format (missing P prefix) rejected",
+			json: `{
+				"rules": {
+					"1": {"max_expiry_duration": "7D"}
+				}
+			}`,
+			expectError: true,
+			errorMatch:  "max_expiry_duration",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			policy, err := New([]byte(tt.json))
+
+			if tt.expectError {
+				if err == nil {
+					t.Errorf("New() should have rejected invalid config, but returned policy: %+v", policy)
+					return
+				}
+				if tt.errorMatch != "" && !contains(err.Error(), tt.errorMatch) {
+					t.Errorf("Error %q should contain %q", err.Error(), tt.errorMatch)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("New() unexpected error for valid config: %v", err)
+				}
+				if policy == nil {
+					t.Error("New() returned nil policy for valid config")
+				}
+			}
+		})
+	}
+}
+
 // =============================================================================
 // ValidateJSON Tests for New Fields
 // =============================================================================
diff --git a/pkg/policy/policy.go b/pkg/policy/policy.go
index bd9ac2c..65688f9 100644
--- a/pkg/policy/policy.go
+++ b/pkg/policy/policy.go
@@ -468,11 +468,19 @@ func (p *P) UnmarshalJSON(data []byte) error {
 // New creates a new policy from JSON configuration.
 // If policyJSON is empty, returns a policy with default settings.
 // The default_policy field defaults to "allow" if not specified.
+// Returns an error if the policy JSON contains invalid values (e.g., invalid
+// ISO-8601 duration format for max_expiry_duration, invalid regex patterns, etc.).
 func New(policyJSON []byte) (p *P, err error) {
 	p = &P{
 		DefaultPolicy: "allow", // Set default value
 	}
 	if len(policyJSON) > 0 {
+		// Validate JSON before loading to fail fast on invalid configurations.
+		// This prevents silent failures where invalid values (like "T10M" instead
+		// of "PT10M" for max_expiry_duration) are ignored and constraints don't apply.
+		if err = p.ValidateJSON(policyJSON); err != nil {
+			return nil, fmt.Errorf("policy validation failed: %v", err)
+		}
 		if err = json.Unmarshal(policyJSON, p); chk.E(err) {
 			return nil, fmt.Errorf("failed to unmarshal policy JSON: %v", err)
 		}
@@ -1784,7 +1792,7 @@ func (p *P) ValidateJSON(policyJSON []byte) error {
 		// Validate MaxExpiryDuration format
 		if rule.MaxExpiryDuration != "" {
 			if _, err := parseDuration(rule.MaxExpiryDuration); err != nil {
-				return fmt.Errorf("invalid max_expiry_duration %q in kind %d: %v", rule.MaxExpiryDuration, kind, err)
+				return fmt.Errorf("invalid max_expiry_duration %q in kind %d: %v (format must be ISO-8601 duration, e.g. \"PT10M\" for 10 minutes, \"P7D\" for 7 days, \"P1DT12H\" for 1 day 12 hours)", rule.MaxExpiryDuration, kind, err)
 			}
 		}
 		// Validate FollowsWhitelistAdmins pubkeys
@@ -1815,7 +1823,7 @@ func (p *P) ValidateJSON(policyJSON []byte) error {
 	// Validate global rule MaxExpiryDuration format
 	if tempPolicy.Global.MaxExpiryDuration != "" {
 		if _, err := parseDuration(tempPolicy.Global.MaxExpiryDuration); err != nil {
-			return fmt.Errorf("invalid max_expiry_duration %q in global rule: %v", tempPolicy.Global.MaxExpiryDuration, err)
+			return fmt.Errorf("invalid max_expiry_duration %q in global rule: %v (format must be ISO-8601 duration, e.g. \"PT10M\" for 10 minutes, \"P7D\" for 7 days, \"P1DT12H\" for 1 day 12 hours)", tempPolicy.Global.MaxExpiryDuration, err)
 		}
 	}
 
diff --git a/pkg/version/version b/pkg/version/version
index 9e5a5c6..477e302 100644
--- a/pkg/version/version
+++ b/pkg/version/version
@@ -1 +1 @@
-v0.31.5
+v0.31.6