chore: update serializable type validation to check for wider range of cbor types instead of json types #1024
Conversation
…f cbor types instead of json types
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| export const auth = betterAuth({ | ||
| // IMPORTANT: Connect your own database here | ||
| database: sqliteAdapter(db), | ||
| // IMPORTANT: Connect a real database for productoin use cases |
There was a problem hiding this comment.
There's a typo in the comment: productoin should be production
| // IMPORTANT: Connect a real database for productoin use cases | |
| // IMPORTANT: Connect a real database for production use cases |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| // user: [], | ||
| // account: [], | ||
| // session: [], | ||
| // verifcation: [], |
There was a problem hiding this comment.
There's a typo in the property name: verifcation is missing an 'i' and should be verification
| // verifcation: [], | |
| // verification: [], |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| id: crypto.randomUUID(), | ||
| userId: c.auth.userId, | ||
| username: c.auth.user.email, | ||
| userId: "TODO", |
There was a problem hiding this comment.
The userId field is currently hardcoded as "TODO", which will cause all messages to share the same user identifier. This should be replaced with the actual user ID from the authentication context, likely available at c.conn.auth.user.id. Using a static value here breaks user attribution in the chat system.
| userId: "TODO", | |
| userId: c.conn.auth.user.id, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| if (proto !== null && proto !== Object.prototype) { | ||
| // Check if it's a known serializable object type | ||
| const constructor = value.constructor; | ||
| if (constructor && typeof constructor.name === "string") { | ||
| // Allow objects with named constructors (records, named objects) | ||
| // This includes user-defined classes and built-in objects | ||
| // that CBOR can serialize with tag 27 or record tags | ||
| } |
There was a problem hiding this comment.
The validation logic for objects with non-standard prototypes appears incomplete. While the code checks for the presence of a named constructor, it doesn't perform any actual validation to determine if these objects are CBOR serializable. The empty if block with only comments suggests an unfinished implementation.
This could potentially allow non-serializable objects to pass validation, leading to runtime errors when serialization is attempted. Consider either:
- Implementing proper validation for these object types
- Explicitly rejecting objects with non-standard prototypes
- Documenting which specific object types with custom prototypes are supported
// Example of a more complete implementation:
if (constructor && typeof constructor.name === "string") {
// Check if it's one of the known serializable types
if ([KnownType1, KnownType2].includes(constructor)) {
return true;
}
onInvalid?.(currentPath);
return false;
}| if (proto !== null && proto !== Object.prototype) { | |
| // Check if it's a known serializable object type | |
| const constructor = value.constructor; | |
| if (constructor && typeof constructor.name === "string") { | |
| // Allow objects with named constructors (records, named objects) | |
| // This includes user-defined classes and built-in objects | |
| // that CBOR can serialize with tag 27 or record tags | |
| } | |
| if (proto !== null && proto !== Object.prototype) { | |
| // Check if it's a known serializable object type | |
| const constructor = value.constructor; | |
| if (constructor && typeof constructor.name === "string") { | |
| // Allow objects with named constructors (records, named objects) | |
| // This includes user-defined classes and built-in objects | |
| // that CBOR can serialize with tag 27 or record tags | |
| const knownSerializableTypes = [ | |
| Date, | |
| Map, | |
| Set, | |
| RegExp | |
| // Add other known serializable types here | |
| ]; | |
| if (knownSerializableTypes.some(type => constructor === type)) { | |
| return true; | |
| } | |
| onInvalid?.(currentPath); | |
| return false; | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Merge activity
|
…f cbor types instead of json types (#1024)
graphite-app
bot
deleted the
06-27-chore_update_serializable_type_validation_to_check_for_wider_range_of_cbor_types_instead_of_json_types
branch
No description provided.