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

fix: SASL's signature consists of the string representation of the payload, not base64 #2529

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

marcj
Copy link
Contributor

@marcj marcj commented Aug 25, 2020

payload.value() already returns a string, so calling on it toString('base64')
implies the signature consists of a base64 representation, but the plain string is required.
The first argument ('base64') of String.toString is dropped, so although the code works
as expected (by accident) it implies that it was desired to have a base64 there, which is wrong.

Dropped the toString() call to not confuse driver authors when they want to get inspiration
from this JS driver.

@Alexander-L-G
Copy link
Contributor

@Alexander-L-G Alexander-L-G added the tracked-in-jira Ticket filed in MongoDB's Jira system label Nov 16, 2020
@nbbeeken
Copy link
Contributor

Hi @marcj thanks for your contribution, it looks good to me and I'll be happy to merge but there's a some linter errors, you can fix them with:

npm run check:lint -- --fix

@nbbeeken
Copy link
Contributor

nbbeeken commented Dec 9, 2020

Hi @marcj if you get the chance to run the linter command I can go ahead and merge this, thanks again for contributing!

@marcj
Copy link
Contributor Author

marcj commented Dec 9, 2020

@nbbeeken I get ✖ 258 problems (0 errors, 258 warnings). Which linter errors do you mean? I actually changed only one line, which I don't think generates any error.

@nbbeeken
Copy link
Contributor

nbbeeken commented Dec 9, 2020

I'm getting this error:

src/cmap/auth/scram.ts
184:24  error  Replace `⏎····clientFirstMessageBare(username,·nonce),⏎····payload.value(),⏎····withoutProof⏎··].join(','` with `clientFirstMessageBare(username,·nonce),·payload.value(),·withoutProof].join(⏎····','⏎··`  prettier/prettier
✖ 259 problems (1 error, 258 warnings)

🤔 Not sure why the prettier errors wouldn't show up on your end, did your editor auto format and the changes are still local?

…yload, not base64

`payload.value()` already returns a string, so calling on it `toString('base64')`
implies the signature consists of a base64 representation, but the plain string is required.
The first argument ('base64') of String.toString is dropped, so although the code works
as expected (by accident) it implies that it was desired to have a base64 there, which is wrong.

Dropped the toString() call to not confuse driver authors when they want to get inspiration
from this JS driver.
@marcj
Copy link
Contributor Author

marcj commented Dec 9, 2020

Oh, no it's because of --fix. It just changes the line without complaining afterwards. After the command I have the change locally. Just pushed the modified commit.

@nbbeeken nbbeeken merged commit e7d2693 into mongodb:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracked-in-jira Ticket filed in MongoDB's Jira system
Projects
None yet
3 participants