Skip to content

Hide future notes from timeline #2967

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

Merged
merged 1 commit into from
Apr 18, 2025

Conversation

tyiu
Copy link
Collaborator

@tyiu tyiu commented Apr 8, 2025

Summary

The created_at timestamp can be any value in the future as it can not be verified to be correct / the actual time the author posted. If someone does this, this note gets pinned at the top of the timeline in perpetuity until the system clock surpasses that timestamp and other notes get pulled in. This could be considered a malicious attack (or other users have a clock skew problem).

This change defensively filters out future notes from the timeline until the system clock has actually surpassed the created_at timestamp.

Closes: #2949
Related: damus-io/notepush#17

Checklist

  • I have read (or I am familiar with) the Contribution Guidelines
  • I have tested the changes in this PR
  • I have opened or referred to an existing github issue related to this change.
  • My PR is either small, or I have split it into smaller logical commits that are easier to review
  • I have added the signoff line to all my commits. See Signing off your work
  • I have added appropriate changelog entries for the changes in this PR. See Adding changelog entries
  • I have added appropriate Closes: or Fixes: tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patches

Test report

Device: iPhone 16 Pro Simulator

iOS: 18.4

Damus: 2f875ab

Setup: Settings > General > Date & Time > Toggle off Set Automatically > Set current time to 2 or more minutes into the future (or whatever amount of time you need to run through the test steps)

Steps:

  1. Build and run Damus on the master branch.
  2. Follow the setup steps above to change the system clock.
  3. Switch to Damus and post a note.
  4. Reverse the setup steps to change the system clock back to be set automatically.
  5. Switch to Damus and observe that the note is pinned at the top of the timeline (home, profile, etc)
  6. Observe that the note is pinned at the top of the timeline.
  7. Tap on the note to verify that the timestamp is indeed the future.
  8. Checkout the HEAD of this branch, and build and run Damus.
  9. Observe that the note from (3) is no longer shown on the timeline.
  10. Wait until the system clock naturally passes that timestamp.
  11. When a new note comes in or if you refresh the timeline (by switching away and coming back), the note from (3) will appear.

Results:

  • PASS

@alltheseas
Copy link
Collaborator

Thank you Terry

@alltheseas alltheseas requested a review from danieldaquino April 8, 2025 15:46
@tyiu
Copy link
Collaborator Author

tyiu commented Apr 11, 2025

I may need to do some more work to

  1. Also filter future notes from notifications, and
  2. Add some margin for error (perhaps up to 1 second or so) in the timestamp in case there's a natural clock skew between the sending client and the receiving client. ChatGPT says expect "<10 ms" for NTP synced devices.

@tyiu tyiu marked this pull request as draft April 11, 2025 16:36
@tyiu tyiu force-pushed the hide-future-notes branch from 4fafe4b to 3d772fb Compare April 11, 2025 18:35
Comment on lines 30 to 48
if let item = item.filter({ ev in
self.fine_filter.filter(contacts: contacts, pubkey: ev.pubkey) &&
// Allow notes that are created no more than 1 second in the future
// to account for natural clock skew between sender and receiver.
ev.age >= -1
}) {
Copy link
Collaborator Author

@tyiu tyiu Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@tyiu
Copy link
Collaborator Author

tyiu commented Apr 11, 2025

I may need to do some more work to

1. Also filter future notes from notifications, and

2. Add some margin for error (perhaps up to 1 second or so) in the timestamp in case there's a natural clock skew between the sending client and the receiving client. ChatGPT says expect "<10 ms" for NTP synced devices.

This is done.

@tyiu tyiu marked this pull request as ready for review April 11, 2025 18:37
@tyiu
Copy link
Collaborator Author

tyiu commented Apr 11, 2025

This PR is ready for review. Also please see the corresponding notepush PR.

Do not merge until #2946 is merged as it is more important and due to merge conflicts.

@tyiu tyiu force-pushed the hide-future-notes branch from 3d772fb to 2f875ab Compare April 12, 2025 11:55
Copy link
Collaborator

@danieldaquino danieldaquino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you @tyiu!

@danieldaquino
Copy link
Collaborator

Waiting on #2946 to be merged in order to merge this PR, as per @tyiu's request

Changelog-Fixed: Hide future notes from timeline

Closes: damus-io#2949
Signed-off-by: Terry Yiu <[email protected]>
@danieldaquino danieldaquino merged commit 33150a4 into damus-io:master Apr 18, 2025
@danieldaquino
Copy link
Collaborator

Thanks @tyiu! Resolved conflicts and merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected behavior: notes appear perpetually top of feed and stay there (filter future notes)
3 participants