DoH with HTTPS, fix response handling - #22372
Closed
icing wants to merge 4 commits into
Closed
Conversation
bagder
approved these changes
Jul 23, 2026
There was a problem hiding this comment.
Pull request overview
Fixes DoH resolution handling when the query set includes HTTPS records (including the HTTPS-only case), and adds coverage for processing an HTTPS-RR resolve even when the actual answer is invalid. The PR also refactors tracing by introducing a dedicated DoH trace feature and updating related trace output and documentation.
Changes:
- Add test 2117 to validate HTTPS-RR processing behavior with DoH under ECH/HTTPSRR-enabled configurations.
- Adjust DoH result handling and tracing to better support non-address queries and HTTPS-RR decode flow.
- Introduce a new
Curl_trc_feat_dohtrace feature and documentcurl_global_trace’sdohoption.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/data/test2117 | Adds a new DoH + HTTPS-RR focused regression test (with ECH) validating processing of HTTPS RR resolves. |
| tests/data/Makefile.am | Registers the new test2117 in the test suite list. |
| lib/httpsrr.c | Updates HTTPS RR trace message prefixes/wording. |
| lib/doh.c | Refines DoH probe/consume logic for HTTPS-RR cases and adjusts verbosity/tracing behavior. |
| lib/curl_trc.h | Declares the new DoH trace feature (guarded by !CURL_DISABLE_DOH). |
| lib/curl_trc.c | Defines/registers the new DoH trace feature in the trace feature table. |
| docs/libcurl/curl_global_trace.md | Documents doh tracing, but currently ends up with duplicate/conflicting doh sections. |
Comments suppressed due to low confidence (2)
lib/doh.c:371
- The DoH probe's
CURLOPT_VERBOSEis now only enabled whenCurl_trc_feat_dohis verbose. Becausecurl_global_trace("doh")is currently aliased todnsinCurl_trc_opt(), this condition will remain false in practice and the probe handle loses verbose output compared todnstracing. Consider accepting eitherdohordnsverbosity here (or fix the option parsing).
if(data->set.err && data->set.err != stderr)
ERROR_CHECK_SETOPT(CURLOPT_STDERR, data->set.err);
if(Curl_trc_ft_is_verbose(data, &Curl_trc_feat_doh))
ERROR_CHECK_SETOPT(CURLOPT_VERBOSE, 1L);
lib/doh.c:1255
doh_show()output is now gated solely onCurl_trc_feat_doh. Givencurl_global_trace("doh")is currently parsed as an alias fordns, this prevents the detailed DoH response dump from showing up under DNS tracing. Either makedohindependently enableable, or also respectdnsverbosity for this debug output.
if(Curl_trc_ft_is_verbose(data, &Curl_trc_feat_doh)) {
CURL_TRC_DNS(data, "hostname: %s", dohp->host);
doh_show(data, &de);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix handling of DoH response that only asks for HTTPS records. Add test 2117 for checking that a HTTPS-RR resolve is processed, even though the actual answer is invalid.
- reverse handling of `data->sub_xfer_done` to be called from the `sub` transfer. This allows better management of 1-n relationships. In DoH, a transfer can be the master of an arbitrary number of DoH requests. - clarify tracing of doh/dns features
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix handling of DoH response that only asks for HTTPS records.
Add test 2117 for checking that a HTTPS-RR resolve is processed, even though the actual answer is invalid.