View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 508 | Composr | core | public | 2012-05-27 00:30 | 2017-11-27 17:34 |
| Reporter | Chris Graham | Assigned To | Chris Graham | ||
| Priority | normal | Severity | feature | ||
| Status | resolved | Resolution | fixed | ||
| Summary | 508: Support Content Security Policy | ||||
| Description | A new spec is currently in development: https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html It is led by Firefox, but experimental implementations are in IE10 and Chrome. Safari and Opera do not have anything yet. | ||||
| Additional Information | This will be a lot of work to support, but has a big gain. It means we can greatly reduce the chance of XSS attack and sleep a bit easier, as XSS holes are really hard to consistently avoid (even though the ocProducts version of PHP has isolated Composr much better than other projects). http://www.html5rocks.com/en/tutorials/security/content-security-policy/ | ||||
| Tags | Risk: Breaks themes , Risk: Major rearchitecting , Type: Security, Type: Standards compliance | ||||
| Attach Tags | |||||
| Attached Files | |||||
| Time estimation (hours) | 40 | ||||
| Sponsorship open | |||||
| related to | 2005 | Not Assigned | Guest | Web standards refresh |
| related to | 1575 | Resolved | Chris Graham | Refresh our PHP base line |
| related to | 2095 | Closed | Chris Graham | Bootstrap theme |
| child of | 3362 | Resolved | Chris Graham | Themeing improvements in v11 (idea staging issue) |
|
|
Javascript would need to be altered to use something like this method: http://www.netmagazine.com/tutorials/get-your-javascript-order We could do a $REQUIRE_JAVASCRIPT on each behaviour also, so they can be in entirely separate JS files -- which Composr would automatically be able to merge based upon measured usage (as it does in v10). |
|
|
This functionality will cause web designers problems (as adding custom inline JavaScript code, e.g. for jQuery, is standard practice). So the feature should be off-by-default. Chris must update the setup wizard to include it in the higher security levels. |
|
|
CSP does have nonce support, so we can do inline script support. |
|
|
We now have set_high_security_csp() function which may be used in some very limited circumstances. CHRIS- salman has removed this, review that's okay [CHRIS] |
|
|
Quick note for us to update tut_security to reference CSP. |
|
|
We should have a config option that defines a list of pages that CSP does not run on. This is pages where the user has presumably put some third-party JS they don't know how to secure, or can't secure for some reason. Also a checkbox to turn CSP on overall. |
|
|
Additionally, we should code the web-standards-checker to by-default prohibit all use of CSP-unsafe tags and event handlers. This way any regressions will get picked up within our test set. [CHRIS] |
|
|
Remember to include in the new security level in the Setup Wizard [CHRIS] |
|
|
Once CSP is implemented I think we can disable some of the kids glove mode functionality. [CHRIS] |
|
|
I've made my first review of the CSP code. This JS refresh is a huge positive step forward for us :). This is where we stand now (supersedes all the above). Chris TODOs... 1) Cleanup the PHP side of CSP [WIP] Salman TODOs... 1) Lots of TODOs scattered around the v11 branch, marked 'Salman' 2) Make big changes to the symbol export code, including the code in global.js and SYMBOL_DATA_AS_JSON.php. I have a number of concerns around this: a) Performance of CPU computing the data b) Waste of bandwidth for each page request, especially when certain data is either unneeded or static c) General bloating of HTML d) General bloating of JS library which is already grown huge e) Leakage of privileged information The actual core technique is excellent though, it just needs adjustments. Reimplement using pure JS as no extra data needed: PREVIEW_URL Reimplement via direct copy through to the JS file using Tempcode symbol (allows static compilation, much more efficient): BASE_URL CUSTOM_BASE_URL CUSTOM_BASE_URL_NOHTTP FORUM_BASE_URL BRAND_NAME SESSION_COOKIE_NAME COOKIE_PATH COOKIE_DOMAIN Drop unneeded symbols: GROUP_ID VERSION (which is a security hole) PAGE_TITLE MEMBER_EMAIL AVATAR PHOTO MEMBER_PROFILE_URL COPYRIGHT DOMAIN BASE_URL_S & BASE_URL_NOHTTP_S (overboard really, adding the occasional slash is less cognitive and code overhead than having separate symbols for it) BROWSER_UA OS USER_AGENT IP_ADDRESS TIMEZONE CHARSET I do recognise some of these symbols may have been useful in the future. However, most are either accessible already using JS or Tempcode or metadata. I want us to make it more extensible for remaining cases, especially where third party programmers need to draw in unexpected data. Make a little API for reading out metadata headers easily. That way someone can add a new <meta> header and easily pull that through as a symbol into JavaScript. 3) Change config option mechanism a lot. I really don't like us hard-coding config options the way we are... a) it's not modular b) it's serious bloat to pass them in each request, they could be statically compiled into the JS Each config hook should say whether it goes into the JS statically and then a symbol should be able to embed all the JS into global.js for that (bypassing the need for an HTML header). Please be explicit above passing true or false for all existent hooks, as I like them to be complete. 4) Don't assume URL structure too much. I saw in the code url.includes being used. What if a GET parameter has a script name in it? You have a RUNNING_SCRIPT symbol, so use that where appropriate. 5) Logging code is overboard. We don't need 6 levels of logging. 3 would be more than enough. And let's use the existing Composr terminology, so let's have inform, warn, and fatal. I don't think we need the VERBOSE symbol either, we are already using DEV_MODE for similar purposes. I think probably the rule should be inform only shows on DEV_MODE, otherwise they all show. 6) There seem to be 2 cookie APIs now. Pick one. 7) fadeTransition could be replaced with CSS transitions now? I don't really know, but consider it. 8) Is the $cms.support.cssTransitions check needed? I think all our browsers support it. 9) There seem to be 2 browser detect APIs. Merge them. Also consider the need to keep consistent with browser_matches in global3.php. 10) Documentation will be important. global.js is huge (and will get bigger), which I'm fine with but it needs documentation to offset this issue. I think all the common functions need putting in a table in the Code Book. Also any existing documentation in the Code Book for JavaScript needs updating. 11) There are still a fair few cases of non-camelCase identifiers. E.g. window.notifications_time_barrier. Grep the JS files for "[a-z]+_[a-z]+". For now we are keeping any pre-existing CSS classes as non-camelCase (to avoid merge conflicts), and files and PHP stuff is still staying non-cancelCase. 12) Don't shoot me, but I feel we should make the installer CSP-compliant now. It doesn't have to actually have any of the CSP headers, but I feel it should use the same JavaScript framework for consistency. I don't want developers having to switch coding styles or having liberties taken from the installer code to leak back into the rest of Composr. 13) Some language strings not yet finished. Check these strings: LOAD_SAVED_WARNING ADD_ATTACHMENTS_MEDIA_LIBRARY SETUPWIZARD_SAFE_MODE ERROR_NO_CONTACTS_SELECTED 14) I want a clear policy for whether JS code can be global (or whether global variables are used). You've done a superb job converting my dated code but I feel either there is more to do to get things to your standards, or that the coding standards document needs to document when code can be global vs under $cms. 15) Default parameter handling change (as discussed over e-mail). 16) I'm seeing misspellings of JavaScript in the code, fix function names etc to capitalise it properly. 17) There is a lot of testing to do. For example, the preview buttons on content adding is currently broken. Go through and test every block and everything on the sitemap and every Comcode tag. Notes for Salman... 1) Make sure licence.txt is kept updated as third-party code is used. I've updated it for all the code I found. |
|
|
UPDATE August 2017 ============================= >1) Lots of TODOs scattered around the v11 branch, marked 'Salman' Done. >Reimplement using pure JS as no extra data needed: >PREVIEW_URL I tried going pure JS but failed because there is a usage of $ZONE['zone_default_page'] inside the get_page_name() call in ecv_PREVIEW_URL(). >Reimplement via direct copy through to the JS file using Tempcode symbol (allows static compilation, much more efficient): Done. >Drop unneeded symbols Done. >Make a little API for reading out metadata headers easily. That way someone can add a new <meta> header and easily pull that through as a symbol into JavaScript. Any pointers/examples? >3) Change config option mechanism a lot. Done. >4) Don't assume URL structure too much. I saw in the code url.includes being used. That's actually just existing code I converted to be more modern/readable from "str.indexOf(...) != -1" -> "str.includes()", but I'll keep that in mind. >5) Logging code is overboard. Fixed. >6) There seem to be 2 cookie APIs now. Pick one. Removed $cms.cookies.* in favor of $cms.readCookie/setCookie. >7) fadeTransition could be replaced with CSS transitions now? I don't really know, but consider it. Working on this ATM. >8) Is the $cms.support.cssTransitions check needed? I think all our browsers support it. Removed. >9) There seem to be 2 browser detect APIs. Merge them. Also consider the need to keep consistent with browser_matches in global3.php. Done. Dropped $cms.is[Vendor]() functions. >10) Documentation will be important. global.js is huge (and will get bigger), which I'm fine with but it needs documentation to offset this issue. >I think all the common functions need putting in a table in the Code Book. Also any existing documentation in the Code Book for JavaScript needs updating. TO BE DONE. >11) There are still a fair few cases of non-camelCase identifiers. E.g. window.notifications_time_barrier. >Grep the JS files for "[a-z]+_[a-z]+". Done. Got a bit nervous with some of the changes, refactoring anything JS is so error-prone. :( >For now we are keeping any pre-existing CSS classes as non-camelCase (to avoid merge conflicts) Hmmm :O Virtually no-one uses camelCase for CSS so shouldn't we rather fully adopt one of snake_case or kebab-case...? >12) Don't shoot me, but I feel we should make the installer CSP-compliant now. Lol.. to be done. >13) Some language strings not yet finished. Finished. >14) I want a clear policy for whether JS code can be global (or whether global variables are used). >You've done a superb job converting my dated code but I feel either there is more to do to get things to your standards, >or that the coding standards document needs to document when code can be global vs under $cms. TO BE DONE. >15) Default parameter handling change (as discussed over e-mail). Fixed. >Make sure licence.txt is kept updated as third-party code is used. I've updated it for all the code I found. TO BE DONE. |
|
|
">Reimplement using pure JS as no extra data needed: >PREVIEW_URL I tried going pure JS but failed because there is a usage of $ZONE['zone_default_page'] inside the get_page_name() call in ecv_PREVIEW_URL()." Deep within get_page_name? I think replacing that with the JS equiv of get_param_string('page', '') is fine. ">Make a little API for reading out metadata headers easily. That way someone can add a new <meta> header and easily pull that through as a symbol into JavaScript. Any pointers/examples?" <meta name="foo" content="bar" /> would facilitate $cms.pageMetadata('foo') returning "bar". Forget I said symbol. Just being able to easily transfer data through now we can't just put it inline. ">4) Don't assume URL structure too much. I saw in the code url.includes being used. That's actually just existing code I converted to be more modern/readable from "str.indexOf(...) != -1" -> "str.includes()", but I'll keep that in mind." Please correct my sins. "Done. Got a bit nervous with some of the changes, refactoring anything JS is so error-prone. :(" We will have to do a full end-to-end re-test anyway, even after your full functionality-testing sweep of blocks and modules. ">For now we are keeping any pre-existing CSS classes as non-camelCase (to avoid merge conflicts) Hmmm :O Virtually no-one uses camelCase for CSS so shouldn't we rather fully adopt one of snake_case or kebab-case...?" Ah. Could you provide me stats on what's normal. I see everything except what we currently do being common. We can then discuss. |
|
|
Done now: ========= * Reimplement $PREVIEW_URL using pure JS as no extra data needed. * Make a little API for reading out metadata headers easily. (Implemented $cms.pageMeta()) * fadeTransition could be replaced with CSS transitions now. (Replaced with $cms.dom.fadeIn/fadeOut/fadeTo which uses the new Web Animations API's Element.animate(), much cleaner than trying to control CSS transitions from JS) * We should make the installer CSP-compliant now. (Refactored all inline handlers and inline script tags into global.js. Which is itself embedded as the only inline script tag, with a nonce attribute for CSP-compliance. Should I make it external as well now?) Remaining: ========== 4) Don't assume URL structure too much. I saw in the code url.includes being used. What if a GET parameter has a script name in it? You have a RUNNING_SCRIPT symbol, so use that where appropriate. > That's actually just existing code I converted to be more modern/readable from "str.indexOf(...) != -1" -> "str.includes()", but I'll keep that in mind. Please correct my sins. 10) Documentation will be important. global.js is huge (and will get bigger), which I'm fine with but it needs documentation to offset this issue. I think all the common functions need putting in a table in the Code Book. Also any existing documentation in the Code Book for JavaScript needs updating. 14) I want a clear policy for whether JS code can be global (or whether global variables are used). You've done a superb job converting my dated code but I feel either there is more to do to get things to your standards, or that the coding standards document needs to document when code can be global vs under $cms. [WORKING ON ATM] 17) There is a lot of testing to do. For example, the preview buttons on content adding is currently broken. Go through and test every block and everything on the sitemap and every Comcode tag. Make sure licence.txt is kept updated as third-party code is used. I've updated it for all the code I found. For now we are keeping any pre-existing CSS classes as non-camelCase (to avoid merge conflicts) > Hmmm :O Virtually no-one uses camelCase for CSS so shouldn't we rather fully adopt one of snake_case or kebab-case...? Could you provide me stats on what's normal. I see everything except what we currently do being common. We can then discuss. |
|
|
>Could you provide me stats on what's normal. I see everything except what we currently do being common. We can then discuss. Let's see... * Twitter Boostrap's style guide recommends hyphens.[1] * ZURB Foundation's guide also says "Class names should have hyphens, and no underscores or capital letters.".[2] * jQuery's CSS style guide also recommends hyphens and lowercase letters.[3] * Wordpress coding standard also says: "Use lowercase and separate words with hyphens when naming selectors. Avoid camelcase and underscores."[4] * Drupal also uses hyphens generally, but it does use double underscores to declare component sub-objects.[5] * Google HTML/CSS Style guide also recommends using hyphens and discourages use of underscores.[6] So I wasn't being selective really, all the major style guides I found recommend hyphens. :P Here's a critique of camelCase in CSS: https://csswizardry.com/2010/12/css-camel-case-seriously-sucks/ 1: http://codeguide.co/#css-syntax 2: https://github.com/zurb/foundation-code-standards/blob/master/class-naming.md 3: https://contribute.jquery.org/style-guide/css/#class-names 4: https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#selectors 5: https://www.drupal.org/docs/develop/standards/css/css-architecture-for-drupal-8#sub-components 6: https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Name_Delimiters |
|
|
Ok I think we'll likely switch to hyphens then, definitely won't be camelCase. |
|
|
Hi, I've done complete testing of the sitemap and all the blocks now. License.txt was already updated by you mostly and I added a couple more credits. So this leaves us with "Documentation" now. |
|
|
>14) I want a clear policy for whether JS code can be global (or whether global variables are used). You've done a superb job converting my dated code but I feel either there is more to do to get things to your standards, or that the coding standards document needs to document when code can be global vs under $cms. My recommendation is to put all functions and such under JavaScript "namespaces" to avoid polluting the JS environment. "Namespaces" here are plain JS objects, named with a dollar sign ($) prefix to make them standout in code from local variables. For addons that could be $<addon-name>, e.g., the "chat" addon can export all its useful functions using a global object named "$chat". As you guessed, the core code doesn't itself adhere to this at the moment, given time constraints, I suggest we do this over time systematically as code is updated. |
|
|
I doubt it's necessary to define new global objects. Your JS framework is very clean, I should think things can just run within anonymous namespaces like the vast majority of your code already does. I have reviewed and only a tiny amount of code contains non-namespaced stuff: - button_realtime_rain - chat - core_form_interfaces - menu_editor - pulse - realtime_rain - theme_colours - translate - activity_feed (ignoring 3rd party libs, which we do not alter - they are an exception to our coding standards) I do think it is worth the time completing. I think it paints a clearer picture of what we want people to do if all our code is consistent. Plus as we're doing a big release and testing cycle, I want to get heavy lifting done in one go. Once done please make sure codebook_standards is clearly documenting the JS standards as you'd except them to be. |
|
|
I have done a quick review, there are still some TODOs, which I have now highlighted more clearly with "508" by the comments. - sources/webstandards2.php:70 - sources/tempcode_compiler.php:1199 - sources/hooks/systems/symbols/SYMBOL_DATA_AS_JSON.php:39 - themes/default/javascript/core_form_interfaces.js:1616 - themes/default/javascript/CMS.UI.js:22 - themes/default/javascript/CMS.BEHAVIORS.js:31 Empty text_ghosts.js can be deleted? Empty shake.js can be deleted? Is DOM.js meant to be CMS.DOM.js? I would prefer it to be CMS_DOM.js actually (and same for other JS filenames that you have double dots in). I know people use double dots in the JS world, but it's not a good idea. For example, Apache will consider all but the first segment to be a file extension, searching it's modules for file extensions. Let's imagine you had CMS.PHP.js, it would literally try and run it as a PHP file on some server configurations. Just on principle we should respect to only ever have one dot. Make sure text files end in a blank space, because it's a Unix standard for all files to end with a single blank line and some lint tools will complain about this. The blank_lines automated test will check this automatically (with a few defined exceptions). I don't like wrong indentation. E.g. posting.js, lines 304-306. Grep the code somehow to find any similar issues and correct. |
|
|
Uploaded screenshot of a bug I found. |
|
|
Okay, I'll get all the global functions neatly under their own namespaces. I am now referring to "namespaces" as "libraries" instead in the documentation I wrote, I think the latter is more in line with JS lingo. I'll also go over the TODOs. >Empty text_ghosts.js can be deleted? >Empty shake.js can be deleted? Yup, I've deleted both of them. They both had a too small amount of JS to justify separate files so their contents were moved to shoutr.js. > Is DOM.js meant to be CMS.DOM.js? No, the library was previously named $cms.dom but is now $dom, so going by the convention of naming files by the global object/library they produce, DOM.js is correct. Good point about the Apache issue though, I've renamed the files with dots in their name. >Make sure text files end in a blank space Thanks, I ran the blank_lines unit test and fixed file endings for a number of files. Hope I remember this. >I don't like wrong indentation. E.g. posting.js Sorry about that, I've fixed a couple more instances I found. Not sure how to grep for it though :(. >Uploaded screenshot of a bug I found. Fixed. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2016-04-23 00:45 | Guest | Note Added: 0003673 | |
| 2016-04-23 00:47 | Chris Graham | Note Added: 0003674 | |
| 2016-04-26 16:07 | Chris Graham | Relationship added | related to 1575 |
| 2016-06-08 00:14 | Chris Graham | Tag Renamed | Major rearchitecting => Risk: Major rearchitecting |
| 2016-07-22 16:46 | Chris Graham | Relationship added | related to 2095 |
| 2016-07-27 23:36 | Chris Graham | Note Added: 0004183 | |
| 2016-08-03 12:18 | Chris Graham | Note Added: 0004206 | |
| 2016-08-03 12:19 | Chris Graham | Note Added: 0004207 | |
| 2016-10-04 21:44 | Chris Graham | Note Added: 0004371 | |
| 2016-12-08 16:15 | Chris Graham | Tag Attached: Type: Standards compliance | |
| 2016-12-08 17:10 | Chris Graham | Note Added: 0004626 | |
| 2016-12-08 18:08 | Chris Graham | Tag Attached: Risk: Breaks themes | |
| 2017-04-28 14:22 | Chris Graham | Note Edited: 0004626 | |
| 2017-04-28 14:26 | Chris Graham | Note Edited: 0003674 | |
| 2017-05-01 18:26 | Chris Graham | Note Edited: 0002414 | |
| 2017-05-01 18:26 | Chris Graham | Additional Information Updated | |
| 2017-05-01 18:27 | Chris Graham | Note Edited: 0004207 | |
| 2017-05-01 18:27 | Chris Graham | Note Edited: 0003674 | |
| 2017-05-01 18:28 | Chris Graham | Note Edited: 0004371 | |
| 2017-05-01 18:28 | Chris Graham | Note Edited: 0004626 | |
| 2017-06-20 00:06 | Chris Graham | Note Edited: 0003673 | View Revisions |
| 2017-06-20 13:06 | Chris Graham | Note Added: 0005142 | |
| 2017-06-20 13:09 | Chris Graham | Note Edited: 0005142 | |
| 2017-06-20 13:11 | Chris Graham | Note Edited: 0005142 | |
| 2017-06-20 13:18 | Chris Graham | Note Edited: 0005142 | |
| 2017-06-20 16:31 | Chris Graham | Note Edited: 0005142 | |
| 2017-07-31 16:47 | Salman | Note Added: 0005178 | |
| 2017-07-31 16:56 | Chris Graham | Note Added: 0005179 | |
| 2017-09-04 10:45 | Salman | Note Added: 0005190 | |
| 2017-10-14 00:58 | Salman | Note Added: 0005203 | |
| 2017-10-14 14:22 | Chris Graham | Note Added: 0005204 | |
| 2017-11-12 06:58 | Salman | Note Added: 0005213 | |
| 2017-11-18 14:58 | Salman | Note Added: 0005217 | |
| 2017-11-20 00:16 | Chris Graham | Relationship added | child of 3362 |
| 2017-11-21 01:59 | Chris Graham | Note Added: 0005235 | |
| 2017-11-21 01:59 | Chris Graham | Note Edited: 0005235 | |
| 2017-11-21 02:04 | Chris Graham | Note Added: 0005236 | |
| 2017-11-22 01:37 | Chris Graham | File Added: Screen Shot 2017-11-22 at 01.36.39.png | |
| 2017-11-22 01:37 | Chris Graham | Note Added: 0005243 | |
| 2017-11-23 21:51 | Salman | Note Added: 0005246 | |
| 2017-11-27 17:34 | Chris Graham | Status | Not Assigned => Resolved |
| 2017-11-27 17:34 | Chris Graham | Resolution | open => fixed |
| 2017-11-27 17:34 | Chris Graham | Assigned To | => Chris Graham |
