github-starred/actual
2
0
Fork 0
mirror of https://github.com/actualbudget/actual.git synced 2026-07-16 07:04:23 -05:00
Code Issues 3.0k Packages Projects Releases 58 Wiki Activity

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

New Issue
Closed
opened 2026-02-28 19:02:38 -06:00 by GiteaMirror · 5 comments
GiteaMirror commented 2026-02-28 19:02:38 -06:00
Owner
Copy Link

Originally created by @steida on GitHub (May 16, 2023).

Verified issue does not already exist?

  • 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

Originally created by @steida on GitHub (May 16, 2023). ### 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: ```ts 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
GiteaMirror added the bugtransactions labels 2026-02-28 19:02:38 -06:00
GiteaMirror commented 2026-02-28 19:02:38 -06:00
Author
Owner
Copy Link

@jlongster commented on GitHub (May 16, 2023):

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.

@jlongster commented on GitHub (May 16, 2023): > 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.
GiteaMirror commented 2026-02-28 19:02:39 -06:00
Author
Owner
Copy Link

@j-f1 commented on GitHub (May 16, 2023):

@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.

@j-f1 commented on GitHub (May 16, 2023): @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.
GiteaMirror commented 2026-02-28 19:02:39 -06:00
Author
Owner
Copy Link

@steida commented on GitHub (May 17, 2023):

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

@steida commented on GitHub (May 17, 2023): @j-f1 I will send a failing test case if I will have one.
GiteaMirror commented 2026-02-28 19:02:39 -06:00
Author
Owner
Copy Link

@steida commented on GitHub (Oct 20, 2023):

@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 75f2bf8b1b/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 commented on GitHub (Oct 20, 2023): @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: ```ts 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.
GiteaMirror commented 2026-02-28 19:02:40 -06:00
Author
Owner
Copy Link

@steida commented on GitHub (May 6, 2024):

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.

@steida commented on GitHub (May 6, 2024): 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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
AI generated
API
bank sync
budgeting
bug
can’t replicate
dependencies
docker
documentation
electron
experimental feature
feature
feedback
goal templates
good first issue
help wanted
importers
maintenance
needs info
needs testing
needs triage
needs votes
openid
payees
pull-request
Mirrored from GitHub Pull Request
 regression
reports
responsive
rules
schedules
server
✨ merged
split transactions
tech debt
tech-support
theme
transaction import
transaction reconciliation
transactions
translations
upstream
user interface
✅ approved
wontfix
No Label bug transactions
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/actual#404
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?