Skip to content
This repository has been archived by the owner on Feb 10, 2022. It is now read-only.

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge pull request #77 from craigspaeth/redirect-back
@craigspaeth (cc @dzucconi): Address open redirect issue
  • Loading branch information
craigspaeth committed Apr 27, 2016
2 parents 8af075c + 218f89d commit c8792e2
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 36 deletions.
15 changes: 13 additions & 2 deletions README.md
@@ -1,6 +1,6 @@
# Artsy Passport

Wires up the common auth handlers for Artsy's [Ezel](http://ezeljs.com)-based apps using [passport](http://passportjs.org/). Used internally at Artsy to DRY up authentication code.
Wires up the common auth handlers, and related security concerns, for Artsy's [Ezel](http://ezeljs.com)-based apps using [passport](http://passportjs.org/). Used internally at Artsy to DRY up authentication code.

## Setup

Expand Down Expand Up @@ -36,7 +36,7 @@ app.use artsyPassport
ARTSY_SECRET: # Artsy client secret
ARTSY_URL: # SSL Artsy url e.g. https://artsy.net
APP_URL: # Url pointing back to your app e.g. http://flare.artsy.net

# Defaults you probably don't need to touch
# -----------------------------------------

Expand Down Expand Up @@ -178,6 +178,17 @@ app.get '/', (req, res) ->

_These forms of user will be null if they're not logged in._

## Sanitize Redirect

If you implement a fancier auth flow that involves client-side redirecting back, you may find this helper useful in avoiding ["open redirect"](https://github.com/artsy/artsy-passport/issues/68) attacks.

````coffeescript
{ sanitizeRedirect } = require 'artsy-passport'

location.href = sanitizeRedirect "http://artsy.net%0D%0Aattacker.com/"
# Notices the url isn't pointing at artsy.net, so just redirects to /
````

## Contributing

Add a `local.artsy.net` entry into your /etc/hosts
Expand Down
34 changes: 1 addition & 33 deletions lib/app/redirectback.coffee
Expand Up @@ -2,39 +2,7 @@
# Redirects back based on query params, session, or w/e else.
# Code stolen from Force, thanks @dzucconi!
#

_ = require 'underscore'
{ parse } = require 'url'

redirectFallback = '/'
whitelistHosts = [
'internal'
'localhost'
'artsy.net'
]
whitelistProtocols = [
'http:'
'https:'
null
]

hasStatus = (args) ->
args.length is 2 and typeof args[0] is 'number'

bareHost = (hostname) ->
return 'internal' unless hostname?
_.last(hostname.split('.'), subdomainOffset = 2).join '.'

normalizeAddress = (address) ->
address.replace /^http(s?):\/+/, 'http://'

safeAddress = (address) ->
parsed = parse(normalizeAddress(address), false, true)
_.contains(whitelistProtocols, parsed.protocol) and
_.contains(whitelistHosts, bareHost(parsed.hostname))

sanitizeRedirect = (address) ->
if safeAddress(address) then address else redirectFallback
sanitizeRedirect = require './sanitize_redirect'

module.exports = (req, res) ->
url = req.body['redirect-to'] or
Expand Down
38 changes: 38 additions & 0 deletions lib/app/sanitize_redirect.coffee
@@ -0,0 +1,38 @@
#
# Cleans out urls so they are safe to redirect to inside of an artsy.net app.
# Code stolen from Force, thanks @dzucconi!
#
_ = require 'underscore'
{ parse } = require 'url'

redirectFallback = '/'
whitelistHosts = [
'internal'
'localhost'
'artsy.net'
]
whitelistProtocols = [
'http:'
'https:'
null
]

hasStatus = (args) ->
args.length is 2 and typeof args[0] is 'number'

bareHost = (hostname) ->
return 'internal' unless hostname?
_.last(hostname.split('.'), subdomainOffset = 2).join '.'

normalizeAddress = (address) ->
address
.replace /^http(s?):\/+/, 'http://'
.replace /\s/g, ''

safeAddress = (address) ->
parsed = parse(normalizeAddress(address), false, true)
_.contains(whitelistProtocols, parsed.protocol) and
_.contains(whitelistHosts, bareHost(parsed.hostname))

module.exports = (address) ->
if safeAddress(address) then address else redirectFallback
2 changes: 2 additions & 0 deletions lib/index.coffee
Expand Up @@ -9,10 +9,12 @@ opts = require './options'
setupApp = require './app'
setupPassport = require './passport'
artsyXapp = require 'artsy-xapp'
sanitizeRedirect = require './app/sanitize_redirect'

module.exports = (options) =>
_.extend opts, options
setupPassport()
setupApp()

module.exports.options = opts
module.exports.sanitizeRedirect = sanitizeRedirect
2 changes: 1 addition & 1 deletion package.json
@@ -1,6 +1,6 @@
{
"name": "artsy-passport",
"version": "1.0.6",
"version": "1.0.7",
"description": "Wires up the common auth handlers for Artsy's [Ezel](ezeljs.com)-based apps using [passport](http://passportjs.org/).",
"keywords": [
"artsy",
Expand Down
58 changes: 58 additions & 0 deletions test/app/sanitize_redirect.coffee
@@ -0,0 +1,58 @@
sinon = require 'sinon'
{ sanitizeRedirect } = require '../../'

describe 'sanitizeRedirect', ->
it 'lets artsy.net through', ->
sanitizeRedirect('http://artsy.net').should.equal 'http://artsy.net'

it 'lets artsy.net without a protocol through', ->
sanitizeRedirect('//artsy.net').should.equal '//artsy.net'

it 'lets artsy.net subdomains through', ->
sanitizeRedirect('http://2013.artsy.net').should.equal 'http://2013.artsy.net'

it 'lets artsy.net subdomains without a protocol through', ->
sanitizeRedirect('//2013.artsy.net').should.equal '//2013.artsy.net'

it 'lets relative paths through', ->
sanitizeRedirect('/path').should.equal '/path'

it 'lets deeply nested artsy.net subdomains through', ->
sanitizeRedirect('https://foo.2013.artsy.net').should.equal 'https://foo.2013.artsy.net'

it 'lets artsy.net links through', ->
sanitizeRedirect('https://artsy.net/about').should.equal 'https://artsy.net/about'

it 'lets localhost through', ->
sanitizeRedirect('http://localhost:3003/artwork/joe-piccillo-a-riposo').should.equal 'http://localhost:3003/artwork/joe-piccillo-a-riposo'

it 'lets internal paths through', ->
sanitizeRedirect('/log_in?redirect-to=/foo/bar').should.equal '/log_in?redirect-to=/foo/bar'

it 'blocks data urls', ->
sanitizeRedirect('data:text/html;base64,PHNjcmlwdD5hbGVydChsb2NhdGlvbi5oYXNoKTs8L3NjcmlwdD4=')[0].should.equal '/'

it 'blocks external links not whitelisted; redirects to root', ->
sanitizeRedirect('http://google.com').should.equal '/'

it 'blocks external links that lack protocol; redirects to root', ->
sanitizeRedirect('//google.com').should.equal '/'

it 'blocks other protocols; redirects to root', ->
sanitizeRedirect('ftp://google.com').should.equal '/'

it 'blocks other protocols; redirects to root', ->
sanitizeRedirect('javascript:alert(1);').should.equal '/'

it 'blocks malformed URLs (1)', ->
sanitizeRedirect('http:/google.com').should.equal '/'

it 'blocks malformed URLs (2)', ->
sanitizeRedirect('https:/google.com').should.equal '/'

it 'blocks malformed URLs (3)', ->
sanitizeRedirect('http:///google.com').should.equal '/'

it 'strips out whitespace', ->
sanitizeRedirect('''http://artsy.net
attacker.com/''').should.equal '/'

0 comments on commit c8792e2

Please sign in to comment.