Disallow sensitive trait on members

Allowing @sensitive trait on members can lead to insecure usage patterns, like forgetting to put @sensitive on ALL members that target the same shape, which can lead to sensitive data not getting handled properly, e.g., not getting redacted from logs, etc. Instead marking the target shape as sensitive prevents modeling mistakes by ensuring every reference to data types that are inherently sensitive are always considered sensitive.
smithy-lang · Jun 13, 2022 · 876cd9b · 876cd9b
1 parent a69bc3c
commit 876cd9b
Show file tree

Hide file tree

Showing 25 changed files with 94 additions and 129 deletions.
diff --git a/docs/source/1.0/spec/core/documentation-traits.rst b/docs/source/1.0/spec/core/documentation-traits.rst
@@ -395,12 +395,12 @@ Conflicts with
 -------------------
 
 Summary
-    Indicates that the data stored in the shape or member is sensitive
-    and MUST be handled with care.
+    Indicates that the data stored in the shape is sensitive and MUST be
+    handled with care.
 Trait selector
-    ``:not(:is(service, operation, resource))``
+    ``:not(:is(service, operation, resource, member))``
 
-    *Any shape that is not a service, operation, or resource.*
+    *Any shape that is not a service, operation, resource, or member.*
 Value type
     Annotation trait
 

diff --git a/smithy-aws-protocol-tests/model/restJson1/validation/sensitive-validation.smithy b/smithy-aws-protocol-tests/model/restJson1/validation/sensitive-validation.smithy
@@ -47,7 +47,9 @@ apply SensitiveValidation @httpMalformedRequestTests([
 ])
 
 structure SensitiveValidationInput {
-    @sensitive
-    @suppress(["SensitiveTrait"])
-    string: PatternString
+    string: SensitivePatternString
 }
+
+@sensitive
+@pattern("^[a-m]+$")
+string SensitivePatternString
diff --git a/smithy-build/src/test/resources/software/amazon/smithy/build/resource-model.json b/smithy-build/src/test/resources/software/amazon/smithy/build/resource-model.json
@@ -40,7 +40,7 @@
                     "target": "ns.foo#MyResourceId",
                     "traits": {
                         "smithy.api#required": {},
-                        "smithy.api#sensitive": {}
+                        "smithy.api#pattern": "^[a-z]+$"
                     }
                 }
             }

diff --git a/...hy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java b/...hy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java
@@ -235,7 +235,7 @@ public void detectsInvalidListMemberChangesInNestedTargets() {
                    equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
                            + "`smithy.example#B1` (list) to `smithy.example#B2` (list). Both the old and new "
                            + "shapes are a list, but their members have differing traits. The newly targeted "
-                           + "shape now has the following additional traits: [smithy.api#sensitive]."));
+                           + "shape now has the following additional traits: [smithy.api#pattern]."));
     }
 
     @Test

diff --git a/...ces/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested1-b.smithy b/...ces/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested1-b.smithy
@@ -6,7 +6,7 @@ list A {
 }
 
 list B2 {
-    @sensitive
+    @pattern("^[a-z]+$")
     member: MyString
 }
 

diff --git a/...y-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java b/...y-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java
@@ -61,6 +61,7 @@
 import software.amazon.smithy.model.traits.PatternTrait;
 import software.amazon.smithy.model.traits.PrivateTrait;
 import software.amazon.smithy.model.traits.RangeTrait;
+import software.amazon.smithy.model.traits.SensitiveTrait;
 import software.amazon.smithy.model.traits.TitleTrait;
 import software.amazon.smithy.model.traits.UniqueItemsTrait;
 import software.amazon.smithy.utils.IoUtils;
@@ -550,6 +551,24 @@ public void supportsMapPatternPropertiesWithDefaultPattern() {
         assertThat(schema.getPatternProperties().get(".+").getType().get(), equalTo("string"));
     }
 
+    @Test
+    public void sensitiveTraitHasNoImpact() {
+        StringShape string1 = StringShape.builder()
+                .id("smithy.api#String")
+                .addTrait(new SensitiveTrait())
+                .build();
+        Model model1 = Model.builder().addShapes(string1).build();
+        SchemaDocument document1 = JsonSchemaConverter.builder().model(model1).build().convertShape(string1);
+
+        StringShape string2 = StringShape.builder()
+                .id("smithy.api#String")
+                .build();
+        Model model2 = Model.builder().addShapes(string2).build();
+        SchemaDocument document2 = JsonSchemaConverter.builder().model(model2).build().convertShape(string2);
+
+        assertThat(document1, equalTo(document2));
+    }
+
     @Test
     public void convertingToBuilderGivesSameResult() {
         Model model = Model.assembler()

diff --git a/smithy-jsonschema/src/test/resources/software/amazon/smithy/jsonschema/test-service.json b/smithy-jsonschema/src/test/resources/software/amazon/smithy/jsonschema/test-service.json
@@ -116,7 +116,10 @@
                 "abc": {
                     "target": "smithy.api#String",
                     "traits": {
-                        "smithy.api#sensitive":  {}
+                        "smithy.api#length":  {
+                            "min": 1,
+                            "max": 100
+                        }
                     }
                 },
                 "def": {
@@ -173,7 +176,7 @@
             "value": {
                 "target": "smithy.api#String",
                 "traits": {
-                    "smithy.api#sensitive":  {}
+                    "smithy.api#pattern": "^[a-z]+$"
                 }
             }
         },

diff --git a/...nschema/src/test/resources/software/amazon/smithy/jsonschema/test-service.jsonschema.json b/...nschema/src/test/resources/software/amazon/smithy/jsonschema/test-service.jsonschema.json
@@ -4,7 +4,9 @@
       "type": "object",
       "properties": {
         "abc": {
-          "type": "string"
+          "type": "string",
+          "minLength": 1,
+          "maxLength": 100
         },
         "def": {
           "type": "number"
@@ -64,7 +66,8 @@
     "Map": {
       "type": "object",
       "additionalProperties": {
-        "type": "string"
+        "type": "string",
+        "pattern": "^[a-z]+$"
       },
       "propertyNames": {
         "type": "string",

diff --git a/...main/java/software/amazon/smithy/model/validation/validators/SensitiveTraitValidator.java b/...main/java/software/amazon/smithy/model/validation/validators/SensitiveTraitValidator.java
diff --git a/...el/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator b/...el/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator
@@ -31,7 +31,6 @@ software.amazon.smithy.model.validation.validators.ResourceCycleValidator
 software.amazon.smithy.model.validation.validators.ResourceIdentifierBindingValidator
 software.amazon.smithy.model.validation.validators.ResourceIdentifierValidator
 software.amazon.smithy.model.validation.validators.ResourceLifecycleValidator
-software.amazon.smithy.model.validation.validators.SensitiveTraitValidator
 software.amazon.smithy.model.validation.validators.ServiceValidator
 software.amazon.smithy.model.validation.validators.ShapeIdConflictValidator
 software.amazon.smithy.model.validation.validators.ShapeRecursionValidator

diff --git a/smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy b/smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy
@@ -532,8 +532,8 @@ string resourceIdentifier
 @trait
 structure private {}
 
-/// Indicates that the data stored in the shape or member is sensitive and MUST be handled with care.
-@trait(selector: ":not(:test(service, operation, resource))")
+/// Indicates that the data stored in the shape is sensitive and MUST be handled with care.
+@trait(selector: ":not(:test(service, operation, resource, member))")
 structure sensitive {}
 
 /// Defines the version or date in which a shape or member was added to the model.

diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java
@@ -62,6 +62,7 @@
 import software.amazon.smithy.model.traits.DeprecatedTrait;
 import software.amazon.smithy.model.traits.DocumentationTrait;
 import software.amazon.smithy.model.traits.DynamicTrait;
+import software.amazon.smithy.model.traits.InternalTrait;
 import software.amazon.smithy.model.traits.MediaTypeTrait;
 import software.amazon.smithy.model.traits.SensitiveTrait;
 import software.amazon.smithy.model.traits.SuppressTrait;
@@ -547,7 +548,7 @@ public void allowsConflictingShapesThatAreEqual() {
                            + "}\n";
         String document2 = "namespace foo.baz\n"
                            + "structure Foo {\n"
-                           + "    @sensitive\n"
+                           + "    @internal\n"
                            + "    foo: String,\n"
                            + "}\n";
         ValidatedResult<Model> result = new ModelAssembler()
@@ -561,7 +562,7 @@ public void allowsConflictingShapesThatAreEqual() {
         StructureShape shape = result.unwrap().expectShape(ShapeId.from("foo.baz#Foo"), StructureShape.class);
         assertTrue(shape.hasTrait(DeprecatedTrait.class));
         assertTrue(shape.getMember("foo").isPresent());
-        assertTrue(shape.getMember("foo").get().hasTrait(SensitiveTrait.class));
+        assertTrue(shape.getMember("foo").get().hasTrait(InternalTrait.class));
     }
 
     @Test

diff --git a/...esources/software/amazon/smithy/model/errorfiles/validators/sensitive-trait-target.errors b/...esources/software/amazon/smithy/model/errorfiles/validators/sensitive-trait-target.errors
diff --git a/...esources/software/amazon/smithy/model/errorfiles/validators/sensitive-trait-target.smithy b/...esources/software/amazon/smithy/model/errorfiles/validators/sensitive-trait-target.smithy
diff --git a/.../test/resources/software/amazon/smithy/model/errorfiles/validators/sensitive-trait.errors b/.../test/resources/software/amazon/smithy/model/errorfiles/validators/sensitive-trait.errors
@@ -1 +1 @@
-[ERROR] ns.foo#Operation: Trait `sensitive` cannot be applied to `ns.foo#Operation`. This trait may only be applied to shapes that match the following selector: :not(:test(service, operation, resource)) | TraitTarget
+[ERROR] ns.foo#Operation: Trait `sensitive` cannot be applied to `ns.foo#Operation`. This trait may only be applied to shapes that match the following selector: :not(:test(service, operation, resource, member)) | TraitTarget
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/list-and-sets.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/list-and-sets.json
@@ -37,7 +37,7 @@
             "member": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
@@ -51,7 +51,7 @@
             "member": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
@@ -65,7 +65,7 @@
             "member": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
@@ -110,7 +110,7 @@
             "member": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
@@ -124,7 +124,7 @@
             "member": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
@@ -138,7 +138,7 @@
             "member": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },

diff --git a/...y-model/src/test/resources/software/amazon/smithy/model/loader/valid/list-and-sets.smithy b/...y-model/src/test/resources/software/amazon/smithy/model/loader/valid/list-and-sets.smithy
@@ -21,21 +21,21 @@ list D {
 @deprecated
 @since("1.0")
 list E {
-  @sensitive @since("1.1")
+  @internal @since("1.1")
   member: String
 }
 
 @deprecated @since("1.0")
 list F {
-  @sensitive
+  @internal
   @since("1.1")
   member: String
 }
 
 @deprecated @since("1.0")
 list G
 {
-@sensitive
+@internal
 @since("1.1")
 member: String
 }
@@ -58,15 +58,15 @@ set K {
 
 @deprecated
 @since("1.0")
-set L {@sensitive @since("1.1") member: String}
+set L {@internal @since("1.1") member: String}
 
 @deprecated @since("1.0")
-set M {@sensitive @since("1.1") member: String }
+set M {@internal @since("1.1") member: String }
 
 @deprecated @since("1.0")
 set N
 {
-@sensitive
+@internal
 @since("1.1")
 member: String
 }
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/maps.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/maps.json
@@ -49,7 +49,7 @@
             "key": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
@@ -66,14 +66,14 @@
             "key": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
             "value": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
@@ -87,7 +87,7 @@
             "key": {
                 "target": "com.example#String",
                 "traits": {
-                    "smithy.api#sensitive": {},
+                    "smithy.api#internal": {},
                     "smithy.api#since": "1.1"
                 }
             },
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		[ERROR] ns.foo#Operation: Trait `sensitive` cannot be applied to `ns.foo#Operation`. This trait may only be applied to shapes that match the following selector: :not(:test(service, operation, resource)) \| TraitTarget
		[ERROR] ns.foo#Operation: Trait `sensitive` cannot be applied to `ns.foo#Operation`. This trait may only be applied to shapes that match the following selector: :not(:test(service, operation, resource, member)) \| TraitTarget