Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

EventID incorrectly inherits from DomainSpecificString #14638

Open
clokep opened this issue Dec 7, 2022 · 4 comments
Open

EventID incorrectly inherits from DomainSpecificString #14638

clokep opened this issue Dec 7, 2022 · 4 comments
Labels
O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Cleanup Things we want to get rid of, but aren't actively causing pain

Comments

@clokep
Copy link
Member

clokep commented Dec 7, 2022

Not super happy with the startswith but EventID.is_valid is broken for non-V1/2 event IDs so unsure how to proceed.

Originally posted by @Fizzadar in #14632 (comment)

Modern event IDs are now hashes and do not include a domain name, so they shouldn't inherit from DomainSepcificString.

@clokep clokep added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. O-Occasional Affects or can be seen by some users regularly or most users rarely labels Dec 7, 2022
@reivilibre
Copy link
Contributor

good spot. We should define a new string type for this with a corrected grammar?

@clokep
Copy link
Member Author

clokep commented Dec 8, 2022

good spot. We should define a new string type for this with a corrected grammar?

Yep! I don't know if there's anything that would break if it no longer has the same interface as DomainSpecificString.

I think the grammar is just $[...characters...] unfortunately.

@reivilibre
Copy link
Contributor

That wouldn't surprise me, I guess. Probably null bytes might be accidentally excluded because of database support but everything else might be acceptable.
v3+ event IDs have a more strict grammar owing to the fact they are an encoded hash, so we could define an alphabet for those. For the non-v3+ event IDs, we can enforce that there is a domain.

@DMRobertson
Copy link
Contributor

v3+ event IDs have a more strict grammar owing to the fact they are an encoded hash, so we could define an alphabet for those.

I think they also have an expected length?

For the non-v3+ event IDs, we can enforce that there is a domain.

Similarly, I think the spec might define a maximum length of an event ID here

@MadLittleMods MadLittleMods changed the title EventID incorrectly inherits from DomainSpecificString EventID incorrectly inherits from DomainSpecificString Apr 25, 2023
@MadLittleMods MadLittleMods added the Z-Cleanup Things we want to get rid of, but aren't actively causing pain label Apr 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Cleanup Things we want to get rid of, but aren't actively causing pain
Projects
None yet
Development

No branches or pull requests

4 participants