loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.95k stars 1.07k forks source link

LB4 Cors is always enabled after full upgrade of loopback #5368

Closed besongsamuel closed 4 years ago

besongsamuel commented 4 years ago

Steps to reproduce

1- Update to the following packages "@loopback/authentication": "^4.2.3", "@loopback/authentication-passport": "^2.1.3", "@loopback/authorization": "^0.5.8", "@loopback/boot": "^2.2.0", "@loopback/context": "^3.7.0", "@loopback/core": "^2.5.0", "@loopback/openapi-v3": "^1.11.0", "@loopback/repository": "^2.4.0", "@loopback/rest": "^4.0.0",

2- Enable Cors by setting the cors attribute of the rest config to the value below { origin: '*', methods: 'GET,HEAD,PUT,PATCH,POST,DELETE', preflightContinue: false, optionsSuccessStatus: 204, maxAge: 86400, credentials: true, }

3 - Start up a new REST server :

Current Behavior

Every Request from a different domain fails with a message Access to XMLHttpRequest at 'http://127.0.0.1:3000/users/login' from origin 'http://localhost:4200' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource.

Expected Behavior

The Request should not be blocked by CORS since it is disabled. This issue only happens after an upgrage as stated above.

Link to reproduction sandbox

In WORK

Additional information

darwin x64 12.13.1

@loopback/authentication@4.2.3 ├── @loopback/authentication-passport@2.1.3 ├── @loopback/authorization@0.5.8 ├── UNMET PEER DEPENDENCY @loopback/boot@2.2.0 ├── UNMET PEER DEPENDENCY @loopback/context@3.7.0 ├── UNMET PEER DEPENDENCY @loopback/core@2.5.0 ├── @loopback/openapi-v3@1.11.0 ├── UNMET PEER DEPENDENCY @loopback/repository@2.4.0 ├── UNMET PEER DEPENDENCY @loopback/rest@4.0.0 ├── @loopback/rest-explorer@1.4.7 ├── @loopback/service-proxy@1.3.14 ├── loopback-connector-mongodb@4.2.0 ├── loopback-connector-mysql@5.4.2 ├── loopback-next@4.0.0-alpha.1 ├── loopback4-authentication@2.1.3 ├── loopback4-authorization@2.3.2 ├── loopback4-soft-delete@1.2.0 npm ERR! peer dep missing: @loopback/boot@^1.4.4, required by loopback4-authentication@2.1.3 npm ERR! peer dep missing: @loopback/boot@^1.4.4, required by loopback4-authorization@2.3.2 npm ERR! peer dep missing: @loopback/boot@^1.4.4, required by loopback4-soft-delete@1.2.0 npm ERR! peer dep missing: @loopback/context@^1.20.2, required by loopback4-authentication@2.1.3 npm ERR! peer dep missing: @loopback/context@^1.20.2, required by loopback4-authorization@2.3.2 npm ERR! peer dep missing: @loopback/context@^1.20.2, required by loopback4-soft-delete@1.2.0 npm ERR! peer dep missing: @loopback/core@^1.8.5, required by loopback4-authentication@2.1.3 npm ERR! peer dep missing: @loopback/core@^1.8.5, required by loopback4-authorization@2.3.2 npm ERR! peer dep missing: @loopback/core@^1.8.5, required by loopback4-soft-delete@1.2.0 npm ERR! peer dep missing: @loopback/repository@^1.8.2, required by loopback4-soft-delete@1.2.0 npm ERR! peer dep missing: @loopback/rest@^1.16.3, required by loopback4-authentication@2.1.3 npm ERR! peer dep missing: @loopback/rest@^1.16.3, required by loopback4-authorization@2.3.2

Related Issues

See Reporting Issues for more tips on writing good issues

raymondfeng commented 4 years ago

@besongsamuel Did you use lb4 update to upgrade all LB4 packages?

raymondfeng commented 4 years ago

I was able to make cross-domain APIs using http://explorer.loopback.io/?url=http://localhost:3000/openapi.json.

besongsamuel commented 4 years ago

I just updated using lb4 update and it seems to be resolved. Thanks. I think you can close issue. Thanks for your quick reply.

dhmlau commented 4 years ago

Closing as resolved.

riv0tril commented 4 years ago

Same issue here, appears after making update with npm i -g @loopback/cli update

raymondfeng commented 4 years ago

Same issue here, appears after making update with npm i -g @loopback/cli update

You can run the following:

npm i -g @loopback/cli

Go into your project dir:

lb4 update
dougal83 commented 4 years ago

@rivotril0 That is interesting. Does running lb4 update on affected project(s) resolve the issue after the update to cli?

riv0tril commented 4 years ago

@rivotril0 That is interesting. Does running lb4 update on affected project(s) resolve the issue after the update to cli?

no, actualy if i runlb4 update now i get The project dependencies are compatible with @loopback/cli@2.6.0 and nothing happens

riv0tril commented 4 years ago

adding dependencies

"dependencies": {
    "@loopback/authentication": "^4.2.3",
    "@loopback/authorization": "^0.5.8",
    "@loopback/boot": "^2.2.0",
    "@loopback/context": "^3.7.0",
    "@loopback/core": "^2.5.0",
    "@loopback/openapi-v3": "^3.3.1",
    "@loopback/repository": "^2.4.0",
    "@loopback/rest": "^4.0.0",
    "@loopback/rest-explorer": "^2.2.0",
    "@loopback/security": "^0.2.8",
    "@loopback/service-proxy": "^2.2.0",
  },
riv0tril commented 4 years ago

after performing the cli update, the prompt ask for updating project deps, the issue start here. Also apart from cors being enabled, the application ignores the options passed into ApplicationConfig object related to cors

raymondfeng commented 4 years ago

Try remove node_modules and run npm install again for the project

riv0tril commented 4 years ago

Still blocked by cors

riv0tril commented 4 years ago

Also i notice the following, GET method endpoints are treated as OPTION

message:"Endpoint "OPTIONS /boo/foo" not found."
name:"NotFoundError"
stack:"NotFoundError: Endpoint "OPTIONS /boo/foo" not found.
    at ExternalRoute.invokeHandler (some_window_path\node_modules\@loopback\rest\dist\router\external-express-routes.js:67:15)
    at async MySequence.handle (some_window_path\dist\sequence.js:23:28)
    at async HttpHandler._handleRequest (some_window_path\@loopback\rest\dist\http-handler.js:63:9)"
raymondfeng commented 4 years ago

@rivotril0 Can you provide a git repo to reproduce the problem?

raymondfeng commented 4 years ago

Also i notice the following, GET method endpoints are treated as OPTION

That's probably due to cors preflight check. It should be handled by cors middleware.

riv0tril commented 4 years ago

Also i notice the following, GET method endpoints are treated as OPTION

message:"Endpoint "OPTIONS /boo/foo" not found."
name:"NotFoundError"
stack:"NotFoundError: Endpoint "OPTIONS /boo/foo" not found.
    at ExternalRoute.invokeHandler (some_window_path\node_modules\@loopback\rest\dist\router\external-express-routes.js:67:15)
    at async MySequence.handle (some_window_path\dist\sequence.js:23:28)
    at async HttpHandler._handleRequest (some_window_path\@loopback\rest\dist\http-handler.js:63:9)"

Ignore this, seems to be the preflight

raymondfeng commented 4 years ago

You can use DEBUG=loopback:middleware loopback:rest:* env var to see if cors is triggered.

Did you try to clean up the project as follows?

rm -rf node_modules
npm i
riv0tril commented 4 years ago

You can use DEBUG=loopback:middleware loopback:rest:* env var to see if cors is triggered.

the output has nothing related to cors

rm -rf node_modules npm i

yes, and the issue still there

riv0tril commented 4 years ago

from DEBUG=loopback:context

loopback:context:restserver [RestServer-548cf9fe-44cf-454c-a5e8-490439a8a539] [middleware.cors] Adding binding: +562ms
loopback:context:binding Bind middleware.cors to constant: [AsyncFunction] +279ms
raymondfeng commented 4 years ago

With DEBUG=loopback:middlewre, you should be able to see something similar as follows:

  loopback:middleware Invoke middleware chain for GET /ping with options {
  chain: 'middlewareChain.default',
  orderedGroups: [ 'cors', 'apiSpec', '' ]
} +0ms
  loopback:middleware Middleware for extension point "middlewareChain.default": [ 'middleware.cors', 'middleware.apiSpec.defaults' ] +1ms
  loopback:middleware [corsMiddleware] Handler calling next() undefined +0ms
  loopback:middleware [corsMiddleware] Proceed with downstream interceptors +0ms
  loopback:middleware [router] Handler calling next() undefined +0ms
  loopback:middleware [router] Proceed with downstream interceptors +0ms
  loopback:middleware [router] Result received from downstream interceptors +0ms
  loopback:middleware [corsMiddleware] Result received from downstream interceptors +0ms
  loopback:middleware Invoke middleware chain for GET /favicon.ico with options {
  chain: 'middlewareChain.default',
  orderedGroups: [ 'cors', 'apiSpec', '' ]
} +176ms
  loopback:middleware Middleware for extension point "middlewareChain.default": [ 'middleware.cors', 'middleware.apiSpec.defaults' ] +0ms
  loopback:middleware [corsMiddleware] Handler calling next() undefined +175ms
  loopback:middleware [corsMiddleware] Proceed with downstream interceptors +0ms
  loopback:middleware [router] Handler calling next() undefined +1ms
  loopback:middleware [router] Proceed with downstream interceptors +0ms
  loopback:middleware [router] Result received from downstream interceptors +0ms
  loopback:middleware [corsMiddleware] Result received from downstream interceptors +0ms
  loopback:middleware [router] Handler calling next() undefined +0ms
  loopback:middleware [router] Handler calling next() undefined +2ms
raymondfeng commented 4 years ago

I realized if you have src/sequence.ts generated from old version of CLI, you need to update it with:

import {inject} from '@loopback/context';
import {
  FindRoute,
  InvokeMethod,
  InvokeMiddleware,
  ParseParams,
  Reject,
  RequestContext,
  RestBindings,
  Send,
  SequenceHandler,
} from '@loopback/rest';

const SequenceActions = RestBindings.SequenceActions;

export class MySequence implements SequenceHandler {
  /**
   * Optional invoker for registered middleware in a chain.
   * To be injected via SequenceActions.INVOKE_MIDDLEWARE.
   */
  @inject(SequenceActions.INVOKE_MIDDLEWARE, {optional: true})
  protected invokeMiddleware: InvokeMiddleware = () => false;

  constructor(
    @inject(SequenceActions.FIND_ROUTE) protected findRoute: FindRoute,
    @inject(SequenceActions.PARSE_PARAMS) protected parseParams: ParseParams,
    @inject(SequenceActions.INVOKE_METHOD) protected invoke: InvokeMethod,
    @inject(SequenceActions.SEND) public send: Send,
    @inject(SequenceActions.REJECT) public reject: Reject,
  ) {}

  async handle(context: RequestContext) {
    try {
      const {request, response} = context;
      const finished = await this.invokeMiddleware(context);
      if (finished) return;
      const route = this.findRoute(request);
      const args = await this.parseParams(request, route);
      const result = await this.invoke(route, args);
      this.send(response, result);
    } catch (err) {
      this.reject(context, err);
    }
  }
}
raymondfeng commented 4 years ago

Or simply use DefaultSequence from @loopback/rest.

In src/application.ts:

import {DefaultSequence} from '@loopback/rest';
// ...
constructor(options: ApplicationConfig = {}) {
    super(options);

    // Set up the custom sequence
    this.sequence(DefaultSequence);
    // ..
}
riv0tril commented 4 years ago

const finished = await this.invokeMiddleware(context); if (finished) return;

Do the trick, thanks for your help. Could you share the pr for this braking change?

raymondfeng commented 4 years ago

It's a new major release for @loopback/rest - see https://github.com/strongloop/loopback-next/pull/5118. It's my fault to miss the part to mention that sequence.ts needs to be updated for old apps. Newly generated apps have the correct code.

besongsamuel commented 4 years ago

Thanks @raymondfeng I just spent a couple of hours trying to figure this out

pratikjaiswal15 commented 4 years ago

Thank you @raymondfeng setting DefaultSequence solved the issue.

bajtos commented 4 years ago

It's a new major release for @loopback/rest - see https://github.com/strongloop/loopback-next/pull/5118. It's my fault to miss the part to mention that sequence.ts needs to be updated for old apps. Newly generated apps have the correct code.

@raymondfeng can you please update release notes for @loopback/rest@4.0.0 to mention this breaking change and provide instructions for upgrading users?

raymondfeng commented 4 years ago

@raymondfeng can you please update release notes for @loopback/rest@4.0.0 to mention this breaking change and provide instructions for upgrading users?

What's the best way to do so? Update README?

evert-arias commented 4 years ago

Thank you @raymondfeng, this solved the issue.

raymondfeng commented 4 years ago

@bajtos See https://github.com/strongloop/loopback-next/pull/5391

Tenvan commented 4 years ago

boah.. hi guys.. i have searched for this issue since two days.

      const finished = await this.invokeMiddleware(context);
      if (finished) return;

this was the solution for me ... yeah :)

thx to all ;)

ashkank83 commented 4 years ago

@raymondfeng Thanks for the comment as other said, adding

const finished = await this.invokeMiddleware(context);
      if (finished) return;

as well as adding the InvokeMiddleware import and inject line fixed my issue as well.

Question: When we run lb4 update it updates all the dependencies regardless of them having breaking changes (major releases) or not. In cases like this update the developer will not know about changes which are necessary to make. Is there any recommendation about how to run lb4 update or what to check before and after running lb4 update to not miss such important changes?
Thanks

achrinza commented 4 years ago

One way to check for potential changes is via the changelog docs page.

ashkank83 commented 4 years ago

One way to check for potential changes is via the changelog docs page.

@achrinza Thanks for the reply. I know we can go through change log for each component and check what has changed, however it maybe more efficient if the CLI can give a warning when a major update is happening i.e. if @loopback/rest is being updated from 3.x to 4.x which means there are breaking changes.

achrinza commented 4 years ago

@ashkank83 Apologies for the delayed reply, I believe the CLI already gives a generic warning for semver major upgrades:

https://github.com/strongloop/loopback-next/blob/cf38dca9a06ef1f3447930468bd852dc377e6ff8/packages/cli/generators/update/index.js#L39-L49

Though I agree it could be improved to provide more helpful information (e.g. which packages, and aggregate relevant parts of the changelog).

This probably ties in with #4864

philip13 commented 3 years ago

I realized if you have src/sequence.ts generated from old version of CLI, you need to update it with:

import {inject} from '@loopback/context';
import {
  FindRoute,
  InvokeMethod,
  InvokeMiddleware,
  ParseParams,
  Reject,
  RequestContext,
  RestBindings,
  Send,
  SequenceHandler,
} from '@loopback/rest';

const SequenceActions = RestBindings.SequenceActions;

export class MySequence implements SequenceHandler {
  /**
   * Optional invoker for registered middleware in a chain.
   * To be injected via SequenceActions.INVOKE_MIDDLEWARE.
   */
  @inject(SequenceActions.INVOKE_MIDDLEWARE, {optional: true})
  protected invokeMiddleware: InvokeMiddleware = () => false;

  constructor(
    @inject(SequenceActions.FIND_ROUTE) protected findRoute: FindRoute,
    @inject(SequenceActions.PARSE_PARAMS) protected parseParams: ParseParams,
    @inject(SequenceActions.INVOKE_METHOD) protected invoke: InvokeMethod,
    @inject(SequenceActions.SEND) public send: Send,
    @inject(SequenceActions.REJECT) public reject: Reject,
  ) {}

  async handle(context: RequestContext) {
    try {
      const {request, response} = context;
      const finished = await this.invokeMiddleware(context);
      if (finished) return;
      const route = this.findRoute(request);
      const args = await this.parseParams(request, route);
      const result = await this.invoke(route, args);
      this.send(response, result);
    } catch (err) {
      this.reject(context, err);
    }
  }
}

Thank you, that's was the problem, const finished = await this.invokeMiddleware(context); it needs to fix