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

Add support for HTTP security headers. #1016

Closed
wants to merge 12 commits into from

Conversation

ScottHelme
Copy link

I often find myself writing configurations for CSP, HSTS and HPKP. These changes add support for these configurations to Prism.

alias: 'keyword'
},
'safe': {
pattern: /('self'|'none'|'strict-dynamic'|('nonce-|'sha256-)[a-zA-Z0-9]{1,}('))/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't {1,} be written as +? Also why is the last single quote between parentheses?
Finally, couldn't the single quotes be put outside of the alternative to prevent unneeded repetition?

@Golmote
Copy link
Contributor

Golmote commented Aug 24, 2016

Thanks for contributing! Please take a look at the comments I made regarding the code itself.
Also, it looks to me that hpkp and hsts look alike a lot. Couldn't they be merged as a single component?

It would be great if you could add examples and tests. Please refer to my comment #1014 (comment) for details.


Prism.languages.csp = {
'directive': {
pattern: /\b(?:base-uri|block-all-mixed-content|(?:child|connect|default|font|frame|img|manifest|media|object|script|style|worker)-src|disown-opener|form-action|frame-ancestors|plugin-types|referrer|reflected-xss|report-to|report-uri |require-sri-for|sandbox|upgrade-insecure-requests)\b/i,
Copy link
Contributor

Choose a reason for hiding this comment

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

There’s extra space after “report-uri”.

Copy link
Author

Choose a reason for hiding this comment

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

This was intentional :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Even with the \b at the end?

@ScottHelme
Copy link
Author

Hey :-)

I fixed up the code based on the comments. I think that should now be good.

I will have a look at the requirements for the examples and tests.

alias: 'keyword'
},
'safe': {
pattern: /([0-9]{7,})/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Parentheses are not needed around the whole pattern. Same goes for the regexp below and the others in the hsts component. ;)

# Conflicts:
#	components/prism-csp.js
#	components/prism-csp.min.js
#	components/prism-hpkp.js
#	components/prism-hpkp.min.js
#	components/prism-hsts.min.js
@ScottHelme
Copy link
Author

I've learnt a lot more about regexp than I intended ;-)

How is it looking now?


Prism.languages.csp = {
'directive': {
pattern: /\b(?:(?:base-uri|form-action|frame-ancestors|plugin-types|referrer|reflected-xss|report-to|report-uri|require-sri-for|sandbox) |(?:block-all-mixed-content|disown-opener|upgrade-insecure-requests(?: |;))|(?:child|connect|default|font|frame|img|manifest|media|object|script|style|worker)-src )/i,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure there is still a problem with the parentheses in the second group.

@ScottHelme
Copy link
Author

Getting there... :-D

@Golmote
Copy link
Contributor

Golmote commented Aug 25, 2016

I won't merge until I'm back on a computer next week (currently reviewing with my phone). In the meantime, if you can write examples and tests, and think about my suggestion to merge hpkp and hsts, it would be really great!

@ScottHelme
Copy link
Author

We could merge HPKP and HSTS but they're different technologies and may diverge further in the future. Also, the numeric value ranges are different for the two. From my side it makes sense to keep these separate.

As for the examples and test I will take a look at what's required.

@papandreou
Copy link
Contributor

Very interested in seeing this landed. Anything I can do to help out?

@ScottHelme
Copy link
Author

I think it's just the examples and test that are outstanding.

@papandreou
Copy link
Contributor

papandreou commented Jan 29, 2018

@ScottHelme, fixed some bugs and added the missing pieces over here: #1275

If you'd rather work some more on this, feel free to cherry-pick the relevant changes back here :)

@Golmote
Copy link
Contributor

Golmote commented Jan 31, 2018

Closing after #1275 was merged.

@Golmote Golmote closed this Jan 31, 2018
@mattes
Copy link

mattes commented Jan 31, 2018

Big +1 on this, too. We just contributed the Content-Security-Policy highlighting to the ace editor. ajaxorg/ace#3511. Would be great to have it for prism as well.

@mattes
Copy link

mattes commented Jan 31, 2018

Oops, had an outdated cached tab open. Glad it's merged!

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.

5 participants