Skip to content
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

feat(rest): remove support for HTTP redirect to an external API Explorer #6290

Closed
wants to merge 2 commits into from

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Sep 7, 2020

A bit of history: Back in 2017/2018, we wanted to offer REST API Explorer early on, because we considered it a cornerstone of developer experience. The framework did not provide all the necessary infrastructure yet, therefore we decided to take a short-cut and host swagger-ui at loopback.io. Eventually, we implemented self-hosted REST API Explorer towards the end of 2018 and kept support for external API Explorer for backwards compatibility.

I feel it's past time to drop that legacy. Since we will are already introducing breaking changes in #6288 that will trigger a semver-major release of @loopback/rest, I'd like to take this opportunity to get rid of external API explorer too.

BREAKING CHANGE

LoopBack no longer supports automatic redirect to an externally hosted API Explorer UI. Instead, we provide an extension that implements a self-hosted API Explorer UI. See the following documentation page for instructions on setting up a self-hosted API Explorer:

By default, applications scaffolded using lb4 app are coming with a pre-configured API Explorer. If you have scaffolded your application with @loopback/cli version 1.2.0 (published in Nov 2018) or later, then no changes are necessary in your project.

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

BREAKING CHANGE: LoopBack no longer supports automatic redirect to an externally hosted
API Explorer UI. Instead, we provide  an extension that implements a self-hosted API
Explorer UI. See the following documentation page for instructions on
setting up a self-hosted API Explorer:

- https://loopback.io/doc/en/lb4/Self-hosted-rest-api-explorer.html

By default, applications scaffolded using `lb4 app` are coming with a
pre-configured API Explorer. If you have scaffolded your application
with `@loopback/cli` version `1.2.0` (published in Nov 2018) or later,
then no changes are necessary in your project.

Signed-off-by: Miroslav Bajtoš <[email protected]>
@raymondfeng
Copy link
Contributor

@bajtos I wonder if we should remove the feature. Some companies may have a site-wide API explorer to render OpenAPI specs in a consistent look-and-feel. Maybe it's better to consolidate the redirect into @loopback/rest-explorer?

@bajtos
Copy link
Member Author

bajtos commented Sep 8, 2020

@bajtos I wonder if we should remove the feature. Some companies may have a site-wide API explorer to render OpenAPI specs in a consistent look-and-feel. Maybe it's better to consolidate the redirect into @loopback/rest-explorer?

Interesting, I didn't consider the use case of having a site-wide API explorer. Let's keep the feature then.

I am not sure if it's a good idea to put redirect-to-explorer to @loopback/rest-explorer. Scenario to consider:

  1. My app is using redirect-to-explorer from @loopback/rest-explorer. We don't use swagger-ui shipped inside @loopback/rest-explorer.
  2. A security vulnerability is discovered in swagger-ui.
  3. My app is failing security checks, despite the fact that it does not use any swagger-ui functionality.

This is not a big deal for apps running the latest version of all modules, but it may become a problem in the future, as we are already witnessing for LoopBack 3 - see strongloop/loopback-component-explorer#263.

I am closing this pull request.

@bajtos bajtos closed this Sep 8, 2020
@bajtos bajtos deleted the feat/remove-external-api-explorer branch September 8, 2020 07:44
@hacksparrow
Copy link
Contributor

@bajtos I am actually in favor for this PR. What is a site-wide API explorer?

@hacksparrow
Copy link
Contributor

@raymondfeng can you elaborate "site-wide API explorer"?

The lighter the framework, the better.

@raymondfeng
Copy link
Contributor

For example, Acme has adopted a company-wide such as https://api-explorer.acme.com and it would like to redirect LB applications to the web page for a consistent look-and-feel.

BTW, the redirected api explorer is also a good validation of CORS support.

At least, we should poll the community to see if people are using the redirect.

@hacksparrow
Copy link
Contributor

hacksparrow commented Sep 9, 2020

For example, Acme has adopted a company-wide such as https://api-explorer.acme.com and it would like to redirect LB applications to the web page for a consistent look-and-feel.

Such a requirement can be easily be implemented by users themselves without having to depend on our configuration (and implementation) for a hosted Explorer.

The problem with us providing the hosted Explorer option:

  1. User comments out this.component(RestExplorerComponent) to disable Explorer, only to be forwarded to http://explorer.loopback.io/. To really disable it, they have to set rest.restExplorer.enabled to false. Disabling something should be as simple and straightforward as possible.
  2. We have to maintain http://explorer.loopback.io/.
  3. Not a lot, but the code for restExplorer contributes to the size of the framework, and brings in security problems.
  4. rest.restExplorer is NOT what users assume it would be.

Add @bajtos's points too.

Let's take this "major" opportunity to fix it.

@hacksparrow
Copy link
Contributor

Also, if I am not wrong, the decision to enable CORS by default was made to support the hosted Explorer. By not supporting the legacy Explorer behavior we will be able to make CORS a more conscious effort on the user's part; then disabling CORS would simply mean not using the CORS extension.

Currently disabling CORS is done by passing { rest: { cors: { origin: false } } } in the app config.

First off, that's not a pleasant looking config object for humans. By extracting CORS to a standalone extension we can skip the rest and cors keys and get to the root CORS config directly.

Secondly, someone might re-enable CORS by setting origin to true (if false is for disabling, true should be for enabling, right?), which will result in reflecting the request origin - a terrible security misconfiguration.

CORS is a complicated config, which can lead to lot of security issues if done wrong. We should avoid all defaults and let users takes 100% responsibility of how they set it up.

Read how "safe" CORS configs can go wrong - https://portswigger.net/web-security/cors.

Once LB4 grows in popularity, security will become a major concern. We should address it now.

@achrinza

@hacksparrow
Copy link
Contributor

After this PR lands we should do this - Extract CORS to a standalone extension.

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