Skip to content

Conversation

@Dhravya
Copy link
Contributor

@Dhravya Dhravya commented Oct 3, 2022

No description provided.

@Dhravya Dhravya marked this pull request as ready for review October 4, 2022 05:53
@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 4, 2022

Hey @MattIPv4 can you please review this and let me know if any changes need to be made? I was unable to test it

Copy link
Owner

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

Thank you for the work so far, looking good but you're missing the actual logic changes needed within the DNS lookup to set the flag.

I recommend following the steps in https://github.com/MattIPv4/DNS-over-Discord#development so that you can test this functionality is actually behaving as you'd expect locally.

(For testing, dnssec-failed.org should result in a servfail normally, but should return A records when the cdflag is set.)

Add separate capture group

Co-authored-by: Matt Cowley <[email protected]>
@Dhravya Dhravya marked this pull request as draft October 4, 2022 11:20
@MattIPv4
Copy link
Owner

MattIPv4 commented Oct 5, 2022

👋 I'm not sure what exactly your last commit did, but you appear to have caused a diff across every single file in the repo now? Have you changed the line endings on everything perhaps?

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

That's weird - I've not changed anything manually, except for the dns.js file

@MattIPv4
Copy link
Owner

MattIPv4 commented Oct 5, 2022

If you're using a Windows system, make sure git is configured to use lf endings, not crlf endings. It should warn you about this though when you staged/committed this massive change.

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

Oh - I got it now. Any way to undo it?

@MattIPv4
Copy link
Owner

MattIPv4 commented Oct 5, 2022

If you've updated git to use lf endings, I'd expect it'd now let you commit again to revert it all -- or you could just drop the last commit and redo the change.

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

HELL YES! Done
image

@Dhravya Dhravya marked this pull request as ready for review October 5, 2022 12:48
@Dhravya Dhravya requested a review from MattIPv4 October 5, 2022 12:49
@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

Ok I was testing if everything works fine and I noticed a mistake i made

So I followed the cloudflare docs (https://developers.cloudflare.com/1.1.1.1/encryption/dns-over-https/) and added the cd bit to the JSON and DNS lookup snippets, but this causes an error when using google's DNS lookup (8.8.8.8)

SyntaxError: Unexpected token '<', "<!DOCTYPE "... is not valid JSON
    at async performLookup (worker.js:10086:42)
    at async Promise.all (index 0)
    at async handleDig (worker.js:14844:21)
    at async worker.js:17271:35 at line 0, col 0

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

Wait.... it should still work with 8.8.8.8, that's odd
image

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

dns.google is returning Error 400 (Bad Request)!!

@MattIPv4
Copy link
Owner

MattIPv4 commented Oct 5, 2022

Before I can start reviewing this, you're going to need to revert the line ending changes that you've made to every file here

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

Ok! I'll revert the commit and do the same changes again

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

Done! @MattIPv4

Dhravya and others added 3 commits October 5, 2022 19:37
Co-authored-by: Matt Cowley <[email protected]>
Co-authored-by: Matt Cowley <[email protected]>
Co-authored-by: Matt Cowley <[email protected]>
@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 5, 2022

Hey @MattIPv4 I think all the changes are done (soo sorry for all the trouble)

Copy link
Owner

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

There are a few eslint errors that need resolving

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 6, 2022

Fixed eslint errors

@MattIPv4 MattIPv4 changed the title cdflag option Allow disabling DNSSEC with cdflag Oct 6, 2022
Dhravya and others added 7 commits October 6, 2022 17:29
@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 6, 2022

Made the changes

Co-authored-by: Matt Cowley <[email protected]>
Copy link
Owner

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for working on this and seeing it through 💙

@Dhravya
Copy link
Contributor Author

Dhravya commented Oct 6, 2022

cool, thanks a lot! I genuinely learnt so much through this one PR and I definitely look forward to contributing more to this repository!

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