-
Notifications
You must be signed in to change notification settings - Fork 23
Conversation
@@ -32,7 +32,7 @@ async function sleepSeconds(seconds) { | |||
|
|||
// Caches the password. | |||
function cachePassword(password) { | |||
devicePassword = password; | |||
devicePassword = Buffer.from(password, "base64").toString("utf-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do the decoding once as soon as the route is hit instead of everywhere it's used in the logic modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried yesterday and didn't succeed (maybe I've missed something)
logic/auth.js
Outdated
@@ -195,7 +198,7 @@ async function seed(user) { | |||
try { | |||
const { seed } = await diskLogic.readUserFile(); | |||
|
|||
const decryptedSeed = await iocane.createSession().decrypt(seed, user.plainTextPassword); | |||
const decryptedSeed = await iocane.createSession().decrypt(seed, Buffer.from(user.plainTextPassword, "base64").toString("utf-8")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this? Looks like user.plainTextPassword
is already decoded in the change above so this may not be needed.
If it is needed then it's confusing to have a variable called plainTextPassword
when the value is not plain text but is Base64 encoded. It should be base64EncodedPassword
.
But if we can decode as soon as the route is hit then this change isn't needed at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I've tested it and had no errors but you're right its not normal I will check it now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were clearly right I've just patched it in fc5d2ca
Closed by #78 |
Linked to getumbrel/umbrel-dashboard#297
Linked to getumbrel/umbrel-middleware#79