Make remote follow/vote/reply instance-based and remembered#178
Open
nyanrus wants to merge 1 commit into
Open
Conversation
Remote interactions previously required the user's full fediverse
handle every time, only to redirect to their home instance. The
username part was never used to build the interaction URL.
- Add /api/resolve-instance: resolves an instance domain to a remote
interaction URL via NodeInfo software detection, with a canonical
WebFinger template path when a full handle is supplied. Includes a
basic SSRF guard and fetch timeout. Replaces /api/webfinger and
/api/instance-lookup.
- Remember used instances in localStorage; derive a default instance
from the primary linked fediverse account for logged-in users.
- Add RemoteInteractionButton: a split button ("Follow with {instance}"
+ a kebab menu to switch/add instances) shared by the follow, vote
and discussion dialogs. First-time use shows the instance's NodeInfo
title for confirmation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR consolidates three separate remote interaction flows (follow, vote, discussion) into a unified ChangesRemote Interaction Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/RemoteInteractionButton.tsx`:
- Around line 76-78: Check the response status before calling res.json() in the
instance lookup logic: first if (!res.ok) read the raw response (e.g., await
res.text()) and throw an Error that includes the HTTP status and the body text
so non-JSON errors (500 HTML, etc.) are surfaced; only call await res.json() and
return it as ResolvedInstance when res.ok is true. Update the block that
currently does "const data = await res.json(); if (!res.ok) throw new
Error(...); return data as ResolvedInstance;" to perform the status check first
and use res.text() as a fallback error payload.
In `@src/hooks/useRemoteInstance.ts`:
- Around line 14-27: The current getPrimaryLinkedDomain implementation assigns a
resolved null Promise to the module-scoped primaryLinkedDomain on any failure,
so transient fetch failures are permanently cached; change it so
primaryLinkedDomain is only set when the fetch yields a non-null domain (or
successful response), and on error or no-domain result do not overwrite the
cached Promise (or explicitly clear/reset primaryLinkedDomain to null so future
calls retry); update the logic around primaryLinkedDomain assignment in
getPrimaryLinkedDomain to only cache successful domain results while letting
failures return null without persisting that null as the cached Promise.
In `@src/lib/remote-instance.ts`:
- Around line 52-75: Domain comparisons are case-sensitive causing duplicate MRU
entries and failed removals; normalize domains to a canonical form (e.g.,
lowercased, trimmed) at write-time. Update rememberInstance to normalize
instance.domain before deduping and when storing (ensure instances contain the
normalized domain and lastUsedAt), update removeInstance to normalize the input
domain before filtering and when checking against store.defaultInstance, and
update setDefaultInstance to normalize non-null domains before writing; keep
getStore/writeStore usage and types (CachedInstance, RemoteInstanceStore)
unchanged and ensure null defaultInstance remains allowed.
- Around line 30-34: The JSON.parse result for RemoteInstanceStore is currently
trusted too early; update the code around parsed (the parsed variable and
returned object) to validate types: ensure parsed.defaultInstance is a string
(otherwise set defaultInstance to null), ensure parsed.instances is an array and
filter/map its elements to only include objects with the expected string fields
(e.g., domain/name keys used by downstream code) and discard malformed entries,
and default to an empty array if validation fails; in short, add typeof checks
and Array.isArray + element-level validation before returning from the function
that reads the cached store.
In `@src/server/controllers/api/resolve-instance-domain.ts`:
- Around line 1-5: The helper module resolve-instance-domain.ts is domain logic
and should be moved out of the controllers layer: relocate the file from
src/server/controllers/... to an appropriate service/lib package (e.g.,
src/server/services or src/server/lib), keep its exported helper functions (the
same exported function names in resolve-instance-domain.ts), update all import
sites to point to the new path, and adjust any tests or index barrel files that
referenced the old path; ensure module-level comments and exported API remain
unchanged so callers (e.g., any controller that imported
resolve-instance-domain) only need import path updates.
- Around line 42-49: The current string-only checks in the
resolve-instance-domain logic (the value variable) can be bypassed by DNS names
that resolve to private/loopback addresses; update the code in the
resolve-instance-domain handler (where value is validated) to perform a DNS
resolution (e.g. dns.lookup/dns.promises.resolve) for the hostname and validate
every resolved IP (A/AAAA) before allowing outbound requests: for each address,
use net.isIP and then reject if it falls into loopback (127.0.0.0/8, ::1),
RFC1918 private ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), link-local
(169.254.0.0/16, fe80::/10) or other non-public ranges; ensure you handle
multiple records, IPv6, and fail-closed on resolution errors, and keep the
existing string checks as a fast first filter.
In `@src/server/controllers/api/resolve-instance.ts`:
- Around line 79-131: Extract the resolution and orchestration logic out of the
POST controller into a new service function (e.g. resolveInstanceInteraction or
resolveInstance) that accepts the parsed inputs (rawInstance, target) and
returns a structured result ({ domain, title, software, interactionUrl } or
throws a well-defined error); move calls to normalizeDomain, fetchNodeInfo,
fetchWebFingerTemplate, templateForSoftware and the template selection/URI
building into that service and have POST only parse/validate the body minimally,
call the service, and map success to Response.json(...) and service errors to
appropriate status codes; ensure the service accepts env.federationHandleDomain
(or reads it internally) and exposes clear error types/messages so the
controller can translate them into HTTP responses.
- Around line 70-76: The returned WebFinger template URL must be validated
before being returned: parse the chosen template (the result of
wf?.links?.find(...)? .template) into a URL object (catching errors), ensure
protocol === "https:", and check the hostname against an allowlist (or apply a
stricter rule like same-origin/known hosts) and reject otherwise (return null).
Update the code path that returns the template at the current expression and the
analogous location referenced for lines 118-120 so both places parse, validate,
and only return the template when it passes protocol and host checks.
- Around line 48-55: The code currently fetches index.links[*].href directly via
fetchJson (see href, index.links, fetchJson, NodeInfoDoc) which allows SSRF;
before calling fetchJson validate the candidate href: parse it as a URL, ensure
protocol is "https:" and that its normalized hostname (and port if relevant)
matches the normalized host of the requested instance (i.e., the instance/host
you are resolving), and only then proceed to fetch; if no link passes
validation, return { software: null, title: null } instead of fetching. Apply
this check to the selection logic that chooses the "/2.1", "/2.0" or first link
so untrusted schemes/hosts are rejected prior to calling fetchJson.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b744b750-bd51-4c97-bad7-75d89adcefbe
📒 Files selected for processing (12)
src/components/RemoteDiscussionDialog.tsxsrc/components/RemoteFollowDialog.tsxsrc/components/RemoteInteractionButton.tsxsrc/components/RemoteVoteDialog.tsxsrc/hooks/useRemoteInstance.tssrc/lib/remote-instance.tssrc/server-entry.tssrc/server/controllers/api/instance-lookup.tssrc/server/controllers/api/resolve-instance-domain.tssrc/server/controllers/api/resolve-instance.test.tssrc/server/controllers/api/resolve-instance.tssrc/server/controllers/api/webfinger.ts
💤 Files with no reviewable changes (2)
- src/server/controllers/api/instance-lookup.ts
- src/server/controllers/api/webfinger.ts
Comment on lines
+76
to
+78
| const data = await res.json(); | ||
| if (!res.ok) throw new Error(data.error || "Instance lookup failed."); | ||
| return data as ResolvedInstance; |
There was a problem hiding this comment.
Check response status before parsing JSON to surface server errors correctly.
If the server returns a non-JSON error response (e.g., 500 with HTML), res.json() throws a SyntaxError before the status check, resulting in a generic "Instance lookup failed" message instead of the actual error.
Proposed fix
- const data = await res.json();
- if (!res.ok) throw new Error(data.error || "Instance lookup failed.");
+ if (!res.ok) {
+ const data = await res.json().catch(() => ({}));
+ throw new Error(data.error || `Instance lookup failed (${res.status}).`);
+ }
+ const data = await res.json();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/RemoteInteractionButton.tsx` around lines 76 - 78, Check the
response status before calling res.json() in the instance lookup logic: first if
(!res.ok) read the raw response (e.g., await res.text()) and throw an Error that
includes the HTTP status and the body text so non-JSON errors (500 HTML, etc.)
are surfaced; only call await res.json() and return it as ResolvedInstance when
res.ok is true. Update the block that currently does "const data = await
res.json(); if (!res.ok) throw new Error(...); return data as ResolvedInstance;"
to perform the status check first and use res.text() as a fallback error
payload.
Comment on lines
+14
to
+27
| let primaryLinkedDomain: Promise<string | null> | null = null; | ||
|
|
||
| function getPrimaryLinkedDomain(): Promise<string | null> { | ||
| primaryLinkedDomain ??= fetch("/api/auth/linked-accounts") | ||
| .then((r) => (r.ok ? r.json() : null)) | ||
| .then((data) => { | ||
| const accounts: Array<{ fediverseHandle?: string; isPrimary?: boolean }> = | ||
| data?.accounts ?? []; | ||
| const primary = accounts.find((a) => a.isPrimary) ?? accounts[0]; | ||
| const domain = primary?.fediverseHandle?.split("@").pop(); | ||
| return domain ? domain.toLowerCase() : null; | ||
| }) | ||
| .catch(() => null); // anonymous or offline — localStorage only | ||
| return primaryLinkedDomain; |
There was a problem hiding this comment.
Avoid permanently caching null for linked-domain lookup.
If the first request fails transiently, primaryLinkedDomain stays a resolved null Promise and the hook never retries in this page session, so linked-account defaulting is disabled until reload.
Suggested fix
let primaryLinkedDomain: Promise<string | null> | null = null;
function getPrimaryLinkedDomain(): Promise<string | null> {
- primaryLinkedDomain ??= fetch("/api/auth/linked-accounts")
- .then((r) => (r.ok ? r.json() : null))
- .then((data) => {
- const accounts: Array<{ fediverseHandle?: string; isPrimary?: boolean }> =
- data?.accounts ?? [];
- const primary = accounts.find((a) => a.isPrimary) ?? accounts[0];
- const domain = primary?.fediverseHandle?.split("@").pop();
- return domain ? domain.toLowerCase() : null;
- })
- .catch(() => null); // anonymous or offline — localStorage only
+ if (!primaryLinkedDomain) {
+ primaryLinkedDomain = fetch("/api/auth/linked-accounts")
+ .then((r) => (r.ok ? r.json() : null))
+ .then((data) => {
+ const accounts: Array<{ fediverseHandle?: string; isPrimary?: boolean }> =
+ data?.accounts ?? [];
+ const primary = accounts.find((a) => a.isPrimary) ?? accounts[0];
+ const domain = primary?.fediverseHandle?.split("@").pop();
+ return domain ? domain.toLowerCase() : null;
+ })
+ .then((domain) => {
+ if (!domain) primaryLinkedDomain = null; // allow retry later
+ return domain;
+ })
+ .catch(() => {
+ primaryLinkedDomain = null; // allow retry after transient failure
+ return null;
+ });
+ }
return primaryLinkedDomain;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useRemoteInstance.ts` around lines 14 - 27, The current
getPrimaryLinkedDomain implementation assigns a resolved null Promise to the
module-scoped primaryLinkedDomain on any failure, so transient fetch failures
are permanently cached; change it so primaryLinkedDomain is only set when the
fetch yields a non-null domain (or successful response), and on error or
no-domain result do not overwrite the cached Promise (or explicitly clear/reset
primaryLinkedDomain to null so future calls retry); update the logic around
primaryLinkedDomain assignment in getPrimaryLinkedDomain to only cache
successful domain results while letting failures return null without persisting
that null as the cached Promise.
Comment on lines
+30
to
+34
| const parsed = JSON.parse(raw) as RemoteInstanceStore; | ||
| return { | ||
| defaultInstance: parsed.defaultInstance ?? null, | ||
| instances: Array.isArray(parsed.instances) ? parsed.instances : [], | ||
| }; |
There was a problem hiding this comment.
Harden cached payload parsing before returning store data.
JSON.parse output is trusted too early. A malformed/stale localStorage shape can push non-string domains/defaults into state and break downstream matching/rendering.
Suggested fix
- const parsed = JSON.parse(raw) as RemoteInstanceStore;
- return {
- defaultInstance: parsed.defaultInstance ?? null,
- instances: Array.isArray(parsed.instances) ? parsed.instances : [],
- };
+ const parsed: unknown = JSON.parse(raw);
+ const record = (parsed && typeof parsed === "object" ? parsed : {}) as {
+ defaultInstance?: unknown;
+ instances?: unknown;
+ };
+ return {
+ defaultInstance:
+ typeof record.defaultInstance === "string"
+ ? record.defaultInstance.toLowerCase()
+ : null,
+ instances: Array.isArray(record.instances)
+ ? record.instances.filter(
+ (i): i is CachedInstance =>
+ !!i &&
+ typeof i === "object" &&
+ typeof (i as CachedInstance).domain === "string" &&
+ typeof (i as CachedInstance).title === "string" &&
+ (typeof (i as CachedInstance).software === "string" ||
+ (i as CachedInstance).software === null) &&
+ typeof (i as CachedInstance).lastUsedAt === "number",
+ )
+ : [],
+ };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/remote-instance.ts` around lines 30 - 34, The JSON.parse result for
RemoteInstanceStore is currently trusted too early; update the code around
parsed (the parsed variable and returned object) to validate types: ensure
parsed.defaultInstance is a string (otherwise set defaultInstance to null),
ensure parsed.instances is an array and filter/map its elements to only include
objects with the expected string fields (e.g., domain/name keys used by
downstream code) and discard malformed entries, and default to an empty array if
validation fails; in short, add typeof checks and Array.isArray + element-level
validation before returning from the function that reads the cached store.
Comment on lines
+52
to
+75
| export function rememberInstance( | ||
| instance: Pick<CachedInstance, "domain" | "title" | "software">, | ||
| ): RemoteInstanceStore { | ||
| const store = getStore(); | ||
| const others = store.instances.filter((i) => i.domain !== instance.domain); | ||
| return writeStore({ | ||
| // First instance the user ever uses becomes the implicit default. | ||
| defaultInstance: store.defaultInstance ?? instance.domain, | ||
| instances: [{ ...instance, lastUsedAt: Date.now() }, ...others], | ||
| }); | ||
| } | ||
|
|
||
| export function setDefaultInstance(domain: string | null): RemoteInstanceStore { | ||
| return writeStore({ ...getStore(), defaultInstance: domain }); | ||
| } | ||
|
|
||
| export function removeInstance(domain: string): RemoteInstanceStore { | ||
| const store = getStore(); | ||
| return writeStore({ | ||
| defaultInstance: | ||
| store.defaultInstance === domain ? null : store.defaultInstance, | ||
| instances: store.instances.filter((i) => i.domain !== domain), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Normalize domains at write-time to avoid duplicate MRU entries and failed removals.
Comparisons are case-sensitive (i.domain !== instance.domain / store.defaultInstance === domain). Different casing of the same domain can produce duplicates and prevent correct delete/default clearing.
Suggested fix
+function normalizeDomain(domain: string): string {
+ return domain.trim().toLowerCase();
+}
+
export function rememberInstance(
instance: Pick<CachedInstance, "domain" | "title" | "software">,
): RemoteInstanceStore {
+ const normalizedDomain = normalizeDomain(instance.domain);
const store = getStore();
- const others = store.instances.filter((i) => i.domain !== instance.domain);
+ const others = store.instances.filter(
+ (i) => normalizeDomain(i.domain) !== normalizedDomain,
+ );
return writeStore({
- defaultInstance: store.defaultInstance ?? instance.domain,
- instances: [{ ...instance, lastUsedAt: Date.now() }, ...others],
+ defaultInstance: store.defaultInstance ?? normalizedDomain,
+ instances: [
+ { ...instance, domain: normalizedDomain, lastUsedAt: Date.now() },
+ ...others,
+ ],
});
}
export function setDefaultInstance(domain: string | null): RemoteInstanceStore {
- return writeStore({ ...getStore(), defaultInstance: domain });
+ return writeStore({
+ ...getStore(),
+ defaultInstance: domain ? normalizeDomain(domain) : null,
+ });
}
export function removeInstance(domain: string): RemoteInstanceStore {
+ const normalizedDomain = normalizeDomain(domain);
const store = getStore();
return writeStore({
defaultInstance:
- store.defaultInstance === domain ? null : store.defaultInstance,
- instances: store.instances.filter((i) => i.domain !== domain),
+ store.defaultInstance &&
+ normalizeDomain(store.defaultInstance) === normalizedDomain
+ ? null
+ : store.defaultInstance,
+ instances: store.instances.filter(
+ (i) => normalizeDomain(i.domain) !== normalizedDomain,
+ ),
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function rememberInstance( | |
| instance: Pick<CachedInstance, "domain" | "title" | "software">, | |
| ): RemoteInstanceStore { | |
| const store = getStore(); | |
| const others = store.instances.filter((i) => i.domain !== instance.domain); | |
| return writeStore({ | |
| // First instance the user ever uses becomes the implicit default. | |
| defaultInstance: store.defaultInstance ?? instance.domain, | |
| instances: [{ ...instance, lastUsedAt: Date.now() }, ...others], | |
| }); | |
| } | |
| export function setDefaultInstance(domain: string | null): RemoteInstanceStore { | |
| return writeStore({ ...getStore(), defaultInstance: domain }); | |
| } | |
| export function removeInstance(domain: string): RemoteInstanceStore { | |
| const store = getStore(); | |
| return writeStore({ | |
| defaultInstance: | |
| store.defaultInstance === domain ? null : store.defaultInstance, | |
| instances: store.instances.filter((i) => i.domain !== domain), | |
| }); | |
| } | |
| function normalizeDomain(domain: string): string { | |
| return domain.trim().toLowerCase(); | |
| } | |
| export function rememberInstance( | |
| instance: Pick<CachedInstance, "domain" | "title" | "software">, | |
| ): RemoteInstanceStore { | |
| const normalizedDomain = normalizeDomain(instance.domain); | |
| const store = getStore(); | |
| const others = store.instances.filter( | |
| (i) => normalizeDomain(i.domain) !== normalizedDomain, | |
| ); | |
| return writeStore({ | |
| // First instance the user ever uses becomes the implicit default. | |
| defaultInstance: store.defaultInstance ?? normalizedDomain, | |
| instances: [ | |
| { ...instance, domain: normalizedDomain, lastUsedAt: Date.now() }, | |
| ...others, | |
| ], | |
| }); | |
| } | |
| export function setDefaultInstance(domain: string | null): RemoteInstanceStore { | |
| return writeStore({ | |
| ...getStore(), | |
| defaultInstance: domain ? normalizeDomain(domain) : null, | |
| }); | |
| } | |
| export function removeInstance(domain: string): RemoteInstanceStore { | |
| const normalizedDomain = normalizeDomain(domain); | |
| const store = getStore(); | |
| return writeStore({ | |
| defaultInstance: | |
| store.defaultInstance && | |
| normalizeDomain(store.defaultInstance) === normalizedDomain | |
| ? null | |
| : store.defaultInstance, | |
| instances: store.instances.filter( | |
| (i) => normalizeDomain(i.domain) !== normalizedDomain, | |
| ), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/remote-instance.ts` around lines 52 - 75, Domain comparisons are
case-sensitive causing duplicate MRU entries and failed removals; normalize
domains to a canonical form (e.g., lowercased, trimmed) at write-time. Update
rememberInstance to normalize instance.domain before deduping and when storing
(ensure instances contain the normalized domain and lastUsedAt), update
removeInstance to normalize the input domain before filtering and when checking
against store.defaultInstance, and update setDefaultInstance to normalize
non-null domains before writing; keep getStore/writeStore usage and types
(CachedInstance, RemoteInstanceStore) unchanged and ensure null defaultInstance
remains allowed.
Comment on lines
+1
to
+5
| /** | ||
| * Pure helpers for resolving a fediverse instance into a remote interaction | ||
| * URL. Kept dependency-free so it can be unit tested in isolation (mirrors the | ||
| * `ticketing` / `ticketing-domain` split used elsewhere in the codebase). | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move this helper out of the controller layer.
This module is endpoint-independent domain/template logic, so it should live in a service/lib location rather than src/server/controllers/**.
As per coding guidelines "Controller layer files must be one file per endpoint, organized by domain (e.g., controllers/events/update.ts)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/controllers/api/resolve-instance-domain.ts` around lines 1 - 5,
The helper module resolve-instance-domain.ts is domain logic and should be moved
out of the controllers layer: relocate the file from src/server/controllers/...
to an appropriate service/lib package (e.g., src/server/services or
src/server/lib), keep its exported helper functions (the same exported function
names in resolve-instance-domain.ts), update all import sites to point to the
new path, and adjust any tests or index barrel files that referenced the old
path; ensure module-level comments and exported API remain unchanged so callers
(e.g., any controller that imported resolve-instance-domain) only need import
path updates.
Comment on lines
+42
to
+49
| // Reject loopback / internal-only names. | ||
| if ( | ||
| value === "localhost" || | ||
| value.endsWith(".localhost") || | ||
| value.endsWith(".local") || | ||
| value.endsWith(".internal") || | ||
| /^\d+\.\d+\.\d+\.\d+$/.test(value) | ||
| ) { |
There was a problem hiding this comment.
SSRF guard is bypassable via DNS-resolving public hostnames.
Line 48 blocks only IPv4 literals, but hostnames like 127.0.0.1.nip.io still pass and can resolve to loopback/private IPs at fetch time. Add a DNS/IP-range check before outbound requests, not only string checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/controllers/api/resolve-instance-domain.ts` around lines 42 - 49,
The current string-only checks in the resolve-instance-domain logic (the value
variable) can be bypassed by DNS names that resolve to private/loopback
addresses; update the code in the resolve-instance-domain handler (where value
is validated) to perform a DNS resolution (e.g. dns.lookup/dns.promises.resolve)
for the hostname and validate every resolved IP (A/AAAA) before allowing
outbound requests: for each address, use net.isIP and then reject if it falls
into loopback (127.0.0.0/8, ::1), RFC1918 private ranges (10.0.0.0/8,
172.16.0.0/12, 192.168.0.0/16), link-local (169.254.0.0/16, fe80::/10) or other
non-public ranges; ensure you handle multiple records, IPv6, and fail-closed on
resolution errors, and keep the existing string checks as a fast first filter.
Comment on lines
+48
to
+55
| const href = | ||
| index?.links?.find((l) => l.rel?.endsWith("/2.1"))?.href ?? | ||
| index?.links?.find((l) => l.rel?.endsWith("/2.0"))?.href ?? | ||
| index?.links?.[0]?.href; | ||
| if (!href) return { software: null, title: null }; | ||
|
|
||
| const doc = await fetchJson<NodeInfoDoc>(href); | ||
| return { |
There was a problem hiding this comment.
Do not fetch untrusted NodeInfo href without origin/scheme validation.
Line 49/Line 54 trusts index.links[*].href from a remote server and fetches it directly. This enables second-hop SSRF. Restrict to https and the same normalized host as the requested instance.
Proposed hardening diff
async function fetchNodeInfo(
domain: string,
): Promise<{ software: string | null; title: string | null }> {
@@
- const doc = await fetchJson<NodeInfoDoc>(href);
+ let nodeInfoUrl: URL;
+ try {
+ nodeInfoUrl = new URL(href);
+ } catch {
+ return { software: null, title: null };
+ }
+ if (nodeInfoUrl.protocol !== "https:") {
+ return { software: null, title: null };
+ }
+ if (normalizeDomain(nodeInfoUrl.hostname) !== domain) {
+ return { software: null, title: null };
+ }
+
+ const doc = await fetchJson<NodeInfoDoc>(nodeInfoUrl.toString());
return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/controllers/api/resolve-instance.ts` around lines 48 - 55, The
code currently fetches index.links[*].href directly via fetchJson (see href,
index.links, fetchJson, NodeInfoDoc) which allows SSRF; before calling fetchJson
validate the candidate href: parse it as a URL, ensure protocol is "https:" and
that its normalized hostname (and port if relevant) matches the normalized host
of the requested instance (i.e., the instance/host you are resolving), and only
then proceed to fetch; if no link passes validation, return { software: null,
title: null } instead of fetching. Apply this check to the selection logic that
chooses the "/2.1", "/2.0" or first link so untrusted schemes/hosts are rejected
prior to calling fetchJson.
Comment on lines
+70
to
+76
| return ( | ||
| wf?.links?.find((l) => l.rel === "https://w3id.org/fep/3b86/Object") | ||
| ?.template ?? | ||
| wf?.links?.find((l) => l.rel === "http://ostatus.org/schema/1.0/subscribe") | ||
| ?.template ?? | ||
| null | ||
| ); |
There was a problem hiding this comment.
Validate the final interaction URL before returning it.
A remote WebFinger template can be attacker-controlled. Before returning interactionUrl, enforce an allowlist (e.g., https: only, valid host), otherwise clients may follow unsafe URLs.
Proposed validation diff
const interactionUrl = template
.replace("{domain}", domain)
.replace("{uri}", encodeURIComponent(targetUri));
+ let parsed: URL;
+ try {
+ parsed = new URL(interactionUrl);
+ } catch {
+ return Response.json({ error: "Invalid interaction URL template." }, { status: 400 });
+ }
+ if (parsed.protocol !== "https:") {
+ return Response.json({ error: "Unsupported interaction URL scheme." }, { status: 400 });
+ }
return Response.json({
domain,
title: title ?? domain,
software,
- interactionUrl,
+ interactionUrl: parsed.toString(),
});Also applies to: 118-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/controllers/api/resolve-instance.ts` around lines 70 - 76, The
returned WebFinger template URL must be validated before being returned: parse
the chosen template (the result of wf?.links?.find(...)? .template) into a URL
object (catching errors), ensure protocol === "https:", and check the hostname
against an allowlist (or apply a stricter rule like same-origin/known hosts) and
reject otherwise (return null). Update the code path that returns the template
at the current expression and the analogous location referenced for lines
118-120 so both places parse, validate, and only return the template when it
passes protocol and host checks.
Comment on lines
+79
to
+131
| export async function POST({ request }: { request: Request }) { | ||
| const body = (await request.json().catch(() => null)) as { | ||
| instance?: string; | ||
| target?: { type?: "handle" | "url"; value?: string }; | ||
| } | null; | ||
|
|
||
| const rawInstance = body?.instance; | ||
| const target = body?.target; | ||
| if (!rawInstance || typeof rawInstance !== "string") { | ||
| return Response.json({ error: "An instance is required." }, { status: 400 }); | ||
| } | ||
| if (!target?.value || (target.type !== "handle" && target.type !== "url")) { | ||
| return Response.json({ error: "A valid target is required." }, { status: 400 }); | ||
| } | ||
|
|
||
| const domain = normalizeDomain(rawInstance); | ||
| if (!domain) { | ||
| return Response.json( | ||
| { error: "Enter a valid instance domain, e.g. mastodon.social" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| // The {uri} the remote instance will act on. | ||
| const targetUri = | ||
| target.type === "handle" | ||
| ? `${target.value.replace(/^@/, "")}@${env.federationHandleDomain}` | ||
| : target.value; | ||
|
|
||
| try { | ||
| const { software, title } = await fetchNodeInfo(domain); | ||
|
|
||
| // Prefer the canonical WebFinger template when a full handle was supplied. | ||
| let template: string | null = null; | ||
| if (rawInstance.trim().replace(/^@/, "").includes("@")) { | ||
| template = await fetchWebFingerTemplate(rawInstance); | ||
| } | ||
| template ??= templateForSoftware(software); | ||
|
|
||
| const interactionUrl = template | ||
| .replace("{domain}", domain) | ||
| .replace("{uri}", encodeURIComponent(targetUri)); | ||
|
|
||
| return Response.json({ | ||
| domain, | ||
| title: title ?? domain, | ||
| software, | ||
| interactionUrl, | ||
| }); | ||
| } catch { | ||
| return Response.json({ error: "Instance lookup failed." }, { status: 500 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract NodeInfo/WebFinger/template orchestration into a service.
POST currently embeds resolver business logic; the controller should stay on HTTP concerns and delegate resolution/validation orchestration to a service.
As per coding guidelines "Controller functions must handle HTTP concerns (auth check, body parsing, status codes, response serialization) ... Controller functions must not contain business logic — orchestration and validation must be in the service layer."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/controllers/api/resolve-instance.ts` around lines 79 - 131,
Extract the resolution and orchestration logic out of the POST controller into a
new service function (e.g. resolveInstanceInteraction or resolveInstance) that
accepts the parsed inputs (rawInstance, target) and returns a structured result
({ domain, title, software, interactionUrl } or throws a well-defined error);
move calls to normalizeDomain, fetchNodeInfo, fetchWebFingerTemplate,
templateForSoftware and the template selection/URI building into that service
and have POST only parse/validate the body minimally, call the service, and map
success to Response.json(...) and service errors to appropriate status codes;
ensure the service accepts env.federationHandleDomain (or reads it internally)
and exposes clear error types/messages so the controller can translate them into
HTTP responses.
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
1 participant
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.
Remote interactions previously required the user's full fediverse handle every time, only to redirect to their home instance. The username part was never used to build the interaction URL.
Summary by CodeRabbit
New Features
Refactor