Validator: No longer warns about CSP

[erosman]

Since the latest push, the validator no longer warns about CSP.

Example:
https://addons.mozilla.org/editors/review/850282/

[EnTeQuAk]

As far as I know we explicitly enabled them again since that matches current add-on policies.

See https://github.com/mozilla/addons-linter/blob/master/tests/parsers/test.manifestjson.js#L423-L426

Also… to verify issues faster and give more proper responses it'd be wonderful if you could add a little bit more detail - e.g what rules weren't warned about but you expected them to etc.

Closing as this is expected and not a bug.

[erosman]

The issue should have been apparent by testing the link I gave in my report.

Example addon:
https://addons.mozilla.org/editors/review/850282/

Validator: No mention of CSP
https://addons.mozilla.org/en-US/developers/addon/talking-webb/file/714747/validation

manifest.json
https://addons.mozilla.org/en-US/firefox/files/browse/714747/file/manifest.json#top

Issue
"content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'",

Problem:
Validator No longer warns about existence of CSP and unsafe-eval

I have my own scripts that picks up CSP and unsafe-eval but others may miss it. As the example clearly shows, the validator did not pick up the potential security risk of CSP & unsafe-eval

Closing as this is expected and not a bug.

I didn't understand. Is it expected to miss unsafe-eval in CSP in validator?

[EnTeQuAk]

Is it expected to miss unsafe-eval in CSP in validator?

As far as I know it's more that there are valid cases where it might make sense. Summoning @wagnerand

[wagnerand]

I just re-read the pull request discussions. The reason, I asked for this (in #1434 (review)) was that unsafe-eval is not strictly forbidden.

That said, @erosman is probably right in that we might want to add a warning for unsafe-eval (note that we can ignore unsafe-inline, since it's unsupported as I learned in the meantime). But that warning should be about eval and is unrelated to remote script execution, which is what that patch was about (https://github.com/mozilla/addons-linter/pull/1434/files#diff-abf32190003257adb7218839b0bce1ffR52).

[kewisch]

Can you share more details on why unsafe-inline is unimportant? AIUI, if there is code that manages to inject DOM elements from a remote source, this could also be a script tag, which unsafe-inline will prevent. Sure, this may not be as common, but as the workaround is just making that script a separate file I don't see why we should relax this rule.

[wagnerand]

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/content_security_policy says that unsafe-inline is entirely unsupported.

[EnTeQuAk]

@wagnerand we don't have any way of saying that unsafe-inline is unsupported, so maybe flagging it might be worth doing too now that we aim to flag unsafe-eval too? Especially once it get's supported?

[wagnerand]

@EnTeQuAk I am inclined to say we just ignore unsafe-inline.

Also, let me find out whether we plan on supporting it at some point.

[wagnerand]

According to @kmaglione, we don't plan on supporting it.

[erosman]

Another example

https://addons.mozilla.org/editors/review/851577/
https://addons.mozilla.org/en-US/firefox/files/browse/717300/file/manifest.json#top
"content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'"

[ValentinaPC]

Verified this on AMO-dev FF55(Win 7).
@EnTeQuAk : If I understood correctly well we should show a warning for unsafe-eval as in the example provided above, but we don't. I misunderstood?
Attaching used webextension.
eval in CSP.zip

[erosman]

If I understood well we should show a warning for unsefe-eval as in the example provided above, but we don't. I misunderstood?

There used to be a warning about CSP, regardless of its content.
For example here:
https://addons.mozilla.org/en-US/developers/addon/preview-app-v2/file/704122/validation

Of course, that warning was not always needed since CSP in itself is not a problem.

From what I understand, the validator was updated to only show a warning when CSP exists AND it contains unsefe-eval, URL, etc.

The updated validator now misses the CSP altogether as shown in the examples.

[ValentinaPC]

From what I understand, the validator was updated to only show a warning when CSP exists AND it contains unsefe-eval, URL, etc.

The updated validator now misses the CSP altogether as shown in the examples.

Just to be sure....

CSP - no unsave eval => warning

CSP - with unsave eval => no warning

Are ok?

[wagnerand]

Doesn't seem right to me.

The linter should raise a warning (respectively) for

1. Using a CSP that allows remote scripts, for example: script-src https:;
2. Using a CSP that allows eval, for example: script-src 'self' 'unsafe-eval';

So, the CSP script-src https: 'unsafe-eval'; should raise two warnings.

[wagnerand]

Also, this works for me locally using the file from #1506 (comment).

 ./bin/addons-linter ~/Downloads/eval.in.CSP.zip
Validation Summary:

errors          0
notices         0
warnings        1

WARNINGS:

Code                       Message                                            Description                                                                                       File            Line   Column
MANIFEST_CSP_UNSAFE_EVAL   Using 'eval' has strong security and performance   In most cases the same result can be achieved differently, therefore it is generally prohibited   manifest.json
                           implications.

@ValentinaPC Did you test on -dev? This isn't on -dev yet.

[EnTeQuAk]

I'll update the issue once this is on dev, I'll probably release a new version Monday morning.

[ValentinaPC]

@EnTeQuAk : not yet on -dev? Thanks!

[EnTeQuAk]

Ah, I forgot this issue. It is now, got a release yesterday. It's on -dev and -stage.

[ValentinaPC]

I see no warning at validation and a warning in the report page. Is this ok?

Used the same extension as before.

[wagnerand]

Yeah, I can confirm that. @EnTeQuAk do you have time to look into this?

[ValentinaPC]

Also, when I use both options in the form:
"content_security_policy": "script-src 'self' 'unsafe-eval' https://example.com; object-src 'self'"
I receive 1 warning at validation and 2 warnings in the report page.
If I reverse the order and use the form:
"content_security_policy": "script-src 'self' https://example.com; 'unsafe-eval' object-src 'self'"
I receive 1 warning validation and 1 warning in the report page

[EnTeQuAk]

Mhmm, interesting. I'll look into it later today

On Wed, Sep 13, 2017, at 02:08 PM, ValentinaPC wrote: Also, when I use both options in the form: "content_security_policy": "script-src 'self' 'unsafe-eval' https://example.com; object-src 'self'"> I receive 1 warning at validation and 2 warnings in the report page.> If I reverse the order and use the form: "content_security_policy": "script-src 'self' https://example.com; 'unsafe- eval' object-src 'self'"> I receive 1 warning validation and 1 warning in the report page — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub[1], or mute the thread[2].>

Links: 1. #1506 (comment) 2. https://github.com/notifications/unsubscribe-auth/AAIfGX7nWxpM5EKnAmbEa9VmGL153Dn7ks5sh8XagaJpZM4PLFnu

[EnTeQuAk]

@ValentinaPC @wagnerand yeah, I can confirm that and it looks similar (or even a duplicate) to #1547

I'll use #1547 to track this and work on it there.