PR suggestion: move isSafe check as a functional-contsraint

zapier · Jan 29, 2025 · 6ed5ac8 · 6ed5ac8
1 parent 09e53fd
commit 6ed5ac8
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 35 deletions.
diff --git a/packages/schema/exported-schema.json b/packages/schema/exported-schema.json
@@ -465,24 +465,7 @@
           "type": "boolean"
         }
       },
-      "additionalProperties": false,
-      "not": {
-        "properties": {
-          "key": {
-            "enum": [
-              "access_token",
-              "refresh_token",
-              "api_key",
-              "password",
-              "secret"
-            ]
-          },
-          "isSafe": {
-            "const": true
-          }
-        },
-        "required": ["key", "isSafe"]
-      }
+      "additionalProperties": false
     },
     "RequestSchema": {
       "id": "/RequestSchema",

diff --git a/packages/schema/lib/functional-constraints/AuthFieldisSafe.js b/packages/schema/lib/functional-constraints/AuthFieldisSafe.js
@@ -0,0 +1,64 @@
+'use strict';
+
+const jsonschema = require('jsonschema');
+
+const AUTH_FIELD_ID = '/AuthFieldSchema';
+const AUTH_FIELDS_ID = '/AuthFieldsSchema';
+
+const FORBIDDEN_KEYS = [
+  'access_token',
+  'refresh_token',
+  'api_key',
+  'password',
+  'secret',
+];
+
+const checkAuthField = (field) => {
+  const errors = [];
+
+  if (FORBIDDEN_KEYS.includes(field.key) && field.isSafe === true) {
+    errors.push(
+      new jsonschema.ValidationError(
+        `Cannot set isSafe=true for the sensitive key "${field.key}".`,
+        field,
+        '/AuthFieldSchema',
+        'instance.key',
+        'sensitive',
+        'key',
+      ),
+    );
+  }
+
+  return errors;
+};
+
+module.exports = (definition, mainSchema) => {
+  const errors = [];
+  // Done to validate anti-examples declaratively defined in the schema
+  if ([AUTH_FIELD_ID, AUTH_FIELDS_ID].includes(mainSchema.id)) {
+    const definitions = Array.isArray(definition) ? definition : [definition];
+
+    definitions.forEach((field, index) => {
+      checkAuthField(field).forEach((err) => {
+        err.property = `instance[${index}]`;
+        err.stack = err.stack.replace('instance.key', `instance[${index}].key`);
+        errors.push(err);
+      });
+    });
+  }
+
+  // If there's no authentication or no fields, we have nothing to check
+  if (!definition.authentication || !definition.authentication.fields) {
+    return errors;
+  }
+
+  definition.authentication.fields.forEach((field, index) => {
+    checkAuthField(field).forEach((err) => {
+      err.property = `instance[${index}]`;
+      err.stack = err.stack.replace('instance.key', `instance[${index}].key`);
+      errors.push(err);
+    });
+  });
+
+  return errors;
+};
diff --git a/packages/schema/lib/functional-constraints/index.js b/packages/schema/lib/functional-constraints/index.js
@@ -21,6 +21,7 @@ const checks = [
   require('./bufferedCreateConstraints'),
   require('./requirePerformConditionally'),
   require('./pollingThrottle'),
+  require('./AuthFieldisSafe'),
 ];
 
 const runFunctionalConstraints = (definition, mainSchema) => {

diff --git a/packages/schema/lib/schemas/AuthFieldSchema.js b/packages/schema/lib/schemas/AuthFieldSchema.js
@@ -17,22 +17,6 @@ module.exports = makeSchema(
       },
     },
 
-    // Do not allow certain isSafe = True if sensitive tokens exists
-    not: {
-      properties: {
-        key: {
-          enum: [
-            'access_token',
-            'refresh_token',
-            'api_key',
-            'password',
-            'secret',
-          ],
-        },
-        isSafe: { const: true },
-      },
-      required: ['key', 'isSafe'],
-    },
     // Add examples and anti-examples specifically for basic & custom auth
     examples: [
       // Basic Auth - email & password

diff --git a/packages/schema/lib/utils/makeValidator.js b/packages/schema/lib/utils/makeValidator.js
@@ -64,6 +64,7 @@ const processBaseError = (err, path) => {
  */
 const cleanError = (validationError, path, validator, definition) => {
   if (ambiguousTypes.includes(validationError.name)) {
+    console.log(validationError, 'hereeeee');
     // flatObjectSchema requires each property to be a type. instead of recursing down, it's more valuable to say "hey, it's not of these types"
     if (validationError.argument.every((s) => s.includes('subschema'))) {
       return processBaseError(validationError, path);

diff --git a/packages/schema/test/index.js b/packages/schema/test/index.js
@@ -282,12 +282,16 @@ describe('app', () => {
       const results = schema.validateAppDefinition(appCopy);
 
       // Expect at least one error because "password" can't have isSafe = true
+      console.log(results);
       results.errors.should.have.length(1);
       should(results.valid).eql(false);
 
       const [error] = results.errors;
       // Check that the error specifically complains about a "not" rule or something similar
-      error.name.should.equal('not');
+      error.name.should.equal('sensitive');
+      error.message.should.equal(
+        'Cannot set isSafe=true for the sensitive key "password".',
+      );
     });
   });