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.

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

Sandbox BGS does not accept events with missing prev field #327

Open
DavidBuchanan314 opened this issue Sep 21, 2023 · 6 comments
Open

Sandbox BGS does not accept events with missing prev field #327

DavidBuchanan314 opened this issue Sep 21, 2023 · 6 comments
Labels
bug Something isn't working correctness Core protocol and data storage issues

Comments

@DavidBuchanan314
Copy link

DavidBuchanan314 commented Sep 21, 2023

IIUC, as of repo v3, the prev field is optional. I got rid of it while updating picopds to repo v3. However, I couldn't get the sandbox BGS to accept my commit events until I added dummy prev fields (set to null) to my commit objects and firehose events (I'm not sure if both were strictly needed, but I changed both at once and it started working after that).

@DavidBuchanan314 DavidBuchanan314 changed the title BGS does not accept events with missing prev field Sandbox BGS does not accept events with missing prev field Sep 21, 2023
@bnewbold
Copy link
Collaborator

Thanks for the report, will take a look!

@whyrusleeping
Copy link
Collaborator

hrm... the prev field on the generated struct probably wants to be omitempty, but i dont think thats actually going to cause an issue.

@whyrusleeping
Copy link
Collaborator

I see some signature validation failures on retroid repos

@DavidBuchanan314
Copy link
Author

Do you have timestamps for those validation failures? I might be able to figure out what I was doing at the time.

At one point I'd forgotten to update my _atproto DNS record, although I don't think that would manifest itself as signature validation errors

@whyrusleeping
Copy link
Collaborator

yeah i saw the bad dns record one.

I'm 90% sure whats going on here is that you're signing over the 'commit' with the 'prev' field entirely missing, where when i check the signature i'm computing the bytes to have 'prev=null'.
Since i'm properly validating the signatures on the typescript code, i can assume that they are doing the same thing here. Actually removing the prev field is going to be tricky, missing field != null will be fun...

@bnewbold
Copy link
Collaborator

👀

yeah, prev is both optional and nullable per the v3 spec. it was nullable previously and we want to allow just not including it at all, but also wanted to be as compatible as possible during the transition.

some languages should be able to validate whatever is in the commit. golang will have a bit of trouble... I guess trying both variations shouldn't be too bad performance-wise as long as it is really clear which is expected and 99.99% of signature validations are happy path?

this could be a major problem for, eg, label signing and validation though (several fields, with no-value-provided common for several), will want to ensure there is no ambiguity in that case.

cc: @dholms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctness Core protocol and data storage issues
Projects
None yet
Development

No branches or pull requests

3 participants