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

Posts from other profiles showing in profile view timeline #1846

Closed
fishcharlie opened this issue Dec 29, 2023 · 12 comments
Closed

Posts from other profiles showing in profile view timeline #1846

fishcharlie opened this issue Dec 29, 2023 · 12 comments

Comments

@fishcharlie
Copy link
Contributor

Current Result

Profile timeline view shows posts/notes from users other than the user I'm viewing.

Expected Result

I expect when viewing a profile that it only shows me posts/notes from that specific user and posts/notes that user has reposted.

Steps to reproduce

From my perspective:

  1. Open any profile
  2. Scroll through profile timeline
  3. Observe that other posts not by the profile owner or reposted by profile owner are included in that timeline

Obviously I wouldn't be surprised if more steps took place prior to this to get damus into this state, but I can not identify what those would be.

Video Recording

nostrTimelineBug.mp4

Version

Damus:

1.6 (28) 2585a37
TestFlight

Also observed on a2cac14 (development build I was using).

iOS:

17.2.1
iPhone 15 Pro Max

Other Information

  • Obviously the Charlie Fish is my account and is currently the account I'm logged into
  • The AirportStatusBot account is a separate nostr account that I created. I have never logged into this account on damus. But I am following it from my Charlie Fish account.
@alltheseas alltheseas added the Needs recreation Issues requires concrete steps for recreation label Dec 29, 2023
@alltheseas
Copy link
Collaborator

Never seen this before. Thanks @fishcharlie

@jb55 @danieldaquino @tyiu @ericholguin have yall seen this behavior?

@danieldaquino
Copy link
Contributor

Same, I haven't seen this before either. 🤔

@fishcharlie
Copy link
Contributor Author

Notes:

  • The first time my breakpoint on Line 36 in InnerTImelineView.swift (the evs variable) only contains my posts, it doesn't include any of the posts from other timelines
  • However, when I continued my debugging session, the breakpoint on line 36 in InnerTImelineView.swift got hit again, and posts from other profiles were now included in the evs variable
  • Digging into EventHolder.swift I set breakpoints in each add method and filtered those breakpoints to only hit when the npub pubkey isn't mine
  • This generated the following stack trace when it was hit:
Screenshot 2024-01-01 at 4 55 46 PM
  • After digging into the RelayConnection (basically root of the stack trace), the self.url is wss://nostrrelay.win (which is my personal nostr relay running custom software)
    • This makes me think it might be an issue with my relay not handling filters properly
  • I'm unsure if Damus should fix this or not.
  • If it were up to me, I'd truly consider adding a guard self.pubkey == ev.pubkey else { break } right above add_event(ev) in ProfileModel.swift line 121. This would ensure that for this ProfileModel we are only handling events that match the profiles pubkey (even if the relay sends bad data).

I'd be happy to submit a patch to add that guard statement if it is determined that Damus should be defensive here and protect against relays not filtering data properly. If not we can close this issue.

I should also note that a client like https://iris.to handles this as expected (I don't see other people's posts in my profile view). I'm unsure about other clients.


TLDR:

Question for Damus maintainers: should I submit a patch to filter events that don't match your pubkey in ProfileModel.swift, to protect against relays sending bad data (not properly filtering events sent)? Or should Damus not be responsible for that?

@alltheseas
Copy link
Collaborator

Can this be exploited by a malicious relay to add notes where they shouldnt be?

cc @jb55

@fishcharlie
Copy link
Contributor Author

@alltheseas From my perspective, yes. When Damus asks for a users notes, a relay can return any list of notes it wants (not necessarily from that user that Damus asked about). And Damus will (currently) happily show them on every users profile page (even if the notes aren't associated with that user).

Technically a relay returning notes that aren't associated with that user is a NIP spec violation. But obviously bugs (and malicious actors) can exist.


Also. If anyone wants to test this. You can add wss://nostrrelay.win to your relays list in Damus, and open any profile page. Posts from myself (and my AirportStatusBot) will be included in that view. (At least until I fix the bug on my end).

@alltheseas
Copy link
Collaborator

Recreated. Your airport npub appears on @fishcakeday profile.

image

@alltheseas alltheseas added relay and removed Needs recreation Issues requires concrete steps for recreation labels Jan 2, 2024
@jb55
Copy link
Collaborator

jb55 commented Jan 2, 2024 via email

@fishcharlie
Copy link
Contributor Author

@jb55 Ok it sounds like you have a better solution than just adding a guard to check that the pubkeys match.

I will work on fixing this on my end for the relay.

@alltheseas Not sure if you want to close this issue or link it to whatever ticket @jb55 has for changing how Damus pulls notes. But that's up to you 😃.

Thanks all for the insight here.

@jb55
Copy link
Collaborator

jb55 commented Jan 2, 2024 via email

@fishcharlie
Copy link
Contributor Author

I'm ok with having the guard for now until we have switched over to this
new model.

@jb55 Awesome, I'll submit a patch hopefully today or tomorrow for that.

@alltheseas
Copy link
Collaborator

Thanks @fishcharlie

I added #1851 as a follow on long-term solution in the local relay migration

@fishcharlie
Copy link
Contributor Author

Patch sent: https://groups.google.com/a/damus.io/g/patches/c/urCE7i1-HOg

jb55 pushed a commit that referenced this issue Jan 5, 2024
Closes: #1846
Lightning-Invoice: lnbc1pjef2gupp5ffv0he47r6s6us9s2pfxy023mx8lutwlh3sq365rzgmmj6efl8nsdqqcqzpgxqrrs0fppq65gwnyvf5pn5zj5ryx9s4n7y58clk7yqsp5v7pa2ges4rgvtt0nh6lnt4cevm8n2ql9p7kqstwfp4wutf8faa8q9qyyssqwx8t9kk0m3jj6vu0kvftl3nc8zqyfl5l8ne058q5dnqyad3cqfz8vdnna5g0vy9f2ttwugc0sr20p0hsem84g8xd85ptnwgmryrf4lqqmygv34
Signed-off-by: Charlie Fish <contact@charlie.fish>
Reviewed-by: William Casarin <jb55@jb55.com>
Signed-off-by: William Casarin <jb55@jb55.com>
Changelog-Fixed: Fixed bug where sometimes notes from other profiles appear on profile pages
@jb55 jb55 closed this as completed in 4c37bfc Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants