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

Backslashes in a password need to be escaped #760

Merged
merged 3 commits into from
May 7, 2024

Conversation

stevenpost
Copy link
Contributor

This will replace a single backslash with a double backslash in the /root/.mongoshrc.js file. when a password with a backslash is used, it is correctly passed on to the provider for setting the user's password, but things break when attempting to use said password for the admin user.

A small explanation on the amount of backslashes: The first argument is a regular expression, so we need to escape the backslash. The second argument allows for references to capture groups or the entire match using backslashes, for example \0 contains the entire match. This would make us end up with 4 backslashes, but apparantly the template rendering also has backslash escaping, this we need to double the amount of backslashes. So 8 in total.

This will replace a single backslash with a double backslash in the
`/root/.mongoshrc.js` file. when a password with a backslash is used, it
is correctly passed on to the provider for setting the user's password,
but things break when attempting to use said password for the admin
user.

A small explanation on the amount of backslashes: The first argument is
a regular expression, so we need to escape the backslash. The second
argument allows for references to capture groups or the entire match
using backslashes, for example `\0` contains the entire match.  This
would make us end up with 4 backslashes, but apparantly the template
rendering also has backslash escaping, this we need to double the amount
of backslashes. So 8 in total.
@smortex
Copy link
Member

smortex commented May 3, 2024

You mean that if I want to use the password foo\bar I must set the password to foo\\bar in the configuration? If so, this should probably better be reported and fixed upsteam, no?

@h-haaks
Copy link
Contributor

h-haaks commented May 3, 2024

You mean that if I want to use the password foo\bar I must set the password to foo\\bar in the configuration? If so, this should probably better be reported and fixed upsteam, no?

mongoshrc.js is not provided at all by upstream.
It is a pr user file that can be used to alter the interactive mongosh prompt.

But this modules manages the file to use it (and store the password) in the provider implementations ...
Thats also why the file is managed by the server class ..

@h-haaks
Copy link
Contributor

h-haaks commented May 3, 2024

I think we should use a password with \ in the integration tests as well to verify that this is really working.

@stevenpost
Copy link
Contributor Author

You mean that if I want to use the password foo\bar I must set the password to foo\\bar in the configuration?

Yes, that is correct. Most passwords at our site are autogenerated, and I happened to encounter one with a backslash.

I think we should use a password with \ in the integration tests as well to verify that this is really working.

Agreed, I'll add a test case.

This test also lets the server class create the admin user, this
abbreviates the test code a bit and increases the coverage of the
acceptance test.

Note that the explicit ordering of client and server was dropped.
@h-haaks
Copy link
Contributor

h-haaks commented May 6, 2024

Hmm, looking at this a second time it struck me that if we need to escape \ there probably is more special characters that need escaping ...
Quick google I found https://www.javatpoint.com/javascript-special-characters
Guess at least ', ", & should be escaped.

@stevenpost
Copy link
Contributor Author

Since we are already in a single quoted string, I think only ' needs additional escaping. I'll add them all to the test, to be sure :)

This mainly pertains to single quotes, but the test includes others as
well.
@stevenpost
Copy link
Contributor Author

Can we release this as a fix release? Something like 6.0.1?

@h-haaks h-haaks merged commit d1f5c38 into voxpupuli:master May 7, 2024
88 checks passed
@h-haaks h-haaks added the bug Something isn't working label May 7, 2024
@stevenpost stevenpost deleted the admin_password/fix branch May 7, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants