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

Support Content-Security-Policy syntax highlighting #3511

Merged
merged 27 commits into from
Jan 31, 2018
Merged

Support Content-Security-Policy syntax highlighting #3511

merged 27 commits into from
Jan 31, 2018

Conversation

mattes
Copy link
Contributor

@mattes mattes commented Jan 12, 2018

Content-Security-Policy is a great W3C standard to prevent XSS attacks. We would love to contribute the syntax highlighting for it.

https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy

@mattes
Copy link
Contributor Author

mattes commented Jan 12, 2018

CLOUD9 CORPORATE CONTRIBUTOR LICENSE AGREEMENT should be signed.

Now it is possible to highlight CSP releated strings
@nightwing
Copy link
Member

Hi, how are .csp files used, i could find .csp extension used only for Caché Server Page markup

},
{
token: "constant.language.character",
regex: /referrer/,
Copy link
Member

Choose a reason for hiding this comment

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

it would be better to group these into one regex or use a keywordMapper

Choose a reason for hiding this comment

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

@nightwing I'll explain my reasoning and you tell me if it is broken :) from the documentation I understood that the matching will stop once something is found, so there is no need traversing each rule. Thus less work for the editor. If I mush everything in one regex, won't that be more taxing for the IDE? If that is not the case I'll change it.

Copy link
Member

Choose a reason for hiding this comment

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

Using one regexp is faster than using several, so highlighter combines all the rules in the state together

this.regExps[key] = new RegExp("(" + ruleRegExps.join(")|(") + ")|($)", flag);
.
Here it still needs to see which rule did match, and that adds some overhead.

Choose a reason for hiding this comment

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

OK, I understand. Thank you for the explanation.

{
token: "constant.language.character",
regex: /policy-uri/,
comment: "https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/policy-uri"
Copy link
Member

Choose a reason for hiding this comment

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

would be better to remove comment properties and just keep one url in a comment, instead of the comment on line 31

Choose a reason for hiding this comment

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

This is a bit unclear, you say to remove the comment property. That I get, I don't get what should I do with the URL? Because on line 31 o this file I have /* This file was autogenerated from (uuid: ) */. Should I just remove the comment section all-together?

Copy link
Member

Choose a reason for hiding this comment

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

You can remove the /* This file was autogenerated from (uuid: ) */ comment and the one after it. And instead of them add // see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/ comment.

Choose a reason for hiding this comment

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

I see. I thought that since some parts of the file are auto generated I should not touch those parts.

@mattes
Copy link
Contributor Author

mattes commented Jan 15, 2018

how are .csp files used
I don't think writing Content-Security-Policy to files is common. You still want to be able to edit with syntax highlighting though.

I will have a look at the other suggestions.

@nightwing
Copy link
Member

How do you use the mode if the csp rules are not in a separate file?

@mattes
Copy link
Contributor Author

mattes commented Jan 15, 2018

Sorry, with files in my previous comment I meant writing files to disk. In the context of files here a file could look like this for example:

script-src 'strict-dynamic' 'nonce-rAnd0m123' 'unsafe-inline' http: https:; object-src 'none'; base-uri 'none'; report-uri https://csp.example.com;

@nightwing
Copy link
Member

How are you planning to use this mode? Do you use ace on a webpage where users can enter csp rules, and you set csp mode programmatically, or do you want this to use in an ide like cloud9?
I am asking because if i merge this pr, users will get this mode for files with .csp extension, which are not related to this mode.

@mattes
Copy link
Contributor Author

mattes commented Jan 18, 2018

Do you use ace on a webpage where users can enter csp rules, and you set csp mode programmatically.

This is what we are trying to achieve. Sounds like we are on the wrong path here?

@davidgatti
Copy link

@nightwing regarding the Mode situation. I think what we want to do is to highlight CSP keywords so developers can see if they did a typo or not when writing those rules in... well... any language. Is there a way to achieve this that is different from Mode?

Because when I visit this link: https://ace.c9.io/#nav=higlighter I see:

Creating a Syntax Highlighter for Ace, and bellow that Defining a Mode. This is the place where I learned about Mode.

Changed the regexp rules
@mattes
Copy link
Contributor Author

mattes commented Jan 24, 2018

We made some changes. Happy to rebase. Does this look good @nightwing ?

@nightwing
Copy link
Member

I think what we want to do is to highlight CSP keywords so developers can see if they did a typo or not when writing those rules in... well... any language.

@davidgatti to do that we need some way of detecting that a string in another language is something that can be interpreted as csp. Otherwise user will get csp keywords highlighted in strings that are not related to csp at all.

@nightwing
Copy link
Member

@mattes looks mostly good, i think only changes to ace/ext/modelist.js and package.json need to be reverted, but i can do that when merging.
I have not merged this yet, because first i want to make sure that this achieves what we want, because if the goal is to have highlighting in any mode, we may need to rethink the approach a bit.

Fixed semicolon and mixed-spaces-and-tabs
@davidgatti
Copy link

@nightwing OK make sense. One thing that I can add is that the majority of those keywords are unique, the ones that might cause a problem is maybe 'self' for example.

Another thing worth knowing is that you set CSP on the back-end of a site, meaning the back end can be written in any language. This makes it tricky to decide which language to filter out.

The back-end must set the right headers, and to set the headers you need to do it in the back-end.

I hope this will help while contemplating this situation :)

@davidgatti
Copy link

@nightwing I removed the extension, so you don't have to :)

@mattes
Copy link
Contributor Author

mattes commented Jan 29, 2018

Thanks @nightwing for your help on this. The goal is to have Ace editor embedded on a website and have it highlight Content-Security-Policies.

To detect a Content-Security-Policy we could look for default-src, but it would be guessing and I don't think this would be reliable.

@nightwing nightwing merged commit e22e0f8 into ajaxorg:master Jan 31, 2018
@nightwing
Copy link
Member

i am afraid there is no way to add this to all modes, i've made some small changes to allow autocompleter to work for csp mode and merged the pr.

@davidgatti
Copy link

@nightwing Ok, so I see that you merged the PR, and so if I visit https://ace.c9.io/build/kitchen-sink.html what should I expect?

@nightwing
Copy link
Member

the mode is hidden because it doesn't correspond to a file type, you can test it by running editor.setOption("mode", "ace/mode/csp") in the console.

@mattes
Copy link
Contributor Author

mattes commented Feb 1, 2018

Thanks everyone!

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.

3 participants