fix: hide access_token and refresh_token details in the web console #1789
Conversation
fschade
force-pushed
the
fix-issue-1788-log-levels
branch
from
b52e784 to
d9225e1
Compare
fschade
changed the title
| logger.debug(`User Loaded`, { | ||
| ...(user.access_token && { | ||
| 'access_token (sha256)': sha256(Buffer.from(user.access_token)).slice(0, 8) | ||
| }), | ||
| ...(user.refresh_token && { | ||
| 'refresh_token (sha256)': sha256(Buffer.from(user.refresh_token)).slice(0, 8) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Hmm I'm not sure if hashing the tokens has any value because you can't decode them. Why not print them as they are since we're only doing this in dev mode?
There was a problem hiding this comment.
console.log can be overwritten which could leak the token to the outside, also services like sentry or similar can access the data:
https://skillstuff.com/if-you-store-tokens-like-this-youre-already-vulnerable/
There was a problem hiding this comment.
Ahh I didn't know, thanks for the info!
There was a problem hiding this comment.
but why are we logging anything at all then? what benefit does a hashed token bring?
There was a problem hiding this comment.
but why are we logging anything at all then? what benefit does a hashed token bring?
I wasn't sure why we log the token either; the only case I can think of is to see that the token refresh is working, that the token is new, and possibly to compare the hash.
There was a problem hiding this comment.
IMO you can remove it entirely 🙈
There was a problem hiding this comment.
IMO you can remove it entirely 🙈
+1, what do you think, should we keep the logger, I prefer it over the native console.*
There was a problem hiding this comment.
I have the impression that it's a bit over-engineered to be honest
There was a problem hiding this comment.
I have the impression that it's a bit over-engineered to be honest
Ok, maybe … 😂, let’s keep the pr open till after my vacation, or just remove the token logs and call it a day 🤗
JammingBen
left a comment
JammingBen
left a comment
There was a problem hiding this comment.
Code LGTM, needs a rebase though to make CI happy.
3 participants
Description
Removes logging of access and refresh tokens to prevent them from potentially leaking through browser extensions, addons, or .... Replaces token logs with hashed fingerprints.
Also introduces a logger that only logs messages when the appropriate log level is set.
Related Issue
How Has This Been Tested?
Types of changes