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

Add inline script hashes to DocumentProps #1373

Merged
merged 7 commits into from Feb 6, 2020

Conversation

@abiro
Copy link
Contributor

abiro commented Feb 5, 2020

Description

This PR adds a new object to the state attribute of DocumentProps called inlineScripts. The keys of the inlineScripts object are descriptive names of the scripts and contain the script source and its SHA256 hash. There is currently only one inline script, the route info hydration script:

  { 
      "routeInfo": { 
          "script": "window.__routeInfo = JSON.parse(\"route data as json string\")", 
          "hash": "sha256-<base64-value>",
      }
  }

These hashes can be used to construct a Content Security Policy in a meta tag without needing unsafe-inline for scripts.

Changes/Tasks

  • Refactor generateRouteInformation and move it from BodyWithMeta.js to exportRoute.js, as the inline route info script hash has to be computed there.
  • Move testing inline route info script from BodyWithMeta.test.js to exportRoute.test.js
  • Update BodyWithMeta.test.js.snap to only check for presence of injected script, not the script's contents, since the script isn't generate in BodyWithMeta anymore.
  • Compute SHA256 hash for route info inline script in exportRoute.
  • Test script hashes in exportRoute.test.js
  • Add inlineScripts member to state in exportRoute that holds the script source and its hash
  • Add inlineScripts to to DocumentProps interface in the type def file.
  • Add inlineScripts to Document documentation in docs/config.md.

Motivation and Context

Resolves #1362

Types of changes

  • Refactoring/add tests (refactoring or adding test which isn't a fix or add a feature)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have updated the documentation accordingly
  • I have updated the CHANGELOG with a summary of my changes
  • My changes have tests around them
@abiro abiro requested a review from SleeplessByte Feb 5, 2020
@abiro

This comment has been minimized.

Copy link
Contributor Author

abiro commented Feb 5, 2020

There is an unrelated change in the commits that prevents git from rewriting line endings in PNG files. I had to add this, because the * text eol=lf in .gitattributes was making git rewrite PNG files in the repo on Ubuntu. Let me know please if I should break this out to a different PR.

@abiro

This comment has been minimized.

Copy link
Contributor Author

abiro commented Feb 5, 2020

I also tried to test this out in my project by doing a yarn pack in packages/react-static and then installing that with npm install file:/..., but then react-static start failed with

Trace: Error: Cannot find module '../lib/utils/binHelper'

I'm wondering if you can suggest a better workflow for local installations?

@SleeplessByte

This comment has been minimized.

Copy link
Contributor

SleeplessByte commented Feb 5, 2020

I'm wondering if you can suggest a better workflow for local installations?

yarn build
yarn link

then in a new project yarn link react-static.

I think it's in CONTRIBUTING

Copy link
Contributor

SleeplessByte left a comment

Seems great!

@@ -10,6 +10,7 @@
- Add unofficial plugin `react-static-plugin-file-watch-reload` to plugins list
- Enable configuring css loader from `react-static-plugin-sass` and `react-static-plugin-less` ([#1348](https://github.com/react-static/react-static/pull/1348))
- Update `react-static-plugin-jss` for react-jss v10+. ([#1367](https://github.com/react-static/react-static/pull/1367))
- Add inline script hashes to `DocumentProps`. These hashes can be used to construct a Content Security Policy in a meta tag without `unsafe-inline` scripts.

This comment has been minimized.

Copy link
@SleeplessByte

SleeplessByte Feb 5, 2020

Contributor

Add the PR link please :)

This comment has been minimized.

Copy link
@abiro

abiro Feb 6, 2020

Author Contributor

Oops, done in fa8d0c0

hash: string
}

export interface InlineScripts {

This comment has been minimized.

Copy link
@SleeplessByte

SleeplessByte Feb 5, 2020

Contributor

We probably want "generic object, including this one":

export interface InlineScripts extends Record<string, InlineScript> {
   routeInfo: InlineScript
}

This should allow us to do this:

const scripts: InlineScripts = {
  routeInfo: { script: '...', hash: '...' }
}
// this should work

scripts['anything'] = { script: '...', hash: '...' }
// this should work

scripts.
// this should autocomplete `routeInfo`

It then disallows:

const scripts: InlineScripts = {
  routeInfo: { script: '...', hash: '...' }
}
scripts['foo'] = 'bar'
// Type '"bar"' is not assignable to type InlineScript

This comment has been minimized.

Copy link
@abiro

abiro Feb 6, 2020

Author Contributor

Nice, added in d76638e

abiro added 3 commits Feb 6, 2020
Based on feedback from @SleeplessByte:

We probably want "generic object, including this
one":

```
export interface InlineScripts extends Record<string, InlineScript> {
   routeInfo: InlineScript
}
```

This should allow us to do this:

```
const scripts: InlineScripts = {
  routeInfo: { script: '...', hash: '...' }
}
// this should work

scripts['anything'] = { script: '...', hash: '...' }
// this should work

scripts.
// this should autocomplete `routeInfo`
```

It then disallows:

```
const scripts: InlineScripts = {
  routeInfo: { script: '...', hash: '...' }
}
scripts['foo'] = 'bar'
// Type '"bar"' is not assignable to type InlineScript
```
@abiro

This comment has been minimized.

Copy link
Contributor Author

abiro commented Feb 6, 2020

I'm wondering if you can suggest a better workflow for local installations?

yarn build
yarn link

then in a new project yarn link react-static.

Thanks, I've tested it out locally now and the feature works as expected.

(Added these instructions to CONTRIBUTING in 7507d07)

@SleeplessByte SleeplessByte merged commit a24a84b into react-static:master Feb 6, 2020
6 checks passed
6 checks passed
Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Mixed content No mixed content detected
Details
Redirect rules 5 redirect rules processed
Details
Travis CI - Pull Request Build Passed
Details
deploy/netlify Deploy preview ready!
Details
@abiro

This comment has been minimized.

Copy link
Contributor Author

abiro commented Feb 6, 2020

Thanks for the quick review/merge @SleeplessByte :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.