-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
fix: use custom base64 decoder to workaround deno bug #406
Conversation
Wait does the std base64 encoder also has this issue or just atob? |
Yeah the std base64 encoder uses |
src/auth/scram.ts
Outdated
@@ -114,6 +114,17 @@ export async function executeScram( | |||
return continueScramConversation(cryptoMethod, result, authContext); | |||
} | |||
|
|||
function decodeBase64(b64: string): Uint8Array { | |||
const b64Web = b64.replace(/-/g, "+").replace(/_/g, "/"); |
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 are converting a URL Safe base64 to an actual base64?
so you issue is that you don't actually use a base64 string, but you use a url safe base64 adoptation.
base64 uses / and + by default. so the current impl is not wrong. we just expect actual base64 strings
do you require a url safe base64? / is this per mongodb driver spec?
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 think it's a really good question lucsoft, but I don't have the answers unfortunately.
I have managed to create a small script to recreate the problem:
import { MongoClient } from '../mod.ts';
for (let x = 0; x < 100000; x++) {
const client = new MongoClient();
const url = `mongodb://testadmin1:testpass1@localhost:27017?directConnection=true`
console.log('url:', url);
await client.connect(url);
await client.close()
console.log('');
}
Run that, and within a few seconds it will throw:
deno-mongo % deno run --allow-net demo/main.ts
url: mongodb://testadmin1:testpass1@localhost:27017?directConnection=true
url: mongodb://testadmin1:testpass1@localhost:27017?directConnection=true
url: mongodb://testadmin1:testpass1@localhost:27017?directConnection=true
url: mongodb://testadmin1:testpass1@localhost:27017?directConnection=true
error: Uncaught (in promise) Error: MongoError: "Connection failed: Failed to decode base64"
throw new MongoDriverError(`Connection failed: ${e.message || e}`);
^
at MongoClient.connect (file:///deno-mongo/src/client.ts:46:13)
at eventLoopTick (ext:core/01_core.js:178:11)
at async file:///deno-mongo/demo/main.ts:8:3
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'm still not sure what's going on, but if I edit the scram.ts
file
Then run my script above, I get:
url: mongodb://testadmin1:testpass1@localhost:27017?directConnection=true
parsedResponse= { v: "" }
decodeBase64 Uint8Array(0) []
url: mongodb://testadmin1:testpass1@localhost:27017?directConnection=true
parsedResponse= { v: "" }
decodeBase64 Uint8Array(0) []
url: mongodb://testadmin1:testpass1@localhost:27017?directConnection=true
parsedResponse= { v: "XwftqO5acqzP3qfL6HMwI" }
error: Uncaught (in promise) InvalidCharacterError: Failed to decode base64
const binString = atob(b64);
^
at atob (ext:deno_web/05_base64.js:28:16)
at Module.decode (https://deno.land/std@0.202.0/encoding/base64.ts:138:21)
at continueScramConversation (file://deno-mongo/src/auth/scram.ts:206:35)
at eventLoopTick (ext:core/01_core.js:178:11)
at async Cluster.authenticateToServer (file://deno-mongo/src/cluster.ts:86:7)
at async Promise.all (index 0)
at async Cluster.authenticate (file://deno-mongo/src/cluster.ts:55:23)
at async MongoClient.connect (file://deno-mongo/src/client.ts:37:7)
at async file://deno-mongo/demo/main.ts:8:3
It's strange, the ones before have a blank parsedResponse, seem to work. But when data get's returned it fails. What is that data?
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.
this is not a fix this is a feature request. is this per spec? 😄 Hope we can resolve this together :)
if you have any questions feel free to ask
Sorry, I'm happy to rebase to any commit message appropriate for this. |
Some fix? Can't use Mongo with Deno by default |
@markwylde any update on this ? I'm having troubles connecting to mongo via deno |
The deno issue still isn’t fixed, so I do think my PR is valid and works as a temporary solution. Unfortunately I’m not a collaborator on this mongo driver project so can’t push it any further. Maybe @lucsoft would be so kind to reconsider my PR? Although I understand their hesitation as ideally deno would fix the upstream bug, and then this PR becomes redundant. Either way, this library is not usable in its current state, so I think it’s better to have this workaround in, then it can be removed if demo fixes their bug. |
@markwylde im happy to merge this i just want to know if other drivers do this too. And also instead of reimpl base64url ourself we should use the deno std for this (https://jsr.io/@std/encoding/1.0.6/base64url.ts) |
i don't think deno has any issues here thats just how atob works. thats why deno std has encoding impl for base64 and base64 urls |
Sorry @lucsoft. I hadn't looked at this repo for a year. At the time of this PR we were using atob. It looks like it has since been refactored to use the std library. I have ran my script that recreated the issue on main: import { MongoClient } from '../mod.ts';
for (let x = 0; x < 100000; x++) {
const client = new MongoClient();
const url = `mongodb://testadmin1:testpass1@localhost:27017?directConnection=true`
console.log('url:', url);
await client.connect(url);
await client.close()
console.log('');
} And I can confirm I could no longer recreate the error. Therefore the initial issue seems to be fixed. |
@davidosemwegie can you create an issue on your case? can you try to use main and look it fixed your problems? |
This PR works around issue #381, a bug that appears to be in Deno:
denoland/deno#19546
It uses a custom base64 function, but maybe we could find another library out there that doesn't use the built in
atob
.