discourse / onebox Goto Github PK

We want to be able to cache the return responses for certain oneboxes. This cache should pretty much be any key/value system that responds to fetch/write. Default should be an in-memory hash.

https://github.com/discourse/discourse/blob/master/lib/oneboxer/whitelist.rb

We want a stylesheet for oneboxes that can be shared to each one, and then individually enhanced/overwritten.

We want to have an easier time handling the matching of urls to oneboxes. We need to determine how to equate a onebox to a regular expression through a nice class level DSL.

In addition we want to use verbexpressions to write those regular expressions in the match setup.

matcher /^https?://play.google.com/.+$/

matcher /^https?://www.collegehumor.com/video/.*$/

Though I think it was rather sloppy implemented in the previous version, I think it's a good idea to keep the code which matches the url close to the onebox conversion code. This way, you define in your onebox what urls you support, which is sort of a precondition for your code.

So I was wondering why the choice was made to move this away?

Since we're introducing a new architecture for each OneBox it'll be nice to have a quick introduction on all the parts required for a new onebox.

1. Creating the Engine object
2. Creating the template (inline or file)
3. (optional) Presenters for data
4. (optional) Sanitizing/checking setup
5. (optional) Setting up OpenGraph

While our current architecture, grabing data from the response of a resource and injecting that data into a handlebars template, avoids many types of attack there's a few known problem areas we don't cover:

1. @markijbema pointed out that we encourage XSS by grabbing image src values
2. There may be ways to inject running JS in any parts of the resource we grab
3. The URL we are given might contain an XSS attack

matcher /^https?://itunes.apple.com/.+$/

The way URLs are matched right now is not very secure.

#100 (diff)

We need a simple maintainable way to turn urls given to Onebox into their coorsponding onebox objects. This object should know what to do if the request fails (404, 500, etc), needs to be authenticated (401), or redirected (302). It should also know what a valid URL looks like and handle that correctly.

matcher /^https?://.*.mp3$/

We want to reduce the duplicated parts of each template by having a shared layout view.

We should create a template that allows video embed code in oneboxes, as well as a module that people can use to start using that template. We'll also need a default template.

Seems similar to OEmbed in Discourse.

https://github.com/dysania/onebox/pull/97/files#r6200518

The (as of now non-existant) gem that gives VebExp web helpers would really be nice for our matcher system.

Spec names should match the files they're testing, e.g. amazon_onebox_spec.rb for amazon_onebox.rb

matcher /^https?://blip.tv/.+$/

http://about.travis-ci.org/docs/user/notifications/

matcher /^https?://(?:www.)?clikthrough.com/theater/video/\d+$/

def matcher(regexp=nil,&blk)
self.regexp = regexp || blk
end

Just noticed this file while browsing around:

https://github.com/dysania/onebox/blob/master/lib/onebox/preview/amazon.rb

In ruby it's convention to use 2 spaces for indenting, but it looks like in this file (maybe others?) also tabs were used. I suspect this is due to editor configuration of one of the committers. Most editors can easily be configured to use spaces, for instance, for sublimetext config you could use this one:

https://gist.github.com/markijbema/5668813

(sorry if this comes of as pedantic, but in my experience having non-standard spacing leads to lots of merge conflicts later on, so better to fix it quickly)

We want to handle open graph if possible as it avoids a lot of HTML parsing problems. Most websites support opengraph so we'll need a module that shares open graph behavior with Engine objects.

We should probably use intridea/opengraph.