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.

Sign up for GitHub

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

harden datetime verification #1702

Merged
merged 12 commits into from
Dec 1, 2023
Merged

harden datetime verification #1702

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cf90833
syntax: add datetime validator (and interop tests)
bnewbold Oct 3, 2023
443ef1f
syntax: improve datetime normalization
bnewbold Oct 3, 2023
1d6c337
lexicon: stronger datetime validation (from syntax package)
bnewbold Oct 3, 2023
bb9a0ed
syntax: make datetime syntax norm test more flexible
bnewbold Oct 3, 2023
756aa57
make fmt
bnewbold Oct 3, 2023
a394556
datetime: docs, normalize and always variant
bnewbold Oct 12, 2023
d5e27d6
bsky replace toSimplifiedISOSafe with normalizeDatetimeAlways
bnewbold Oct 12, 2023
a52bdcc
Merge branch 'main' into bnewbold/harden-datetime
dholms Nov 21, 2023
9220573
more rigorous datetime parsing on record creation
dholms Nov 21, 2023
eac065a
handle negative dates
dholms Nov 21, 2023
91f5d93
syntax: disallow datetimes before year 0010
bnewbold Nov 21, 2023
9f1c382
syntax: datetime normalization functions validate output
bnewbold Nov 21, 2023
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
7 changes: 7 additions & 0 deletions interop-test-files/syntax/datetime_parse_invalid.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# superficial syntax parses ok, but are not valid datetimes for semantic reasons (eg, "month zero")
1985-00-12T23:20:50.123Z
1985-04-00T23:20:50.123Z
1985-13-12T23:20:50.123Z
1985-04-12T25:20:50.123Z
1985-04-12T23:99:50.123Z
1985-04-12T23:20:61.123Z
64 changes: 64 additions & 0 deletions interop-test-files/syntax/datetime_syntax_invalid.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@

# subtle changes to: 1985-04-12T23:20:50.123Z
1985-04-12T23:20:50.123z
01985-04-12T23:20:50.123Z
985-04-12T23:20:50.123Z
1985-04-12T23:20:50.Z
1985-04-32T23;20:50.123Z
1985-04-32T23;20:50.123Z

# en-dash and em-dash
1985—04-32T23;20:50.123Z
1985–04-32T23;20:50.123Z

# whitespace
1985-04-12T23:20:50.123Z
1985-04-12T23:20:50.123Z
1985-04-12T 23:20:50.123Z

# not enough zero padding
1985-4-12T23:20:50.123Z
1985-04-2T23:20:50.123Z
1985-04-12T3:20:50.123Z
1985-04-12T23:0:50.123Z
1985-04-12T23:20:5.123Z

# too much zero padding
01985-04-12T23:20:50.123Z
1985-004-12T23:20:50.123Z
1985-04-012T23:20:50.123Z
1985-04-12T023:20:50.123Z
1985-04-12T23:020:50.123Z
1985-04-12T23:20:050.123Z

# strict capitalization (ISO-8601)
1985-04-12t23:20:50.123Z
1985-04-12T23:20:50.123z

# RFC-3339, but not ISO-8601
1985-04-12T23:20:50.123-00:00
1985-04-12_23:20:50.123Z
1985-04-12 23:20:50.123Z

# ISO-8601, but weird
1985-04-274T23:20:50.123Z

# timezone is required
1985-04-12T23:20:50.123
1985-04-12T23:20:50

1985-04-12
1985-04-12T23:20Z
1985-04-12T23:20:5Z
1985-04-12T23:20:50.123
+001985-04-12T23:20:50.123Z
23:20:50.123Z

1985-04-12T23:20:50.123+00
1985-04-12T23:20:50.123+00:0
1985-04-12T23:20:50.123+0:00
1985-04-12T23:20:50.123
1985-04-12T23:20:50.123+0000
1985-04-12T23:20:50.123+00
1985-04-12T23:20:50.123+
1985-04-12T23:20:50.123-
34 changes: 34 additions & 0 deletions interop-test-files/syntax/datetime_syntax_valid.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# "preferred"
1985-04-12T23:20:50.123Z
1985-04-12T23:20:50.000Z
2000-01-01T00:00:00.000Z
1985-04-12T23:20:50.123456Z
1985-04-12T23:20:50.120Z
1985-04-12T23:20:50.120000Z

# "supported"
1985-04-12T23:20:50.1235678912345Z
1985-04-12T23:20:50.100Z
1985-04-12T23:20:50Z
1985-04-12T23:20:50.0Z
1985-04-12T23:20:50.123+00:00
1985-04-12T23:20:50.123-07:00
1985-04-12T23:20:50.123+07:00
1985-04-12T23:20:50.123+01:45
0985-04-12T23:20:50.123-07:00
1985-04-12T23:20:50.123-07:00
0123-01-01T00:00:00.000Z

# various precisions, up through at least 12 digits
1985-04-12T23:20:50.1Z
1985-04-12T23:20:50.12Z
1985-04-12T23:20:50.123Z
1985-04-12T23:20:50.1234Z
1985-04-12T23:20:50.12345Z
1985-04-12T23:20:50.123456Z
1985-04-12T23:20:50.1234567Z
1985-04-12T23:20:50.12345678Z
1985-04-12T23:20:50.123456789Z
1985-04-12T23:20:50.1234567890Z
1985-04-12T23:20:50.12345678901Z
1985-04-12T23:20:50.123456789012Z
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/block.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Selectable } from 'kysely'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { CID } from 'multiformats/cid'
import * as Block from '../../../lexicon/types/app/bsky/graph/block'
import * as lex from '../../../lexicon/lexicons'
Expand All @@ -27,7 +26,7 @@ const insertFn = async (
cid: cid.toString(),
creator: uri.host,
subjectDid: obj.subject,
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
})
.onConflict((oc) => oc.doNothing())
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/feed-generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Selectable } from 'kysely'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { CID } from 'multiformats/cid'
import * as FeedGenerator from '../../../lexicon/types/app/bsky/feed/generator'
import * as lex from '../../../lexicon/lexicons'
Expand Down Expand Up @@ -33,7 +32,7 @@ const insertFn = async (
? JSON.stringify(obj.descriptionFacets)
: undefined,
avatarCid: obj.avatar?.ref.toString(),
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
})
.onConflict((oc) => oc.doNothing())
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/follow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Selectable } from 'kysely'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { CID } from 'multiformats/cid'
import * as Follow from '../../../lexicon/types/app/bsky/graph/follow'
import * as lex from '../../../lexicon/lexicons'
Expand Down Expand Up @@ -28,7 +27,7 @@ const insertFn = async (
cid: cid.toString(),
creator: uri.host,
subjectDid: obj.subject,
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
})
.onConflict((oc) => oc.doNothing())
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/like.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Selectable } from 'kysely'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { CID } from 'multiformats/cid'
import * as Like from '../../../lexicon/types/app/bsky/feed/like'
import * as lex from '../../../lexicon/lexicons'
Expand Down Expand Up @@ -29,7 +28,7 @@ const insertFn = async (
creator: uri.host,
subject: obj.subject.uri,
subjectCid: obj.subject.cid,
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
})
.onConflict((oc) => oc.doNothing())
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/list-block.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Selectable } from 'kysely'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { CID } from 'multiformats/cid'
import * as ListBlock from '../../../lexicon/types/app/bsky/graph/listblock'
import * as lex from '../../../lexicon/lexicons'
Expand All @@ -27,7 +26,7 @@ const insertFn = async (
cid: cid.toString(),
creator: uri.host,
subjectUri: obj.subject,
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
})
.onConflict((oc) => oc.doNothing())
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/list-item.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Selectable } from 'kysely'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { CID } from 'multiformats/cid'
import * as ListItem from '../../../lexicon/types/app/bsky/graph/listitem'
import * as lex from '../../../lexicon/lexicons'
Expand Down Expand Up @@ -35,7 +34,7 @@ const insertFn = async (
creator: uri.host,
subjectDid: obj.subject,
listUri: obj.list,
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
})
.onConflict((oc) => oc.doNothing())
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Selectable } from 'kysely'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { CID } from 'multiformats/cid'
import * as List from '../../../lexicon/types/app/bsky/graph/list'
import * as lex from '../../../lexicon/lexicons'
Expand Down Expand Up @@ -33,7 +32,7 @@ const insertFn = async (
? JSON.stringify(obj.descriptionFacets)
: undefined,
avatarCid: obj.avatar?.ref.toString(),
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
})
.onConflict((oc) => oc.doNothing())
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/post.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { Insertable, Selectable, sql } from 'kysely'
import { CID } from 'multiformats/cid'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { jsonStringToLex } from '@atproto/lexicon'
import {
Record as PostRecord,
Expand Down Expand Up @@ -68,7 +67,7 @@ const insertFn = async (
cid: cid.toString(),
creator: uri.host,
text: obj.text,
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
replyRoot: obj.reply?.root?.uri || null,
replyRootCid: obj.reply?.root?.cid || null,
replyParent: obj.reply?.parent?.uri || null,
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/repost.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { Selectable } from 'kysely'
import { CID } from 'multiformats/cid'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import * as Repost from '../../../lexicon/types/app/bsky/feed/repost'
import * as lex from '../../../lexicon/lexicons'
import { DatabaseSchema, DatabaseSchemaType } from '../../../db/database-schema'
Expand All @@ -27,7 +26,7 @@ const insertFn = async (
creator: uri.host,
subject: obj.subject.uri,
subjectCid: obj.subject.cid,
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
}
const [inserted] = await Promise.all([
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/indexing/plugins/thread-gate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { AtUri } from '@atproto/syntax'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { InvalidRequestError } from '@atproto/xrpc-server'
import { toSimplifiedISOSafe } from '@atproto/common'
import { CID } from 'multiformats/cid'
import * as Threadgate from '../../../lexicon/types/app/bsky/feed/threadgate'
import * as lex from '../../../lexicon/lexicons'
Expand Down Expand Up @@ -33,7 +32,7 @@ const insertFn = async (
cid: cid.toString(),
creator: uri.host,
postUri: obj.post,
createdAt: toSimplifiedISOSafe(obj.createdAt),
createdAt: normalizeDatetimeAlways(obj.createdAt),
indexedAt: timestamp,
})
.onConflict((oc) => oc.doNothing())
Expand Down
5 changes: 2 additions & 3 deletions packages/bsky/src/services/label/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { sql } from 'kysely'
import { AtUri } from '@atproto/syntax'
import { toSimplifiedISOSafe } from '@atproto/common'
import { AtUri, normalizeDatetimeAlways } from '@atproto/syntax'
import { Database } from '../../db'
import { Label, isSelfLabels } from '../../lexicon/types/com/atproto/label/defs'
import { ids } from '../../lexicon/lexicons'
Expand Down Expand Up @@ -166,7 +165,7 @@ export function getSelfLabels(details: {
const src = new AtUri(uri).host // record creator
const cts =
typeof record.createdAt === 'string'
? toSimplifiedISOSafe(record.createdAt)
? normalizeDatetimeAlways(record.createdAt)
: new Date(0).toISOString()
return record.labels.values.map(({ val }) => {
return { src, uri, cid, val, cts, neg: false }
Expand Down
8 changes: 3 additions & 5 deletions packages/lexicon/src/validators/formats.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,22 @@
import { isValidISODateString } from 'iso-datestring-validator'
import { CID } from 'multiformats/cid'
import { ValidationResult, ValidationError } from '../types'
import {
ensureValidDid,
ensureValidHandle,
ensureValidNsid,
ensureValidAtUri,
ensureValidDatetime,
} from '@atproto/syntax'
import { validateLanguage } from '@atproto/common-web'

export function datetime(path: string, value: string): ValidationResult {
try {
if (!isValidISODateString(value)) {
throw new Error()
}
ensureValidDatetime(value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah this will happen everywhere - on new record creations & when validating existing records

I think we leave this code path as-is for now, and I can add in the more rigorous validation into the pds indexing path

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, makes sense. The normalizeDatetime function here would have pretty similar behavior to isValidISODateString, throwing a more specific error when a totally-not-a-datetime is encountered. But just sticking with exactly the current code would be less risky.

} catch {
return {
success: false,
error: new ValidationError(
`${path} must be an iso8601 formatted datetime`,
`${path} must be an valid atproto datetime (both RFC-3339 and ISO-8601)`,
),
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/lexicon/tests/general.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,9 @@ describe('Record validation', () => {
$type: 'com.example.datetime',
datetime: 'bad date',
}),
).toThrow('Record/datetime must be an iso8601 formatted datetime')
).toThrow(
'Record/datetime must be an valid atproto datetime (both RFC-3339 and ISO-8601)',
)
})

it('Applies uri formatting constraint', () => {
Expand Down
Loading