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

Update/Add security headers #280

Merged
merged 39 commits into from
Feb 20, 2025
Merged

Update/Add security headers #280

merged 39 commits into from
Feb 20, 2025

Conversation

max-debug022
Copy link
Contributor

@max-debug022 max-debug022 commented Jun 26, 2024

This PR improves/updates the HTTP-Headers


TLDR

  • Update helmet to 7.2.0 in API and Admin
  • Coherent CORS-Policy (Cross-Origin-Ressource-Sharing) for the API -> to Admin & Site
  • Coherent CORP/COEP (Cross-Origin-Ressource/Embedder-Policy) for API, Admin & Site
  • Strict COOP (Cross-Origin-Opener-Policy) for API, Admin & Site
  • Removal of deprecated, redundant, or prohibited headers
  • CSP (Content Security Policy) that enforces the JEA (just enough access) principle
This PR implements the "Just-Enough-Access" (JEA) security strategy, ensuring that only necessary privileges are granted while restricting all others. The PR includes a coherent CORS policy so that resources can only be shared within the microservices. Additionally, CORP/COEP have been enforced to control resource access and embedding to authorized origins only. Several Content Security Policy (CSP) errors have been resolved & unnecessary privileges have been removed - Admin has slightly more flexibility than the Site. Reasons for several changes:
  • Update helmet to v7.1.0
  • CORS uses now a maxAge of 600s as Chromium (prior to v76) caps at 10mins
  • CORP only for Same-Site
  • Set COOP for same-origin
  • Remove the empty exposedHeaders field as it is the default
  • Disable unused x-powered-by header
  • Disable deprecated X-XSS-Protection Header
  • Disable deprecated X-Frame-Options Header
  • Enable HSTS with recommended values
  • Disable Referrer Policy
  • ...

@max-debug022 max-debug022 requested a review from mennoxx June 27, 2024 07:52
@nsams nsams removed their request for review June 27, 2024 08:46
@thomasdax98
Copy link
Member

Did you test this in one of our projects in a deployed setting? I fear that there might be some unwanted side-effects relating to different domains in a deployed setting

@max-debug022
Copy link
Contributor Author

Did you test this in one of our projects in a deployed setting? I fear that there might be some unwanted side-effects relating to different domains in a deployed setting

Yes, I tested it on deployed settings.

@dkarnutsch
Copy link
Contributor

Did you test this in one of our projects in a deployed setting? I fear that there might be some unwanted side-effects relating to different domains in a deployed setting

I would not add this to existing projects without extensive testing. However, the starter can define best practices and hopefully all bugs are discovered during development. So I think it is relatively safe to merge this.

@dkarnutsch
Copy link
Contributor

dkarnutsch commented Jul 1, 2024

Some header could possible have a comment, when to extend/change them.

You could also add links to best practices (MDN?), so we can track changes.

@max-debug022
Copy link
Contributor Author

max-debug022 commented Jul 1, 2024

Some header could possible have a comment, when to extend/change them.

You could also add links to best practices (MDN?), so we can track changes.

Luckily I originally added comments to each Header but removed them since I wasn't sure if comments should be in the code. I could add them again, but I think it would be better to add detailed docs on how to modify each header and what effect each header has. What would you prefer?

@dkarnutsch
Copy link
Contributor

Some header could possible have a comment, when to extend/change them.
You could also add links to best practices (MDN?), so we can track changes.

Luckily I originally added comments to each Header but removed them since I wasn't sure if comments should be in the code. I could add them again, but I think it would be better to add detailed docs on how to modify each header and what effect each header has. What would you prefer?

In code, as documentation can drift away. @johnnyomair?

@johnnyomair
Copy link
Collaborator

Some header could possible have a comment, when to extend/change them.
You could also add links to best practices (MDN?), so we can track changes.

Luckily I originally added comments to each Header but removed them since I wasn't sure if comments should be in the code. I could add them again, but I think it would be better to add detailed docs on how to modify each header and what effect each header has. What would you prefer?

In code, as documentation can drift away. @johnnyomair?

Agreed. We could add line comments for each header/item.

@max-debug022
Copy link
Contributor Author

max-debug022 commented Jul 8, 2024

Agreed. We could add line comments for each header/item.

@johnnyomair what headers should be documented and how? I don't think an explanation is useful since anyone can look headers up and they are fairly well explained. And if you understand the header you probably understand the logic behind their value.

For example. x-xss-protection - the first thing in the MDN docs is that it should not be used, so its value is false.

Edit: My initial documentation was somewhat like this:

app.use(
  helmet({
    contentSecurityPolicy: {
      directives: {
        "default-src": ["'none'"], // No external resources are needed
      },
      useDefaults: false, // Disable all other default CSP rules
    },
    xXssProtection: false, // Disable deprecated header
    xFrameOptions: false, // Disable deprecated header
    crossOriginResourcePolicy: "same-origin", // Do not allow cross-origin requests to access the response
    crossOriginEmbedderPolicy: true, // =no-corp Enable COEP
    crossOriginOpenerPolicy: true, // =same-origin Enable COOP
    strictTransportSecurity: {
      // Enable HSTS
      maxAge: 63072000, // 2 years (recommended if including subdomains)
      includeSubDomains: true, // Include all subdomains
      preload: true, // Enable HSTS preload
    },
    referrerPolicy: {
      policy: "no-referrer", // No referrer information needs to be sent
    },
    xContentTypeOptions: true, // = nosniff
    xDnsPrefetchControl: false, // Disable non-standard header as recommended by MDN
    xDownloadOptions: true, // = noopen (forces IE8+ not to open files in the context of the page but instead download them)
    xPermittedCrossDomainPolicies: true, // = none (prevent the browser from MIME-sniffing)
    originAgentCluster: true, // = false (disables the agent cluster)
  }),
);

@thomasdax98
Copy link
Member

Edit: My initial documentation was somewhat like this:

app.use(
  helmet({
    contentSecurityPolicy: {
      directives: {
        "default-src": ["'none'"], // No external resources are needed
      },
      useDefaults: false, // Disable all other default CSP rules
    },
    xXssProtection: false, // Disable deprecated header
    xFrameOptions: false, // Disable deprecated header
    crossOriginResourcePolicy: "same-origin", // Do not allow cross-origin requests to access the response
    crossOriginEmbedderPolicy: true, // =no-corp Enable COEP
    crossOriginOpenerPolicy: true, // =same-origin Enable COOP
    strictTransportSecurity: {
      // Enable HSTS
      maxAge: 63072000, // 2 years (recommended if including subdomains)
      includeSubDomains: true, // Include all subdomains
      preload: true, // Enable HSTS preload
    },
    referrerPolicy: {
      policy: "no-referrer", // No referrer information needs to be sent
    },
    xContentTypeOptions: true, // = nosniff
    xDnsPrefetchControl: false, // Disable non-standard header as recommended by MDN
    xDownloadOptions: true, // = noopen (forces IE8+ not to open files in the context of the page but instead download them)
    xPermittedCrossDomainPolicies: true, // = none (prevent the browser from MIME-sniffing)
    originAgentCluster: true, // = false (disables the agent cluster)
  }),
);

@max-debug022 I like this. During the review I had a hard time wrapping my head around "What does true (or false) mean in this case?"

I think you should add these comments

@max-debug022
Copy link
Contributor Author

I added the comments: cbc0472

thomasdax98
thomasdax98 previously approved these changes Jul 9, 2024
@johnnyomair
Copy link
Collaborator

@johnnyomair what headers should be documented and how? I don't think an explanation is useful since anyone can look headers up and they are fairly well explained. And if you understand the header you probably understand the logic behind their value.

I'd add comments for all settings that aren't obvious.

Also, I'd only change the headers if they differ from helmets default value.

johnnyomair
johnnyomair previously approved these changes Jul 22, 2024
@max-debug022
Copy link
Contributor Author

@max-debug022 max-debug022 force-pushed the security-header-jea branch 2 times, most recently from b8bb4da to d441331 Compare January 23, 2025 07:34
thomasdax98
thomasdax98 previously approved these changes Jan 23, 2025
@fraxachun
Copy link
Contributor

Shouldn't it be comet-demo first?

@johnnyomair
Copy link
Collaborator

Shouldn't it be comet-demo first?

This PR has been started long before we changed to demo first, so I'd like to finish it here and then copy it to Demo

@vivid-planet vivid-planet deleted a comment from sonarqubecloud bot Feb 19, 2025
@johnnyomair johnnyomair merged commit 8974dac into main Feb 20, 2025
3 checks passed
@johnnyomair johnnyomair deleted the security-header-jea branch February 20, 2025 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants