diff --git a/packages/kbn-api-contracts/README.md b/packages/kbn-api-contracts/README.md index b382125983fa8..2e4853c2a07a7 100644 --- a/packages/kbn-api-contracts/README.md +++ b/packages/kbn-api-contracts/README.md @@ -55,40 +55,21 @@ oasdiff detects these as breaking: | **Required property added** | `new-required-request-property` | New required `email` field on request body | | **Optional made required** | `request-parameter-became-required` | `filter` query param becomes required | | **Type changed** | `response-property-type-changed` | `id` changed from string to number | +| **Request body tightened** | `kbn:request-additional-properties-tightened` | Request body schema gains `additionalProperties: false` (clients sending unknown keys now receive 400) | ⚠️ oasdiff classifies these as warnings, but they are promoted to blocking here because Terraform provider configurations depend on these fields. ## Allowlist -For approved breaking changes, add entries to `allowlist.json`: +For approved breaking changes, add entries to `allowlist.json`. **Always prefer the granular form below** — it scopes suppression to one specific breaking change instead of muting everything on the endpoint. -```json -{ - "entries": [ - { - "path": "/api/saved_objects/{type}/{id}", - "method": "delete", - "reason": "Intentional removal as part of saved objects migration", - "approvedBy": "elastic/kibana-core", - "prUrl": "https://github.com/elastic/kibana/pull/12345", - "expiresAt": "2026-12-31" - } - ] -} -``` - -**Required fields:** `path`, `method`, `reason`, `approvedBy` -**Optional fields:** `prUrl`, `expiresAt`, `oasdiffId`, `source` - -### Granular suppression +### Granular form (recommended) -By default, an allowlist entry suppresses **all** breaking changes for a given `(path, method)`. To scope suppression to a specific breaking change, use `oasdiffId` and/or `source`: +Use `oasdiffId` together with `source` to suppress exactly one breaking change. These fields are AND'd with `path` and `method`: the entry only matches changes for which all four fields agree. -- `oasdiffId` — matches the oasdiff rule ID (e.g. `request-property-removed`). See the [Breaking Change Rules](#breaking-change-rules) table for known IDs. +- `oasdiffId` — matches the oasdiff rule ID (e.g. `request-property-removed`, `kbn:request-additional-properties-tightened`). See the [Breaking Change Rules](#breaking-change-rules) table for known IDs. - `source` — matches the JSON pointer / source location reported by oasdiff (e.g. `/components/schemas/Output/properties/name`). -When present, these fields are AND'd: the entry only suppresses changes that match **all** specified fields. Omitted fields are not checked (legacy behavior). - ```json { "path": "/api/fleet/outputs", @@ -100,7 +81,40 @@ When present, these fields are AND'd: the entry only suppresses changes that mat } ``` -This entry only suppresses `request-property-removed` at that specific schema location — other breaking changes on the same endpoint will still be flagged. +Example targeting the new request-body tightening rule: + +```json +{ + "path": "/api/data_views/data_view", + "method": "post", + "reason": "Intentional tightening — coordinated with @elastic/terraform-provider", + "approvedBy": "@elastic/kibana-data-discovery", + "oasdiffId": "kbn:request-additional-properties-tightened", + "source": "/components/schemas/Data_views_create_data_view_request_object" +} +``` + +**Required fields:** `path`, `method`, `reason`, `approvedBy`, `oasdiffId`, `source` (the last two only required for granular suppression). +**Optional fields:** `prUrl`, `expiresAt`. + +### Coarse form (⚠️ avoid unless absolutely necessary — this masks all future breaks on the endpoint) + +Omitting `oasdiffId` and `source` makes the entry suppress **every** breaking change for that `(path, method)`. This is dangerous: a coarse entry approved today silently swallows any unrelated tightening, removal, or type change that lands on the same endpoint in the future. Reach for it only when several distinct, approved changes ship together and a single granular entry per change is impractical. + +```json +{ + "entries": [ + { + "path": "/api/saved_objects/{type}/{id}", + "method": "delete", + "reason": "Intentional removal as part of saved objects migration", + "approvedBy": "elastic/kibana-core", + "prUrl": "https://github.com/elastic/kibana/pull/12345", + "expiresAt": "2026-12-31" + } + ] +} +``` ## API Ownership diff --git a/packages/kbn-api-contracts/scripts/check_contracts.test.ts b/packages/kbn-api-contracts/scripts/check_contracts.test.ts index cca1cd49db539..05f1b6af2b448 100644 --- a/packages/kbn-api-contracts/scripts/check_contracts.test.ts +++ b/packages/kbn-api-contracts/scripts/check_contracts.test.ts @@ -25,10 +25,23 @@ jest.mock('../src/diff/run_oasdiff', () => ({ runOasdiff: jest.fn(), })); +jest.mock('../src/diff/run_oasdiff_structural', () => ({ + runOasdiffStructural: jest.fn(), +})); + jest.mock('../src/diff/parse_oasdiff', () => ({ parseOasdiff: jest.fn(), })); +jest.mock('../src/input/load_oas', () => ({ + loadOas: jest.fn().mockResolvedValue({ + openapi: '3.0.0', + info: { title: 't', version: '1' }, + paths: {}, + components: { schemas: {} }, + }), +})); + jest.mock('../src/terraform/check_terraform_impact', () => ({ checkTerraformImpact: jest.fn(), })); @@ -55,8 +68,9 @@ jest.mock('../src/report/format_failure', () => ({ import { execSync } from 'child_process'; import { writeFileSync, rmSync } from 'fs'; -import { runOasdiff, parseOasdiff, applyAllowlist } from '../src/diff'; +import { runOasdiff, runOasdiffStructural, parseOasdiff, applyAllowlist } from '../src/diff'; import type { BreakingChange } from '../src/diff'; +import { loadOas } from '../src/input/load_oas'; import { checkTerraformImpact } from '../src/terraform/check_terraform_impact'; import { loadAllowlist } from '../src/allowlist/load_allowlist'; import { formatFailure } from '../src/report/format_failure'; @@ -65,7 +79,11 @@ const mockRun = jest.requireMock('@kbn/dev-cli-runner').run as jest.Mock; const mockExecSync = execSync as jest.MockedFunction; const mockWriteFileSync = writeFileSync as jest.MockedFunction; const mockRunOasdiff = runOasdiff as jest.MockedFunction; +const mockRunOasdiffStructural = runOasdiffStructural as jest.MockedFunction< + typeof runOasdiffStructural +>; const mockParseOasdiff = parseOasdiff as jest.MockedFunction; +const mockLoadOas = loadOas as jest.MockedFunction; const mockCheckTerraformImpact = checkTerraformImpact as jest.MockedFunction< typeof checkTerraformImpact >; @@ -98,6 +116,12 @@ describe('check_contracts', () => { error: jest.fn(), }; mockExecSync.mockReturnValue('openapi: 3.0.0\npaths: {}'); + mockLoadOas.mockResolvedValue({ + openapi: '3.0.0', + info: { title: 't', version: '1' }, + paths: {}, + components: { schemas: {} }, + }); }); const defaultFlags = { @@ -424,4 +448,88 @@ describe('check_contracts', () => { expect(mockLog.info).toHaveBeenCalledWith('1 allowlisted change(s) ignored'); }); + + describe('additionalProperties tightening (E2E reverse-index path)', () => { + const realParseOasdiff = jest.requireActual('../src/diff/parse_oasdiff') + .parseOasdiff as typeof parseOasdiff; + + it('surfaces a synthetic component-level entry exactly once for the consumer endpoint', async () => { + const consumerPath = '/api/data_views/data_view'; + const componentName = 'Data_views_create_data_view_request_object'; + + mockLoadOas.mockResolvedValue({ + openapi: '3.0.0', + info: { title: 't', version: '1' }, + paths: { + [consumerPath]: { + post: { + requestBody: { + content: { + 'application/json': { + schema: { $ref: `#/components/schemas/${componentName}` }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + [componentName]: { type: 'object' }, + }, + }, + }); + + mockRunOasdiff.mockReturnValue([]); + mockRunOasdiffStructural.mockReturnValue({ + components: { + schemas: { + modified: { + [componentName]: { + additionalPropertiesAllowed: { from: null, to: false }, + }, + }, + }, + }, + }); + + // Use the real parseOasdiff so the synthetic entry flows through the + // same parse pipeline as oasdiff entries; pass-through applyAllowlist so + // nothing is filtered out. + mockParseOasdiff.mockImplementation(realParseOasdiff); + mockApplyAllowlist.mockImplementation((changes) => ({ + breakingChanges: changes, + allowlistedChanges: [], + })); + + mockCheckTerraformImpact.mockImplementation((changes) => ({ + hasImpact: changes.length > 0, + impactedChanges: changes.map((change) => ({ + change, + terraformResource: 'elasticstack_kibana_data_view', + owners: ['@elastic/kibana-data-discovery'], + })), + })); + mockLoadAllowlist.mockReturnValue({ entries: [] }); + mockFormatFailure.mockReturnValue('FAILURE REPORT'); + + await expect(runCallback({ flags: defaultFlags, log: mockLog })).rejects.toThrow( + 'Found 1 breaking change(s)' + ); + + expect(mockFormatFailure).toHaveBeenCalledTimes(1); + const breakingChanges = mockFormatFailure.mock.calls[0][0]; + expect(breakingChanges).toEqual([ + { + type: 'request_body_tightened', + path: consumerPath, + method: 'POST', + reason: + 'Request body schema disallows extra fields (additionalProperties: false). Clients sending unknown keys will now receive 400.', + oasdiffId: 'kbn:request-additional-properties-tightened', + source: `/components/schemas/${componentName}`, + }, + ]); + }); + }); }); diff --git a/packages/kbn-api-contracts/scripts/check_contracts.ts b/packages/kbn-api-contracts/scripts/check_contracts.ts index 1637fd157b492..9f261d84a208d 100644 --- a/packages/kbn-api-contracts/scripts/check_contracts.ts +++ b/packages/kbn-api-contracts/scripts/check_contracts.ts @@ -11,7 +11,15 @@ import { execSync } from 'child_process'; import { writeFileSync, mkdirSync, rmSync } from 'fs'; import { resolve } from 'path'; import { run } from '@kbn/dev-cli-runner'; -import { runOasdiff, parseOasdiff, applyAllowlist } from '../src/diff'; +import { + runOasdiff, + runOasdiffStructural, + parseOasdiff, + applyAllowlist, + buildRequestBodyIndex, + detectAdditionalPropertiesTightening, +} from '../src/diff'; +import { loadOas } from '../src/input/load_oas'; import { formatFailure } from '../src/report/format_failure'; import { writeImpactReport } from '../src/report/write_impact_report'; import { loadAllowlist } from '../src/allowlist/load_allowlist'; @@ -185,8 +193,10 @@ run( log.info(`Filtering oasdiff to ${terraformApis.length} Terraform provider API paths`); } let diffEntries; + let structuralDiff: unknown; try { diffEntries = runOasdiff(basePath, currentPath, { matchPath }); + structuralDiff = runOasdiffStructural(basePath, currentPath, { matchPath }); } catch (error: unknown) { // Some older branch specs (e.g. 9.3) have example objects incorrectly // placed under `#/components/schemas/` instead of `#/components/examples/`. @@ -202,7 +212,17 @@ run( } throw error; } - const allBreakingChanges = parseOasdiff(diffEntries); + + const currentOas = await loadOas(currentPath); + const requestBodyIndex = buildRequestBodyIndex(currentOas); + const { entries: syntheticEntries, warnings: detectorWarnings } = + detectAdditionalPropertiesTightening(structuralDiff, requestBodyIndex); + + for (const warning of detectorWarnings) { + log.warning(warning); + } + + const allBreakingChanges = parseOasdiff([...diffEntries, ...syntheticEntries]); if (allBreakingChanges.length === 0) { log.success('No breaking changes detected'); diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/component_body_tightened/base.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/component_body_tightened/base.yaml new file mode 100644 index 0000000000000..99ea2d2c9807c --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/component_body_tightened/base.yaml @@ -0,0 +1,22 @@ +openapi: 3.0.0 +info: + title: component_body_tightened + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/Body' + responses: + '200': + description: ok +components: + schemas: + Body: + type: object + properties: + a: + type: string diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/component_body_tightened/current.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/component_body_tightened/current.yaml new file mode 100644 index 0000000000000..c879b32a6d664 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/component_body_tightened/current.yaml @@ -0,0 +1,23 @@ +openapi: 3.0.0 +info: + title: component_body_tightened + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/Body' + responses: + '200': + description: ok +components: + schemas: + Body: + type: object + additionalProperties: false + properties: + a: + type: string diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/inline_body_tightened/base.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/inline_body_tightened/base.yaml new file mode 100644 index 0000000000000..07de8ed02425c --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/inline_body_tightened/base.yaml @@ -0,0 +1,18 @@ +openapi: 3.0.0 +info: + title: inline_body_tightened + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + type: object + properties: + a: + type: string + responses: + '200': + description: ok diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/inline_body_tightened/current.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/inline_body_tightened/current.yaml new file mode 100644 index 0000000000000..d80769c87f8b4 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/inline_body_tightened/current.yaml @@ -0,0 +1,19 @@ +openapi: 3.0.0 +info: + title: inline_body_tightened + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + type: object + additionalProperties: false + properties: + a: + type: string + responses: + '200': + description: ok diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/loosening_no_flag/base.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/loosening_no_flag/base.yaml new file mode 100644 index 0000000000000..8037730b9fcd4 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/loosening_no_flag/base.yaml @@ -0,0 +1,19 @@ +openapi: 3.0.0 +info: + title: loosening_no_flag + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + type: object + additionalProperties: false + properties: + a: + type: string + responses: + '200': + description: ok diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/loosening_no_flag/current.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/loosening_no_flag/current.yaml new file mode 100644 index 0000000000000..5e6d82dfbb1ef --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/loosening_no_flag/current.yaml @@ -0,0 +1,18 @@ +openapi: 3.0.0 +info: + title: loosening_no_flag + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + type: object + properties: + a: + type: string + responses: + '200': + description: ok diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/nested_body_tightened/base.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/nested_body_tightened/base.yaml new file mode 100644 index 0000000000000..17cddc7929b6c --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/nested_body_tightened/base.yaml @@ -0,0 +1,21 @@ +openapi: 3.0.0 +info: + title: nested_body_tightened + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + type: object + properties: + nested: + type: object + properties: + x: + type: string + responses: + '200': + description: ok diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/nested_body_tightened/current.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/nested_body_tightened/current.yaml new file mode 100644 index 0000000000000..7af55075c4f69 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/nested_body_tightened/current.yaml @@ -0,0 +1,22 @@ +openapi: 3.0.0 +info: + title: nested_body_tightened + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + type: object + properties: + nested: + type: object + additionalProperties: false + properties: + x: + type: string + responses: + '200': + description: ok diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/oneof_body_tightened/base.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/oneof_body_tightened/base.yaml new file mode 100644 index 0000000000000..c394e4836eeec --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/oneof_body_tightened/base.yaml @@ -0,0 +1,29 @@ +openapi: 3.0.0 +info: + title: oneof_body_tightened + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + oneOf: + - $ref: '#/components/schemas/A' + - $ref: '#/components/schemas/B' + responses: + '200': + description: ok +components: + schemas: + A: + type: object + properties: + a: + type: string + B: + type: object + properties: + b: + type: string diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/oneof_body_tightened/current.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/oneof_body_tightened/current.yaml new file mode 100644 index 0000000000000..6cc033d24d768 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/oneof_body_tightened/current.yaml @@ -0,0 +1,30 @@ +openapi: 3.0.0 +info: + title: oneof_body_tightened + version: '1' +paths: + /api/test: + post: + requestBody: + content: + application/json: + schema: + oneOf: + - $ref: '#/components/schemas/A' + - $ref: '#/components/schemas/B' + responses: + '200': + description: ok +components: + schemas: + A: + type: object + additionalProperties: false + properties: + a: + type: string + B: + type: object + properties: + b: + type: string diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/response_only_no_flag/base.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/response_only_no_flag/base.yaml new file mode 100644 index 0000000000000..57566fb174d96 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/response_only_no_flag/base.yaml @@ -0,0 +1,21 @@ +openapi: 3.0.0 +info: + title: response_only_no_flag + version: '1' +paths: + /api/test: + get: + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '#/components/schemas/Body' +components: + schemas: + Body: + type: object + properties: + a: + type: string diff --git a/packages/kbn-api-contracts/src/diff/__fixtures__/oas/response_only_no_flag/current.yaml b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/response_only_no_flag/current.yaml new file mode 100644 index 0000000000000..70a3fdbf84773 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/__fixtures__/oas/response_only_no_flag/current.yaml @@ -0,0 +1,22 @@ +openapi: 3.0.0 +info: + title: response_only_no_flag + version: '1' +paths: + /api/test: + get: + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '#/components/schemas/Body' +components: + schemas: + Body: + type: object + additionalProperties: false + properties: + a: + type: string diff --git a/packages/kbn-api-contracts/src/diff/breaking_rules.ts b/packages/kbn-api-contracts/src/diff/breaking_rules.ts index 331b12629d354..4a7b7cb1ee1c1 100644 --- a/packages/kbn-api-contracts/src/diff/breaking_rules.ts +++ b/packages/kbn-api-contracts/src/diff/breaking_rules.ts @@ -16,7 +16,8 @@ export interface BreakingChange { | 'request_property_removed' | 'response_property_removed' | 'parameter_removed' - | 'operation_breaking'; + | 'operation_breaking' + | 'request_body_tightened'; path: string; method?: string; reason: string; diff --git a/packages/kbn-api-contracts/src/diff/build_request_body_index.test.ts b/packages/kbn-api-contracts/src/diff/build_request_body_index.test.ts new file mode 100644 index 0000000000000..ae409e5f49004 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/build_request_body_index.test.ts @@ -0,0 +1,221 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { resolve } from 'path'; +import { loadOas } from '../input/load_oas'; +import { buildRequestBodyIndex } from './build_request_body_index'; +import type { OpenAPISpec } from '../input/load_oas'; + +const FIXTURES_DIR = resolve(__dirname, '__fixtures__', 'oas'); + +const fixturePath = (name: string, file: 'base' | 'current'): string => + resolve(FIXTURES_DIR, name, `${file}.yaml`); + +describe('buildRequestBodyIndex', () => { + it('returns an empty index for a spec with no request bodies', async () => { + const oas = await loadOas(fixturePath('response_only_no_flag', 'current')); + const index = buildRequestBodyIndex(oas); + expect(index.size).toBe(0); + }); + + it('records a direct $ref consumer (component → operation)', async () => { + const oas = await loadOas(fixturePath('component_body_tightened', 'current')); + const index = buildRequestBodyIndex(oas); + expect(index.get('Body')).toEqual([{ path: '/api/test', method: 'POST' }]); + }); + + it('records consumers from oneOf branches', async () => { + const oas = await loadOas(fixturePath('oneof_body_tightened', 'current')); + const index = buildRequestBodyIndex(oas); + expect(index.get('A')).toEqual([{ path: '/api/test', method: 'POST' }]); + expect(index.get('B')).toEqual([{ path: '/api/test', method: 'POST' }]); + }); + + it('records consumers from anyOf branches', () => { + const oas: OpenAPISpec = { + openapi: '3.0.0', + info: { title: 't', version: '1' }, + paths: { + '/api/test': { + post: { + requestBody: { + content: { + 'application/json': { + schema: { + anyOf: [{ $ref: '#/components/schemas/A' }, { $ref: '#/components/schemas/B' }], + }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + A: { type: 'object' }, + B: { type: 'object' }, + }, + }, + }; + const index = buildRequestBodyIndex(oas); + expect(index.get('A')).toEqual([{ path: '/api/test', method: 'POST' }]); + expect(index.get('B')).toEqual([{ path: '/api/test', method: 'POST' }]); + }); + + it('records consumers from allOf branches', () => { + const oas: OpenAPISpec = { + openapi: '3.0.0', + info: { title: 't', version: '1' }, + paths: { + '/api/test': { + put: { + requestBody: { + content: { + 'application/json': { + schema: { + allOf: [{ $ref: '#/components/schemas/A' }, { type: 'object' }], + }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + A: { type: 'object' }, + }, + }, + }; + const index = buildRequestBodyIndex(oas); + expect(index.get('A')).toEqual([{ path: '/api/test', method: 'PUT' }]); + }); + + it('records transitively reachable components via properties, items, and additionalProperties', () => { + const oas: OpenAPISpec = { + openapi: '3.0.0', + info: { title: 't', version: '1' }, + paths: { + '/api/test': { + post: { + requestBody: { + content: { + 'application/json': { + schema: { $ref: '#/components/schemas/Top' }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + Top: { + type: 'object', + properties: { + viaProp: { $ref: '#/components/schemas/ViaProp' }, + viaArray: { + type: 'array', + items: { $ref: '#/components/schemas/ViaItems' }, + }, + viaMap: { + type: 'object', + additionalProperties: { $ref: '#/components/schemas/ViaAddProps' }, + }, + }, + }, + ViaProp: { type: 'object' }, + ViaItems: { type: 'object' }, + ViaAddProps: { type: 'object' }, + }, + }, + }; + const index = buildRequestBodyIndex(oas); + expect(index.get('Top')).toEqual([{ path: '/api/test', method: 'POST' }]); + expect(index.get('ViaProp')).toEqual([{ path: '/api/test', method: 'POST' }]); + expect(index.get('ViaItems')).toEqual([{ path: '/api/test', method: 'POST' }]); + expect(index.get('ViaAddProps')).toEqual([{ path: '/api/test', method: 'POST' }]); + }); + + it('terminates on circular $ref chains', () => { + const oas: OpenAPISpec = { + openapi: '3.0.0', + info: { title: 't', version: '1' }, + paths: { + '/api/test': { + post: { + requestBody: { + content: { + 'application/json': { + schema: { $ref: '#/components/schemas/A' }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + A: { + type: 'object', + properties: { + b: { $ref: '#/components/schemas/B' }, + }, + }, + B: { + type: 'object', + properties: { + a: { $ref: '#/components/schemas/A' }, + }, + }, + }, + }, + }; + const index = buildRequestBodyIndex(oas); + expect(index.get('A')).toEqual([{ path: '/api/test', method: 'POST' }]); + expect(index.get('B')).toEqual([{ path: '/api/test', method: 'POST' }]); + }); + + it('excludes components used only in responses', async () => { + const oas = await loadOas(fixturePath('response_only_no_flag', 'current')); + const index = buildRequestBodyIndex(oas); + expect(index.has('Body')).toBe(false); + }); + + it('aggregates multiple consumers for the same component', () => { + const oas: OpenAPISpec = { + openapi: '3.0.0', + info: { title: 't', version: '1' }, + paths: { + '/api/a': { + post: { + requestBody: { + content: { 'application/json': { schema: { $ref: '#/components/schemas/Body' } } }, + }, + }, + }, + '/api/b': { + put: { + requestBody: { + content: { 'application/json': { schema: { $ref: '#/components/schemas/Body' } } }, + }, + }, + }, + }, + components: { + schemas: { Body: { type: 'object' } }, + }, + }; + const index = buildRequestBodyIndex(oas); + expect(index.get('Body')).toEqual([ + { path: '/api/a', method: 'POST' }, + { path: '/api/b', method: 'PUT' }, + ]); + }); +}); diff --git a/packages/kbn-api-contracts/src/diff/build_request_body_index/collect_reachable_components.ts b/packages/kbn-api-contracts/src/diff/build_request_body_index/collect_reachable_components.ts new file mode 100644 index 0000000000000..eee925bbe107c --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/build_request_body_index/collect_reachable_components.ts @@ -0,0 +1,62 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { isRecord } from '../is_record'; + +const REF_PREFIX = '#/components/schemas/'; + +const refComponentName = (schema: Record): string | undefined => { + const ref = schema.$ref; + return typeof ref === 'string' && ref.startsWith(REF_PREFIX) + ? ref.slice(REF_PREFIX.length) + : undefined; +}; + +const compositionBranches = (schema: Record): unknown[] => + (['allOf', 'oneOf', 'anyOf'] as const).flatMap((key) => + Array.isArray(schema[key]) ? (schema[key] as unknown[]) : [] + ); + +const propertyValues = (schema: Record): unknown[] => + isRecord(schema.properties) ? Object.values(schema.properties) : []; + +const arrayItems = (schema: Record): unknown[] => + schema.items === undefined ? [] : [schema.items]; + +const additionalPropertiesSchema = (schema: Record): unknown[] => + isRecord(schema.additionalProperties) ? [schema.additionalProperties] : []; + +const childSchemas = (schema: Record): unknown[] => [ + ...compositionBranches(schema), + ...propertyValues(schema), + ...arrayItems(schema), + ...additionalPropertiesSchema(schema), +]; + +export const collectReachableComponents = ( + schema: unknown, + seen: Set, + out: Set, + componentSchemas: Record +): void => { + if (!isRecord(schema)) return; + + const name = refComponentName(schema); + if (name !== undefined) { + if (seen.has(name)) return; + seen.add(name); + out.add(name); + collectReachableComponents(componentSchemas[name], seen, out, componentSchemas); + return; + } + + childSchemas(schema).forEach((child) => + collectReachableComponents(child, seen, out, componentSchemas) + ); +}; diff --git a/packages/kbn-api-contracts/src/diff/build_request_body_index/extract_request_body_consumers.ts b/packages/kbn-api-contracts/src/diff/build_request_body_index/extract_request_body_consumers.ts new file mode 100644 index 0000000000000..b2692062ccd57 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/build_request_body_index/extract_request_body_consumers.ts @@ -0,0 +1,41 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { get } from 'lodash'; +import type { OpenAPISpec } from '../../input/load_oas'; +import { isRecord } from '../is_record'; +import type { RequestBodyConsumer } from './types'; + +const HTTP_METHODS = ['get', 'post', 'put', 'patch', 'delete', 'options', 'head', 'trace'] as const; + +export interface RequestBodySchemaContext { + schema: unknown; + consumer: RequestBodyConsumer; +} + +const requestBodySchemas = ( + pathName: string, + pathEntry: unknown, + method: (typeof HTTP_METHODS)[number] +): RequestBodySchemaContext[] => { + const content = get(pathEntry, [method, 'requestBody', 'content']); + if (!isRecord(content)) return []; + const consumer: RequestBodyConsumer = { path: pathName, method: method.toUpperCase() }; + return Object.values(content) + .map((mediaEntry) => get(mediaEntry, 'schema')) + .filter((schema) => schema !== undefined) + .map((schema) => ({ schema, consumer })); +}; + +export const extractRequestBodyConsumers = (oas: OpenAPISpec): RequestBodySchemaContext[] => { + if (!isRecord(oas.paths)) return []; + return Object.entries(oas.paths).flatMap(([pathName, pathEntry]) => + HTTP_METHODS.flatMap((method) => requestBodySchemas(pathName, pathEntry, method)) + ); +}; diff --git a/packages/kbn-api-contracts/src/diff/build_request_body_index/index.ts b/packages/kbn-api-contracts/src/diff/build_request_body_index/index.ts new file mode 100644 index 0000000000000..612f9e4d58870 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/build_request_body_index/index.ts @@ -0,0 +1,54 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { groupBy } from 'lodash'; +import type { OpenAPISpec } from '../../input/load_oas'; +import { isRecord } from '../is_record'; +import { collectReachableComponents } from './collect_reachable_components'; +import { extractRequestBodyConsumers } from './extract_request_body_consumers'; +import type { RequestBodyConsumer, RequestBodyIndex } from './types'; + +export type { RequestBodyConsumer, RequestBodyIndex } from './types'; + +interface ConsumerEntry { + componentName: string; + consumer: RequestBodyConsumer; +} + +const componentSchemasOf = (oas: OpenAPISpec): Record => + isRecord(oas.components) && isRecord(oas.components.schemas) ? oas.components.schemas : {}; + +const reachableFromSchema = ( + schema: unknown, + componentSchemas: Record +): string[] => { + const reachable = new Set(); + collectReachableComponents(schema, new Set(), reachable, componentSchemas); + return [...reachable]; +}; + +export const buildRequestBodyIndex = (oas: OpenAPISpec): RequestBodyIndex => { + const componentSchemas = componentSchemasOf(oas); + + const consumerEntries: ConsumerEntry[] = extractRequestBodyConsumers(oas).flatMap( + ({ schema, consumer }) => + reachableFromSchema(schema, componentSchemas).map((componentName) => ({ + componentName, + consumer, + })) + ); + + const grouped = groupBy(consumerEntries, (entry) => entry.componentName); + return new Map( + Object.entries(grouped).map(([componentName, entries]) => [ + componentName, + entries.map((entry) => entry.consumer), + ]) + ); +}; diff --git a/packages/kbn-api-contracts/src/diff/build_request_body_index/types.ts b/packages/kbn-api-contracts/src/diff/build_request_body_index/types.ts new file mode 100644 index 0000000000000..835b68d69992b --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/build_request_body_index/types.ts @@ -0,0 +1,15 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +export interface RequestBodyConsumer { + path: string; + method: string; +} + +export type RequestBodyIndex = Map; diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/build_entry.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/build_entry.ts new file mode 100644 index 0000000000000..b84831c2c83dd --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/build_entry.ts @@ -0,0 +1,29 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import type { OasdiffEntry } from '../parse_oasdiff'; + +export const REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID = + 'kbn:request-additional-properties-tightened'; + +export const TIGHTENING_TEXT = + 'Request body schema disallows extra fields (additionalProperties: false). Clients sending unknown keys will now receive 400.'; + +export const buildEntry = (input: { + path: string; + method: string; + source: string; +}): OasdiffEntry => ({ + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: input.method, + path: input.path, + source: input.source, +}); diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_component_tightenings.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_component_tightenings.ts new file mode 100644 index 0000000000000..ce4e4b56e501a --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_component_tightenings.ts @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { get } from 'lodash'; +import { isRecord } from '../is_record'; +import { collectTighteningsInSchemaDiff } from './collect_tightenings_in_schema_diff'; +import type { ComponentTightening } from './types'; + +export const collectComponentTightenings = (structuralDiff: unknown): ComponentTightening[] => { + const modifiedSchemas = get(structuralDiff, ['components', 'schemas', 'modified']); + if (!isRecord(modifiedSchemas)) return []; + + return Object.entries(modifiedSchemas) + .map(([componentName, schemaDiff]) => ({ + componentName, + pointers: collectTighteningsInSchemaDiff(schemaDiff, ''), + })) + .filter(({ pointers }) => pointers.length > 0); +}; diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_inline_tightenings.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_inline_tightenings.ts new file mode 100644 index 0000000000000..40b2220af9eb9 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_inline_tightenings.ts @@ -0,0 +1,64 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { get } from 'lodash'; +import type { OasdiffEntry } from '../parse_oasdiff'; +import { isRecord } from '../is_record'; +import { collectTighteningsInSchemaDiff } from './collect_tightenings_in_schema_diff'; +import { buildEntry } from './build_entry'; +import { makeSkipKey } from './make_skip_key'; +import type { SkipKey } from './types'; + +interface InlineSchemaContext { + path: string; + upperMethod: string; + mediaType: string; + schema: unknown; +} + +const inlineSchemaContexts = (structuralDiff: unknown): InlineSchemaContext[] => { + const modifiedPaths = get(structuralDiff, ['paths', 'modified']); + if (!isRecord(modifiedPaths)) return []; + + return Object.entries(modifiedPaths).flatMap(([path, pathEntry]) => { + const opsModified = get(pathEntry, ['operations', 'modified']); + if (!isRecord(opsModified)) return []; + + return Object.entries(opsModified).flatMap(([method, opEntry]) => { + const contentModified = get(opEntry, ['requestBody', 'content', 'modified']); + if (!isRecord(contentModified)) return []; + + const upperMethod = method.toUpperCase(); + return Object.entries(contentModified) + .map(([mediaType, mediaDiff]) => ({ + path, + upperMethod, + mediaType, + schema: get(mediaDiff, 'schema'), + })) + .filter((ctx) => isRecord(ctx.schema)); + }); + }); +}; + +export const collectInlineTightenings = ( + structuralDiff: unknown, + skipKeys: Set +): OasdiffEntry[] => + inlineSchemaContexts(structuralDiff).flatMap(({ path, upperMethod, mediaType, schema }) => + collectTighteningsInSchemaDiff(schema, '') + .filter((pointer) => !skipKeys.has(makeSkipKey(path, upperMethod, pointer))) + .map((pointer) => + buildEntry({ + path, + method: upperMethod, + source: `/requestBody/content/${mediaType}/schema${pointer}`, + }) + ) + ); diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_tightenings_in_schema_diff.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_tightenings_in_schema_diff.ts new file mode 100644 index 0000000000000..a7ff2899d5480 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/collect_tightenings_in_schema_diff.ts @@ -0,0 +1,50 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { isRecord } from '../is_record'; + +const isTighteningTransition = (apa: unknown): boolean => + isRecord(apa) && (apa.from === null || apa.from === true) && apa.to === false; + +const rootTightenings = (schemaDiff: Record, pointer: string): string[] => + isTighteningTransition(schemaDiff.additionalPropertiesAllowed) ? [pointer] : []; + +const propertyTightenings = (schemaDiff: Record, pointer: string): string[] => { + const properties = schemaDiff.properties; + if (!isRecord(properties) || !isRecord(properties.modified)) return []; + return Object.entries(properties.modified).flatMap(([propName, propDiff]) => + collectTighteningsInSchemaDiff(propDiff, `${pointer}/properties/${propName}`) + ); +}; + +const itemTightenings = (schemaDiff: Record, pointer: string): string[] => + isRecord(schemaDiff.items) + ? collectTighteningsInSchemaDiff(schemaDiff.items, `${pointer}/items`) + : []; + +const compositionTightenings = (schemaDiff: Record, pointer: string): string[] => + (['oneOf', 'anyOf', 'allOf'] as const).flatMap((keyword) => { + const container = schemaDiff[keyword]; + if (!isRecord(container) || !Array.isArray(container.modified)) return []; + return container.modified.flatMap((branch, index) => + isRecord(branch) && branch.diff !== undefined + ? collectTighteningsInSchemaDiff(branch.diff, `${pointer}/${keyword}/${index}`) + : [] + ); + }); + +export const collectTighteningsInSchemaDiff = (schemaDiff: unknown, pointer: string): string[] => { + if (!isRecord(schemaDiff)) return []; + return [ + ...rootTightenings(schemaDiff, pointer), + ...propertyTightenings(schemaDiff, pointer), + ...itemTightenings(schemaDiff, pointer), + ...compositionTightenings(schemaDiff, pointer), + ]; +}; diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/expand_component_tightenings.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/expand_component_tightenings.ts new file mode 100644 index 0000000000000..7a503ea9da455 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/expand_component_tightenings.ts @@ -0,0 +1,76 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import type { OasdiffEntry } from '../parse_oasdiff'; +import type { RequestBodyConsumer, RequestBodyIndex } from '../build_request_body_index'; +import { buildEntry } from './build_entry'; +import { makeSkipKey } from './make_skip_key'; +import type { ComponentTightening, SkipKey } from './types'; + +interface Expansion { + consumer: RequestBodyConsumer; + pointer: string; + upperMethod: string; +} + +interface ExpandResult { + entries: OasdiffEntry[]; + warnings: string[]; + skipKeys: Set; +} + +const zeroConsumersWarning = (componentName: string): string => + `Component schema '${componentName}' tightened additionalProperties but has zero request-body consumers; entry intentionally dropped.`; + +const expansionsFor = (pointers: string[], consumers: RequestBodyConsumer[]): Expansion[] => + consumers.flatMap((consumer) => + pointers.map((pointer) => ({ + consumer, + pointer, + upperMethod: consumer.method.toUpperCase(), + })) + ); + +const expandOne = ( + { componentName, pointers }: ComponentTightening, + index: RequestBodyIndex +): ExpandResult => { + const consumers = index.get(componentName) ?? []; + if (consumers.length === 0) { + return { entries: [], warnings: [zeroConsumersWarning(componentName)], skipKeys: new Set() }; + } + const expansions = expansionsFor(pointers, consumers); + return { + entries: expansions.map(({ consumer, upperMethod, pointer }) => + buildEntry({ + path: consumer.path, + method: upperMethod, + source: `/components/schemas/${componentName}${pointer}`, + }) + ), + warnings: [], + skipKeys: new Set( + expansions.map(({ consumer, upperMethod, pointer }) => + makeSkipKey(consumer.path, upperMethod, pointer) + ) + ), + }; +}; + +export const expandComponentTightenings = ( + componentTightenings: ComponentTightening[], + index: RequestBodyIndex +): ExpandResult => { + const expanded = componentTightenings.map((tightening) => expandOne(tightening, index)); + return { + entries: expanded.flatMap((e) => e.entries), + warnings: expanded.flatMap((e) => e.warnings), + skipKeys: new Set(expanded.flatMap((e) => [...e.skipKeys])), + }; +}; diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/index.test.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/index.test.ts new file mode 100644 index 0000000000000..a69b500de70c8 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/index.test.ts @@ -0,0 +1,323 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { + detectAdditionalPropertiesTightening, + REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, +} from '.'; +import type { RequestBodyIndex } from '../build_request_body_index'; + +const TIGHTENING_TEXT = + 'Request body schema disallows extra fields (additionalProperties: false). Clients sending unknown keys will now receive 400.'; + +const inlineDiff = (schemaDiff: unknown, mediaType = 'application/json') => ({ + paths: { + modified: { + '/api/test': { + operations: { + modified: { + POST: { + requestBody: { + content: { + modified: { + [mediaType]: { schema: schemaDiff }, + }, + }, + }, + }, + }, + }, + }, + }, + }, +}); + +const componentDiff = (componentName: string, schemaDiff: unknown) => ({ + components: { + schemas: { + modified: { + [componentName]: schemaDiff, + }, + }, + }, +}); + +describe('detectAdditionalPropertiesTightening', () => { + it('returns no entries and no warnings for an empty diff', () => { + expect(detectAdditionalPropertiesTightening({}, new Map())).toEqual({ + entries: [], + warnings: [], + }); + }); + + it('returns no entries when input is not an object', () => { + expect(detectAdditionalPropertiesTightening(null, new Map())).toEqual({ + entries: [], + warnings: [], + }); + expect(detectAdditionalPropertiesTightening(undefined, new Map())).toEqual({ + entries: [], + warnings: [], + }); + expect(detectAdditionalPropertiesTightening('not-a-diff', new Map())).toEqual({ + entries: [], + warnings: [], + }); + }); + + it('emits one entry for an inline body tightening at the schema root', () => { + const diff = inlineDiff({ + additionalPropertiesAllowed: { from: null, to: false }, + }); + const result = detectAdditionalPropertiesTightening(diff, new Map()); + expect(result.entries).toEqual([ + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'POST', + path: '/api/test', + source: '/requestBody/content/application/json/schema', + }, + ]); + expect(result.warnings).toEqual([]); + }); + + it('emits one entry per consumer when a component schema tightens at the root', () => { + const diff = componentDiff('Body', { + additionalPropertiesAllowed: { from: null, to: false }, + }); + const index: RequestBodyIndex = new Map([ + [ + 'Body', + [ + { path: '/api/a', method: 'POST' }, + { path: '/api/b', method: 'PUT' }, + ], + ], + ]); + const result = detectAdditionalPropertiesTightening(diff, index); + expect(result.entries).toEqual([ + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'POST', + path: '/api/a', + source: '/components/schemas/Body', + }, + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'PUT', + path: '/api/b', + source: '/components/schemas/Body', + }, + ]); + expect(result.warnings).toEqual([]); + }); + + it('emits no entries for a loosening transition (from:false → to:true|null)', () => { + const diff = inlineDiff({ + additionalPropertiesAllowed: { from: false, to: true }, + }); + expect(detectAdditionalPropertiesTightening(diff, new Map()).entries).toEqual([]); + + const diffNull = inlineDiff({ + additionalPropertiesAllowed: { from: false, to: null }, + }); + expect(detectAdditionalPropertiesTightening(diffNull, new Map()).entries).toEqual([]); + }); + + it('emits no entries for a typed-schema additionalProperties addition', () => { + const diff = inlineDiff({ + additionalProperties: { schemaAdded: true, type: 'string' }, + }); + expect(detectAdditionalPropertiesTightening(diff, new Map()).entries).toEqual([]); + }); + + it('returns a warning and drops the entry when a component tightening has zero request-body consumers', () => { + const diff = componentDiff('ResponseOnly', { + additionalPropertiesAllowed: { from: null, to: false }, + }); + const result = detectAdditionalPropertiesTightening(diff, new Map()); + expect(result.entries).toEqual([]); + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toContain('ResponseOnly'); + expect(result.warnings[0]).toContain('zero request-body consumers'); + }); + + it('emits a deeper source pointer for nested-property tightenings (inline)', () => { + const diff = inlineDiff({ + properties: { + modified: { + nested: { + additionalPropertiesAllowed: { from: null, to: false }, + }, + }, + }, + }); + const result = detectAdditionalPropertiesTightening(diff, new Map()); + expect(result.entries).toEqual([ + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'POST', + path: '/api/test', + source: '/requestBody/content/application/json/schema/properties/nested', + }, + ]); + }); + + it('emits a deeper source pointer for nested-property tightenings (component)', () => { + const diff = componentDiff('Body', { + properties: { + modified: { + nested: { + additionalPropertiesAllowed: { from: null, to: false }, + }, + }, + }, + }); + const index: RequestBodyIndex = new Map([['Body', [{ path: '/api/test', method: 'POST' }]]]); + const result = detectAdditionalPropertiesTightening(diff, index); + expect(result.entries).toEqual([ + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'POST', + path: '/api/test', + source: '/components/schemas/Body/properties/nested', + }, + ]); + }); + + it('skips path-side reflections of a component tightening to avoid duplicate entries', () => { + const diff = { + paths: { + modified: { + '/api/test': { + operations: { + modified: { + POST: { + requestBody: { + content: { + modified: { + 'application/json': { + schema: { additionalPropertiesAllowed: { from: null, to: false } }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + modified: { + Body: { additionalPropertiesAllowed: { from: null, to: false } }, + }, + }, + }, + }; + const index: RequestBodyIndex = new Map([['Body', [{ path: '/api/test', method: 'POST' }]]]); + const result = detectAdditionalPropertiesTightening(diff, index); + expect(result.entries).toEqual([ + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'POST', + path: '/api/test', + source: '/components/schemas/Body', + }, + ]); + }); + + it('walks oneOf branches in the structural diff and emits with /oneOf/ source pointers', () => { + const diff = inlineDiff({ + oneOf: { + modified: [ + { + base: { index: 0 }, + revision: { index: 0 }, + diff: { additionalPropertiesAllowed: { from: null, to: false } }, + }, + ], + }, + }); + const result = detectAdditionalPropertiesTightening(diff, new Map()); + expect(result.entries).toEqual([ + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'POST', + path: '/api/test', + source: '/requestBody/content/application/json/schema/oneOf/0', + }, + ]); + }); + + it('walks allOf branches in the structural diff', () => { + const diff = inlineDiff({ + allOf: { + modified: [ + { + base: { index: 0 }, + revision: { index: 0 }, + diff: { additionalPropertiesAllowed: { from: null, to: false } }, + }, + ], + }, + }); + const result = detectAdditionalPropertiesTightening(diff, new Map()); + expect(result.entries).toEqual([ + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'POST', + path: '/api/test', + source: '/requestBody/content/application/json/schema/allOf/0', + }, + ]); + }); + + it('walks anyOf branches in the structural diff', () => { + const diff = inlineDiff({ + anyOf: { + modified: [ + { + base: { index: 1 }, + revision: { index: 1 }, + diff: { additionalPropertiesAllowed: { from: true, to: false } }, + }, + ], + }, + }); + const result = detectAdditionalPropertiesTightening(diff, new Map()); + expect(result.entries).toEqual([ + { + id: REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, + level: 3, + text: TIGHTENING_TEXT, + operation: 'POST', + path: '/api/test', + source: '/requestBody/content/application/json/schema/anyOf/0', + }, + ]); + }); +}); diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/index.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/index.ts new file mode 100644 index 0000000000000..5433561c32927 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/index.ts @@ -0,0 +1,34 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { isRecord } from '../is_record'; +import type { RequestBodyIndex } from '../build_request_body_index'; +import { collectComponentTightenings } from './collect_component_tightenings'; +import { expandComponentTightenings } from './expand_component_tightenings'; +import { collectInlineTightenings } from './collect_inline_tightenings'; +import type { DetectionResult } from './types'; + +export { REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID } from './build_entry'; +export type { DetectionResult } from './types'; + +export const detectAdditionalPropertiesTightening = ( + structuralDiff: unknown, + requestBodyIndex: RequestBodyIndex +): DetectionResult => { + if (!isRecord(structuralDiff)) return { entries: [], warnings: [] }; + + const componentTightenings = collectComponentTightenings(structuralDiff); + const componentExpansion = expandComponentTightenings(componentTightenings, requestBodyIndex); + const inlineEntries = collectInlineTightenings(structuralDiff, componentExpansion.skipKeys); + + return { + entries: [...componentExpansion.entries, ...inlineEntries], + warnings: componentExpansion.warnings, + }; +}; diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/make_skip_key.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/make_skip_key.ts new file mode 100644 index 0000000000000..49ab135b886b8 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/make_skip_key.ts @@ -0,0 +1,13 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import type { SkipKey } from './types'; + +export const makeSkipKey = (path: string, method: string, pointer: string): SkipKey => + `${path}|${method}|${pointer}`; diff --git a/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/types.ts b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/types.ts new file mode 100644 index 0000000000000..32f8461a71021 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/detect_additional_properties_tightening/types.ts @@ -0,0 +1,22 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import type { OasdiffEntry } from '../parse_oasdiff'; + +export interface DetectionResult { + entries: OasdiffEntry[]; + warnings: string[]; +} + +export interface ComponentTightening { + componentName: string; + pointers: string[]; +} + +export type SkipKey = string; diff --git a/packages/kbn-api-contracts/src/diff/index.ts b/packages/kbn-api-contracts/src/diff/index.ts index f4a1cb3e0e5ac..7df4c33a8c490 100644 --- a/packages/kbn-api-contracts/src/diff/index.ts +++ b/packages/kbn-api-contracts/src/diff/index.ts @@ -8,7 +8,14 @@ */ export { runOasdiff } from './run_oasdiff'; +export { runOasdiffStructural } from './run_oasdiff_structural'; export { parseOasdiff } from './parse_oasdiff'; export type { OasdiffEntry } from './parse_oasdiff'; export { applyAllowlist } from './breaking_rules'; export type { BreakingChange, FilterResult } from './breaking_rules'; +export { buildRequestBodyIndex } from './build_request_body_index'; +export type { RequestBodyIndex, RequestBodyConsumer } from './build_request_body_index'; +export { + detectAdditionalPropertiesTightening, + REQUEST_ADDITIONAL_PROPERTIES_TIGHTENED_ID, +} from './detect_additional_properties_tightening'; diff --git a/packages/kbn-api-contracts/src/diff/is_record.ts b/packages/kbn-api-contracts/src/diff/is_record.ts new file mode 100644 index 0000000000000..4b6f4e4a5a0e7 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/is_record.ts @@ -0,0 +1,12 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { isPlainObject } from 'lodash'; + +export const isRecord = (value: unknown): value is Record => isPlainObject(value); diff --git a/packages/kbn-api-contracts/src/diff/parse_oasdiff.test.ts b/packages/kbn-api-contracts/src/diff/parse_oasdiff.test.ts index cd7ad9371e778..43830f3b8d0ea 100644 --- a/packages/kbn-api-contracts/src/diff/parse_oasdiff.test.ts +++ b/packages/kbn-api-contracts/src/diff/parse_oasdiff.test.ts @@ -173,6 +173,29 @@ describe('parseOasdiff', () => { ]); }); + it('maps kbn:request-additional-properties-tightened to request_body_tightened', () => { + const result = parseOasdiff([ + entry({ + id: 'kbn:request-additional-properties-tightened', + text: 'Request body schema disallows extra fields (additionalProperties: false). Clients sending unknown keys will now receive 400.', + operation: 'POST', + path: '/api/data_views/data_view', + source: '/components/schemas/Data_views_create_data_view_request_object', + }), + ]); + expect(result).toEqual([ + { + type: 'request_body_tightened', + path: '/api/data_views/data_view', + method: 'POST', + reason: + 'Request body schema disallows extra fields (additionalProperties: false). Clients sending unknown keys will now receive 400.', + oasdiffId: 'kbn:request-additional-properties-tightened', + source: '/components/schemas/Data_views_create_data_view_request_object', + }, + ]); + }); + it('maps response-optional-property-removed to response_property_removed', () => { const result = parseOasdiff([ entry({ diff --git a/packages/kbn-api-contracts/src/diff/parse_oasdiff.ts b/packages/kbn-api-contracts/src/diff/parse_oasdiff.ts index 90414f4346454..4697207881012 100644 --- a/packages/kbn-api-contracts/src/diff/parse_oasdiff.ts +++ b/packages/kbn-api-contracts/src/diff/parse_oasdiff.ts @@ -28,6 +28,7 @@ const ID_TO_TYPE: Readonly> = { 'request-parameter-removed': 'parameter_removed', 'response-required-property-removed': 'response_property_removed', 'response-optional-property-removed': 'response_property_removed', + 'kbn:request-additional-properties-tightened': 'request_body_tightened', }; // These oasdiff warning-level (level 2) checks are promoted to blocking because diff --git a/packages/kbn-api-contracts/src/diff/run_oasdiff_structural.test.ts b/packages/kbn-api-contracts/src/diff/run_oasdiff_structural.test.ts new file mode 100644 index 0000000000000..aaa6595f4ceff --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/run_oasdiff_structural.test.ts @@ -0,0 +1,179 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { existsSync } from 'node:fs'; +import { execFileSync } from 'child_process'; +import { runOasdiffStructural } from './run_oasdiff_structural'; + +jest.mock('node:fs', () => ({ + existsSync: jest.fn(), +})); + +jest.mock('child_process', () => ({ + execFileSync: jest.fn(), +})); + +const mockExistsSync = existsSync as jest.MockedFunction; +const mockExecFileSync = execFileSync as jest.MockedFunction; + +const sampleDiff = { + paths: { + modified: { + '/api/test': { + operations: { + modified: { + POST: { + requestBody: { + content: { + modified: { + 'application/json': { + schema: { additionalPropertiesAllowed: { from: null, to: false } }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, +}; + +describe('runOasdiffStructural', () => { + beforeEach(() => { + mockExistsSync.mockReturnValue(true); + delete process.env.OASDIFF_BIN; + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + it('throws when basePath is not absolute', () => { + expect(() => runOasdiffStructural('relative/base.yaml', '/tmp/current.yaml')).toThrow( + 'basePath must be an absolute path' + ); + expect(mockExecFileSync).not.toHaveBeenCalled(); + }); + + it('throws when currentPath is not absolute', () => { + expect(() => runOasdiffStructural('/tmp/base.yaml', 'relative/current.yaml')).toThrow( + 'currentPath must be an absolute path' + ); + expect(mockExecFileSync).not.toHaveBeenCalled(); + }); + + it('throws when basePath does not exist', () => { + mockExistsSync.mockImplementation((p) => p !== '/tmp/base.yaml'); + expect(() => runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml')).toThrow( + 'basePath does not exist' + ); + expect(mockExecFileSync).not.toHaveBeenCalled(); + }); + + it('throws when currentPath does not exist', () => { + mockExistsSync.mockImplementation((p) => p !== '/tmp/current.yaml'); + expect(() => runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml')).toThrow( + 'currentPath does not exist' + ); + expect(mockExecFileSync).not.toHaveBeenCalled(); + }); + + it('invokes oasdiff with diff/--format/json and returns parsed JSON', () => { + mockExecFileSync.mockReturnValue(JSON.stringify(sampleDiff)); + + const result = runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml'); + + expect(result).toEqual(sampleDiff); + expect(mockExecFileSync).toHaveBeenCalledWith( + 'oasdiff', + ['diff', '/tmp/base.yaml', '/tmp/current.yaml', '--format', 'json'], + expect.objectContaining({ encoding: 'utf-8', timeout: 240_000 }) + ); + }); + + it('returns an empty object for empty stdout', () => { + mockExecFileSync.mockReturnValue(''); + expect(runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml')).toEqual({}); + }); + + it('parses stdout from exit code 1 (diff found)', () => { + const error = Object.assign(new Error('Process exited with code 1'), { + stdout: JSON.stringify(sampleDiff), + status: 1, + }); + mockExecFileSync.mockImplementation(() => { + throw error; + }); + + expect(runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml')).toEqual(sampleDiff); + }); + + it('throws on exit code 2+', () => { + const error = Object.assign(new Error('Process exited with code 2'), { + stdout: '', + status: 2, + }); + mockExecFileSync.mockImplementation(() => { + throw error; + }); + + expect(() => runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml')).toThrow( + 'Process exited with code 2' + ); + }); + + it('passes --match-path when matchPath option is provided', () => { + mockExecFileSync.mockReturnValue('{}'); + + runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml', { + matchPath: '/api/actions/connector|/api/alerting/rule', + }); + + expect(mockExecFileSync).toHaveBeenCalledWith( + 'oasdiff', + [ + 'diff', + '/tmp/base.yaml', + '/tmp/current.yaml', + '--format', + 'json', + '--match-path', + '/api/actions/connector|/api/alerting/rule', + ], + expect.any(Object) + ); + }); + + it('does not pass --match-path when option is omitted', () => { + mockExecFileSync.mockReturnValue('{}'); + + runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml'); + + expect(mockExecFileSync).toHaveBeenCalledWith( + 'oasdiff', + ['diff', '/tmp/base.yaml', '/tmp/current.yaml', '--format', 'json'], + expect.any(Object) + ); + }); + + it('uses OASDIFF_BIN env var when set', () => { + process.env.OASDIFF_BIN = '/usr/local/bin/oasdiff'; + mockExecFileSync.mockReturnValue('{}'); + + runOasdiffStructural('/tmp/base.yaml', '/tmp/current.yaml'); + + expect(mockExecFileSync).toHaveBeenCalledWith( + '/usr/local/bin/oasdiff', + expect.any(Array), + expect.any(Object) + ); + }); +}); diff --git a/packages/kbn-api-contracts/src/diff/run_oasdiff_structural.ts b/packages/kbn-api-contracts/src/diff/run_oasdiff_structural.ts new file mode 100644 index 0000000000000..711888f51bbd8 --- /dev/null +++ b/packages/kbn-api-contracts/src/diff/run_oasdiff_structural.ts @@ -0,0 +1,74 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import path from 'node:path'; +import { existsSync } from 'node:fs'; +import { execFileSync } from 'child_process'; + +const DIFF_TIMEOUT = 240_000; +const MAX_BUFFER = 50 * 1024 * 1024; + +interface OasdiffStructuralOptions { + matchPath?: string; +} + +const assertAbsoluteExistingPath = (filePath: string, label: string): void => { + if (!path.isAbsolute(filePath)) { + throw new Error(`${label} must be an absolute path, got: ${filePath}`); + } + if (!existsSync(filePath)) { + throw new Error(`${label} does not exist: ${filePath}`); + } +}; + +// Note: we intentionally do NOT pass --flatten-allof. On the full Kibana +// spec, oasdiff 1.15.1 stack-overflows during allOf flattening. Instead, the +// detector walks `allOf.modified[i].diff` branches itself, alongside +// `oneOf` and `anyOf`. +const buildArgs = ( + basePath: string, + currentPath: string, + options?: OasdiffStructuralOptions +): string[] => [ + 'diff', + basePath, + currentPath, + '--format', + 'json', + ...(options?.matchPath ? ['--match-path', options.matchPath] : []), +]; + +const execOasdiff = (bin: string, args: string[]): string => { + try { + return execFileSync(bin, args, { + encoding: 'utf-8', + maxBuffer: MAX_BUFFER, + stdio: ['pipe', 'pipe', 'pipe'], + timeout: DIFF_TIMEOUT, + }); + } catch (error: unknown) { + if ((error as { status?: number }).status === 1) { + return (error as { stdout?: string }).stdout ?? ''; + } + throw error; + } +}; + +export const runOasdiffStructural = ( + basePath: string, + currentPath: string, + options?: OasdiffStructuralOptions +): unknown => { + assertAbsoluteExistingPath(basePath, 'basePath'); + assertAbsoluteExistingPath(currentPath, 'currentPath'); + + const bin = process.env.OASDIFF_BIN ?? 'oasdiff'; + const output = execOasdiff(bin, buildArgs(basePath, currentPath, options)).trim(); + return output ? JSON.parse(output) : {}; +};