Skip to content

Conversation

@tkxkd0159
Copy link
Contributor

mongosh uses @mongodb-js/devtools-connect, and @mongodb-js/devtools-connect uses resolve-mongodb-srv. In the current implementation, if you explicitly indicate the FQDN by adding ".", it will be recognized as a different domain. So we need to fix this.

Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, although I imagine we'd more generally like to adopt the current code in https://github.com/mongodb/node-mongodb-native/blob/761b9bfab8dfc8e3e7e311731d7a5cda1285bc6c/src/utils.ts#L1129-L1159

@addaleax
Copy link
Contributor

@tkxkd0159 Can you look at the linting failures? I think those would be the hard blockers here. Otherwise somebody else can also push updates to this branch

@tkxkd0159
Copy link
Contributor Author

tkxkd0159 commented Nov 25, 2025

This looks good, although I imagine we'd more generally like to adopt the current code in https://github.com/mongodb/node-mongodb-native/blob/761b9bfab8dfc8e3e7e311731d7a5cda1285bc6c/src/utils.ts#L1129-L1159

@addaleax I initially left this logic out and used a lightweight workaround instead. But I agree that it's good to add this logic to prevent short domain DNS spoofing attack. I’ll update the PR accordingly. And there is no await to the asynchronous assertions, so I will add that as well. Thanks for the review!

@tkxkd0159 tkxkd0159 requested a review from addaleax November 25, 2025 08:46
@tkxkd0159
Copy link
Contributor Author

@addaleax Whenever you have some downtime, I’d appreciate a review on this :)

@addaleax addaleax merged commit 5982169 into mongodb-js:main Dec 4, 2025
8 of 10 checks passed
@addaleax
Copy link
Contributor

addaleax commented Dec 4, 2025

@tkxkd0159 Sorry for the delay, this has been merged and published now! ✨

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.

2 participants