harden datetime verification #1702
Conversation
bnewbold
force-pushed
the
bnewbold/harden-datetime
branch
from
6dac5d6
to
a394556
Compare
|
Some updates after discussing with @devinivy earlier in the week. This keeps a proper ensureValid, and that is what we should use for, eg, new record creation. It also has Also rebased on top of The review or feedback i'm still not sure about is whether the full strict validation will only happen on record creation. If that is true, great! Otherwise, if it is also validated when deserializing existing records, maybe we need special logic to distinguish between "new record creation" and "existing record parsing" for this corner-case. A test for this would probably be good. |
| if (!isValidISODateString(value)) { | ||
| throw new Error() | ||
| } | ||
| ensureValidDatetime(value) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
If you could tackle that, would be much appreciated! Noticed tests failed on this branch, I think just flakey, trying a re-run of failed. |
| return | ||
| } | ||
| try { | ||
| ensureValidDatetime(createdAt) |
There was a problem hiding this comment.
added the extra check here
This means records with bad timestamps will still pass "lexicon validation" (which we want so that old records don't break application views), but they will no longer be able to be created on the pds
this specific approach only works as is because the only datetimes on our records are createdAt times & every createdAt property is a datetime. if we get other datetimes, we'll need to tweak this logic
| describe('normalization', () => { | ||
| it('normalizes datetimes', () => { |
There was a problem hiding this comment.
I believe this fun vector originally seen from retr0.id still makes it through, where the normalized date ends-up before year zero, which is invalid:
> normalizeDatetime('0000-01-01T00:00:00+01:00')
'-000001-12-31T23:00:00.000Z'In toSimplifiedISOSafe() you'll find an extra validation check on the normalized output, which is what currently catches this.
There was a problem hiding this comment.
just added a test for that, lmk how it looks 👍
|
I didn't see @dholms small patch, and added a bigger patch which verifies that the output of the normalization function is itself valid. And also just disallows dates starting If Dan's fix is simpler/better we can just revert my patches. I resolved a merge conflict. |
|
Yup yup I think that looks fine! finally gonna get this in 💪 |
…tab-should-show-own-threads * origin/main: (59 commits) Config to start notifications daemon from a specific did (#1922) Feature branch: PDS v2 (#1789) Cleanup outdated notifications in appview, add daemon for similar tasks (#1893) Add flag for running db migrations on appview (#1913) Do not generate notifs when post violates threadgate (#1901) Version packages (#1909) Additional @atproto/api 0.6.24 changeset (#1912) Fix snapshots for list items (#1911) Attach record URI to listItemView (#1758) helpers for rkey and tid syntax; validate rkey at record creation time (#1738) harden datetime verification (#1702) fix(debug): properly type debugCatch wrapper result (#1817) style(xrpc-server): avoid un-neccessary "if" statement (#1826) perf(bsky): avoid re-creating auth functions on every request (#1822) Don't create unnecessary error objects (#1908) fix(pds): include aspectRatio on read-sticky posts (#1824) Handle missing creator on lists and feed generators (#1906) ✨ Expose labels attached with legacy actions when events are queried and fix email event builder (#1905) Evented architecture for moderation system (#1617) Put canReply state on post viewer state instead of thread viewer state (#1882) ...
This adds stronger datetime verification in the syntax package, including a bunch of interop test cases, and swaps that in during Lexicon verification.
The prior verification was pretty lax, so I suspect this might cause some problems to other devs, or with existing records, when we roll it out.