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

Inline scripts should be avoided #83

Closed
michael-e opened this issue Nov 15, 2016 · 7 comments
Closed

Inline scripts should be avoided #83

michael-e opened this issue Nov 15, 2016 · 7 comments

Comments

@michael-e
Copy link
Member

The extension driver adds an inline script to the head of the page:

// add pagination and filter data into symphony context if Symphony does not provide it
Administration::instance()->Page->addElementToHead(
	new XMLElement(
		'script', 
		'if (! Symphony.Context.get(\'env\').pagination) Symphony.Context.get(\'env\').pagination='.json_encode($pagination).';' .
		'if (! Symphony.Context.get(\'env\').filters) Symphony.Context.get(\'env\').filters='.json_encode($generatedFilters).';'
		, array(
			'type' => 'text/javascript'
		)
	)
);

This should be avoided. If you use Content Security Policy HTTP headers, you will always attempt to limit the allowed script sources as much as possible, avoiding inline scripts at all costs. (Security evaluation websites like https://observatory.mozilla.org will blame you for allowing inline scripts because this enables special attack vectors.)

Is there any reason why the snippet above has not simply been added to assets/order_entries.publish.js?

@nitriques
Copy link
Member

Is there any reason why the snippet above has not simply been added to assets/order_entries.publish.js

Laziness is the culprit. If you want, please send a PR and I'll release. Thanks.

@nitriques
Copy link
Member

I was feeling bad, so I fixed it. I am also trying to remove 'unsafe inline' from my CSP directives.

@michael-e Can you test the latest integration i.e. 2.3.7 ? Thanks.

@michael-e
Copy link
Member Author

It works fine for me, but I have no real-life test case for filtered or paginated ordering. (I have not used these features in my projects.)

@nitriques
Copy link
Member

Me neither. But it was working on my 'test' setup... So I ship ?

@michael-e
Copy link
Member Author

Hmmm, yes, I would ship it. But is it a bugfix? I mean, shouldn't it be v2.4.0?

@twiro
Copy link
Member

twiro commented Nov 18, 2016

I tested it too... looks good in normal mode (apart from my general issues with the current behavior of this extension). Looks also good when using it combined with pagination. But I haven't tested filtered ordering yet... but might do so today!

@nitriques
Copy link
Member

But is it a bugfix?

No a security patch. Not a new feature. Shipped as 2.3.7

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

No branches or pull requests

3 participants