Skip to content

fix: normalize domains to ensure FQDN equality#6

Merged
addaleax merged 5 commits into
mongodb-js:mainfrom
tkxkd0159:normalize-domain
Dec 4, 2025
Merged

fix: normalize domains to ensure FQDN equality#6
addaleax merged 5 commits into
mongodb-js:mainfrom
tkxkd0159:normalize-domain

Conversation

@tkxkd0159

Copy link
Copy Markdown
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.

@addaleax addaleax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@addaleax

Copy link
Copy Markdown
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

tkxkd0159 commented Nov 25, 2025

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

addaleax commented Dec 4, 2025

Copy link
Copy Markdown
Contributor

@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