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

Feature: Code injection to theme header/footer via blog settings #1993

Closed
JohnONolan opened this issue Jan 21, 2014 · 23 comments

Comments

Projects
None yet
10 participants
@JohnONolan
Copy link
Member

commented Jan 21, 2014

This has been talked about vaguely in multiple places a few times, so I wanted to open an issue to centralise the discussion and get it on the historical record.

Several platforms have a feature which allows users to inject code into their user-facing site via an account settings panel. I have included one example screenshot implementation of this (Squarespace) below.

Personally - I think this would be great to have in Ghost. It's an instant, easy way to drop in code for analytics, site verification, meta attributes, 3rd party service integrations, and many other "quick things" which would be nice to be able to use without modifying a theme or writing a full app.

I believe, from memory, the arguments against revolved primarily around security concerns. (@ErisDS?)


image

The "Click here for more help" link in the screenshot, goes here: http://help.squarespace.com/customer/portal/articles/438250

@sebgie

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2014

I think that this feature has less security implications than allowing Javascript in the editor. The inserted scripts won't be executed while a user is logged in so I don't see a problem with it. The tricky part is in my opinion to make sure that a user doesn't destroy his blog by adding invalid Javascript/HTML.

Maybe we should come up with another title? Code Injection has a negative perception. It would be great if we could remove jQuery from the ghost_foot helper when this is implemented.

@hswolff

This comment has been minimized.

Copy link
Member

commented Jan 22, 2014

Curious if this isn't more appropriate as an officially supported app? The line between what should go into core is blurry at times for me.

@CWSpear

This comment has been minimized.

Copy link

commented Jan 22, 2014

I like @hswolff's idea of "official" app.

@sebgie how could this script destroy the blog data? It should be just taken as a string and output in the head/footer. It shouldn't ever be executed except for on the client side, which is stuff you could do from the console anyway.

There are definitely better hackers out there than me, so maybe I'm just not understanding, but what, specifically, are some of the security issues for this feature?

@JohnONolan

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2014

@hswolff That line is always a blur, but it revolves around a few key principles which can be checked against:

  • How many people will this feature be used by?
  • How much overhead does the feature introduce for people who don't use it?
  • How much functionality does the feature introduce vs the effort to install an app?
  • Does the feature improve the core goal of creating a great publishing platform?

Loosely speaking, anything which is integral to a publishing platform and will benefit the majority of users significantly should be in core. Things which are optional enhancements and/or apply to a subset of users should be apps. Wherever possible we want to decide between "put it in core" or "leave to an app developer".

Officially supported apps are somewhat akin to sitting on the fence. They create the same workload for the core team, reduce the number of people who will use the feature, and reduce the potential landscape for 3rd party developers to work in because they don't want to compete with an "official" app.

@CWSpear I believe @sebgie is referring to a situation in which someone could enter code in these boxes which would brick their site when the code is executed on the front end. (it could be malicious JS, it could be as simple as a style tag with body { display:none; }

However, as he mentions, this is already possible by entering the same code into the Ghost editor. So the risk is not any greater.

There will always be a degree of risk that it is possible to destroy your site by doing something stupid with these fields. But, that risk is inherent in many, many places in both Ghost and other platforms. While we should prevent it where possible - I believe it should not inhibit progress being made, when the benefits outweigh the pitfalls.

@CWSpear

This comment has been minimized.

Copy link

commented Jan 22, 2014

I think the line should be drawn at "can they undo it." If they could drop the DB or something, that's something we'd need to protect. If they could just remove everything from these textareas and it's back to where they are, then I'd put that in an acceptable tolerance.

You're totally right. We can't protect everyone from themselves. So as long as it isn't irreparable...

@CWSpear

This comment has been minimized.

Copy link

commented Jan 22, 2014

As for putting it in the core, a common use case: I would think the # of people wanting to add analytics to their site would be very high. There are alternatives to accomplish this, but this is among the most flexible.

@sebgie

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2014

That is what I meant:

@CWSpear I believe @sebgie is referring to a situation in which someone could enter code in these boxes which would brick their site when the code is executed on the front end. (it could be malicious JS, it could be as simple as a style tag with body { display:none; }

Even a missing </div> could break the frontpage of a blog.

I think that non techsavvy users will copy/paste broken code they find on the internet, brick their blog and later blame Ghost for not working properly. The initial implementation can be done without code checks and if we see that users have troubles add validation later.

@ErisDS

This comment has been minimized.

Copy link
Member

commented Jan 22, 2014

This is a quick solution to a large number of problems, like comments, analytics, etc etc. We can certainly make it work, and improve it over time to make it as awesome and robust as possible

However, I think it's worth asking is it really the right solution?

This is a great solution for users who are technically savvy. Those who are not will, as @sebgie said, just copy code from the internet... it may or may not be correct. They won't know what they're doing or why. They also won't understand when things break like - they add their analytics code this way, and then forget and also install the analytics app, which also adds the code for them and everything gets tracked twice.

I know we're going to run into a lot of problems with people breaking their blogs when apps come along, but I worry if this is begging for trouble.

@JohnONolan

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2014

IMO ghost is slightly handicapped without this feature (particularly given that we have no admin theme file editing ui). The dangers are valid, but reversible, and (based on the abundance of similar/identical systems elsewhere) manageable - imo.

Again as sebgie said - the same dangers already exist (more so) in the editor - so we wouldn't be introducing something radically different. Its just allowing code in the head/foot rather than only the post body.

I'm fine with not putting it in an official release until we've managed to test it with a decent sample group - however

@totty90

This comment has been minimized.

Copy link

commented Jan 30, 2014

If you are afraid that someone could break the blog then validate html. Something like this http://esprima.org/demo/validate.html but for html. It could also try to validate js.

@ErisDS

This comment has been minimized.

Copy link
Member

commented Feb 3, 2014

I think we should have a stab at this and try it out with a sample group. To do this we need a decision on where in the admin is should live and what it looks like.

@JohnONolan JohnONolan self-assigned this Feb 3, 2014

@JohnONolan

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2014

Right, here's the UI for this + notes. Front end is open for anyone to take a stab at.

Notes:

"Omg this looks like SquareSpace" - yes it does. They did a great job and there's really no need to reinvent the wheel here. It's two textareas with labels.

All surrounding app frame areas should be ignored (header / side menu) these are just for context. Only the content area and active nav item are relevant to this issue.

  • The body copy at the top is 1.4rem (14px)
  • The bold labels are 1.4rem
  • The label descriptions are 1.3rem
  • The line numbers and code are 0.9rem (but may be better at 1rem)

The code textareas should be a standard CodeMirror implementation. We already use CodeMirror for the Ghost editor, so all the dependencies should already be there.

The code inputs should be developed as individual components which can be re-used on other screens if required - not just specific to this code injection screen. They should have a min-height of 250px and expand (vertically) as they are filled with text. They should not expand horizontally, and text-wrapping should be on. This means no horizontal or vertical scrollbars of any kind within the textareas.

The code injection menu item should be added as the last item in the settings nav to the left.

Every component on this screen must be fluid so that it will scale down appropriately to mobile devices.

The nav icon will need to be added to Ghost's custom icon font. Whoever picks this up should talk to me about that directly on IRC as the last thing to do when everything else is ready. It's a bit of pain, so I'll help you round it.

Bonus points for sexy(er) syntax highlighting if possible, without introducing a ton of overhead. (in which case it isn't worth it)

Beware of potential conflicts between the editor's codemirror CSS which could potentially (inadvertently) apply here. If this happens, the editor's styles will need to be scoped appropriately.

NB: As with all parts of the UI - try to always use existing CSS patterns first before writing new custom styles. Colours and margins can be off in mockups, but SASS variables are consistent.

code injection 2x

(Click on the image to see it bigger. Derp.)

@JohnONolan JohnONolan removed their assignment Feb 22, 2014

@ErisDS

This comment has been minimized.

Copy link
Member

commented Feb 22, 2014

Sorry.. I just had this idea - how about, at the bottom, putting a red 'reset' or 'abort' button that clears the fields, so if someone does something silly, like putting </textarea> and breaking the page, they have a get-out-of-jail-free-card?

This button would need to 1) remove the values from the database, and 2) re render the page accordingly

@javorszky

This comment has been minimized.

Copy link
Member

commented Feb 22, 2014

👍 for Plugins tab :P.

These would be present on ALL pages though, right? Home / post / page?

@JohnONolan JohnONolan added the styles label Feb 22, 2014

@JohnONolan

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2014

@ErisDS I'm 90% sure the CodeMirror should handle that automatically. The inputs, after all, are technically divs - not textareas at all. I think we should implement it first and then try to break it before adding UI that solves breakages.

@javorszky They would be injected into {{ghost_head}} and {{ghost_foot}} per the screenshot ;) so, yes.

@shashankmehta

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2014

I'm working on this. Should have a PR ready soon.

Current state:
screenshot from 2014-03-02 16 42 17

@shashankmehta

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2014

I have added the frontend for this.
Could you please let me know if

  • Styles for codemirror component are in the right file
  • The CodeMirror component has been defined in the right file
  • CSS is consistent. I have tried to use existing components wherever I could. But I might have missed some since this is just my 3rd PR and first one with frontend
  • Any other major/minor errors that I might have made, I'll fix and update the PR.
@JohnONolan

This comment has been minimized.

Copy link
Member Author

commented May 25, 2014

Last PR fell out of date with the various changes since then. I've implemented the base UI for this now in https://github.com/TryGhost/Ghost-UI/commit/3f51d36c67c629fb624771e2f8f5ca0378ba5423 - remaining work is to take the placeholder markup from that commit and wire it up to a new view + add the codemirror components. Similar to #2324

@ErisDS ErisDS referenced this issue Jul 1, 2014

Closed

[Feature] Multi-User #2982

26 of 26 tasks complete

@ErisDS ErisDS modified the milestones: 0.5.x Feature Release, 0.5 Multi-user Jul 8, 2014

@ErisDS ErisDS added the help wanted label Jul 31, 2014

@JohnONolan

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2014

I thought of something else this feature needs: We should detect whether {{ghost_head}} and {{ghost_foot}} exist in the current theme. If they don't, the code injection screen should display an error message explaining that it will not work, with a link to a documentation article about how to add the appropriate hooks to your theme.

@javorszky

This comment has been minimized.

Copy link
Member

commented Aug 12, 2014

Semantics, but usually the people using the blog != the people who made the theme.

If the message is for the theme devs while they're developing the theme, 👍, otherwise the message should be "hey, send this link to whoever made your theme".

@ErisDS ErisDS changed the title Feature: Allow code injection to header/footer of theme via blog settings Feature: Code injection to theme header/footer via blog settings Aug 19, 2014

@ErisDS ErisDS added feature and removed help wanted labels Sep 2, 2014

ErisDS added a commit to ErisDS/Ghost that referenced this issue Dec 4, 2014

Code Injection - adds perms, shortcuts, icon, flag
refs TryGhost#1993

- adds ctrl/cmd+s for save
- adds config flag
- adds icon on settings page, puts items in the right order
- sorts out permissions for all settings pages with consistent configuration

@ErisDS ErisDS referenced this issue Feb 19, 2015

Closed

[WIP] Code Injection messaging improvements #4932

0 of 3 tasks complete
@ckib16

This comment has been minimized.

Copy link

commented Mar 5, 2015

Hey all - just wanted to say thanks for developing this feature. I was a SquareSpace user for a long time and can confirm they definitely got their code injection implementation right. SS was the first platform I had ever used something like this with. Their clear UI and instructions made it very easy to use - just copy & paste.

I think you are on the right path to keeping it simple for the user!

@ErisDS

This comment has been minimized.

Copy link
Member

commented Mar 5, 2015

@ckib16 Glad you like it :)

There are a few last pieces of the code injection puzzle to land before we can move it out of labs, the big one is here: #4995 + I have a PR with some text amends to sort out.

ErisDS added a commit to ErisDS/Ghost that referenced this issue Mar 7, 2015

@amio

This comment has been minimized.

Copy link

commented Mar 11, 2015

For the icon part, how about build a grunt task for generating icon and example usage html?

I had made one like this:
https://github.com/baixing/baicons
For adding new icon, developer can simply place the svg file into source directory,
then excute gulp build to generate the full font set including ttf, woff, eot, and svg, and with a demo page.

ErisDS added a commit to ErisDS/Ghost that referenced this issue Apr 9, 2015

No more feature flag for Code Injection
closes TryGhost#1993

- remove the feature/config flag that means code injection has to be enabled

@sebgie sebgie closed this in #5125 Apr 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.