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

Error when trying to use Storage #92

Closed
bodabaker opened this issue Mar 11, 2022 · 5 comments
Closed

Error when trying to use Storage #92

bodabaker opened this issue Mar 11, 2022 · 5 comments

Comments

@bodabaker
Copy link

bodabaker commented Mar 11, 2022

When trying to create const storage like so:

import { Storage } from 'https://cdn.skypack.dev/megajs@1'

const storage = new Storage({
  email: '[email protected]',
  password: 'pass',
  userAgent: null,
}).ready;

It gives this error:

Uncaught (in promise) RangeError: Trying to access beyond buffer length
    M https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:612
    readInt32BE https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:688
    Cn https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:3906
    a https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:4917
    login https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:4945
    k https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:3856
    promise callback*k https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:3856
    request https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:4324
    login https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:4941
    Fr https://cdn.skypack.dev/-/[email protected]/dist=es2019,mode=imports/optimized/megajs.js:4899
    <anonymous> http://127.0.0.1:5555/app.js:3
megajs.js:612:13
@qgustavor
Copy link
Owner

qgustavor commented Mar 11, 2022

You missed the àwait since .ready is a promise that returns the Storage object. But, even so, that should not throw a RangeError.

Could not reproduce this error: https://replit.com/@qgustavor/megajs-issue-92#index.ts

Edit: also could not reproduce in a browser.

@qgustavor
Copy link
Owner

From your stack trace seems the error got thrown in the password derivation function, so looks like I could not reproduce this issue because the password in the example code is not the password that's causing this issue.

Since this code is almost the same thing as tonistiigi's code, the only changes being stylistic changes, it should work pretty well... should, but looks like that is this issue.

As you can check this code depends mostly on the password length. What is the length of password which is causing this issue for you? As you can see here currently test code checks for passwords with 8 byte length and 64 byte length, so maybe you found some edge case this code is not handling well.

@bodabaker
Copy link
Author

bodabaker commented Mar 11, 2022

@qgustavor yeah that was just an example. My password is actually 10 characters so I'm gonna shorten it later and see if it works

Edit: it finally works now thanks for the help!

@qgustavor
Copy link
Owner

qgustavor commented Mar 11, 2022

Maybe it was a bad idea to make the test code test only multiples of 16 bytes. The fix is simple: instead of if (i + j < password.length) { it's just if (i + j < password.length - 3) {. I will add a test case too.

Edit: There is another issue: at the moment only integration tests are run in Deno (which use the compiled library), not unit tests (which load parts of the code from the source), so, in fact, this code is not being tested. I will add the test case anyway, but fixing the tests will be part of another issue.

@qgustavor
Copy link
Owner

qgustavor commented Mar 11, 2022

I just a noticed a non-stylistic change in the code: tonistiigi's code is a.readInt32BE(i+j, true). This second argument is a deprecated noAssert argument that was removed in Node v10.0.0. Fixing the condition is better than using this argument (well, because it works past Node 10).

[email protected] was been released. I will close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants