juicycleff / ultimate-backend

Multi tenant SaaS starter kit with cqrs graphql microservice architecture, apollo federation, event source and authentication
https://juicycleff.github.io/ultimate-backend-docs
MIT License
2.6k stars 407 forks source link

Optional settings for harden security #211

Closed Jordan-Hall closed 3 years ago

Jordan-Hall commented 3 years ago

Not sure if this is the right thing to do. However, when you cant just set halemt and cors without csurf.

Jordan-Hall commented 3 years ago

what is the reason to check for emptyObject and then using the config object under the if. did you mean not empty? just read the codes and didn't get it.

because by default it should apply all of the security measures. It was a way to keep the current setup but also disable sections

amirtgm commented 3 years ago

That's ok but what i meant was the logic you wrote

    const emptyObject = Object.keys(config).length === 0;    
    if (emptyObject || config.helmet) {      
         this.app.use(helmet(config.helmet));    
    }

if there is no helmet property you are passing undefined to helmet function. IMO it should be if (!emptyObject || config.helmet) . this way it will disable it.

Jordan-Hall commented 3 years ago

I believe its currently passing in unfinded. So it's keeping it the way its currently working

juicycleff commented 3 years ago

@Jordan-Hall Thanks for this. Please could you fix the merge conflict?

Jordan-Hall commented 3 years ago

Done.

@amirtgm using !emptyObject would be a breaking change and isn't the desired change. By default it should enable all three

juicycleff commented 3 years ago

Thanks @Jordan-Hall