Swagger UI should work without Node.js polyfills - rewrite generateCodeVerifier to avoid transitive dependency on Buffer (Node.js API) #7898

teodorlu · 2022-03-09T12:30:00Z

This PR rewrites generateCodeVerifier to not require the "randombytes" package.

Motivation and Context

As of today, I cannot use swagger-ui-react in my Vite.js application without polyfills. When I try to build, I error out on a missing Buffer in safe-buffer. src/core/utils.js depends on randombytes, which depends on safebuffer. safebuffer uses Buffer - which is a Node.js API, and requires polyfilling outside of Node.js.

I'd like to avoid having to polyfill.

Additional changes may be required to make swagger-ui-react work out of the box with a non-polyfilled application, but this is a start. I'm making this as a PR to make a specific suggestion -- and hear whether avoiding polyfills is something you're interested in.

There are some references to randombytes in model-example.jsx, but that looks like example code to me.

How Has This Been Tested?

I've tested the function manually in a separate Node.js REPL
The unit test for generateCodeVerifier is still passing
I've added a unit test that checks that the output from generateCodeVerifier is a valid base64 string.

My PR contains...

No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
Dependency changes (any modification to dependencies in package.json)
- I've removed the dependency on randombytes from src/core/utils.js
- src/core/components/model-example.jsx still uses randombytes. That looks like example code to me. I'm hoping it's possible to replace the sample code with something else, so that we can remove the dependency on randombytes. Suggestions welcome from maintainers that are more familiar with the Swagger UI codebase than I.
- I haven't touched randombytes in package.json (yet)
Bug fixes (non-breaking change which fixes an issue)
Improvements (misc. changes to existing features)
Features (non-breaking change which adds functionality)

My changes...

are breaking changes to a public API (config options, System API, major UI change, etc).
are breaking changes to a private API (Redux, component props, utility functions, etc.).
are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
are not breaking changes.

Documentation

My changes do not require a change to the project documentation.
My changes require a change to the project documentation.
If yes to above: I have updated the documentation accordingly.

Automated tests

My changes can not or do not need to be tested.
My changes can and should be tested by unit and/or integration tests.
If yes to above: I have added tests to cover my changes.
If yes to above: I have taken care to cover edge cases in my tests.
All new and existing tests passed.

The old code used to rely on the "randombytes" package, which depends on Buffer being present. This requires a polyfill to work in browser. By doing bit of hand coding, we can solve this ourselves.

teodorlu · 2022-03-09T12:41:24Z

I added a unittest that ensures that we don't throw when decoding base64 - which passes.

tim-lai · 2022-03-10T01:01:03Z

@teodorlu Thanks for the PR! SwaggerUI had a recent patch update that changed the build system to webpack@5. One of the benefits to this change was to remove extraneous Node.js dependencies and polyfills. However, the caveat is that Node.js polyfills are no longer included by default by webpack@5. This is true for any project based on webpack@5, not just SwaggerUI.

Buffer and process/browser are the two known required Node.js polyfills, and SwaggerUI webpack configuration specifically loads them as a plugin here. There are also known fallbacks and aliases specified here.

All that said, I think that it can't be guaranteed that SwaggerUI could and would remain free of any Node.js polyfills or fallbacks moving forward. So while I'm open to replacing randombytes with a different library (but not within SwaggerUI code directly), which I also think has been added and removed multiple times over time within SwaggerUI, if it's possible, it might be simpler to add the required dependencies to your project.

I'm not familiar with Vite.js or its requirements, but I can say that I've been able to add dependencies to CreateReactApp@5-based projects. CreateReactApp@5 was released a couple months ago, and is also built with webpack@5.

I hope this helps. Let me know, thanks.

teodorlu · 2022-03-10T11:20:49Z

Thanks for taking the time to give a solid response.

It looks like polyfills in Swagger UI is a bigger piece of work than I imagined. If we merged this PR, released and I tried again, we'd probably just hit another polyfill issue.

A first question is whether Swagger UI should be supported from a Vite.js application. If yes, how?

I decided to migrate my application from react-scripts 4 to Vite for build performance. I saw build times decrease by a factor of about 10. Under the hood, Vite relies on esbuild rather than webpack. So something that works out of the box with Webpack doesn't necessarily work with Vite. And you have to think about backwards compatibility with a large user base too.

You'll also have to think about testing, and if you decide to support Vite, I guess you'll want CI checks in place to ensure that Swagger UI keeps on working with Vite.

I think a better place to start is a minimal working example of my build issues with Swagger UI on Vite. Then perhaps some docs for how to configure the correct polyfills. I'll see what I can do.

Thanks again -- Swagger UI is of great help to me and my team!

Teodor

teodorlu · 2022-03-10T11:47:36Z

Here's a minimal example that reproduces the load issues for SwaggerUI on a Vite app - with stacktrace pointing to randombytes: https://github.com/teodorlu/swagger-ui-on-vite

I'm going to see what I can do on my end with polyfills. I'm still not super faimilar with Node.js javascript development, so I have some things to learn. I'll report back if I have any meaningful things to contribute to docs.

tim-lai · 2022-03-10T20:48:48Z

@teodorlu While we wouldn't directly support Vite.js at this time, I would happy to merge in additional documentation for Vite.js users.

char0n · 2022-03-14T13:26:11Z

Hi guys,

I've looked into the Vite.js briefly and here are my observations:

Vite is based on rullup
Vite is also based on esbuild
Vite allows to be configured to overcame the issues @teodorlu sees

It's just another build systems that does the same thing but in different way. @teodorlu I've invested couple of minutes trying to amend your vite config to support swagger-ui-react. Here is the PR that fixes your problems: teodorlu/swagger-ui-on-vite#1

Note: IMHO we can close this PR. Every build system has it's own intricacies and it doesn't really make sense to try to compensate for all them in swagger-ui codebase.

teodorlu · 2022-03-14T13:58:36Z

I agree that Swagger UI needs to keep its scope narrow enough to maintain. Supporting everything in the world doesn't work.

Yet - I'd argue that removing dependencies is a good thing. If we don't really need the dependency on "buffer" and it causes problems, why keep it? From a security perspective, less dependencies means smaller attack surface, and fewer places for vulnerabilities to sneak in.

Caveat: I haven't worked through removing the dependency to uncover whether that produces more problems. Meaning that this PR isn't done (yet). Landing it and removing the dependency on "buffer" would probably require some effort from Swagger UI team members. And I'd guess they've got plenty of things to do!

char0n · 2022-03-16T09:26:30Z

Yet - I'd argue that removing dependencies is a good thing. If we don't really need the dependency on "buffer" and it causes problems, why keep it? From a security perspective, less dependencies means smaller attack surface, and fewer places for vulnerabilities to sneak in.

Sure, it's hard to oppose this argument - so I do agree. If we can replace vendor library with a native JavaScript alternative or replace the library with simple custom implementation (let's do it).

Caveat: I haven't worked through removing the dependency to uncover whether that produces more problems. Meaning that this PR isn't done (yet). Landing it and removing the dependency on "buffer" would probably require some effort from Swagger UI team members. And I'd guess they've got plenty of things to do!

Right, go ahead with your PR and let's see where we get.

teodorlu · 2022-03-16T11:20:31Z

@char0n - thanks for the response.

Possible further steps:

Find other "randombytes" call sites and propose alternatives
Try to build Swagger UI NPM packages - and see if we discover more problems.

I have a few other things I need to do as well at work - but I'll see what I can do. It would help us if we can remove some vulnerable Swagger UI dependencies, and fixing that upstream (here) is better for everyone.

BeMoreDog · 2022-03-24T16:44:05Z

Hi 🙂 We're getting the exact same problem in our Vue 2 webpack 5 app with versions newer than 4.5.2. Adding the buffer polyfill to the webpack config in vue.config.js doesn't fix the issue, so if you can remove it without issues, that'd be great!

teodorlu · 2022-03-25T13:46:09Z

Hi @BeMoreDog! I haven't had the time to move this further.

In the meantime, here's a snippet from our main.tsx which works around the issue. Perhaps a temporary fix for you too :)

// Polyfill the Node.js Buffer API for use in Swagger UI's transitive dependencies
import { Buffer } from "buffer";
declare global {
  interface Window {
    Buffer: any;
  }
}
window.Buffer = Buffer;

char0n · 2022-03-26T15:39:51Z

I've produces #7946 that deals with Node.js polyfills and other problems in main SwaggerUI fragment. If you continue with this PR, please pay attention also to webpack config in the PR which needs to be compensated after dependency changes.

Refs #7898 Refs #7944 Refs #7935 Refs #7910 Refs #7908 Refs #7902

fidgi · 2022-06-19T13:02:57Z

src/core/utils.js

+  const arr = []
+  const maxValue = Math.pow(2,8) - 1;
+  for (var i = 0; i < 32; ++i) {
+    const randomByte = Math.floor(Math.random() * maxValue)


As mentionned here randomBytes() should be preferred over Math.random() for any use related to security .

generateCodeVerifier: avoid dependencies

698827d

The old code used to rely on the "randombytes" package, which depends on Buffer being present. This requires a polyfill to work in browser. By doing bit of hand coding, we can solve this ourselves.

teodorlu mentioned this pull request Mar 9, 2022

swagger-ui-react broken with react-scripts 5.0.0 #7893

Closed

teodorlu added 2 commits March 9, 2022 13:37

Ensure generateCodeVerifier produces base64

5e1a244

Avoid deprecation warning

22b4e45

teodorlu marked this pull request as ready for review March 9, 2022 12:41

teodorlu added 2 commits March 9, 2022 13:42

Don't load randombytes

67cbd17

Describe how test works

efc145c

teodorlu changed the title ~~generateCodeVerifier: avoid dependencies~~ Swagger UI should work without polyfills - rewrite generateCodeVerifier to avoid transitive dependency on Buffer (Node.js) Mar 9, 2022

teodorlu changed the title ~~Swagger UI should work without polyfills - rewrite generateCodeVerifier to avoid transitive dependency on Buffer (Node.js)~~ Swagger UI should work without Node.js polyfills - rewrite generateCodeVerifier to avoid transitive dependency on Buffer (Node.js) Mar 9, 2022

tim-lai self-assigned this Mar 10, 2022

scottohara mentioned this pull request Mar 12, 2022

Better guidance for integrating Swagger UI >=4.6.x into webpack 5 based projects #7908

Closed

char0n mentioned this pull request Mar 26, 2022

feat(build): add support for Create React App without eject #7946

Merged

17 tasks

char0n added a commit that referenced this pull request Mar 28, 2022

feat(build): add support for Create React App without eject (#7946)

81ef19c

Refs #7898 Refs #7944 Refs #7935 Refs #7910 Refs #7908 Refs #7902

fidgi reviewed Jun 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swagger UI should work without Node.js polyfills - rewrite generateCodeVerifier to avoid transitive dependency on Buffer (Node.js API) #7898

Swagger UI should work without Node.js polyfills - rewrite generateCodeVerifier to avoid transitive dependency on Buffer (Node.js API) #7898

teodorlu commented Mar 9, 2022 •

edited

Loading

teodorlu commented Mar 9, 2022

tim-lai commented Mar 10, 2022

teodorlu commented Mar 10, 2022 •

edited

Loading

teodorlu commented Mar 10, 2022 •

edited

Loading

tim-lai commented Mar 10, 2022

char0n commented Mar 14, 2022 •

edited

Loading

teodorlu commented Mar 14, 2022

char0n commented Mar 16, 2022

teodorlu commented Mar 16, 2022 •

edited

Loading

BeMoreDog commented Mar 24, 2022

teodorlu commented Mar 25, 2022

char0n commented Mar 26, 2022

fidgi Jun 19, 2022

Swagger UI should work without Node.js polyfills - rewrite generateCodeVerifier to avoid transitive dependency on Buffer (Node.js API) #7898

Are you sure you want to change the base?

Swagger UI should work without Node.js polyfills - rewrite generateCodeVerifier to avoid transitive dependency on Buffer (Node.js API) #7898

Conversation

teodorlu commented Mar 9, 2022 • edited Loading

Motivation and Context

How Has This Been Tested?

My PR contains...

My changes...

Documentation

Automated tests

teodorlu commented Mar 9, 2022

tim-lai commented Mar 10, 2022

teodorlu commented Mar 10, 2022 • edited Loading

teodorlu commented Mar 10, 2022 • edited Loading

tim-lai commented Mar 10, 2022

char0n commented Mar 14, 2022 • edited Loading

teodorlu commented Mar 14, 2022

char0n commented Mar 16, 2022

teodorlu commented Mar 16, 2022 • edited Loading

BeMoreDog commented Mar 24, 2022

teodorlu commented Mar 25, 2022

char0n commented Mar 26, 2022

fidgi Jun 19, 2022

Choose a reason for hiding this comment

teodorlu commented Mar 9, 2022 •

edited

Loading

teodorlu commented Mar 10, 2022 •

edited

Loading

teodorlu commented Mar 10, 2022 •

edited

Loading

char0n commented Mar 14, 2022 •

edited

Loading

teodorlu commented Mar 16, 2022 •

edited

Loading