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.

Sign up for GitHub

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

Jump to bottom

Move to stricter CSP #11028

Merged
merged 3 commits into from
Oct 14, 2018
Merged

Move to stricter CSP #11028

merged 3 commits into from
Oct 14, 2018

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Sep 3, 2018 •
edited
Loading

Do not allow unsafe eval by default.

🚧

Last merge for https://github.com/orgs/nextcloud/projects/18
This will be rebased several times to validate CSP behavior

TODO:

  • This will kill of OC.AddScript converted by mr @ChristophWurst
    • Need to look into affected apps

@rullzer rullzer added the 2. developing Work in progress label Sep 3, 2018
@rullzer rullzer added this to the Nextcloud 15 milestone Sep 3, 2018
@rullzer rullzer force-pushed the feature/no_eval_csp branch from efcec80 to d839df2 Compare September 3, 2018 13:28
@nextcloud-bot nextcloud-bot mentioned this pull request Sep 3, 2018
Closed
@rullzer
Copy link
Member Author

rullzer commented Sep 28, 2018

Next one:

server/apps/files/js/filemultiselectmenu.js

Line 12 in 0c77b93

var TEMPLATE_MENU =

@rullzer rullzer force-pushed the feature/no_eval_csp branch from ad7e58b to 7b986d3 Compare October 1, 2018 18:54
@juliusknorr juliusknorr mentioned this pull request Oct 2, 2018
Merged
@MorrisJobke MorrisJobke mentioned this pull request Oct 3, 2018
Merged
1 task
@rullzer rullzer force-pushed the feature/no_eval_csp branch from 7b986d3 to c9c8c60 Compare October 4, 2018 09:01
@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 4, 2018
@rullzer
Copy link
Member Author

rullzer commented Oct 4, 2018

So I think this is good to get in.

Other stuff we should fix once we run into it.
But if we merge it we can announce it for 15 so app devs can start preparing as well.

ChristophWurst
ChristophWurst approved these changes Oct 9, 2018
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Let's do this

lib/public/AppFramework/Http/ContentSecurityPolicy.php Outdated
@@ -49,7 +49,7 @@ class ContentSecurityPolicy extends EmptyContentSecurityPolicy {
* TODO: Disallow per default
Copy link
Member

Choose a reason for hiding this comment

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

remove

@ChristophWurst
Copy link
Member

This will kill of OC.AddScript

* Need to look into affected apps

https://github.com/search?q=%22OC.AddScript%22&type=Code have fun 🙈

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 9, 2018
@ChristophWurst
Copy link
Member

💥
bildschirmfoto von 2018-10-09 08-50-24

@MorrisJobke
Copy link
Member

  • Need to look into affected apps

I only have found richdocuments and files_texteditor. 🤷‍♂️

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 9, 2018
@ChristophWurst
Copy link
Member

I only have found richdocuments and files_texteditor. man_shrugging

I've pushed my fix that replaces the eval code with $.getScript. Could you please test if this actually works for these apps?

@rullzer
Copy link
Member Author

rullzer commented Oct 9, 2018

@ChristophWurst richdocuments still works with this! yay!

@rullzer rullzer force-pushed the feature/no_eval_csp branch from dd1d0ce to ea0d692 Compare October 10, 2018 11:43
MorrisJobke
MorrisJobke reviewed Oct 10, 2018
View reviewed changes
.htaccess Outdated
#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####

ErrorDocument 403 /
ErrorDocument 404 /
Copy link
Member

Choose a reason for hiding this comment

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

🙈

MorrisJobke
MorrisJobke approved these changes Oct 10, 2018
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and didn't explode 👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 10, 2018
@MorrisJobke
Copy link
Member

JS unit fails.

PhantomJS 2.1.1 (Linux 0.0.0) OC.L10N tests plurals generates plural for default text when translation does not exist FAILED
	Expected spy warn to have been called.
	core/js/tests/specs/l10nSpec.js:90:41
PhantomJS 2.1.1 (Linux 0.0.0) OC.L10N tests plurals generates plural with default function when no forms specified FAILED
	Expected spy warn to have been called.
	core/js/tests/specs/l10nSpec.js:110:41
	TypeError: undefined is not an object (evaluating 'locale.length') in core/js/l10n.js (line 9)
	_getPlural@core/js/l10n.js:9:5835
	translatePlural@core/js/l10n.js:9:4906
	[native code]
	checkPlurals@core/js/tests/specs/l10nSpec.js:73:6
	core/js/tests/specs/l10nSpec.js:111:16
PhantomJS 2.1.1 (Linux 0.0.0) OC.L10N tests plurals generates plural with generated function when forms is specified FAILED
	TypeError: undefined is not an object (evaluating 'locale.length') in core/js/l10n.js (line 9)
	_getPlural@core/js/l10n.js:9:5835
	translatePlural@core/js/l10n.js:9:4906
	[native code]
	checkPlurals@core/js/tests/specs/l10nSpec.js:73:6
	core/js/tests/specs/l10nSpec.js:118:16
PhantomJS 2.1.1 (Linux 0.0.0) OC.L10N tests plurals generates plural with function when forms is specified as function FAILED
	TypeError: undefined is not an object (evaluating 'locale.length') in core/js/l10n.js (line 9)
	_getPlural@core/js/l10n.js:9:5835
	translatePlural@core/js/l10n.js:9:4906
	[native code]
	checkPlurals@core/js/tests/specs/l10nSpec.js:73:6
	core/js/tests/specs/l10nSpec.js:130:16
PhantomJS 2.1.1 (Linux 0.0.0) OC.L10N tests async loading of translations calls callback if translation already available FAILED
	Expected spy warn to have been called.
	core/js/tests/specs/l10nSpec.js:167:41

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Oct 11, 2018
@rullzer rullzer force-pushed the feature/no_eval_csp branch from f738ae3 to 385cfff Compare October 11, 2018 18:27
rullzer and others added 3 commits October 14, 2018 20:45
@rullzer
Disallow unsafe-eval by default
5b61ef9 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@ChristophWurst @rullzer
Use jQuery.getScript to dynamically load script
365f683 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@rullzer
Fix plural function to be hardcoded
056a74e 
No more weird eval to construct a plural function.
We just use the plural function from symfony.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the feature/no_eval_csp branch from 385cfff to 056a74e Compare October 14, 2018 18:45
@rullzer rullzer merged commit 82a5833 into master Oct 14, 2018
@rullzer rullzer deleted the feature/no_eval_csp branch October 14, 2018 19:17
@rullzer rullzer added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Oct 14, 2018
@tcitworld tcitworld mentioned this pull request Oct 18, 2018
Merged
30 tasks
@rullzer rullzer mentioned this pull request Nov 8, 2018
Closed
@MorrisJobke
Copy link
Member

@Arvidas mind to open a new ticket and link here?

This was referenced Mar 11, 2019
Closed
Merged
tcitworld added a commit that referenced this pull request Dec 12, 2020
@tcitworld
Update comment to reflect current CSP policy
71cf926 
JS unsafe-eval was removed a long time ago in #11028
@tcitworld tcitworld mentioned this pull request Dec 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@danxuliu danxuliu Awaiting requested review from danxuliu

Assignees

@rullzer rullzer

@ChristophWurst ChristophWurst

Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Milestone
Nextcloud 15
Development

Successfully merging this pull request may close these issues.
3 participants
@rullzer @ChristophWurst @MorrisJobke