Disable default x-frame-options header

mortenhillbom commented 1 week ago

User description

Disabling the x-frame-options header that is set to sameorigin by helmet as default. The header made it hard to embed applications using SSO. Instead, the responsibility of setting this header should be solely on the frontend on the application itself.

Before submitting this PR:

Checklist

[ ] No breaking changes
[ ] Tests pass
[ ] New features have new tests
[ ] Documentation is updated

Breaking changes

Avoid breaking changes and regressions. If you feel it is unavoidable, make it explicit in your PR comment so we can review it and see how to handle it.

Tests

please make sure your changes pass the current tests (Use the make test or the make watch command).
if you are introducing a new feature, please write as much tests as possible.

Documentation

Please make sure the documentation is updated accordingly, in particular:

Workflows. Workflows are Mermaid sequence diagrams
Schema. The schema in a Mermaid ER diagram
Environment variables. Please adjust the .env.example file accordingly
OpenApi specifications. We are using inline JSDoc annotations

PR Type

Bug fix

Description

Disabled the default x-frame-options header set by Helmet to sameorigin.
Modified the Helmet configuration to not set the x-frame-options header, allowing applications using SSO to embed without issues.

Changes walkthrough 📝

Relevant files

Bug fix

app.ts

Disable default x-frame-options header in Helmet configuration

src/app.ts

Disabled the default x-frame-options header set by Helmet.

Configured Helmet to not set the x-frame-options header.

+1/-1

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro[bot] commented 1 week ago

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/nhost/hasura-auth/commit/3487118f5891f8b7bdc279bf09354bf940c11e43)

⏱️ Estimated effort to review [1-5]	2
🧪 Relevant tests	No
🔒 Security concerns	- Clickjacking Vulnerability: Removing the `x-frame-options` header without implementing alternative protections can make the application vulnerable to clickjacking attacks. This should be addressed either by frontend controls or other server-side security headers.
⚡ Key issues to review	Possible Security Risk: Disabling the `x-frame-options` header can expose the application to clickjacking attacks. It's important to ensure that other measures are in place to mitigate this risk, especially if the application is embedded in third-party sites.

codiumai-pr-agent-pro[bot] commented 1 week ago

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Adjust the helmet configuration to selectively allow framing from trusted domains instead of disabling it entirely ___ Consider using a more specific configuration for the `helmet` middleware instead of disabling `xFrameOptions` globally. This change might inadvertently lower the security by allowing your app to be embedded in a frame from any origin. If the intention is to allow specific origins, you can configure `xFrameOptions` to allow only those. [src/app.ts [22]](https://github.com/nhost/hasura-auth/pull/534/files#diff-841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27bR22-R22) ```diff -app.use(helmet({ xFrameOptions: false }), json()); +app.use(helmet({ + xFrameOptions: { action: 'ALLOW-FROM', domain: 'https://example.com' } +}), json()); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: The suggestion addresses a significant security concern by preventing the application from being embedded in a frame from any origin, which could lead to clickjacking attacks. It proposes a more secure configuration for the `helmet` middleware.	9

mortenhillbom commented 1 week ago

@dbarrosop If preferred I guess it could be set like this instead? Keeping some secure defaults, but giving users the possibility to allow certain domains

app.use(
  helmet({
    xFrameOptions: {
      action: 'ALLOW-FROM',
      domain: ENV.AUTH_ACCESS_CONTROL_ALLOWED_REDIRECT_URLS,
    },
  }),
  json()
);

dbarrosop commented 1 week ago

If preferred I guess it could be set like this instead?

I will do a bit more of reading to confirm but I think it is fine to just disable it, this header is meant to protect you from clickjacking attacks but this is an API returning json or doing redirects, there is nothing to click.

codiumai-pr-agent-pro[bot] commented 1 week ago

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Modify the xFrameOptions setting to 'SAMEORIGIN' for enhanced security ___ Consider setting `xFrameOptions` to a specific policy rather than disabling it entirely to maintain some level of protection against clickjacking. For example, you can set it to 'SAMEORIGIN' to allow framing of the site only by pages in the same origin. [src/app.ts [22]](https://github.com/nhost/hasura-auth/pull/534/files#diff-841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27bR22-R22) ```diff -app.use(helmet({ xFrameOptions: false }), json()); +app.use(helmet({ xFrameOptions: 'SAMEORIGIN' }), json()); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: The suggestion improves security by setting `xFrameOptions` to 'SAMEORIGIN' instead of disabling it entirely, which helps protect against clickjacking attacks.	9

dbarrosop commented 1 week ago

thx for pointing us to the right place, I am closing this PR in favor of #535

nhost / hasura-auth