Skip to content

feat(dns-instrument): capture redirect chain hops via onHeadersReceived#1158

Merged
vringar merged 3 commits into
masterfrom
fix/dns-redirect-chain-v2
Apr 6, 2026
Merged

feat(dns-instrument): capture redirect chain hops via onHeadersReceived#1158
vringar merged 3 commits into
masterfrom
fix/dns-redirect-chain-v2

Conversation

@vringar

@vringar vringar commented Apr 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Move DNS instrumentation from onCompleted to onHeadersReceived to capture all redirect chain hops
  • Add redirect_url column to dns_responses schema (SQL, Parquet, TypeScript)
  • Do NOT modify shared PendingResponse class — DNS works independently
  • Rename onCompleteDnsHandler to onHeadersReceivedDnsHandler
  • Test verifies captured row content (URLs), not just count

Supersedes #1021. Incorporates fixes from adversarial review (VDD methodology).

VDD Review History

  • Round 1: Found PendingResponse pollution breaking HTTP instrument, non-unique request_id, count-only test
  • Round 2: Fixed PendingResponse (untouched), added redirect_url. Found parquet_schema desync, test_values missing
  • Round 3: All findings fixed. Parquet synced, test asserts content.
  • Final verdict: Begrudgingly Adequate (converged)

Test plan

  • test_dns_instrument.py verifies redirect_url content per hop
  • Schema consistency: schema.sql, parquet_schema.py, schema.ts all have redirect_url
  • pre-commit run --all-files passes

vringar added 2 commits April 5, 2026 12:44
…op tracking [CL-6]

- Remove onHeadersReceivedEventDetails from shared PendingResponse
- Add redirect_url column to dns_responses schema
- Rename onCompleteDnsHandler to onHeadersReceivedDnsHandler
Copilot AI review requested due to automatic review settings April 5, 2026 12:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the DNS instrumentation to log at webRequest.onHeadersReceived instead of onCompleted, enabling capture of DNS records across redirect chains, and synchronizes storage schemas to persist the associated URL per hop.

Changes:

  • Switch DNS logging trigger to onHeadersReceived and record the per-event URL as redirect_url.
  • Add redirect_url to the dns_responses schema across SQLite (SQL), Parquet (PyArrow), and the WebExtension TypeScript types.
  • Strengthen the DNS instrumentation test to assert that recorded rows contain URL content (not just row count).

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Extension/src/background/dns-instrument.ts Moves DNS collection to onHeadersReceived and stores redirect_url = details.url.
Extension/src/types/browser-web-request-event-details.ts Adds a typed onHeadersReceived details interface with optional ip.
Extension/src/schema.ts Adds optional redirect_url to the DnsResolved record type.
openwpm/storage/schema.sql Adds redirect_url column to dns_responses.
openwpm/storage/parquet_schema.py Adds redirect_url (and syncs used_address) for dns_responses Parquet schema.
test/test_dns_instrument.py Adds assertions around redirect_url presence/content.
test/storage/test_values.py Updates generated test values for dns_responses to include redirect_url/used_address.
.gitignore Ignores local Crosslink/Claude generated state files and directories.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +22 to +27
assert result["redirect_url"] is not None
assert "test.localhost:8000" in result["redirect_url"]

# Each redirect hop should record the URL it was associated with
redirect_urls = [r["redirect_url"] for r in results]
assert all(url is not None for url in redirect_urls)

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

Because the test indexes into results[0] from an unfiltered SELECT * FROM dns_responses (no ORDER BY), the new behavior of logging on onHeadersReceived (redirect hops + potentially additional requests) can make row ordering nondeterministic and the assertions flaky. Consider filtering the query to the expected request/hostname and/or adding an explicit ORDER BY (e.g., id or time_stamp) before selecting the row(s) to assert against.

Copilot uses AI. Check for mistakes.
@codecov

codecov Bot commented Apr 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.16%. Comparing base (58218c7) to head (087f858).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1158   +/-   ##
=======================================
  Coverage   62.16%   62.16%           
=======================================
  Files          40       40           
  Lines        3898     3898           
=======================================
  Hits         2423     2423           
  Misses       1475     1475           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Adds a /CONNECTION_ABORT/ handler to the test server that sends headers
then forcibly closes the connection, and a test proving DNS data is still
captured. This guards against reverting from onHeadersReceived to
onCompleted, which would miss aborted requests.
@vringar vringar added this pull request to the merge queue Apr 6, 2026
Merged via the queue into master with commit a088fd3 Apr 6, 2026
14 checks passed
@vringar vringar deleted the fix/dns-redirect-chain-v2 branch April 6, 2026 00:46
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