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

CSHARP-5471: Incorrectly resolving the authentication mechanism param… #1605

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

papafe
Copy link
Contributor

@papafe papafe commented Jan 29, 2025

…eters from SRV connection string

@papafe papafe marked this pull request as ready for review January 29, 2025 13:22
@papafe papafe requested a review from a team as a code owner January 29, 2025 13:22
@papafe papafe requested review from JamesKovacs and sanych-sun and removed request for a team and JamesKovacs January 29, 2025 13:22
@papafe
Copy link
Contributor Author

papafe commented Jan 29, 2025

Hey @sanych-sun, I've tagged you since it seems you've worked on this recently.
A couple of notes:

  • Not escaping the extra options while resolving the connection string seems to solve the issue.
  • This means that some tests were failing due to the fact that the stringified connection string after resolving is does not have escaped values. This seems to be a minor issue, as we still parse the string correctly internally.
  • I could not find anything in the specs about how should a resolved string look like, and even though Resolve is part of the public API, this could be seen as a bug fix.
  • This fix solves two different cases:
    1. authMechanismProperties=TOKEN_RESOURCE:mongodb%3A%2F%2Ftest-cluster, where the option is already escaped, but does not contain encoded commas
    2. authMechanismProperties=prop:ab%2Ccd%2Cef%2Cjh, where the option is already escaped and contains encoded commas

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

Successfully merging this pull request may close these issues.

1 participant