Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions app/controllers/levelcode/web_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class WebController < ApplicationController
# be written in: RFC 3986's path and query characters and well-formed percent escapes (#safe_next).
NEXT_MAX = 512
NEXT_CHARS = %r{\A(?:[A-Za-z0-9\-._~!$&'()*+,;=:@/?]|%\h\h)+\z}
# What this app serves under /ai/ itself (config/routes.rb, the "ai" scope): never a page to land on.
NEXT_SERVER_NAMES = %w[auth authorize_editor checkout billing signout csrf].freeze

# POST /ai/auth/email (JSON) — passwordless step 1: email a 6-digit code.
def email_request
Expand Down Expand Up @@ -266,6 +268,14 @@ def deliver_signed_in(user)
end
end

# Is this first segment after /ai/ one of this app's own endpoints, as written or decoded? A name is read as the
# router would: in lower case, up to the first character that is not a letter, digit or underscore (`csrf.json`
# and `csrf;x` are `csrf`). Read both ways, `%63srf` and `checkout%A9` are endpoints too. The SPA's isServerName.
def next_server_name?(segment)
name = ->(s) { s.scrub.downcase[/\A[a-z0-9_]*/] }
NEXT_SERVER_NAMES.include?(name.(segment)) || NEXT_SERVER_NAMES.include?(name.(URI.decode_uri_component(segment)))
end

# The login page again after a failed sign-in, still bringing the visitor to the page they asked for.
def login_again(error)
query = { error: error }
Expand Down Expand Up @@ -300,8 +310,9 @@ def web_editor_home
# SPA's safeNext (src/levelcode/next.ts in onetime): a page under /ai/, written only in the characters a URL
# path and query allow (so no blank, control character, backslash, fragment or non-ASCII), with a query kept
# (`/ai/pricing?plan=pro`). Refused: `//` (a host to a browser), a percent-encoded slash or backslash, a dot
# segment, plain or encoded (`/ai/%2e%2e/x` is /x to a browser), and the server's own endpoints under /ai/auth/
# and /ai/csrf, which are not pages. It is only ever a same-site path, so it takes the visitor nowhere this
# segment, plain or encoded (`/ai/%2e%2e/x` is /x to a browser), and the server's own endpoints
# (NEXT_SERVER_NAMES), which are not pages, in every spelling the router answers: /ai/csrf.json, /ai/csrf/,
# /ai/%63srf, /ai/CSRF, /ai/auth/…. It is only ever a same-site path, so it takes the visitor nowhere this
# site's own links could not.
def safe_next(raw)
return nil unless raw.is_a?(String) && raw.length.between?(5, NEXT_MAX) && raw.start_with?("/ai/")
Expand All @@ -310,7 +321,7 @@ def safe_next(raw)

path = raw.split("?", 2).first
return nil if path.split("/").any? { |segment| %w[. ..].include?(segment.gsub(/%2e/i, ".")) }
return nil if path.start_with?("/ai/auth/") || path == "/ai/csrf"
return nil if next_server_name?(path.split("/")[2].to_s)

uri = URI.parse(raw)
return nil if uri.scheme || uri.host || uri.userinfo
Expand Down
13 changes: 12 additions & 1 deletion spec/requests/levelcode/web_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,10 @@ def verify(extra = {})
'/ai/../users/edit', '/ai/%2e%2e/users/edit', '/ai/%2E./x', '/ai/.%2e/users/edit', '/ai/%2e./users/edit', '/ai/%2e/x',
'/ai/docs/%2e%2e/%2e%2e/users/edit', '/other/path', 'javascript:alert(1)', '/ai/x#y',
"/ai/x\nSet-Cookie:a=b", '/ai/' + ('a' * 600), '/ai/caf' + [ 0xe9 ].pack('U') + '/x', '/ai/<script>', '/ai/x%zz',
'/ai/auth/handoff?code=attacker', '/ai/auth/oauth/github', '/ai/csrf' ].each_with_index do |bad, i|
'/ai/auth/handoff?code=attacker', '/ai/auth/oauth/github', '/ai/csrf',
# an endpoint in every spelling the router answers: a format suffix, a trailing slash, an encoded letter, capitals
'/ai/csrf.json', '/ai/csrf/', '/ai/%63srf', '/ai/CSRF', '/ai/auth.json/handoff', '/ai/%61uth/handoff?code=attacker',
'/ai/authorize_editor', '/ai/checkout', '/ai/billing', '/ai/signout', '/ai/checkout%A9' ].each_with_index do |bad, i|
post '/ai/auth/verify', params: { email: "bad-next-#{i}@example.com", code: '123456', next: bad }
expect(json).to eq('redirect' => editor_home), bad.inspect
end
Expand All @@ -554,6 +557,14 @@ def verify(extra = {})
expect(json).to eq('redirect' => '/ai/docs/a.%2e')
end

it 'takes a page whose name only starts like an endpoint\'s, or has an endpoint\'s name further down' do
verify(next: '/ai/csrfx')
expect(json).to eq('redirect' => '/ai/csrfx')

post '/ai/auth/verify', params: { email: 'lands-2@example.com', code: '123456', next: '/ai/docs/auth' }
expect(json).to eq('redirect' => '/ai/docs/auth')
end

it 'leaves an editor that is signing in alone: it gets its code, and `next` is not looked at' do
with_web_editor('https://app.levelcode.test', url: 'https://app.levelcode.test/')
allow(Levelcode::OneTimeCode).to receive(:issue).and_return('one-time-code')
Expand Down
Loading