vh13294 / nestjs-url-generator

Dynamically generating & signing URL based on controller method reference.
MIT License
44 stars 17 forks source link

Allow appUrl with protocol #3

Closed jensdev closed 3 years ago

jensdev commented 3 years ago

Hi,

Thanks for this package.

I had some troubles because I was using an appUrl with protocol. The change checks whether a protocol is used in the appUrl and adds it to the host.

Thanks

jensdev commented 3 years ago

At the moment I'm using a custom guard, based on the signed-url-guard with a different validateRequest passing in the protocol to check the signature.

private validateRequest(request: any): boolean {
    if (!request.headers.host) {
      throw new MethodNotAllowedException(
        'Unable to derive host name from request',
      );
    }

    return this.urlGeneratorService.isSignatureValid({
      host: `${request.protocol}://${request.headers.host}`,
      routePath: request._parsedUrl.pathname,
      query: request.query,
    });
  }
vh13294 commented 3 years ago

Hi, I will take a look in a few days.

Just one question, shouldn't http vs https be handled by express/nestjs middleware/plugin?

Anyway, thanks for the contribution.

jensdev commented 3 years ago

Hi, thanks for the response. My issue occurs in the following case

my .env file

URL=https://mydomain.com

app.module.ts

  UrlGeneratorModule.forRootAsync({
    imports: [ConfigModule],
    useFactory: async (cfg: ConfigService) => ({
      secret: cfg.get('SIGNING_SECRET'),
      appUrl: cfg.get('URL'),
    }),
    inject: [ConfigService],
  }),

So the signature is based on the appUrl with protocol. But in the SignedUrlGuard it's passing in the req.headers.host value which does not contain the protocol: https://

If you don't think this is an issue, feel free to close. I'll just use my custom guard

vh13294 commented 3 years ago

fixed in https://github.com/vh13294/nestjs-url-generator/pull/4