Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle validation improvements #1336

Merged
merged 9 commits into from
Jul 14, 2023
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
58 changes: 4 additions & 54 deletions packages/identifier/src/handle.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import { reservedSubdomains } from './reserved'

export const INVALID_HANDLE = 'handle.invalid'

// Currently these are registration-time restrictions, not protocol-level
// restrictions. We have a couple accounts in the wild that we need to clean up
// before hard-disallow.
// See also: https://en.wikipedia.org/wiki/Top-level_domain#Reserved_domains
const DISALLOWED_TLDS = [
export const DISALLOWED_TLDS = [
'.local',
'.arpa',
'.invalid',
Expand Down Expand Up @@ -106,60 +104,12 @@ export const isValidHandle = (handle: string): boolean => {
}
throw err
}
return true
}

export const ensureHandleServiceConstraints = (
handle: string,
availableUserDomains: string[],
reserved = reservedSubdomains,
): void => {
const disallowedTld = DISALLOWED_TLDS.find((domain) =>
handle.endsWith(domain),
)
if (disallowedTld) {
throw new DisallowedDomainError('Handle TLD is invalid or disallowed')
}
const supportedDomain = availableUserDomains.find((domain) =>
handle.endsWith(domain),
)
if (!supportedDomain) {
throw new UnsupportedDomainError('Not a supported handle domain')
}
const front = handle.slice(0, handle.length - supportedDomain.length)
if (front.indexOf('.') > -1) {
throw new InvalidHandleError('Invalid characters in handle')
}
if (front.length < 3) {
throw new InvalidHandleError('Handle too short')
}
if (handle.length > 30) {
throw new InvalidHandleError('Handle too long')
}
if (reserved[front]) {
throw new ReservedHandleError('Reserved handle')
}
return true
}

export const fulfillsHandleServiceConstraints = (
handle: string,
availableUserDomains: string[],
reserved = reservedSubdomains,
): boolean => {
try {
ensureHandleServiceConstraints(handle, availableUserDomains, reserved)
} catch (err) {
if (
err instanceof InvalidHandleError ||
err instanceof ReservedHandleError ||
err instanceof UnsupportedDomainError ||
err instanceof DisallowedDomainError
) {
return false
}
throw err
}
return true
export const isValidTld = (handle: string): boolean => {
return !DISALLOWED_TLDS.some((domain) => handle.endsWith(domain))
}

export class InvalidHandleError extends Error {}
Expand Down
1 change: 0 additions & 1 deletion packages/identifier/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
export * from './handle'
export * from './did'
export * from './reserved'
26 changes: 1 addition & 25 deletions packages/identifier/tests/handle.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {
ensureValidHandle,
ensureHandleServiceConstraints,
normalizeAndEnsureValidHandle,
ensureValidHandleRegex,
InvalidHandleError,
Expand Down Expand Up @@ -193,30 +192,7 @@ describe('handle validation', () => {
})
})

describe('service constraints & normalization', () => {
const domains = ['.bsky.app', '.test']
it('throw on handles that violate service constraints', () => {
const expectThrow = (handle: string, err: string) => {
expect(() => ensureHandleServiceConstraints(handle, domains)).toThrow(err)
}

expectThrow('john.bsky.io', 'Not a supported handle domain')
expectThrow('john.com', 'Not a supported handle domain')
expectThrow('j.test', 'Handle too short')
expectThrow('uk.test', 'Handle too short')
expectThrow('john.test.bsky.app', 'Invalid characters in handle')
expectThrow('about.test', 'Reserved handle')
expectThrow('atp.test', 'Reserved handle')
expectThrow('barackobama.test', 'Reserved handle')

expectThrow('atproto.local', 'Handle TLD is invalid or disallowed')
expectThrow('atproto.arpa', 'Handle TLD is invalid or disallowed')
expectThrow('atproto.invalid', 'Handle TLD is invalid or disallowed')
expectThrow('atproto.localhost', 'Handle TLD is invalid or disallowed')
expectThrow('atproto.onion', 'Handle TLD is invalid or disallowed')
expectThrow('atproto.internal', 'Handle TLD is invalid or disallowed')
})

describe('normalization', () => {
it('normalizes handles', () => {
const normalized = normalizeAndEnsureValidHandle('JoHn.TeST')
expect(normalized).toBe('john.test')
Expand Down
42 changes: 9 additions & 33 deletions packages/pds/src/api/com/atproto/admin/updateAccountHandle.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { AuthRequiredError, InvalidRequestError } from '@atproto/xrpc-server'
import * as ident from '@atproto/identifier'
import { normalizeAndValidateHandle } from '../../../../handle'
import { Server } from '../../../../lexicon'
import AppContext from '../../../../context'
import {
Expand All @@ -11,42 +11,18 @@ import { httpLogger } from '../../../../logger'
export default function (server: Server, ctx: AppContext) {
server.com.atproto.admin.updateAccountHandle({
auth: ctx.roleVerifier,
handler: async ({ input, req, auth }) => {
handler: async ({ input, auth }) => {
if (!auth.credentials.admin) {
throw new AuthRequiredError('Insufficient privileges')
}
const { did } = input.body
let handle: string
try {
handle = ident.normalizeAndEnsureValidHandle(input.body.handle)
} catch (err) {
if (err instanceof ident.InvalidHandleError) {
throw new InvalidRequestError(err.message, 'InvalidHandle')
} else {
throw err
}
}
try {
ident.ensureHandleServiceConstraints(
handle,
ctx.cfg.availableUserDomains,
)
} catch (err) {
if (err instanceof ident.UnsupportedDomainError) {
throw new InvalidRequestError(
'Unsupported domain',
'UnsupportedDomain',
)
} else if (err instanceof ident.ReservedHandleError) {
// we allow this
req.log.info(
{ did, handle: input.body },
'admin setting reserved handle',
)
} else {
throw err
}
}
const handle = await normalizeAndValidateHandle({
ctx,
handle: input.body.handle,
did,
allowReserved: true,
})

const existingAccnt = await ctx.services.account(ctx.db).getAccount(did)
if (!existingAccnt) {
throw new InvalidRequestError(`Account not found: ${did}`)
Expand Down
45 changes: 11 additions & 34 deletions packages/pds/src/api/com/atproto/identity/updateHandle.ts
Original file line number Diff line number Diff line change
@@ -1,51 +1,24 @@
import { InvalidRequestError } from '@atproto/xrpc-server'
import * as ident from '@atproto/identifier'
import { normalizeAndValidateHandle } from '../../../../handle'
import { Server } from '../../../../lexicon'
import AppContext from '../../../../context'
import {
HandleSequenceToken,
UserAlreadyExistsError,
} from '../../../../services/account'
import { httpLogger } from '../../../../logger'
import { backgroundHandleCheckForFlag } from '../../../../handle/moderation'

export default function (server: Server, ctx: AppContext) {
server.com.atproto.identity.updateHandle({
auth: ctx.accessVerifierCheckTakedown,
handler: async ({ auth, input }) => {
const requester = auth.credentials.did
let handle: string
try {
handle = ident.normalizeAndEnsureValidHandle(input.body.handle)
} catch (err) {
if (err instanceof ident.InvalidHandleError) {
throw new InvalidRequestError(err.message, 'InvalidHandle')
}
throw err
}

// test against our service constraints
// if not a supported domain, then we must check that the domain correctly links to the DID
try {
ident.ensureHandleServiceConstraints(
handle,
ctx.cfg.availableUserDomains,
)
} catch (err) {
if (err instanceof ident.UnsupportedDomainError) {
const did = await ctx.idResolver.handle.resolve(handle)
if (did !== requester) {
throw new InvalidRequestError(
'External handle did not resolve to DID',
)
}
} else if (err instanceof ident.InvalidHandleError) {
throw new InvalidRequestError(err.message, 'InvalidHandle')
} else if (err instanceof ident.ReservedHandleError) {
throw new InvalidRequestError(err.message, 'HandleNotAvailable')
} else {
throw err
}
}
const handle = await normalizeAndValidateHandle({
ctx,
handle: input.body.handle,
did: requester,
})

// Pessimistic check to handle spam: also enforced by updateHandle() and the db.
const available = await ctx.services
Expand Down Expand Up @@ -81,6 +54,10 @@ export default function (server: Server, ctx: AppContext) {
'failed to sequence handle update',
)
}

if (ctx.cfg.unacceptableHandleWordsB64) {
backgroundHandleCheckForFlag({ ctx, handle, did: requester })
}
},
})
}
41 changes: 11 additions & 30 deletions packages/pds/src/api/com/atproto/server/createAccount.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { InvalidRequestError } from '@atproto/xrpc-server'
import * as ident from '@atproto/identifier'
import { normalizeAndValidateHandle } from '../../../../handle'
import * as plc from '@did-plc/lib'
import * as scrypt from '../../../../db/scrypt'
import { Server } from '../../../../lexicon'
Expand All @@ -9,6 +9,7 @@ import { UserAlreadyExistsError } from '../../../../services/account'
import AppContext from '../../../../context'
import Database from '../../../../db'
import { AtprotoData } from '@atproto/identity'
import { backgroundHandleCheckForFlag } from '../../../../handle/moderation'

export default function (server: Server, ctx: AppContext) {
server.com.atproto.server.createAccount(async ({ input, req }) => {
Expand All @@ -22,7 +23,11 @@ export default function (server: Server, ctx: AppContext) {
}

// normalize & ensure valid handle
const handle = await ensureValidHandle(ctx, input.body)
const handle = await normalizeAndValidateHandle({
ctx,
handle: input.body.handle,
did: input.body.did,
})

// check that the invite code still has uses
if (ctx.cfg.inviteRequired && inviteCode) {
Expand Down Expand Up @@ -101,6 +106,10 @@ export default function (server: Server, ctx: AppContext) {
}
})

if (ctx.cfg.unacceptableHandleWordsB64) {
backgroundHandleCheckForFlag({ ctx, handle, did: result.did })
}

return {
encoding: 'application/json',
body: {
Expand Down Expand Up @@ -146,34 +155,6 @@ export const ensureCodeIsAvailable = async (
}
}

const ensureValidHandle = async (
ctx: AppContext,
input: CreateAccountInput,
): Promise<string> => {
try {
const handle = ident.normalizeAndEnsureValidHandle(input.handle)
ident.ensureHandleServiceConstraints(handle, ctx.cfg.availableUserDomains)
return handle
} catch (err) {
if (err instanceof ident.InvalidHandleError) {
throw new InvalidRequestError(err.message, 'InvalidHandle')
} else if (err instanceof ident.ReservedHandleError) {
throw new InvalidRequestError(err.message, 'HandleNotAvailable')
} else if (err instanceof ident.UnsupportedDomainError) {
if (input.did === undefined) {
throw new InvalidRequestError(err.message, 'UnsupportedDomain')
}
const resolvedHandleDid = await ctx.idResolver.handle.resolve(
input.handle,
)
if (input.did !== resolvedHandleDid) {
throw new InvalidRequestError('External handle did not resolve to DID')
}
}
throw err
}
}

const getDidAndPlcOp = async (
ctx: AppContext,
handle: string,
Expand Down
19 changes: 19 additions & 0 deletions packages/pds/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ export interface ServerConfigValues {
hiveApiKey?: string
labelerDid: string
labelerKeywords: Record<string, string>
unacceptableHandleWordsB64?: string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Loading this file would be better for transparency. Keep the env var though, the file should be a fallback used in prod!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think it'll be great if we know what are the words that are considered as unacceptable words here for transparency

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just for transparency sake, but for future instances as well. A list contained solely in an env var means no other created instances get access

falsePositiveHandleWordsB64?: string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Loading this file would be better for transparency. Keep the env var though, the file should be a fallback used in prod!


feedGenDid?: string

Expand Down Expand Up @@ -163,6 +165,13 @@ export class ServerConfig {
const labelerDid = process.env.LABELER_DID || 'did:example:labeler'
const labelerKeywords = {}

const unacceptableHandleWordsB64 = nonemptyString(
process.env.UNACCEPTABLE_HANDLE_WORDS_B64,
)
const falsePositiveHandleWordsB64 = nonemptyString(
process.env.FALSE_POSITIVE_HANDLE_WORDS_B64,
)

const feedGenDid = process.env.FEED_GEN_DID

const dbPostgresUrl = process.env.DB_POSTGRES_URL
Expand Down Expand Up @@ -234,6 +243,8 @@ export class ServerConfig {
hiveApiKey,
labelerDid,
labelerKeywords,
unacceptableHandleWordsB64,
falsePositiveHandleWordsB64,
feedGenDid,
maxSubscriptionBuffer,
repoBackfillLimitMs,
Expand Down Expand Up @@ -425,6 +436,14 @@ export class ServerConfig {
return this.cfg.labelerKeywords
}

get unacceptableHandleWordsB64() {
return this.cfg.unacceptableHandleWordsB64
}

get falsePositiveHandleWordsB64() {
return this.cfg.falsePositiveHandleWordsB64
}

get feedGenDid() {
return this.cfg.feedGenDid
}
Expand Down
Loading