Skip to content

Commit

Permalink
improve security of magic links further and improve the UX for the wh…
Browse files Browse the repository at this point in the history
…ole thing
  • Loading branch information
kentcdodds committed Oct 6, 2021
1 parent a573934 commit 63b8fe5
Show file tree
Hide file tree
Showing 13 changed files with 85 additions and 67 deletions.
2 changes: 1 addition & 1 deletion app/__test_routes__/login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export const loader: LoaderFunction = async ({request}) => {
return redirect(
getMagicLink({
emailAddress: email,
validateEmail: false,
validateSessionMagicLink: false,
domainUrl: getDomainUrl(request),
}),
)
Expand Down
43 changes: 23 additions & 20 deletions app/components/form-elements.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ type InputProps =
| ({type: 'textarea'} & JSX.IntrinsicElements['textarea'])
| JSX.IntrinsicElements['input']

function Input(props: InputProps) {
const Input = React.forwardRef<HTMLInputElement, InputProps>(function Input(
props,
ref,
) {
const className = clsx(
'placeholder-gray-500 dark:disabled:text-blueGray-500 focus-ring px-11 py-8 w-full text-black disabled:text-gray-400 dark:text-white text-lg font-medium bg-gray-100 dark:bg-gray-800 rounded-lg',
props.className,
Expand All @@ -37,9 +40,10 @@ function Input(props: InputProps) {
<input
{...(props as JSX.IntrinsicElements['input'])}
className={className}
ref={ref}
/>
)
}
})

interface InputErrorProps {
id: string
Expand All @@ -58,23 +62,20 @@ function InputError({children, id}: InputErrorProps) {
)
}

function Field({
defaultValue,
error,
name,
label,
className,
description,
id,
...props
}: {
defaultValue?: string | null
name: string
label: string
className?: string
error?: string | null
description?: React.ReactNode
} & InputProps) {
const Field = React.forwardRef<
HTMLInputElement,
{
defaultValue?: string | null
name: string
label: string
className?: string
error?: string | null
description?: React.ReactNode
} & InputProps
>(function Field(
{defaultValue, error, name, label, className, description, id, ...props},
ref,
) {
const prefix = useId()
const inputId = id ?? `${prefix}-${name}`
const errorId = `${inputId}-error`
Expand All @@ -94,6 +95,8 @@ function Field({
</div>

<Input
// @ts-expect-error no idea 🤷‍♂️
ref={ref}
{...(props as InputProps)}
name={name}
id={inputId}
Expand All @@ -106,7 +109,7 @@ function Field({
/>
</div>
)
}
})

function ButtonGroup({
children,
Expand Down
12 changes: 6 additions & 6 deletions app/components/navbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,13 @@ function ProfileButton({
imageAlt,
team,
user,
hasActiveMagicLink,
magicLinkVerified,
}: {
imageUrl: string
imageAlt: string
team: OptionalTeam
user: User | null
hasActiveMagicLink: boolean
magicLinkVerified: boolean | undefined
}) {
const controls = useAnimation()
const [ref, state] = useElementState()
Expand Down Expand Up @@ -283,9 +283,9 @@ function ProfileButton({
return (
<Link
prefetch="intent"
to={user ? '/me' : hasActiveMagicLink ? '/signup' : '/login'}
to={user ? '/me' : magicLinkVerified ? '/signup' : '/login'}
aria-label={
user ? 'My Account' : hasActiveMagicLink ? 'Finish signing up' : 'Login'
user ? 'My Account' : magicLinkVerified ? 'Finish signing up' : 'Login'
}
className={clsx(
'inline-flex items-center justify-center ml-4 w-14 h-14 rounded-full focus:outline-none',
Expand All @@ -309,7 +309,7 @@ function Navbar() {
const {requestInfo, userInfo, user} = useRootData()
const avatar = userInfo
? userInfo.avatar
: requestInfo.session.email && requestInfo.session.hasActiveMagicLink
: requestInfo.session.email
? {
src: getAvatar(requestInfo.session.email, {
fallback: kodyProfiles[team].src,
Expand Down Expand Up @@ -348,7 +348,7 @@ function Navbar() {
</div>

<ProfileButton
hasActiveMagicLink={requestInfo.session.hasActiveMagicLink}
magicLinkVerified={requestInfo.session.magicLinkVerified}
imageUrl={avatar.src}
imageAlt={avatar.alt}
team={team}
Expand Down
15 changes: 2 additions & 13 deletions app/root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import {getUserInfo} from './utils/user-info.server'
import {getClientSession} from './utils/client.server'
import type {Timings} from './utils/metrics.server'
import {time, getServerTimeHeader} from './utils/metrics.server'
import {validateMagicLink} from './utils/prisma.server'
import {useScrollRestoration} from './utils/scroll'
import {Navbar} from './components/navbar'
import {Spacer} from './components/spacer'
Expand Down Expand Up @@ -126,7 +125,7 @@ export type LoaderData = {
path: string
session: {
email: string | undefined
hasActiveMagicLink: boolean
magicLinkVerified: boolean | undefined
theme: Theme | null
}
}
Expand All @@ -152,16 +151,6 @@ export const loader: LoaderFunction = async ({request}) => {
fn: () => session.getUser(),
})

const magicLink = loginInfoSession.getMagicLink()
let hasActiveMagicLink = false
if (typeof magicLink === 'string') {
try {
await validateMagicLink(magicLink, loginInfoSession.getEmail())
hasActiveMagicLink = true
} catch {
loginInfoSession.unsetMagicLink()
}
}
const randomFooterImageKeys = Object.keys(illustrationImages)
const randomFooterImageKey = randomFooterImageKeys[
Math.floor(Math.random() * randomFooterImageKeys.length)
Expand All @@ -183,7 +172,7 @@ export const loader: LoaderFunction = async ({request}) => {
path: new URL(request.url).pathname,
session: {
email: loginInfoSession.getEmail(),
hasActiveMagicLink,
magicLinkVerified: loginInfoSession.getMagicLinkVerified(),
theme: themeSession.getTheme(),
},
},
Expand Down
4 changes: 2 additions & 2 deletions app/routes/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,14 @@ export default function IndexRoute() {
arrowUrl="#intro"
arrowLabel="Learn more about Kent"
action={
<>
<div className="flex flex-col gap-4 mr-auto">
<ButtonLink to="/blog" variant="primary" prefetch="intent">
Read the blog
</ButtonLink>
<ButtonLink to="/courses" variant="secondary" prefetch="intent">
Take a course
</ButtonLink>
</>
</div>
}
/>

Expand Down
6 changes: 5 additions & 1 deletion app/routes/login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ export const action: ActionFunction = async ({request}) => {

try {
const domainUrl = getDomainUrl(request)
await sendToken({emailAddress, domainUrl})
const magicLink = await sendToken({emailAddress, domainUrl})
loginSession.setMagicLink(magicLink)
return redirect(`/login`, {
headers: await loginSession.getHeaders(),
})
Expand All @@ -117,6 +118,7 @@ export const action: ActionFunction = async ({request}) => {

function Login() {
const data = useLoaderData<LoaderData>()
const inputRef = React.useRef<HTMLInputElement>(null)
const [submitted, setSubmitted] = React.useState(false)

const [formValues, setFormValues] = React.useState({
Expand Down Expand Up @@ -150,6 +152,7 @@ function Login() {
</div>

<Input
ref={inputRef}
autoFocus
aria-describedby={
data.error ? 'error-message' : 'success-message'
Expand All @@ -173,6 +176,7 @@ function Login() {
onClick={() => {
setFormValues({email: ''})
setSubmitted(false)
inputRef.current?.focus()
}}
>
Reset
Expand Down
1 change: 1 addition & 0 deletions app/routes/magic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const loader: LoaderFunction = async ({request}) => {
const loginInfoSession = await getLoginInfoSession(request)
try {
const session = await getUserSessionFromMagicLink(request)
loginInfoSession.setMagicLinkVerified(true)
if (session) {
const headers = new Headers()
loginInfoSession.clean()
Expand Down
2 changes: 1 addition & 1 deletion app/routes/me.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export const loader: LoaderFunction = async ({request}) => {
const qrLoginCode = await getQrCodeDataURL(
getMagicLink({
emailAddress: user.email,
validateEmail: false,
validateSessionMagicLink: false,
domainUrl: getDomainUrl(request),
}),
)
Expand Down
23 changes: 20 additions & 3 deletions app/routes/signup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type {KCDHandle, Team} from '~/types'
import {useTeam} from '~/utils/team-provider'
import {getSession, getUser} from '~/utils/session.server'
import {getLoginInfoSession} from '~/utils/login.server'
import {prismaWrite, validateMagicLink} from '~/utils/prisma.server'
import {prismaWrite, prismaRead, validateMagicLink} from '~/utils/prisma.server'
import {getErrorStack, teams} from '~/utils/misc'
import {tagKCDSiteSubscriber} from '../convertkit/convertkit.server'
import {Grid} from '~/components/grid'
Expand Down Expand Up @@ -80,7 +80,7 @@ export const action: ActionFunction = async ({request}) => {
throw new Error('email and magicLink required.')
}

email = await validateMagicLink(magicLink, loginInfoSession.getEmail())
email = await validateMagicLink(magicLink, loginInfoSession.getMagicLink())
} catch (error: unknown) {
console.error(getErrorStack(error))

Expand Down Expand Up @@ -140,8 +140,25 @@ export const loader: LoaderFunction = async ({request}) => {
if (user) return redirect('/me')

const loginInfoSession = await getLoginInfoSession(request)
const magicLink = loginInfoSession.getMagicLink()
const email = loginInfoSession.getEmail()
if (!email) {
if (!magicLink || !email) {
loginInfoSession.clean()
loginInfoSession.flashError('Invalid magic link. Try again.')
return redirect('/login', {
headers: await loginInfoSession.getHeaders(),
})
}

const userForMagicLink = await prismaRead.user.findFirst({
where: {email},
select: {id: true},
})

if (userForMagicLink) {
// user exists, but they haven't clicked their magic link yet
// we don't want to tell them that a user exists with that email though
// so we'll invalidate the magic link and force them to try again.
loginInfoSession.clean()
loginInfoSession.flashError('Invalid magic link. Try again.')
return redirect('/login', {
Expand Down
6 changes: 6 additions & 0 deletions app/utils/login.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,18 @@ async function getLoginInfoSession(request: Request) {
getMagicLink: () => session.get('magicLink') as string | undefined,
setMagicLink: (magicLink: string) => session.set('magicLink', magicLink),
unsetMagicLink: () => session.unset('magicLink'),
getMagicLinkVerified: () =>
session.get('magicLinkVerified') as boolean | undefined,
setMagicLinkVerified: (verified: boolean) =>
session.set('magicLinkVerified', verified),
unsetMagicLinkVerified: () => session.unset('magicLinkVerified'),
getError: () => session.get('error') as string | undefined,
flashError: (error: string) => session.flash('error', error),
clean: () => {
session.unset('email')
session.unset('magicLink')
session.unset('error')
session.unset('magicLinkVerified')
},
destroy: () => loginInfoStorage.destroySession(session),
commit,
Expand Down
21 changes: 9 additions & 12 deletions app/utils/prisma.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,21 +114,21 @@ const magicLinkSearchParam = 'kodyKey'
type MagicLinkPayload = {
emailAddress: string
creationDate: string
validateEmail: boolean
validateSessionMagicLink: boolean
}

function getMagicLink({
emailAddress,
validateEmail,
validateSessionMagicLink,
domainUrl,
}: {
emailAddress: string
validateEmail: boolean
validateSessionMagicLink: boolean
domainUrl: string
}) {
const payload: MagicLinkPayload = {
emailAddress,
validateEmail,
validateSessionMagicLink,
creationDate: new Date().toISOString(),
}
const stringToEncrypt = JSON.stringify(payload)
Expand All @@ -139,17 +139,16 @@ function getMagicLink({
return url.toString()
}

async function validateMagicLink(link: string, sessionEmailAddress?: string) {
let emailAddress, linkCreationDateString, validateEmail
async function validateMagicLink(link: string, sessionMagicLink?: string) {
let emailAddress, linkCreationDateString, validateSessionMagicLink
try {
const url = new URL(link)
const encryptedString = url.searchParams.get(magicLinkSearchParam) ?? '[]'
const decryptedString = decrypt(encryptedString)
const payload = JSON.parse(decryptedString) as MagicLinkPayload
console.log({payload, sessionEmailAddress})
emailAddress = payload.emailAddress
linkCreationDateString = payload.creationDate
validateEmail = payload.validateEmail
validateSessionMagicLink = payload.validateSessionMagicLink
} catch (error: unknown) {
console.error(error)
throw new Error('Sign in link invalid. Please request a new one.')
Expand All @@ -160,10 +159,8 @@ async function validateMagicLink(link: string, sessionEmailAddress?: string) {
throw new Error('Sign in link invalid. Please request a new one.')
}

if (validateEmail && emailAddress !== sessionEmailAddress) {
console.error(
`magic link payload email address does not match sessionEmailAddress`,
)
if (validateSessionMagicLink && link !== sessionMagicLink) {
console.error(`Magic link does not match sessionMagicLink`)
throw new Error(
`You must open the magic link on the same device it was created from for security reasons. Please request a new link.`,
)
Expand Down

0 comments on commit 63b8fe5

Please sign in to comment.