Skip to content

Reducing security schema duplicates in OpenAPI parser #2479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dracomithril
Copy link
Contributor

@dracomithril dracomithril commented Aug 19, 2025

When security schemas are provided with duplicates we end up with array with repeated security expectations
example:

{
  "security": [
      {
          "security1": [],
          "security2": []
      },
      {
          "security1": [],
          "security3": []
      },
      {
          "security1": [],
          "security4": []
      },
      {
          "security5": [],
          "security2": []
      },
      {
          "security5": [],
          "security3": []
      },
      {
          "security5": [],
          "security4": []
      }
  ]
}

we end up with array where expected values are repeated

[
      {
        name: "X-TENANT",
        type: "apiKey",
      },
      {
        scheme: "bearer",
        type: "http",
      },
      {
        name: "X-TENANT",
        type: "apiKey",
      },
      {
        name: "X-SERVICE-AUTHENTICATION",
        type: "apiKey",
      },
      {
        in: "query",
        name: "tenant_code",
        type: "apiKey",
      },
      {
        in: "query",
        name: "tenant_code",
        type: "apiKey",
      },
      {
        name: "X-SERVICE-AUTHENTICATION",
        type: "apiKey",
      },
]

this PR is to resolve that and obtain short list of security measures that need to be meet

Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Aug 19, 2025

⚠️ No Changeset found

Latest commit: 36ce55f

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

Copy link

vercel bot commented Aug 19, 2025

@michalgrezel is attempting to deploy a commit to the Hey API Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. internal ⚙️ Internal development related labels Aug 19, 2025
Copy link

pkg-pr-new bot commented Aug 19, 2025

Open in StackBlitz

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/nuxt@2479
npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/openapi-ts@2479
npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/vite-plugin@2479

commit: 36ce55f

Copy link

codecov bot commented Aug 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 23.73%. Comparing base (df9b830) to head (36ce55f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2479      +/-   ##
==========================================
+ Coverage   22.78%   23.73%   +0.94%     
==========================================
  Files         338      338              
  Lines       33733    33733              
  Branches     1353     1404      +51     
==========================================
+ Hits         7686     8005     +319     
+ Misses      26037    25718     -319     
  Partials       10       10              
Flag Coverage Δ
unittests 23.73% <100.00%> (+0.94%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dracomithril dracomithril force-pushed the reduce_security_schema_duplicates branch from cd77abf to 2e36dcd Compare August 19, 2025 07:36
Copy link
Member

@mrlubos mrlubos left a comment

Choose a reason for hiding this comment

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

Hey @dracomithril, I'm not opposed to this change, but you need to make sure it doesn't discard unique values such as

security:
  - oauth2: [read]
  - oauth2: [write]

@dracomithril
Copy link
Contributor Author

Hey @dracomithril, I'm not opposed to this change, but you need to make sure it doesn't discard unique values such as

security:
  - oauth2: [read]
  - oauth2: [write]

Thank you @mrlubos for pointing that out. Are there any examples of using scopes?
From what I see in documentation oauth2 can have multiple flows but I do not see logic that is covering that case right now or maybe I'm missing where that logic is present?

@mrlubos
Copy link
Member

mrlubos commented Aug 19, 2025

I don't think there are any examples. It's not oauth2 specific either.

security:
  - foo: [scope1]
  - foo: [scope2]

Your changes are keyed only based on name. That could mean the user receives only scope1 but the token they hold is valid for scope2, so they never try to submit it for the operation

@dracomithril
Copy link
Contributor Author

dracomithril commented Aug 19, 2025

so I checked in the code and current implementation does not care about scope at all at least from what I see

if (context.spec.components) {
for (const name in context.spec.components.securitySchemes) {
const securityOrReference =
context.spec.components.securitySchemes[name]!;
const securitySchemeObject =
'$ref' in securityOrReference
? context.resolveRef<SecuritySchemeObject>(securityOrReference.$ref)
: securityOrReference;
securitySchemesMap.set(name, securitySchemeObject);

so what ever scope you provide it will still resolve in the same way would we would need to to is extend object in securitySchemesMap to contain scope but right now I do not see the use as it is omitted later

@dracomithril
Copy link
Contributor Author

dracomithril commented Aug 19, 2025

to add to it from what I understand in context.spec.components.securitySchemes we will only have unique names so in example that you are showing this definition is for specific path

security:
  - foo: [scope1]
  - foo: [scope2]

but in context.spec.components.securitySchemes we will have unique names

# openapi 3.0.x
spec:
  components:
     securitySchemes:
        foo:
           type: 'oauth2'
           flows:
             password:
                scope: 
                   scope1: 'Grants read access'
                   scope2: 'Grants write access'
                tokenUrl:  'http://some/auth'         

so in current implementation it will still resolve to the same thing we would need to extend logic to have specific path scope added so then on auth() developer can decide what flow to use

@dracomithril dracomithril force-pushed the reduce_security_schema_duplicates branch 4 times, most recently from 71bfe9e to 1fdf359 Compare August 19, 2025 20:54
@dracomithril dracomithril changed the title reduce duplicates Reducing security schema duplicates in OpenAPI parser Aug 19, 2025
@dracomithril dracomithril force-pushed the reduce_security_schema_duplicates branch from 1fdf359 to 9b7b235 Compare August 19, 2025 21:00
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Aug 19, 2025
@dracomithril dracomithril force-pushed the reduce_security_schema_duplicates branch 2 times, most recently from 5d81037 to 82549d9 Compare August 19, 2025 21:07
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Aug 19, 2025
@dracomithril dracomithril force-pushed the reduce_security_schema_duplicates branch from 82549d9 to 192a1f5 Compare August 19, 2025 21:10
@dracomithril dracomithril force-pushed the reduce_security_schema_duplicates branch from 192a1f5 to 36ce55f Compare August 19, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal ⚙️ Internal development related size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants