[Bug]: No check for invalid timestamps from the client

[steida]

Verified issue does not already exist?

• [X] I have searched and found no existing issue

What happened?

Here https://github.com/actualbudget/actual/blob/master/packages/loot-core/src/server/crdt/merkle.ts#L17

I don't think there is a reason why key has to be padded. Especially when here it's not padded:

https://github.com/actualbudget/actual/blob/master/packages/loot-core/src/server/crdt/merkle.ts#L25

I suppose this is the correct code:

const keyToTimestamp = (key: string): Millis =>
  (parseInt(key.length > 0 ? key : "0", 3) * 1000 * 60) as Millis;

What error did you receive?

No response

Where are you hosting Actual?

None

What browsers are you seeing the problem on?

No response

Operating System

None

[jlongster]

Especially when here it's not padded

It doesn't pad it when you already have a full time because... you already know it's a fully formed time! There's nothing to pad. Base 3 of a date (in ms) is always going to be of 16 length.

Caveat: that is until Nov 5, 2051 8:20. That is the upper limit of the system currently, but there are other ways to encode/decode the time that if Actual still exists in 30 years, that can be solved. After that time, the length of a base 3 encoded time will be 17.

Your code above is not equivalent. It's assuming that the encoding of the time in the trie starts from the right, when in fact is starts from the left. That's why the padding is required: Say you walk down the trie for 3 nodes and get 102. Each node in the trie represents a "window" of time. You want to get the "time" represented from this node: you only have 102. That function will pad it with 0s to get the lower bound of time for that node: new Date(parseInt('102' + '0'.repeat(16 - 3), 3) * 1000 * 60)

Which results in May 6, 2003. This is an extreme case, but you can take any node in the trie, even if you haven't traversed all the way down to the bottom, and generate a lower bound time for it. This is useful because you know all child nodes happens after that time. That property is what we use for different kinds of checks.

Yes, there is the caveat above. We'll have to figure out something in 30 years, but it's more of a limitation than a bug.

[j-f1]

@steida Is this causing any incorrect behavior that you can see? If not I’m probably gonna close this since James seems to have a good rationale for why it is the way it is and I don’t want to touch the core syncing logic unless it has serious bugs.

[steida]

@j-f1 I will send a failing test case if I will have one.

[steida]

@jlongster Omg, I was so silly. 😂 Now I perfectly understand why the time has to be padded. Let me explain why my brain was rejecting it. With such padding, the lowest supported time is 860934420000 (1997). And that's OK, but it also means a timestamp with lower time must not be allowed to be created and stored in the Merkle tree because that would make the Merkle tree unsyncable forever because older timestamps would not be selected from DB. Just for fun, that's how Evolu defines Millis now:

export const AllowedTimeRange = {
  greaterThan: 860934419999,
  lessThan: 2582803260000,
};

export const Millis = Schema.number.pipe(
  Schema.greaterThan(AllowedTimeRange.greaterThan),
  Schema.lessThan(AllowedTimeRange.lessThan),
  Schema.brand("Millis"),
);

It looks like ActualBudget has no such check https://github.com/actualbudget/actual/blob/75f2bf8b1b1879836a72d92cfa8998883cceb0ea/packages/loot-core/src/server/sync/index.ts#L360, only clock drift in the client, but if client has time < 1997, there is no drift and invalid timestamp can be created and can destroy server Merkle tree forever.

[steida]

By the way, https://github.com/evoluhq/evolu is the Actual Budget CRDT on steroids. I plan to provide the free server for syncing (the current evolu.world is already free, but it's not ready for production yet), and it could be valuable for Actual Budget users.