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

Invalid CSP not detected #2634

Closed
erosman opened this issue Jun 7, 2019 · 9 comments
Closed

Invalid CSP not detected #2634

erosman opened this issue Jun 7, 2019 · 9 comments

Comments

@erosman
Copy link
Contributor

erosman commented Jun 7, 2019

ref:

https://addons.mozilla.org/en-US/developers/addon/kiwiback/file/3020642/validation
https://reviewers.addons.mozilla.org/en-US/firefox/files/browse/3020642/

manifest.json

...
],
    "content_security_policy": "script-src 'self' object-src 'self'",
    "homepage_url": "https://www.kiwiback.cz/",
    "permissions": [
          "tabs",
...

@wagnerand

@muffinresearch
Copy link
Contributor

@erosman to make it easier for someone who doesn't necessarily have reviewer access, would it be possible for you to provide more info in the bug report rather than link to reviewer pages?

@erosman
Copy link
Contributor Author

erosman commented Jun 26, 2019

@muffinresearch The relevant information that was included in the post is:
"content_security_policy": "script-src 'self' object-src 'self'",

That can be tested by entering it into any manifest.json

Furthermore, the developer link was given for the validator result which is not on reviewers interface. Can't it be accessed by the dev team?

The addon in question is: https://addons.mozilla.org/en-US/firefox/addon/kiwiback/

Please let me know if further information is required.

@muffinresearch
Copy link
Contributor

muffinresearch commented Jun 26, 2019

@erosman If you assume minimal prior knowledge then that would make for a better bug report. As originally filed any reader is having to guess at the what the issue you're reporting actually is.

The template for bug reports has headings to help here, e.g. "Describe the problem and steps to reproduce it", "What happened?", "What did you expect to happen?" and so on.

In addition a screenshot of the specific error in this case would help a lot for clarity and immediacy.

@erosman
Copy link
Contributor Author

erosman commented Jun 26, 2019

I assumed developers have access to https://addons.mozilla.org/en-US/developers/addon/kiwiback/file/3020642/validation
If not, I was not aware of it.

That was the reason I thought giving a link would resolve any queries.
That is how we have been doing it previously.

For example: #2114 #2086 #1943 #1708 #1767 #1547 ........ etc

I also left a ping for Andreas who does have access to all those. In fact the bug is aimed at the review admin to follow up on.

Screenshot_2019-06-26 Validation Results for kiwiback_penize_z_nakupu_zpet-0 2 6 8-fx xpi KiwiBack - peníze z nákupů zpět A

@muffinresearch
Copy link
Contributor

Yes, validation reports should be accessible.

However, currently there's not enough information so that the reader (who may not be an admin reviewer) understands why it's a problem and what the expected change would be.

  • Why is the CSP error false?
  • Are both messages incorrect or just one?
  • Should the message be improved or removed. If it can be improved, what should the message say?

If the issue has more context from the start this helps us to immediately understand the intent of the issue and how to go about fixing it, which benefits us all.

@erosman
Copy link
Contributor Author

erosman commented Jun 26, 2019

Why is the CSP error false?

As evident from the posted snippet, there is no allowance remote code execution

Are both messages incorrect or just one?

Only 1 message relates to CPS and this bug report.

Should the message be improved or removed. If it can be improved, what should the message say?

As per topic title, it is a false report.

Just looking at the corresponding code snippet that was posted in the first post, i.e. "script-src 'self' object-src 'self'" should be enough to ascertain the cause of the issue without even having a need to look at the links provided.

@muffinresearch
Copy link
Contributor

muffinresearch commented Jul 24, 2019

@erosman Isn't this an invalid CSP - this is missing the ; between directives?

On that basis fixing this should be about detecting the invalid CSP and showing a relevant warning for that. Attached is a test-case that reproduces the problem.

sarle-wasshog-cobst.zip

@muffinresearch muffinresearch changed the title Validator CSP false error Invalid CSP not detected Jul 24, 2019
@erosman
Copy link
Contributor Author

erosman commented Jul 24, 2019

Isn't this an invalid CSP - this is missing the ; between directives?

Indeed it is and the error should indicate CSP error/invalid instead of
"content_security_policy" allows remote code execution in manifest.json

@stale
Copy link

stale bot commented Jan 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

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

2 participants