fix: resolve root security during join#2827
Conversation
🦋 Changeset detectedLatest commit: 162da39 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
| } from '../../utils/miscellaneous.js'; | ||
| import type { CommandArgs } from '../../wrapper.js'; | ||
| import { COMPONENTS } from '../split/constants.js'; | ||
| // import { COMPONENTS } from '../split/constants.js'; |
There was a problem hiding this comment.
Commented-out import instead of removal
Low Severity
The COMPONENTS import is commented out (// import { COMPONENTS } from ...) rather than deleted. This constant is no longer used in index.ts because the new getEffectiveComponentsPrefix utility handles it internally. The commented-out line is dead code that appears to be left over from development.
Reviewed by Cursor Bugbot for commit b647fd9. Configure here.
|
@cursor review |
| return ''; | ||
| } | ||
|
|
||
| const raw = info?.title ?? apiFilename; |
There was a problem hiding this comment.
Nullish coalescing fails for empty title string
Low Severity
info?.title ?? apiFilename uses nullish coalescing (??), which only falls back to apiFilename when title is null or undefined. If info.title is an empty string '', the fallback is skipped and the function returns '' — a falsy prefix that silently disables all conflict resolution for duplicate security scheme names. Using || instead would correctly fall back to apiFilename for empty strings.
Reviewed by Cursor Bugbot for commit 5bc9605. Configure here.
Daryna-del
force-pushed
the
fix/resolve-root-security-in-join
branch
from
5bc9605 to
ecc35f1
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 162da39. Configure here.
|
|
||
| if (!duplicateSecuritySchemeNames.size) { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
Auto-prefixing applies to all APIs, not just conflicting ones
Medium Severity
getEffectiveComponentsPrefix checks duplicateSecuritySchemeNames.size which is a global set of all duplicate scheme names across all APIs. When any pair of APIs shares a security scheme name, every API in the join gets auto-prefixed — including APIs that have no conflicting scheme names at all. For example, joining three APIs where A and B both define oauth2 but C only defines a unique apiKey, API C's components and $refs would be unnecessarily rewritten with a prefix. The check needs to verify whether the current API's scheme names intersect with duplicateSecuritySchemeNames, not just whether the global set is non-empty.
Reviewed by Cursor Bugbot for commit 162da39. Configure here.
| apiFilename, | ||
| prefixComponentsWithInfoProp, | ||
| duplicateSecuritySchemeNames, | ||
| }); |
There was a problem hiding this comment.
Info description prefix diverges from main component prefix
Low Severity
addInfoSectionAndSpecVersion computes its componentsPrefix via getInfoPrefix, which only returns a prefix when prefixComponentsWithInfoProp is explicitly provided. The main loop now uses getEffectiveComponentsPrefix, which can auto-generate a prefix when duplicate security scheme names exist. This means component references in the first API's info.description text won't be rewritten to match the auto-prefixed component names, leaving stale references.
Reviewed by Cursor Bugbot for commit 162da39. Configure here.
tatomyr
left a comment
tatomyr
left a comment
There was a problem hiding this comment.
Please also address the bugbot comments.
| } from '../../utils/miscellaneous.js'; | ||
| import type { CommandArgs } from '../../wrapper.js'; | ||
| import { COMPONENTS } from '../split/constants.js'; | ||
| // import { COMPONENTS } from '../split/constants.js'; |
| }, | ||
| }); | ||
| } | ||
| if (!security && openapi.hasOwnProperty('security')) { |
There was a problem hiding this comment.
Why the previous solution was not working? I see it refers to the root security already.
3 participants
What/Why/How?
Fixes a bug where root-level
securityfrom joined API descriptions was silently dropped in the output.Previously, the
joincommand ignored thesecurityfield defined at the root of each input spec. According to OpenAPI semantics, root-level security serves as a default for all operations that don't declare their own security. Simply merging root securities into the joined output would incorrectly apply one API's security to another API's operations.Reference
Closes #1409
Testing
Screenshots (optional)
Check yourself
Security
Note
Medium Risk
Changes how
joincomputes operationsecurityand auto-prefixes components when duplicate security scheme names exist, which can alter merged OpenAPI output and downstream client behavior. Logic is localized and covered by new e2e fixtures, reducing regression risk.Overview
Fixes
joinso operations inherit their own spec’s root-/path-levelsecuritywhen the operation doesn’t explicitly definesecurity, matching OpenAPI defaulting semantics.Adds
resolveOperationSecurityto compute effective operation security (operation → pathItem → root) and introducesgetEffectiveComponentsPrefixto auto-prefix components only when needed (e.g., duplicatesecuritySchemesnames across input specs). Adds new e2e join cases covering root security propagation, operation override behavior, and duplicate/no-duplicate scheme prefixing.Reviewed by Cursor Bugbot for commit 162da39. Bugbot is set up for automated code reviews on this repo. Configure here.