Make recursive lookups honor -p/--allow-private-address#718
Make recursive lookups honor -p/--allow-private-address#718dahlia wants to merge 7 commits intofedify-dev:mainfrom
-p/--allow-private-address#718Conversation
Make `fedify lookup --recurse` honor the same explicit `-p`/`--allow-private-address` opt-in that `--traverse` already uses for discovered URLs. Update the recursive lookup failure hints to use Optique option markup, refresh the CLI docs and changelog, and add regression tests covering both the default blocked behavior and the opt-in path using `srvx`. Fixes fedify-dev#700 Assisted-by: Codex:gpt-5.4
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMake Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User / CLI"
participant Lookup as "fedify lookup"
participant Validator as "URL Validator"
participant Remote as "Remote HTTP Server"
User->>Lookup: run lookup --recurse [--allow-private-address]
Lookup->>Remote: fetch root URL
Remote-->>Lookup: root object (references discovered URL)
Lookup->>Validator: validate discovered URL (public/private)
alt discovered URL is public OR -p set
Validator-->>Lookup: OK
Lookup->>Remote: fetch discovered URL
Remote-->>Lookup: discovered object
Lookup-->>User: success, output objects
else discovered URL is private AND -p not set
Validator-->>Lookup: private/blocked
Lookup-->>User: error (recursive-private-address hint)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
issues-auto-labeler
Bot
added
component/cli
dahlia
added
type/enhancement
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/lookup.ts (1)
917-997:⚠️ Potential issue | 🟠 MajorKeep recursive JSON-LD context fetches outside the
-pscope.
recursiveContextLoader = contextLoadermakes--allow-private-addressapply to indirect@contextURLs while parsing recursively fetched objects. The PR scope says only follow-up object fetches should honor the opt-in, not JSON-LD context fetches, so use a strict recursive context loader here while keepingrecursiveLookupDocumentLoadercommand-configured.Suggested fix
const initialLookupDocumentLoader: DocumentLoader = initialAuthLoader ?? initialDocumentLoader; const recursiveLookupDocumentLoader: DocumentLoader = authLoader ?? documentLoader; - const recursiveContextLoader = contextLoader; + const recursiveBaseContextLoader = await getContextLoader({ + userAgent: command.userAgent, + allowPrivateAddress: false, + }); + const recursiveContextLoader = wrapDocumentLoaderWithTimeout( + recursiveBaseContextLoader, + command.timeout, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lookup.ts` around lines 917 - 997, The code sets recursiveContextLoader = contextLoader which causes the --allow-private-address/-p option to apply to recursive JSON-LD `@context` fetches; change this so recursive context fetches use the strict/non-private-aware loader instead of the command-configured contextLoader: create/use a strictContextLoader (or reuse the original non-privileged loader you have for initial parsing) and pass that as recursiveContextLoader when calling collectRecursiveObjects while keeping recursiveLookupDocumentLoader as the command-configured document loader; update the reference to recursiveContextLoader in the collectRecursiveObjects call to use this strict loader.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGES.md`:
- Line 124: The CHANGES.md bug-fix entry currently lists only [[`#700`], [`#718`]];
update that entry to include the contributor attribution in the same format used
elsewhere (e.g., add the maintainer/author token such as [`@your-github-handle`]
alongside the issue/PR markers), editing the exact token sequence [[`#700`],
[`#718`]] to read [[`#700`], [`#718`], [`@contributor`]] so the changelog includes the
author attribution.
---
Outside diff comments:
In `@packages/cli/src/lookup.ts`:
- Around line 917-997: The code sets recursiveContextLoader = contextLoader
which causes the --allow-private-address/-p option to apply to recursive JSON-LD
`@context` fetches; change this so recursive context fetches use the
strict/non-private-aware loader instead of the command-configured contextLoader:
create/use a strictContextLoader (or reuse the original non-privileged loader
you have for initial parsing) and pass that as recursiveContextLoader when
calling collectRecursiveObjects while keeping recursiveLookupDocumentLoader as
the command-configured document loader; update the reference to
recursiveContextLoader in the collectRecursiveObjects call to use this strict
loader.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93110527-6fa8-4b24-abd5-a3ee6d948535
📒 Files selected for processing (4)
CHANGES.mddocs/cli.mdpackages/cli/src/lookup.test.tspackages/cli/src/lookup.ts
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
This PR aligns fedify lookup’s SSRF/private-address policy across discovery modes by making --recurse honor the same explicit -p/--allow-private-address opt-in that --traverse already uses, while keeping the default behavior strict.
Changes:
- Updated recursive lookup to reuse the “discovered URL” document/context loaders configured with
allowPrivateAddressrather than hard-denying private targets. - Improved CLI failure hints to consistently suggest
-p/--allow-private-address(and-S/--suppress-errorswhere relevant) using Optique option-name markup. - Added regression tests covering default denial vs explicit opt-in for recursive fetches, plus updated docs and changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/cli/src/lookup.ts | Makes recursive follow-up fetches use loaders configured by --allow-private-address and updates related user-facing hints. |
| packages/cli/src/lookup.test.ts | Adds srvx-backed regression tests verifying recursive private targets are blocked by default and allowed when explicitly opted in. |
| docs/cli.md | Updates CLI documentation to reflect the new --recurse behavior with -p/--allow-private-address. |
| CHANGES.md | Adds an entry documenting the behavior change and links to the issue/PR. |
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the fedify lookup --recurse command to honor the -p/--allow-private-address flag, enabling recursive lookups to private or localhost addresses. The implementation includes documentation updates, new regression tests, and a refactoring of the runLookup function to use consistent document loaders. Feedback points out a logic flaw in the error handling for recursive lookups where "not found" responses for private URLs might incorrectly trigger a suggestion to use the private address flag instead of an authorized fetch.
Limit the `-p`/`--allow-private-address` opt-in to recursive object fetches and keep indirect JSON-LD `@context` loads on the strict loader. Add a regression test that proves recursive private contexts stay blocked even when recursive object fetches are explicitly allowed. Addresses fedify-dev#718 (review) Assisted-by: Codex:gpt-5.4
Add the author attribution to the new `CHANGES.md` entry so it records the issue, PR, and contributor together as requested by the contribution guidelines. Addresses fedify-dev#718 (comment) Assisted-by: Codex:gpt-5.4
dahlia
force-pushed
the
issue-700-recurse-private-address
branch
from
1872a70 to
46e252b
Compare
|
Addressed the recursive JSON-LD |
|
@codex review |
|
/gemini review |
dahlia
changed the title
--allow-private-address
dahlia
changed the title
--allow-private-address-p/--allow-private-address
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/lookup.test.ts`:
- Around line 1234-1261: Update the "runLookup - keeps recursive private
contexts blocked" test to also assert the stderr hint output for the
private-context case: after calling runLookupAndCaptureExitCode (and before or
after reading testFile), capture the stderr produced by that run and assert it
contains the expected hint text (the current suggestion pointing to
"-a/--authorized-fetch" or the exact message used by lookup.ts), using the same
helpers in this test harness
(runLookupAndCaptureExitCode/createLookupRunCommand/withRecursiveLookupServer);
this pins the UX for the private `@context` branch so future changes to
lookup.ts's hint text will be test-visible.
In `@packages/cli/src/lookup.ts`:
- Around line 1043-1058: The current error-handling prints the "Try with
-a/--authorized-fetch" hint whenever authLoader == null, which is misleading
when command.allowPrivateAddress is true and recursion failed due to the strict
context loader; update the branch in the RecursiveLookupError path so that if
command.allowPrivateAddress is true you do not emit the auth-fetch hint (or
instead emit a specific "recursive private context blocked" message).
Concretely, modify the block that checks !command.allowPrivateAddress && await
isPrivateAddressTarget(error.target) / else if (authLoader == null) to: if the
private-address check matched keep the existing printLookupFailureHint; else if
authLoader == null && command.allowPrivateAddress is false then print the
auth-fetch hint via printError; otherwise (authLoader == null &&
command.allowPrivateAddress is true) suppress the auth hint or call a new
message for a blocked recursive private context. Ensure you reference
command.allowPrivateAddress, isPrivateAddressTarget, authLoader,
printLookupFailureHint, printError, and the RecursiveLookupError handling in
your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee11883c-ca40-46da-b453-66f73ccbbf39
📒 Files selected for processing (3)
CHANGES.mdpackages/cli/src/lookup.test.tspackages/cli/src/lookup.ts
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the fedify lookup command to honor the -p/--allow-private-address flag during recursive lookups, allowing users to opt-in to private address access for discovered objects. The implementation includes documentation updates, new regression tests, and a security measure to ensure recursive JSON-LD context URLs remain blocked. Additionally, CLI error messages were refactored for consistency. I have no feedback to provide.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Clarify that `-p`/`--allow-private-address` only applies to recursive object fetches, while recursive JSON-LD `@context` loads remain blocked. Also document the jsonld error-shape dependency in `getPrivateContextUrl()` so future library upgrades know which fields and message format this detection relies on. Addresses fedify-dev#718 (comment) Addresses fedify-dev#718 (comment) Addresses fedify-dev#718 (comment) Addresses fedify-dev#718 (comment) Assisted-by: Codex:gpt-5.4
Extract the repeated stderr-capture setup into a small helper so recursive lookup tests share one implementation and always restore `process.stderr.write` in a single place. Addresses fedify-dev#718 (comment) Assisted-by: Codex:gpt-5.4
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the fedify lookup --recurse command to honor the -p/--allow-private-address flag for recursively discovered object URLs, while ensuring that recursive JSON-LD @context URLs remain blocked for security. The changes include updated documentation, new test cases for private address handling, and refactored error messaging. Review feedback highlights an inconsistency in how the private address policy is applied to the --traverse mode compared to the documented behavior for recursive fetches, and suggests refactoring duplicated logic used to select document loaders.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4e1ecd9e5
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Require the expected jsonld error name and message shape together before treating a recursive failure as a blocked private-context case, and align the inline comment with that predicate. Addresses fedify-dev#718 (comment) Addresses fedify-dev#718 (comment) Assisted-by: Codex:gpt-5.4
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/lookup.ts`:
- Around line 1074-1089: The code constructs a sentinel UrlError("Invalid or
private address") and relies on string matching in isPrivateAddressError; add a
small helper emitRecursivePrivateAddressHint(authLoader) that directly prints
the intended recursive-private-address hint (call printError with the same
message currently emitted for that case) and replace the
printLookupFailureHint(... new UrlError("Invalid or private address")) call with
emitRecursivePrivateAddressHint(authLoader) so the behavior no longer depends on
a fragile message-substring match between printLookupFailureHint and
isPrivateAddressError.
- Around line 1092-1103: The code calls getPrivateContextUrl(error) but discards
the returned URL and logs a generic message; change the block to capture the
returned URL (e.g., const privateContextUrl = await
getPrivateContextUrl(error);) and, when not null, include the blocked URL in the
printError message by interpolating it with the url() helper from
`@optique/core/message` (instead of only printing the boolean case), keeping the
existing optionNames and finalizeAndExit behavior so the message reads something
like "Recursive JSON-LD context URL <url(...)> is blocked..." to show the exact
blocked context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ff2959b-6a39-4684-ae2a-b87c17cbaeb9
📒 Files selected for processing (1)
packages/cli/src/lookup.ts
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Stop routing recursive private-address hints through a synthetic UrlError, surface the blocked recursive context URL directly, and make private-context detection prefer structured URL fields before falling back to the jsonld error message. Also relax the recursive private-target stderr assertion so it does not depend on Optique's exact presentation formatting. Addresses fedify-dev#718 (comment) Addresses fedify-dev#718 (comment) Addresses fedify-dev#718 (comment) Addresses fedify-dev#718 (comment) Addresses fedify-dev#718 (comment) Assisted-by: Codex:gpt-5.4
There was a problem hiding this comment.
Code Review
This pull request updates the fedify lookup --recurse command to honor the -p/--allow-private-address flag for recursively discovered object URLs, matching the behavior of the --traverse flag. Documentation and tests have been updated to reflect this change, and error handling has been improved to provide clearer feedback when lookups are blocked. A review comment suggests further improving the error message for blocked recursive JSON-LD context URLs by including the specific URL in the output.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/cli/src/lookup.ts (1095)
The error message for blocked recursive JSON-LD context URLs would be more helpful if it included the specific URL that was blocked. Since privateContextUrl is available as a URL object, you can include its href in the message.
message: "Recursive JSON-LD context URL " + privateContextUrl.href + " is always blocked, even with "
|
@codex review |
|
/gemini review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request modifies the fedify lookup --recurse command to respect the -p/--allow-private-address flag for recursively discovered object URLs, matching the behavior of the --traverse flag. While it allows opting into private addresses for objects, it continues to block recursive JSON-LD @context URLs to mitigate SSRF risks. The implementation includes enhanced error reporting with specific hints for private address issues and a suite of new tests to verify these behaviors. I have no feedback to provide.
| async function getPrivateUrlCandidate(candidate: unknown): Promise<URL | null> { | ||
| if (typeof candidate !== "string" && !(candidate instanceof URL)) return null; | ||
|
|
||
| try { | ||
| const url = new URL(candidate); | ||
| try { | ||
| await validatePublicUrl(url.href); | ||
| return null; | ||
| } catch (validationError) { | ||
| return isPrivateAddressError(validationError) ? url : null; | ||
| } | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| async function isPrivateAddressTarget(target: string): Promise<boolean> { | ||
| return await getPrivateUrlCandidate(target) != null; | ||
| } |
There was a problem hiding this comment.
isPrivateAddressTarget()/getPrivateUrlCandidate() uses validatePublicUrl(), which performs DNS lookups to detect private addresses. In the recursive error path this means a potentially slow network-dependent DNS resolution just to decide whether to print the --allow-private-address hint, and it will run even for non-private targets (e.g., a public URL that simply 404s and ends up as RecursiveLookupError). Consider avoiding DNS here by short-circuiting on cheap checks (e.g., localhost/loopback and literal IPs/private ranges) and/or only calling validatePublicUrl() when the hostname is already an IP/localhost; alternatively, plumb through the underlying UrlError instead of re-validating so hinting doesn't require extra lookups.
Summary
Fixes #700.
fedify lookupalready lets-p/--allow-private-addressopt into private-address access for URLs discovered by--traverse, but recursive fetches triggered by--recursestill rejected those targets unconditionally. That made the option feel tied to one mode instead of expressing a consistent policy for discovered URLs.This change makes recursive follow-up fetches honor the same explicit opt-in while keeping the default behavior strict. Private and localhost targets discovered through recursion are still blocked unless the user passes
-p/--allow-private-address.Changes
allowPrivateAddressinstead of hardcoding recursive fetches to deny private addresses.-p/--allow-private-addressand render option names with Optique message markup.srvxfor the local recursive test server.Testing
pnpm --dir packages/cli exec node --test --experimental-transform-types src/lookup.test.tspnpm --dir packages/cli testmise run check-each climise run check:md