From cd514545d1d04615dd73a9d70db52bac41b9b849 Mon Sep 17 00:00:00 2001 From: Aramis Date: Sat, 10 Jun 2023 16:51:11 -0400 Subject: [PATCH] feat(config-loader): Make children of secret parents secret themselves. Signed-off-by: Aramis --- .changeset/selfish-coats-shout.md | 55 +++++ packages/config-loader/src/schema/collect.ts | 2 +- .../config-loader/src/schema/compile.test.ts | 214 +++++++++++++++++- packages/config-loader/src/schema/compile.ts | 71 +++++- 4 files changed, 327 insertions(+), 15 deletions(-) create mode 100644 .changeset/selfish-coats-shout.md diff --git a/.changeset/selfish-coats-shout.md b/.changeset/selfish-coats-shout.md new file mode 100644 index 0000000000..071c47e371 --- /dev/null +++ b/.changeset/selfish-coats-shout.md @@ -0,0 +1,55 @@ +--- +'@backstage/config-loader': minor +--- + +Adds a new `deepVisibility` schema keyword that sets child visibility recursively to the defined value, respecting preexisting values or child `deepVisibility`. + +Example usage: + +```ts +export interface Config { + /** + * Enforces a default of `secret` instead of `backend` for this object. + * @deepVisibility secret + */ + mySecretProperty: { + type: 'object'; + properties: { + secretValue: { + type: 'string'; + }; + + verySecretProperty: { + type: 'string'; + }; + }; + }; +} +``` + +Example of a schema that would not be allowed: + +```ts +export interface Config { + /** + * Set the top level property to secret, enforcing a default of `secret` instead of `backend` for this object. + * @deepVisibility secret + */ + mySecretProperty: { + type: 'object'; + properties: { + frontendUrl: { + /** + * We can NOT override the visibility to reveal a property to the front end. + * @visibility frontend + */ + type: 'string'; + }; + + verySecretProperty: { + type: 'string'; + }; + }; + }; +} +``` diff --git a/packages/config-loader/src/schema/collect.ts b/packages/config-loader/src/schema/collect.ts index 98feffebb2..ac3e4af341 100644 --- a/packages/config-loader/src/schema/collect.ts +++ b/packages/config-loader/src/schema/collect.ts @@ -190,7 +190,7 @@ async function compileTsSchemas(paths: string[]) { // This enables the use of these tags in TSDoc comments { required: true, - validationKeywords: ['visibility', 'deprecated'], + validationKeywords: ['visibility', 'deepVisibility', 'deprecated'], }, [path.split(sep).join('/')], // Unix paths are expected for all OSes here ) as JsonObject | null; diff --git a/packages/config-loader/src/schema/compile.test.ts b/packages/config-loader/src/schema/compile.test.ts index df81923684..914a813d9f 100644 --- a/packages/config-loader/src/schema/compile.test.ts +++ b/packages/config-loader/src/schema/compile.test.ts @@ -70,7 +70,7 @@ describe('compileConfigSchemas', () => { c: { type: 'string' }, d: { type: 'array', - visibility: 'secret', + visibility: 'frontend', items: { type: 'string', visibility: 'frontend' }, }, }, @@ -86,7 +86,7 @@ describe('compileConfigSchemas', () => { c: { type: 'string', visibility: 'backend' }, d: { type: 'array', - visibility: 'secret', + visibility: 'frontend', items: { type: 'string' }, }, }, @@ -102,7 +102,7 @@ describe('compileConfigSchemas', () => { Object.entries({ '/a': 'frontend', '/b': 'secret', - '/d': 'secret', + '/d': 'frontend', '/d/0': 'frontend', }), ), @@ -110,7 +110,7 @@ describe('compileConfigSchemas', () => { Object.entries({ '/properties/a': 'frontend', '/properties/b': 'secret', - '/properties/d': 'secret', + '/properties/d': 'frontend', '/properties/d/items': 'frontend', }), ), @@ -231,3 +231,209 @@ describe('compileConfigSchemas', () => { }); }); }); + +describe('deepVisibility', () => { + it('should pass secret visibility to children, but respect existing backend/secret visibility', () => { + const validate = compileConfigSchemas([ + { + path: 'a1', + value: { + type: 'object', + properties: { + a: { type: 'string', visibility: 'backend' }, + b: { type: 'string', visibility: 'backend' }, + c: { type: 'string' }, + d: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + { + path: 'a2', + value: { + type: 'object', + deepVisibility: 'secret', + properties: { + a: { type: 'string' }, + b: { type: 'string', visibility: 'secret' }, + c: { type: 'string', visibility: 'backend' }, + d: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + ]); + expect( + validate([ + { + data: { a: 'a', b: 'b', c: 'c', d: ['d'] }, + context: 'test', + }, + ]), + ).toEqual({ + visibilityByDataPath: new Map( + Object.entries({ + '': 'secret', + '/b': 'secret', + '/d': 'secret', + '/d/0': 'secret', + }), + ), + visibilityBySchemaPath: new Map( + Object.entries({ + '': 'secret', + '/properties/b': 'secret', + '/properties/d': 'secret', + '/properties/d/items': 'secret', + }), + ), + deprecationByDataPath: new Map(), + }); + }); + + it('should pass secret visibility to children, but throws when overriding with frontend visibility', () => { + expect(() => + compileConfigSchemas([ + { + path: 'a1', + value: { + type: 'object', + properties: { + a: { type: 'string', visibility: 'frontend' }, + b: { type: 'string', visibility: 'backend' }, + c: { type: 'string' }, + d: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + { + path: 'a2', + value: { + type: 'object', + deepVisibility: 'secret', + visibility: 'secret', + properties: { + a: { type: 'string' }, + b: { type: 'string', visibility: 'secret' }, + c: { type: 'string', visibility: 'frontend' }, + d: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + ]), + ).toThrow( + "Config schema visibility is both 'frontend' and 'secret' for /properties/a", + ); + }); + + it('should throw when children have a different deepVisibility', () => { + expect(() => + compileConfigSchemas([ + { + path: 'a1', + value: { + type: 'object', + properties: { + a: { + type: 'object', + properties: { + a: { type: 'string' }, + }, + }, + b: { type: 'string', visibility: 'backend' }, + c: { type: 'string' }, + d: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + { + path: 'a2', + value: { + type: 'object', + deepVisibility: 'secret', + properties: { + a: { + type: 'object', + properties: { + a: { type: 'string', visibility: 'frontend' }, + }, + }, + b: { type: 'string', visibility: 'secret' }, + c: { type: 'string', visibility: 'backend' }, + d: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + ]), + ).toThrow( + `Config schema visibility is both 'frontend' and 'secret' for /properties/a`, + ); + }); + + it('should throw when ancestor and children have a different deepVisibility', () => { + expect(() => + compileConfigSchemas([ + { + path: 'a1', + value: { + type: 'object', + properties: { + a: { + type: 'object', + properties: { + a: { type: 'string' }, + }, + }, + b: { type: 'string', visibility: 'backend' }, + c: { type: 'string' }, + d: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + { + path: 'a2', + value: { + type: 'object', + deepVisibility: 'secret', + properties: { + a: { + type: 'object', + deepVisibility: 'frontend', + properties: { + a: { type: 'string', visibility: 'frontend' }, + }, + }, + b: { type: 'string', visibility: 'secret' }, + c: { type: 'string', visibility: 'backend' }, + d: { + type: 'array', + items: { type: 'string' }, + }, + }, + }, + }, + ]), + ).toThrow( + `Config schema visibility is both 'frontend' and 'secret' for /properties/a`, + ); + }); +}); diff --git a/packages/config-loader/src/schema/compile.ts b/packages/config-loader/src/schema/compile.ts index 63a8e4b8a6..5d448a590a 100644 --- a/packages/config-loader/src/schema/compile.ts +++ b/packages/config-loader/src/schema/compile.ts @@ -74,6 +74,20 @@ export function compileConfigSchemas( }; }, }) + .addKeyword({ + keyword: 'deepVisibility', + metaSchema: { + type: 'string', + /** + * Disallow 'backend' deepVisibility to prevent cases of permission escaping. + * + * Something like: + * - deepVisibility secret -> backend -> frontend. + * - deepVisibility secret -> backend -> visibility frontend. + */ + enum: ['frontend', 'secret'], + }, + }) .removeKeyword('deprecated') // remove `deprecated` keyword so that we can implement our own compiler .addKeyword({ keyword: 'deprecated', @@ -101,17 +115,54 @@ export function compileConfigSchemas( const merged = mergeConfigSchemas(schemas.map(_ => _.value)); - if (options?.noUndeclaredProperties) { - traverse(merged, (schema: SchemaObject) => { - /** - * The `additionalProperties` key can only be applied to `type: object` in the JSON - * schema. - */ - if (schema?.type === 'object') { - schema.additionalProperties ||= false; + traverse( + merged, + ( + schema: SchemaObject, + jsonPtr: string, + _1: any, + _2: any, + _3?: any, + parentSchema?: SchemaObject, + ) => { + // Inherit parent deepVisibility if we don't define one ourselves. + if (parentSchema?.deepVisibility) { + schema.deepVisibility ??= parentSchema?.deepVisibility; } - }); - } + + // Apply deep visibility to self. + if (schema?.deepVisibility) { + // This runs before we compile the AJV so visibilityByDataPath has the + // correct data. + schema.visibility ??= schema.deepVisibility; + + if (parentSchema) { + /** + * Validate that we're not trying to override a child's visibility + * by setting the parent deep visibility. + */ + const values = [schema.visibility, parentSchema.visibility]; + const hasFrontend = values.some(e => e === 'frontend'); + const hasSecret = values.some(e => e === 'secret'); + if (hasFrontend && hasSecret) { + throw new Error( + `Config schema visibility is both 'frontend' and 'secret' for ${jsonPtr}`, + ); + } + } + } + + if (options?.noUndeclaredProperties) { + /** + * The `additionalProperties` key can only be applied to `type: object` in the JSON + * schema. + */ + if (schema?.type === 'object') { + schema.additionalProperties ||= false; + } + } + }, + ); const validate = ajv.compile(merged);