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

Fix no html only headers #2618

Merged
merged 5 commits into from
Jun 21, 2019
Merged

Conversation

sarvaje
Copy link
Contributor

@sarvaje sarvaje commented Jun 18, 2019

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Fix #2342
Fix #2349

@Malvoz
Copy link
Member

Malvoz commented Jun 19, 2019

This is great! :)

Before closing #2342, I don't think we should overlook #2342 (comment):

X-Frame-Options might be relevant to any response really.

And aside from the potentially sizable list of content-types with the ability to fetch files, or execute scripts such as the common PDF (application/pdf) — CSP is also applicable to SVG (image/svg+xml).

Copy link
Member

@Malvoz Malvoz left a comment

Choose a reason for hiding this comment

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

@sarvaje Unless I'm missing something, I think you missed the occurrences of X-WebKit-CSP?

@sarvaje
Copy link
Contributor Author

sarvaje commented Jun 19, 2019

@sarvaje Unless I'm missing something, I think you missed the occurrences of X-WebKit-CSP?

@Malvoz you are not missing something, I miss that occurrence.

@sarvaje
Copy link
Contributor Author

sarvaje commented Jun 19, 2019

And aside from the potentially sizable list of content-types with the ability to fetch files, or execute scripts such as the common PDF (application/pdf) — CSP is also applicable to SVG (image/svg+xml).

Maybe we should think in having a new hint only for CSP.
@antross @molant @Malvoz what do you thing?

@molant
Copy link
Member

molant commented Jun 19, 2019

There's #25
It's been in the backlog for a long time.

@sarvaje
Copy link
Contributor Author

sarvaje commented Jun 19, 2019

There's #25
It's been in the backlog for a long time.

Ups, I didn't remember that hehehe

@sarvaje
Copy link
Contributor Author

sarvaje commented Jun 19, 2019

Changes done!

Copy link
Member

@antross antross left a comment

Choose a reason for hiding this comment

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

Looking good.

Can you add explicit test cases for application/xhtml+xml and text/xml too? Now that I'm looking at the code I'm not convinced it's going to handle the text/xml case correctly in particular.

I saw a few places X-Frame-Options still needs to be removed from the documentation (couldn't reference them since they were too far away from the diffs).

Also feel free to clean up any other redundant type specifications you see while we're in here.

packages/hint-no-html-only-headers/README.md Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/src/hint.ts Outdated Show resolved Hide resolved
@sarvaje
Copy link
Contributor Author

sarvaje commented Jun 20, 2019

it would be nice to have a second opinion about the changes in the servers configuration.

packages/hint-no-html-only-headers/README.md Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/README.md Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/README.md Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/README.md Outdated Show resolved Hide resolved
packages/hint-no-html-only-headers/README.md Outdated Show resolved Hide resolved
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.

[Bug] Allow CSP headers for worker scripts [Docs Bug] Header advice wrong
4 participants