Skip to content

Remove external DNS from the WebFinger unit test - #1275

Merged
dahlia merged 1 commit into
fedify-dev:2.0-maintenancefrom
dahlia:bugfix/webfinger-test-dns
Oct 9, 2026
Merged

dahlia merged 1 commit into
fedify-dev:2.0-maintenancefrom
dahlia:bugfix/webfinger-test-dns

Conversation

@dahlia

@dahlia dahlia commented Oct 8, 2026

Copy link
Copy Markdown
Member

The test in packages/cli/src/webfinger/mod.test.ts mocks HTTP responses but still resolves real hostnames through the SSRF guard, allowing DNS latency to exhaust Bun's timeout. Pass allowPrivateAddress: true for these mocked lookups so the existing alias assertions run without external DNS.

A temporary DNS-blocking harness failed before the patch and passed with zero DNS calls afterward. mise run test-each cli passed all 78 tests on each of Deno, Node.js, and Bun. The dedicated SSRF tests also passed.

Fixes #1273.

The WebFinger unit test mocked HTTP responses but still resolved the
public hostnames through the SSRF guard.  Skip that guard for these
mocked lookups so external DNS latency cannot exhaust Bun's timeout.
Retain both alias assertions and fetch restoration.

A temporary DNS-failure harness fails before the patch and passes with
zero DNS calls afterward.  The CLI suite passes on Deno, Node.js, and
Bun.  Note the option rename needed when merging into 2.4 or later.

Fixes fedify-dev#1273

Changelog: none
Assisted-by: Codex:gpt-6.1-sol
Assisted-by: Claude Code:claude-opus-5-5
@dahlia dahlia self-assigned this Oct 8, 2026
@dahlia dahlia added component/cli CLI tools related runtime/bun Bun runtime related component/ci CI/CD workflows and GitHub Actions labels Oct 8, 2026
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e8bf40ac-45cd-4f24-83c7-3fa329b33d32
📥 Commits

Reviewing files that changed from the base of the PR and between 122e896 and 0bece1f.

📒 Files selected for processing (1)
  • packages/cli/src/webfinger/mod.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The lookupSingleWebFinger test now passes allowPrivateAddress: true for each resource. Comments explain that the mocked HTTP responses do not prevent the SSRF guard from performing external DNS lookups and note a TODO for a later merge target.

Changes

WebFinger test

Layer / File(s) Summary
Mocked lookup setup
packages/cli/src/webfinger/mod.test.ts
Each lookupSingleWebFinger call now passes allowPrivateAddress: true. Comments document the external DNS lookup and a TODO to use allowPrivateAddresses for the specified merge target.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 0bece

This test-only change avoids DNS validation for mocked lookups while leaving production validation and its dedicated tests intact. No outstanding merge risk is indicated.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: removing external DNS activity from the WebFinger unit test.
Description check Passed The description directly explains the test failure, the allowPrivateAddress: true fix, validation results, and linked issue.
Linked Issues check Passed Issue #1273 requires the WebFinger unit test to avoid external DNS and HTTP services while retaining alias assertions and SSRF coverage in dedicated tests. The PR changes both mocked lookups in `packa…
Out of Scope Changes check Passed The supplied change summary shows a focused four-line addition and one-line removal in the WebFinger test. The change directly supports issue #1273 by disabling DNS-dependent URL validation for mocked…
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-08T23:41:18.413933Z 0bece1f PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@codecov

codecov Bot commented Oct 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dahlia
dahlia merged commit c64d2e1 into fedify-dev:2.0-maintenance Oct 9, 2026
17 checks passed
@dahlia
dahlia deleted the bugfix/webfinger-test-dns branch October 9, 2026 02:42
@dahlia dahlia linked an issue Oct 9, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/ci CI/CD workflows and GitHub Actions component/cli CLI tools related runtime/bun Bun runtime related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove external DNS dependency from the CLI WebFinger test

1 participant