Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Update lib/reddit_enhancement_suite.user.js #210

Merged
merged 1 commit into from Mar 9, 2013

Conversation

Projects
None yet
2 participants
Contributor

hypah commented Nov 15, 2012

  • Added image expando support for quickmeme's mobile domain
  • Also added an "add captions" feature for quickmeme links

A nuisance for people with RES who are interested in participating in communities such as the AdviceAnimals subreddit is that there is no straightforward way to add captions to anything they see, because links modified by RES point straight to an image rather than a page with a captioning ability. Consequently, RES users have a somewhat crippled experience in these communities.

This modification removes that problem by using a middleman domain with nuanced HTTP header detection which determines whether the URL was loaded in an expando or in its own window/tab.

When determined to be expando/inline, it directs straight to the quickmeme image (which is the current behavior of RES). When determined to be its own window/tab, it instead serves an HTML page containing the image and a small and unobtrusive "add captions" button which dynamically embeds an HTML5-friendly, no-Flash-required image captioning script when clicked.

The server errs on the side of caution by only serving HTML if it is 100% certain the image link was loaded in its own window/tab. Otherwise it falls back on normal image loading.

It has been tested in Opera, Firefox, Chrome, Safari, and IE 7/8/9.

The modification is barely noticeable to people who don't care about adding their own captions to anything, while giving the captioning ability back to people who do care.

It is hosted on a dedicated softlayer server with a 2 Gbps uplink and 10 TB/month bandwidth allotment. I think it would be cool to make this eventually work with other meme sites too, but I started with quickmeme because it's the most popular. Let me know what you guys think!

@hypah hypah Update lib/reddit_enhancement_suite.user.js
Added image expando support for quickmeme's mobile domain and an "add captions" feature for quickmeme links
d9f7eff
Owner

honestbleeps commented Nov 15, 2012

It has been tested in Opera, Firefox, Chrome, Safari, and IE 7/8/9.

Cool, how did you test this in IE7-9?

I'm definitely going to check this out and see how it works, but I'm a little nervous about relying on a 3rd party service that is untested, etc... Clearly it seems your intentions are good - I just have to be careful with over 1 million users - especially their security, so I'll need a little more time than normal to look at this code.

Contributor

hypah commented Nov 15, 2012

I should clarify that what I tested in those browsers was the environment detection method (new window vs. embedded DOM object); I only tested the RES tweak in Chrome.

I'm not sure what a good solution is for the security issue, because I know bandwidth billing was an annoyance when you released a RES update last winter; however, if that ever ceases to be a problem, I would be more than willing to give you the PHP script and hand it over to you.

I've thought about your future plans before and always wanted to offer my server to you for things like settings/tag saving, but I was never really confident enough to start a conversation about it, because if I were in your shoes and someone else offered to host portions of my own project, I would say no.

I would recommend at least pulling the (www.|m.) change for now - it makes the image expando work with this domain: http://www.reddit.com/domain/m.quickmeme.com/new

Contributor

hypah commented Nov 16, 2012

I think I have a better idea. It would largely eliminate security issues as well as the reliance on a middleman domain.

How about we instead have the "add captions" link appear on the same line as "comments", "share", "save", etc., but only for whitelisted, compatible domains like quickmeme, memegenerator, etc.? Clicking this would unfold an editor within reddit itself.

If all the required javascript were to be set in stone on github as another immutable part of RES, this would basically eliminate the security risks as well as potential latency and bandwidth issues which would have existed with the current redirect approach. It also feels like more of an actual RES feature to do it this way.

All the client-side scripts would be embedded in RES. The only security risk I can think of would be the "Generate" button. When it is clicked, the browser sends the typed captions to my server script. My server then generates the image, saves it to quickmeme, and returns the final saved meme URL to the client. The user is then redirected to that URL. If my server were ever compromised, a malicious party could replace the redirect URL with something else.

The simplest solution would be for the client to whitelist domains it allows itself to redirect to. The whitelist could be an array of strings in the RES source code like allowed_domains = ["quickmeme.com", "memegenerator.net", "etc"]. If the requested redirect URL's domain were not whitelisted, then the browser could silently ignore it instead of calling window.location = "URL". This would basically make it impossible for anyone who somehow gains access to my server to do anything naughty beyond breaking the Generate button.

I could be overlooking something, but I feel like that might get rid of all of the possible security issues.

What do you think?

@honestbleeps honestbleeps added a commit that referenced this pull request Mar 9, 2013

@honestbleeps honestbleeps Merge pull request #210 from hypah/patch-2
Update lib/reddit_enhancement_suite.user.js
2604b8e

@honestbleeps honestbleeps merged commit 2604b8e into honestbleeps:master Mar 9, 2013

@hypah hypah deleted the hypah:patch-2 branch May 30, 2013

@patricksnape patricksnape pushed a commit to patricksnape/Reddit-Enhancement-Suite that referenced this pull request Jun 23, 2013

@honestbleeps honestbleeps Merge pull request #210 from hypah/patch-2
Update lib/reddit_enhancement_suite.user.js
134610f

@honestbleeps honestbleeps added a commit that referenced this pull request Sep 2, 2013

@honestbleeps honestbleeps Merge pull request #210 from hypah/patch-2
Update lib/reddit_enhancement_suite.user.js
e648ed3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment