helpers for rkey and tid syntax; validate rkey at record creation time #1738
Conversation
|
Would this change prevent users from liking/adding-to-list a FeedGenerator with a |
| // @TODO: validate against Lexicon record 'key' type, not just overall recordkey syntax | ||
| ensureValidRecordKey(rkey) |
There was a problem hiding this comment.
Since this is coarse and lexicon-unaware, I wonder if we should push this check down into the repo code. If the PDS is lexicon-aware, it could enforce the stricter key type check as a part of record validation.
There was a problem hiding this comment.
That makes sense to me in terms of correctness.
repo: I'm not super confident what the blast radius of impact would be of verifying NSID+RecordKey would be in, eg, repo/src/util.ts parseDataKey(). That seems to run both on writes and reads? So could break things like the existing with-: record. We could initially check only in the create/write path, maybe creating a separate parseDataKeyPermissive as a migration/transition thing.
PDS: I expect PDS to be lexicon-aware enough to do generic Lexicon validation, possibly by fetching/resolving (and caching) new Lexicon files in the future. For that, passing rkey to the "validate record" function would make sense to me.
There was a problem hiding this comment.
@dholms any feels on this?
There was a problem hiding this comment.
I think it's a little bit more comfortable living here for the time being.
We don't do NSID validation on collection names in the repo code & that strikes me as similar to this.
So far the repo/MST is pretty unaware of these syntatic things & is primarily concerned with the tree structure & cbor encoding. Though I think there's a good argument for pushing some more lexicon/syntax awareness into it.
In short, I'd leave it here for now but consider adjusting where we draw the abstraction boundary
|
@ericvolp12 this PR as written does not change validation of AT URI fields, so it doesn't directly impact liking or add-to-list of existing records (like a feedgen record with |
| @@ -0,0 +1,26 @@ | |||
| export const ensureValidRecordKey = (rkey: string): void => { | |||
| if (rkey.length > 512 || rkey.length < 1) { | |||
There was a problem hiding this comment.
i'm pretty tempted to limit this to less than 512
I think we talked about 16 at one point (tho i think that's too low). but maybe 64?
There was a problem hiding this comment.
I'm not totally against that, but there are a couple things to discuss. I opened an issue in the specs repo: bluesky-social/atproto-website#250
There was a problem hiding this comment.
cool cool - that doesn't have to hold this up 👍
…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) ...
More syntax validators, with interop tests.
cc: @ericvolp12 who has a popular feedgen record with a colon (
:) in the rkey, which is disallowed by the spec (but wasn't being enforced by PDS), and is a character not in the RFC 3986 "Unreserved Characters" list, which is what the rkey syntax rules are based on. Colon is inpcharand we could expand to support it in theory. I'd kind of rather find a migration path for the current record... create a new record w/o colon and update all the subscriptions as a on-time fix? Regardless, my understanding is that this current PR would not break the existing feed generator, only prevent new records with:from being created.