Adjust names of security schemes to match prefixed name#3861
Adjust names of security schemes to match prefixed name#3861StratusFearMe21 wants to merge 6 commits into
Conversation
|
|
|
|
@StratusFearMe21 is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
size:XS
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new issues found.
TL;DR — When merging multiple OpenAPI specs, components.securitySchemes entries are renamed with a per-source prefix, but operation/path-level security requirements were not, leaving them pointing at the original (now non-existent) scheme names. This PR rewrites the keys of every security requirement object during cloneAndRewrite, so the references match the renamed components.
Key changes
- Prefix
securityrequirement keys to match renamed schemes — IncloneAndRewrite, security requirement objects get each key rewritten to${opIdPrefix}_${key}, matching the prefix applied tocomponents.securitySchemesentries.
Summary ｜ 1 file ｜ 1 commit ｜ base: main ← fix-merged-security-schemes
Before: Operation-level
security: [{ apiKey: [] }]survived merging unchanged while the matchingcomponents.securitySchemes.apiKeywas renamed to<prefix>_apiKey, leaving a dangling reference.
After: Both sides get the same<prefix>_apiKeyname, so the merged document is internally consistent.
The new branch is placed before the operationId handler in the same per-key chain; opIdPrefix is the same per-source prefix used for the components rename at line 505, so the strings match exactly. Empty/opt-out shapes (security: [], security: [{}]) and AND-shaped multi-key requirements ({ a: [], b: [] }) are handled correctly by the plain Object.entries(s) walk.
packages/json-schema-ref-parser/src/index.ts
Claude Opus ｜ 𝕏
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3861 +/- ##
==========================================
+ Coverage 37.78% 37.84% +0.05%
==========================================
Files 580 580
Lines 20777 20791 +14
Branches 6047 6045 -2
==========================================
+ Hits 7851 7868 +17
+ Misses 10522 10518 -4
- Partials 2404 2405 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@hey-api/codegen-core
@hey-api/json-schema-ref-parser
@hey-api/nuxt
@hey-api/openapi-ts
@hey-api/shared
@hey-api/spec-types
@hey-api/types
@hey-api/vite-plugin
commit: |
|
@StratusFearMe21 can you add a test please? |
dosubot
Bot
added
size:S
|
Done, the test has been added 👌. In order to make the bug testable, I did have to modify how the |
|
@pullfrog review the pull request |
There was a problem hiding this comment.
Important
Core fix and regression test look right, but a debug console.dir was left in the new test and the getResolvedInput refactor silently drops a previously-supported case for inline objects that have neither openapi/swagger nor $id.
TL;DR — When multiple input files are merged via bundleMany, operation-level security references kept their original scheme names while the schemes themselves were renamed with a source prefix in components.securitySchemes, producing dangling references. This PR prefixes scheme names inside security arrays the same way securitySchemes are renamed, and tightens getResolvedInput to treat inline OpenAPI/Swagger objects as JSON (preserving any $id as the base path) so external $refs in inline specs can resolve.
Key changes
- Prefix security scheme names on operations — In
cloneAndRewrite, when asecurityarray is encountered, each entry's keys are rewritten to${opIdPrefix}_${schemeName}to match the renamedcomponents.securitySchemesentries. - Inline OpenAPI/Swagger detection in
getResolvedInput— Objects withopenapiorswaggerare now treated as inline JSON, with$id(if present) used as the base path for resolving external$refs. - Tolerate trailing slashes in
baseName—baseName('file:///spec1/')now returnsspec1instead of falling back toschema. - Regression test — New
mergeManytest asserts that operation-levelsecurityentries from two specs end up referencing the prefixed scheme names.
Summary ｜ 2 files ｜ 2 commits ｜ base: main ← fix-merged-security-schemes
Prefixed security references on operations
Before:
security: [{ bearerAuth: [] }]was emitted verbatim while the matching scheme was renamed tospec1_bearerAuth, leaving the operation pointing at a name that no longer exists.
After: the operation emitssecurity: [{ spec1_bearerAuth: [] }], matchingcomponents.securitySchemes.
The rewrite happens inside cloneAndRewrite, so it inherits the same opIdPrefix already used for operationId renaming.
packages/json-schema-ref-parser/src/index.ts · packages/json-schema-ref-parser/src/__tests__/bundle.test.ts
Inline OpenAPI/Swagger spec handling
Before: an inline spec with
$id(e.g.{ ...spec, $id: 'file:///spec1.json' }) hit the$idbranch and gotpath: 'file://:80',type: 'url',schema: undefined, then the bundler tried to fetch the synthetic URL.
After: the spec is recognized viaopenapi/swagger, kept asschema, and$idis preserved aspathso external$refs resolve relative to it.
This is what makes the new test's bundleMany({ pathOrUrlOrSchemas: [{ ...spec1, $id: 'file:///spec1.json' }, ...] }) invocation actually work. One concern: the refactor removed the previous else branch that set schema = pathOrUrlOrSchema; type = 'json' for any object input. Inline objects that have neither openapi/swagger nor $id now fall through with schema: undefined and type: 'url', which used to be a supported case for plain JSON Schema inputs in this package. See the inline comment.
packages/json-schema-ref-parser/src/index.ts
Claude Opus ｜ 𝕏
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
|
Separate issue. #3861 fixes a bug in #3817 is a runtime/SDK feature request: expose the original
|
|
@pullfrog respond to comments |
|
Restored the missing fallback Task list (5/5 completed)
|
Without the else branch, an inline object input that has neither openapi/swagger nor $id falls through with schema undefined and type 'url', which broke the package's general-purpose JSON Schema bundling contract.
| resolvedInput.type = 'file'; | ||
| } else if (!resolvedInput.path && pathOrUrlOrSchema && typeof pathOrUrlOrSchema === 'object') { | ||
| if ('$id' in pathOrUrlOrSchema && pathOrUrlOrSchema.$id) { | ||
| if ( |
There was a problem hiding this comment.
@StratusFearMe21 why did you need to add this piece? I see the test fails without it, but I'm concerned about tying internals to OpenAPI/Swagger like this
There was a problem hiding this comment.
That piece is so that the regression test can set resolvedInput.path on each input schema without the type of the resolvedInput being set to 'url'. This is so that when the test calls bundleMany on the input, the prefixes on the output securitySchemas is predictable.
There was a problem hiding this comment.
I'm not sure I understand, is this just to satisfy the test suite? Or is this needed for a real world usage?
There was a problem hiding this comment.
I put it there to satisfy the test suite, but if you were using this package in the real world, passing multiple JSON objects to bundleMany would be broken without this change since both schemas would resolve to the same path . Either url.cwd() or 'schema'.
There was a problem hiding this comment.
Might be better to handle that as a separate issue. I'm also concerned about this package, it feels like it's doing too much too poorly because I'm sure there are other similar edge cases we just didn't run into yet
There was a problem hiding this comment.
Why $id on a schema object is essential to bundleMany
Line 45 is in getResolvedInput, which is called by parseMany (line 250) — the first step of bundleMany. The critical logic is at lines 44-59:
When a JSON object (not a file path or URL) is passed as an input, resolvedInput.path starts as '' (line 30). Without a $id, the path stays empty, and line 62-64 kicks in:
if (resolvedInput.type !== 'json') {
resolvedInput.path = url.resolve(url.cwd(), resolvedInput.path);
}This means a schema without $id falls through entirely — type stays 'url', no path is set, and it gets resolved against url.cwd() (the current working directory).
But when the schema does have $id (and is an OpenAPI/Swagger document), lines 51-52 set resolvedInput.path to the $id value and type to 'json'. This is critical for bundleMany because:
-
parseMany(line 295) stores this path inthis.schemaManySources[i]:this.schemaManySources.push(path && path.length ? path : url.cwd());
-
mergeMany(line 489-490) uses that source path to compute the prefix used for namespacing all components:const sourcePath = this.schemaManySources[i] || `multi://input/${i + 1}`; const prefix = baseName(sourcePath);
-
That prefix is then used to rename all components (schemas, parameters, securitySchemes, etc.) to avoid collisions between the multiple inputs:
const newName = `${prefix}_${name}`; // e.g., "petstore_Pet" instead of "Pet"
-
It's also used to rewrite
$refpointers (line 512), prefix operation IDs (line 479), prefix tag names (line 522), and resolve relative external references (line 463).
Without $id, an inline schema object would get path = '', which would make baseName return a generic name (or fall back to the CWD-derived path), causing unpredictable or colliding prefixes when multiple JSON objects are bundled together. The $id provides a stable, unique identity for each inline schema, enabling the entire namespacing and reference-rewriting machinery of bundleMany to work correctly.')
| const withoutHash = p.split('#')[0]!; | ||
| const parts = withoutHash.split('/'); | ||
| const withoutTrailingSlash = withoutHash.replace(/\/+$/, ''); | ||
| const parts = withoutTrailingSlash.split('/'); |
There was a problem hiding this comment.
@StratusFearMe21 why is this change needed? Reverting it doesn't fail tests
There was a problem hiding this comment.
Oops, I didn't mean to keep that in this PR, that's my mistake.
It fixes another bug where if the path on the resolvedInput is set to url.cwd(), because that function always puts a trailing slash at the end of the path, splitting the path /foo/bar/baz/ for example would return ['foo', 'bar', 'baz', '']. Because the last element in parts is empty, baseName would just return 'schema' instead of the last component of the path.
There was a problem hiding this comment.
Do you want to open a separate pull request for that?
2 participants
This PR fixes how security schemes are referenced when there are multiple input files. Each security scheme referenced in each path should have the same prefixes that are applied to the securitySchemes component when multiple files are used.