Skip to content

chore: add e2e tests for security propagation during join#2827

Merged
Daryna-del merged 17 commits into
mainfrom
fix/resolve-root-security-in-join
Jun 3, 2026
Merged

chore: add e2e tests for security propagation during join#2827
Daryna-del merged 17 commits into
mainfrom
fix/resolve-root-security-in-join

Conversation

@Daryna-del
Copy link
Copy Markdown
Contributor

@Daryna-del Daryna-del commented May 20, 2026

What/Why/How?

Added e2e tests for security propagation during join

Reference

#1409

Testing

Screenshots (optional)

Check yourself

  • This PR follows the contributing guide
  • All new/updated code is covered by tests
  • Core code changed? - Tested with other Redocly products (internal contributions only)
  • New package installed? - Tested in different environments (browser/node)
  • Documentation update has been considered

Security

  • The security impact of the change has been considered
  • Code follows company security practices and guidelines

Note

Low Risk
Only new e2e fixtures and snapshots; no production join or security logic is modified in this diff.

Overview
Adds join e2e coverage for how root-level security behaves when merging OpenAPI specs, tied to #1409.

Two new parameterized cases are registered in join.test.ts: root-security-in-both-specs (each input has its own root security and paths) and root-security-without-paths (one spec has root security but empty paths). Each case includes foo.yaml / bar.yaml fixtures and a snapshot.txt golden output from the CLI join command.

The snapshots document expected behavior: per-operation security is applied from each source’s root requirement (e.g. oauth2 vs bearerAuth), components.securitySchemes are merged, and root security is not left on the merged document when operations carry the effective requirements.

Reviewed by Cursor Bugbot for commit 525a38c. Bugbot is set up for automated code reviews on this repo. Configure here.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 20, 2026

⚠️ No Changeset found

Latest commit: 525a38c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

Performance Benchmark (Lower is Faster)

CLI Version Bundle Lint Check Config
cli-latest ▓ 1.00x ± 0.01 ▓ 1.00x (Fastest) ▓ 1.03x ± 0.01
cli-next ▓ 1.00x (Fastest) ▓ 1.01x ± 0.01 ▓ 1.00x (Fastest)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 81.04% (🎯 80%) 7246 / 8941
🔵 Statements 80.39% (🎯 80%) 7528 / 9364
🔵 Functions 84.22% (🎯 83%) 1458 / 1731
🔵 Branches 72.63% (🎯 72%) 4899 / 6745
File CoverageNo changed files found.
Generated in workflow #10045 for commit 525a38c by the Vitest Coverage Report Action

@Daryna-del
Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread packages/cli/src/commands/join/utils/get-effective-components-prefix.ts Outdated
Comment thread packages/cli/src/commands/join/utils/get-effective-components-prefix.ts Outdated
@Daryna-del
Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread tests/e2e/join/merged-root-security-from-other-apis/snapshot.txt Outdated
Comment thread packages/cli/src/commands/join/index.ts Outdated
@Daryna-del
Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread packages/cli/src/commands/join/utils/get-effective-components-prefix.ts Outdated
@Daryna-del Daryna-del force-pushed the fix/resolve-root-security-in-join branch from 5bc9605 to ecc35f1 Compare May 20, 2026 11:29
@Daryna-del Daryna-del marked this pull request as ready for review May 20, 2026 11:40
@Daryna-del Daryna-del requested review from a team as code owners May 20, 2026 11:40
Comment thread .changeset/curvy-ghosts-listen.md Outdated
Comment thread packages/cli/src/commands/join/utils/resolve-operation-security.ts Outdated
Comment thread packages/cli/src/commands/join/utils/get-effective-components-prefix.ts Outdated
Comment thread packages/cli/src/commands/join/index.ts Outdated
Copy link
Copy Markdown
Collaborator

@tatomyr tatomyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also address the bugbot comments.

Comment thread packages/cli/src/commands/join/index.ts Outdated
} from '../../utils/miscellaneous.js';
import type { CommandArgs } from '../../wrapper.js';
import { COMPONENTS } from '../split/constants.js';
// import { COMPONENTS } from '../split/constants.js';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove comments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

},
});
}
if (!security && openapi.hasOwnProperty('security')) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the previous solution was not working? I see it refers to the root security already.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. Previous solution was working for each case, except when we have paths: {} and security on root level. Simplified the solution to cover this case, please, review one more time.

Comment thread packages/cli/src/commands/join/index.ts Outdated
Comment thread packages/cli/src/commands/join/utils/collect-paths.ts Outdated
Comment thread packages/cli/src/commands/join/utils/resolve-operation-security.ts Outdated
Comment thread packages/cli/src/commands/join/utils/collect-paths.ts Outdated
@Daryna-del Daryna-del force-pushed the fix/resolve-root-security-in-join branch from 11a4935 to 09e577e Compare May 25, 2026 11:51
200:
description: OK
400:
description: Bad request No newline at end of file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: Bad request
description: Bad request

Let's add a newline on file endings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to each file

info:
title: "spec1"
version: 1.0.0
servers:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add only essential fields and remove others.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

servers:
- url: https://api.example.com
paths:
/post:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's bad practice to use verbs in paths (especially mixing post and get), so I'm against using it in our examples.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

description: Bad request
tags:
- bar_other
security:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm getting it correctly, this path comes from an API description that doesn't have security defined on it; so how it got the security requirement from another description?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, updated tests

authorizationUrl: https://example.com/oauth/authorize
tokenUrl: https://example.com/oauth/token
scopes: {}
oauth1:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names are hard to follow. Please use something more meaningful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@Daryna-del Daryna-del force-pushed the fix/resolve-root-security-in-join branch from b887c33 to 1ecab94 Compare May 27, 2026 08:20
@Daryna-del Daryna-del changed the title fix: resolve root security during join tests: add e2e tests for security propagation during join May 27, 2026
@Daryna-del Daryna-del changed the title tests: add e2e tests for security propagation during join chore: add e2e tests for security propagation during join May 27, 2026
Comment thread tests/e2e/join/root-security-without-paths/snapshot.txt Outdated
Comment thread tests/e2e/join/root-security-in-both-specs/snapshot.txt Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ab620eb. Configure here.

Comment thread tests/e2e/join/root-security-in-both-specs/joined.yaml Outdated
@Daryna-del Daryna-del force-pushed the fix/resolve-root-security-in-join branch 2 times, most recently from c4ad9b7 to 4b8bca5 Compare May 29, 2026 07:22
@Daryna-del Daryna-del requested a review from tatomyr May 29, 2026 07:23
@Daryna-del Daryna-del force-pushed the fix/resolve-root-security-in-join branch from 4b8bca5 to cdac599 Compare June 2, 2026 19:59
@Daryna-del Daryna-del force-pushed the fix/resolve-root-security-in-join branch from cdac599 to 5f66807 Compare June 3, 2026 06:45
@Daryna-del Daryna-del self-assigned this Jun 3, 2026
@Daryna-del Daryna-del merged commit d7e0b7a into main Jun 3, 2026
51 of 52 checks passed
@Daryna-del Daryna-del deleted the fix/resolve-root-security-in-join branch June 3, 2026 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants