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.

Sign up for GitHub

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

Jump to bottom

user:id in sentry does not correspond to user in psql db (because bigint -> text -> int casting overflow) #2040

Closed
2 tasks done
gobengo opened this issue Oct 17, 2022 · 1 comment
Closed
2 tasks done

user:id in sentry does not correspond to user in psql db (because bigint -> text -> int casting overflow) #2040

gobengo opened this issue Oct 17, 2022 · 1 comment
Assignees
@gobengo
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization question Further information is requested

Comments

@gobengo
Copy link
Contributor

gobengo commented Oct 17, 2022 •
edited

Logging user.id (number) does not always correspond to psql user id (bigint).

I noticed in sentry there was an error with user:id 315318734258499200. I queried the db for that user and got no results. Then I noticed the last two digits of that user:id were 00, a hint there might be an overflow.

It looks like it's because of these parseInt:

I introduced them :/ when I added tsc typechecking and it complained about passing a string user.id to Logging#setUser

Plan

  • change setUser to expect a string user id instead of a number
  • remove the corresponding parseInts that lead to loss of useful user id
@gobengo gobengo added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Oct 17, 2022
@gobengo gobengo self-assigned this Oct 17, 2022
This was referenced Oct 17, 2022
Merged
Merged
gobengo added a commit that referenced this issue Oct 18, 2022
@gobengo
fix: issue with logging user being set to different value than underl…
9659ab0 
…ying user.id bigint::text (#2042)

Motivation:
* #2040
gobengo added a commit to nftstorage/nft.storage that referenced this issue Oct 18, 2022
@gobengo
chore: api Logging#setUser expects user.id as string instead of number (
109462f 
#2233)

Motivation:
* this way `Logging#setUser` can accomodate user ids that correspond to
bigints
* avoid this mistake
web3-storage/web3.storage#2040

Related
* I noticed that the db-types user definition has `id: number`. I wonder
if it should change to `bigint` or `text` (and underlying queries will
need to change).
https://github.com/nftstorage/nft.storage/blob/6fcd38923506965012505473b57afde2ce22973e/packages/api/src/utils/db-types.d.ts#L1188
* nft.storage prod counter isn't at risk of bumping into this soon, but
it is a bigint underneath so it could happen.
@JeffLowe JeffLowe added the question Further information is requested label Nov 22, 2022
@gobengo
Copy link
Contributor Author

gobengo commented Apr 14, 2023

this was done some time ago

@gobengo gobengo closed this as completed Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gobengo gobengo

Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization question Further information is requested
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@gobengo @JeffLowe