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

Added CSP nonce #1037

Merged
merged 21 commits into from
Oct 2, 2019
Merged

Added CSP nonce #1037

merged 21 commits into from
Oct 2, 2019

Conversation

Zweder
Copy link

@Zweder Zweder commented Jan 1, 2019

I added a nonce property to DragDropContext which is added to the injected style tags to make this project work with strict ContentSecurityProtection settings

@treshugart
Copy link

Thanks, @Zweder. Alex will have a look at this when he's back. See #1010 for more details on his absence.

@alexreardon
Copy link
Collaborator

This looks good to me. I'll have a play. @Zweder can you confirm that this fixes #1050?

@alexreardon
Copy link
Collaborator

For now let's see if this resolves the issue. After that we will need to do some productisation:

  • commenting code to explain why nonce is there
  • adding to docs to explain nonce prop

@alexreardon
Copy link
Collaborator

There are a number of CSS in JS solutions that add css to a page without requiring this sort of thing. I think it would be worth looking into how they get around this issue

@mechanicalbot
Copy link

Hey @alexreardon
Since no better solution was proposed, maybe we will stick to that?
Is there anything I can do to merge this sooner?

@alexreardon
Copy link
Collaborator

Let's pick this up: #1050 (comment)

@funktr0n
Copy link

Hey guys, distant observer here but I just wanted to see where things were with this. Is this PR the only way forward for nonce support or are there other alternatives now?

@Zweder
Copy link
Author

Zweder commented Jun 10, 2019

Right back on it. A cypress test will be up next

@alexreardon
Copy link
Collaborator

alexreardon commented Jun 10, 2019 via email

process.on('exit', () => {
storybook.kill();
standalone.kill();
});

waitPort({
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain this change?

Copy link
Author

Choose a reason for hiding this comment

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

standalone is a mini webpack server and client instance, the nonce has to be generated at the server

package.json Outdated
@@ -130,6 +130,7 @@
"stylelint-config-styled-components": "^0.1.1",
"stylelint-processor-styled-components": "^1.8.0",
"testcafe-reporter-xunit": "^2.1.0",
"uuid": "^3.3.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just use a local sequence generator to avoid another dependency?

Eg:

let count = 0;
function getId(): string {
  return `id-${count++}`;
}

Copy link
Author

Choose a reason for hiding this comment

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

I should be a cryptographically save random number but to prove a test i will change it

Copy link
Author

Choose a reason for hiding this comment

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

This looks better i think

let count = 0; function getNonce(): string { return ThisShouldBeACryptographicallySecurePseudoRandomNumber-${count++}; }

@alexreardon
Copy link
Collaborator

Thanks for your hard work on this one. I think it is close to being good to go. I just had a few minor thoughts

@alexreardon
Copy link
Collaborator

It is fantastic that we are actually testing this in a browser. Well done

test: /jsx?$/,
// type: "javascript/auto",
exclude: [/node_modules/],
use: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we defer to our standard babel rc file for this?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, cleaned up

const path = require('path');
const config = require('./webpack.config');

const fs = new MemoryFS();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this used?

Copy link
Author

Choose a reason for hiding this comment

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

This is the setup for the standalone webpack server and client, which run from memory saves a lot of pollution of output files

const compiler = webpack(config);

// $ExpectError
compiler.outputFileSystem = fs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you explain this one to me? I am not a webpack ninja

Copy link
Author

Choose a reason for hiding this comment

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

I've never managed to get Flow happy about specifiing a outputFileSystem, but i dont like to cleanup everything webpack outputs

@NFabrizio
Copy link

Just a general question. Would it be preferable to either rely on a meta tag instead of a passed prop or allow both?

I ask this since there are a few other packages that pull in react-beautiful-dnd that may or may not allow the passing of a nonce from the consumer to the react-beautiful-dnd component.

A decent example of this is JSS which uses a meta tag named "csp-nonce". Here is how they are implementing their use of that meta tag.
https://github.com/cssinjs/jss/blob/0af646d31962518f9603493825d43f3410ff6c14/packages/jss/src/DomRenderer.js#L255

Their implementation allows downstream consumers to pass a nonce to their components effectively without having to rely on the middleman to pass the nonce through.

@alexreardon
Copy link
Collaborator

That seems really smart @NFabrizio. What do you think @Zweder ?

@Zweder
Copy link
Author

Zweder commented Sep 9, 2019

You are right @NFabrizio, actually thats the whole idea behind this PR but in a different way. Accessing the dom is not efficient thats why you schould provide the nonce with the help of React.Context which in turn gets the nonce from a meta tag or something else. That means that you only access the dom once in the lifetime of the app, and you can reuse that context provider for every third party component that needs a nonce.

In src/test/standalone/server.js i do add a meta tag csp-nonce and in src/test/standalone/client.js it picks up the meta tag and provides the nonce to the DragDropContext in src/test/standalone/app.jsx without React.Context for simplicity

@Zweder
Copy link
Author

Zweder commented Sep 9, 2019

@alexreardon It would be nice if you could find some time to review my commits

@alexreardon
Copy link
Collaborator

This looks good to me. Can we please target the dev branch? (12.0). It would be great to get this in!

@Zweder Zweder changed the base branch from master to dev September 19, 2019 16:16
@Zweder
Copy link
Author

Zweder commented Sep 19, 2019

@alexreardon On it, i've got to fix the tests, the rest was a easy merge

@Zweder
Copy link
Author

Zweder commented Sep 21, 2019

@alexreardon this PR is now in sync with the dev branch. Some tests are failing but are not related to this PR.

@alexreardon
Copy link
Collaborator

I'll try to take a look at this soon 👍

@alexreardon
Copy link
Collaborator

This is looking really good! One more thing: can you create a content-security-policy guide? Explaining what the problem is, and how you use a nonce to get going

@alexreardon
Copy link
Collaborator

#1317

@alexreardon
Copy link
Collaborator

If you do not have time, I can try to put something together

@Zweder
Copy link
Author

Zweder commented Sep 26, 2019 via email

@Zweder
Copy link
Author

Zweder commented Sep 30, 2019

I've added content-security-policy it's insired on the JSS CSP docs but it shows how to use it and explains what it's for. Have a look

@alexreardon
Copy link
Collaborator

Fantastic work on this!

@alexreardon alexreardon merged commit 53b4599 into atlassian:dev Oct 2, 2019
@alexreardon
Copy link
Collaborator

👏 This will be going out in 12.0 #1487

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.

6 participants