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

refactor(@angular/build): Auto-CSP support as an index file transformation. #28639

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

aaronshim
Copy link
Contributor

@aaronshim aaronshim commented Oct 15, 2024

Auto-CSP is a feature to rewrite the <script> tags in a index.html file to either hash their contents or rewrite them as a dynamic loader script that can be hashed. These hashes will be placed in a CSP inside a <meta> tag inside the <head> of the document to ensure that the scripts running on the page are those known during the compile-time of the client-side rendered application.

This is an application of the recommended Strict CSP approach that is also recommended by the OWASP CSP Cheat Sheet and inspired by https://github.com/google/strict-csp. The approach of having build tooling auto-generate a hash-based CSP addresses the main concern around hash-based CSPs that generating and updating the hashes manually is difficult and error-prone.

PR Checklist

Please check to confirm your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

There is currently no logic to process the index.html to auto-generate hash-based CSPs.

What is the new behavior?

Adding logic to process the index.html to auto-generate hash-based CSPs.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@angular-robot angular-robot bot added detected: feature PR contains a feature commit area: @angular/build labels Oct 15, 2024
@dgp1130 dgp1130 added the target: major This PR is targeted for the next major release label Oct 15, 2024
Copy link
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together! Very excited to see this feature land.

I left a bunch of readability / refactoring suggestions, but none of them are really critical and what you have is pretty reasonable so feel free to decline any which aren't worth applying.

For the commit message, we should use refactor here since it doesn't actually expose any functionality yet. When you get to the "keystone" commit which exposes the relevant option for end users, that's where you would put feat to be included in the changelog.

packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
@angular-robot angular-robot bot removed the detected: feature PR contains a feature commit label Oct 16, 2024
@aaronshim aaronshim changed the title feat(@angular/build): Auto-CSP support as an index file transformation. refactor(@angular/build): Auto-CSP support as an index file transformation. Oct 16, 2024
packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
},
): string {
hashes = hashes || [];
let strictCspTemplate = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I mean something like:

const strictCspTemplate = new Map<string, string[]>(Object.entries({
  'script-src': [`'strict-dynamic'`, ...hashes],
  // ...
}));

// Update / assign like:
strictCspTemplate.set('require-trusted-types-for', ['script']);

// Consume at the end:
return Array.from(strictCspTemplate.entries()).map(([directive, value]) => `${directive} ${values.join(' ')};`)
    .join('');

Since all the keys are static string literals, it's really not that important so feel free to ignore. Just a thing I commonly recommend for any dynamic key usage since that commonly leads to prototype pollution vulnerabilities.

packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through these comments! I don't have any significant concerns at this point, @clydin can you take a quick look too?

@dgp1130
Copy link
Collaborator

dgp1130 commented Oct 18, 2024

FYI: For a review, the commits you have are probably fine, but once this is approved and ready to submit, you'll need to squash/amend them to match the commit message conventions so we can merge it. Just a heads up.

@aaronshim
Copy link
Contributor Author

FYI: For a review, the commits you have are probably fine, but once this is approved and ready to submit, you'll need to squash/amend them to match the commit message conventions so we can merge it. Just a heads up.

For sure! I realized that the GitHub code review interface was hiding comments as "outdated" and such when I amended and force pushed on the same commit, so I decided to stop doing that until we are ready to merge!

packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
packages/angular/build/src/utils/index-file/auto-csp.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
…ation.

Auto-CSP is a feature to rewrite the `<script>` tags in a index.html
file to either hash their contents or rewrite them as a dynamic loader
script that can be hashed. These hashes will be placed in a CSP inside a
`<meta>` tag inside the `<head>` of the document to ensure that the
scripts running on the page are those known during the compile-time of
the client-side rendered application.
@dgp1130 dgp1130 added the action: merge The PR is ready for merge by the caretaker label Oct 22, 2024
@dgp1130 dgp1130 merged commit 6beffd1 into angular:main Oct 22, 2024
31 checks passed
aaronshim added a commit to aaronshim/angular-cli that referenced this pull request Oct 22, 2024
Following up on the logic provided in angular#28639, we want to offer an opt-in
option in angular.json to enable the auto-CSP transformation.
aaronshim added a commit to aaronshim/angular-cli that referenced this pull request Oct 22, 2024
Following up on the logic provided in angular#28639, we want to offer an opt-in
option in angular.json to enable the auto-CSP transformation.

For now, builds for `ng serve` will have Auto-CSP disabled.
aaronshim added a commit to aaronshim/angular-cli that referenced this pull request Oct 22, 2024
Following up on the logic provided in angular#28639, we want to offer an opt-in
option in angular.json to enable the auto-CSP transformation.

For now, builds for `ng serve` will have Auto-CSP disabled.
dgp1130 pushed a commit that referenced this pull request Oct 22, 2024
Following up on the logic provided in #28639, we want to offer an opt-in
option in angular.json to enable the auto-CSP transformation.

For now, builds for `ng serve` will have Auto-CSP disabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: @angular/build target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants