Add alternate WebFinger domain support#1821
Open
JohnONolan wants to merge 1 commit into
Open
Conversation
|
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:
WalkthroughThis PR adds support for custom WebFinger hosts on accounts. The implementation adds a Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Comment |
JohnONolan
force-pushed
the
codex/alternate-webfinger-domain
branch
from
8bb9401 to
d6e922a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/account/account.service.ts`:
- Around line 78-83: The WebfingerHostError union of string literals should be
replaced with structured error objects and the validateWebfingerHost() and
setWebfingerHost() flows should return those objects inside the existing Result
type; update the WebfingerHostError definition to an object-based discriminated
union (e.g., { kind: 'invalid-domain', host: string } | { kind: 'conflict',
host: string, conflictingActor?: string } | { kind: 'not-reachable', host:
string, cause?: Error } | { kind: 'invalid-webfinger', host: string, response?:
unknown } | { kind: 'wrong-actor', expected: string, actual: string }) and
change all returns in validateWebfingerHost and setWebfingerHost (and related
logic between lines ~270-410) to construct and return these error objects
instead of bare strings so callers receive contextual fields like host or actor
URL inside the Result error branch.
- Around line 442-459: The pre-create check uses
accountRepository.getByWebfingerHandle(draft.username, normalizedHost) which
misses accounts that implicitly own the handle via their domain (webfinger_host
=== null); replace that call and the subsequent conflict check with
accountRepository.hasWebfingerHandleConflict(draft.username, normalizedHost,
draft.apId) (or equivalent API on accountRepository) so it detects both explicit
webfinger_host and default-domain conflicts before creating in
createInternalAccount(), and add a regression test that sets up an existing
account with webfinger_host = null and domain = "www.example.com" to assert
creation of `@username`@www.example.com is rejected.
In `@src/account/utils.ts`:
- Line 1: This file fails the repository formatting checks across lines 1–134;
run the project's formatter (e.g., npm run format, prettier --write, or eslint
--fix as configured) on src/account/utils.ts to reformat imports and the entire
file so CI passes; ensure the top import (isIP from 'node:net') and all
functions/exports in this file are formatted consistently with the repo rules
before committing.
In `@src/http/api/webfinger.controller.ts`:
- Around line 67-70: The site lookup uses the original resourceHost instead of
the normalized value; update the fallback to call this.siteService.getSiteByHost
with normalizedResourceHost for both attempts (i.e., replace the second await
this.siteService.getSiteByHost(`www.${resourceHost}`) with await
this.siteService.getSiteByHost(`www.${normalizedResourceHost}`)) so the lookup
always uses the normalized host when using SiteService.getSiteByHost in the
WebfingerController logic.
- Around line 112-116: The current try/catch around
this.accountRepository.getBySite(site) swallows all errors and returns null,
turning backend failures into 404s; remove the blanket catch or replace it with
a targeted check that only maps known "not found" repository responses to null
(e.g. catch a specific NotFoundError or check error.code/message) and rethrow
any other exceptions so they propagate; update the code path around getBySite to
either let errors bubble up or explicitly rethrow unexpected errors while only
converting explicit not-found conditions to null.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97c6e91b-efd3-4c6c-9ad1-4b292d993bb5
📒 Files selected for processing (41)
migrate/migrations/000081_add-webfinger-host-to-accounts.down.sqlmigrate/migrations/000081_add-webfinger-host-to-accounts.up.sqlsrc/account/account.entity.tssrc/account/account.repository.knex.integration.test.tssrc/account/account.repository.knex.tssrc/account/account.service.integration.test.tssrc/account/account.service.tssrc/account/account.service.unit.test.tssrc/account/types.tssrc/account/utils.tssrc/account/utils.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.unit.test.tssrc/feed/feed.service.tssrc/helpers/activitypub/activity.tssrc/http/api/account.controller.tssrc/http/api/account.controller.unit.test.tssrc/http/api/feed.controller.tssrc/http/api/feed.unit.test.tssrc/http/api/helpers/post.tssrc/http/api/notification.controller.tssrc/http/api/site.controller.tssrc/http/api/views/account.follows.view.tssrc/http/api/views/account.posts.view.tssrc/http/api/views/account.search.view.integration.test.tssrc/http/api/views/account.search.view.tssrc/http/api/views/account.view.tssrc/http/api/views/blocks.view.tssrc/http/api/views/explore.view.tssrc/http/api/views/recommendations.view.tssrc/http/api/views/reply.chain.view.tssrc/http/api/webfinger.controller.tssrc/http/api/webfinger.unit.test.tssrc/http/host-data-context-loader.tssrc/http/routing/route-registry.tssrc/http/routing/route-registry.unit.test.tssrc/notification/__snapshots__/get-notifications-data.jsonsrc/notification/notification.service.tssrc/post/post.entity.tssrc/post/post.repository.knex.ts
JohnONolan
force-pushed
the
codex/alternate-webfinger-domain
branch
from
d6e922a to
493a04a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/http/api/webfinger.controller.ts (1)
57-72: 🏗️ Heavy liftMove the new account-resolution path behind a service.
This change adds more repository-driven lookup logic in the controller, which pushes account resolution rules into the HTTP layer. Please keep the controller focused on request/response handling and expose this lookup flow through an
AccountServicemethod instead.As per coding guidelines, "Repositories should handle all data access and not be used directly by controllers; use services as the abstraction 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/http/api/webfinger.controller.ts` around lines 57 - 72, The controller currently performs repository/service lookups (accountRepository.getByWebfingerHandle, siteService.getSiteByHost and getAccountBySiteOrNull) to resolve accounts; move this lookup logic into a new AccountService method (e.g., resolveAccountForWebfinger(resourceUsername, normalizedResourceHost, resourceHost)) that encapsulates the custom-domain lookup, fallback to site/www.host, and account resolution, and returns either the resolved account or null. Replace the inline logic in the controller with a single call to AccountService.resolveAccountForWebfinger and then call createWebfingerResponse only if that service returns an account; remove direct repository calls from the controller so the controller only handles request/response flow.
🤖 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/http/api/account.controller.ts`:
- Around line 445-469: The route decorators APIRoute('GET', 'account/domain')
and APIRoute('PUT', 'account/domain') on the current-user handlers
handleGetAccountDomain and handleUpdateAccountDomain should be changed to
top-level paths (e.g., 'domain') because these operate on the current user, not
a specific account; update the APIRoute values to 'domain', ensure any
references to accountDomainResponse remain valid, and adjust any
routing/README/OpenAPI docs or tests that reference the old 'account/domain'
paths to the new top-level route.
In `@src/http/api/site.controller.ts`:
- Around line 37-45: The responseBody currently sets domain to
account.webfingerHost which can be null; change it to use the same fallback as
handle by assigning domain using getAccountHandleHost(account) (the same
function used to derive handle) so domain always returns the actor host when no
custom WebFinger host exists; update the response construction in the block that
builds responseBody (where domain, handle, actorUrl are set) to use
getAccountHandleHost(account) for domain.
In `@src/http/api/views/recommendations.view.ts`:
- Around line 87-91: The query exposes the effective account domain via
COALESCE(accounts.webfinger_host, accounts.domain) but still joins domain_blocks
on accounts.domain_hash, causing mismatches when a webfinger_host is used;
update the domain_blocks join to use the effective-hash equivalent
COALESCE(accounts.webfinger_host_hash, accounts.domain_hash) so the block check
uses the same resolved domain as the select (adjust the join condition where
domain_blocks is joined to reference COALESCE(accounts.webfinger_host_hash,
accounts.domain_hash) instead of accounts.domain_hash).
---
Nitpick comments:
In `@src/http/api/webfinger.controller.ts`:
- Around line 57-72: The controller currently performs repository/service
lookups (accountRepository.getByWebfingerHandle, siteService.getSiteByHost and
getAccountBySiteOrNull) to resolve accounts; move this lookup logic into a new
AccountService method (e.g., resolveAccountForWebfinger(resourceUsername,
normalizedResourceHost, resourceHost)) that encapsulates the custom-domain
lookup, fallback to site/www.host, and account resolution, and returns either
the resolved account or null. Replace the inline logic in the controller with a
single call to AccountService.resolveAccountForWebfinger and then call
createWebfingerResponse only if that service returns an account; remove direct
repository calls from the controller so the controller only handles
request/response flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d31d9dad-43e5-44c6-b069-5be7b2f8fd4f
📒 Files selected for processing (41)
migrate/migrations/000081_add-webfinger-host-to-accounts.down.sqlmigrate/migrations/000081_add-webfinger-host-to-accounts.up.sqlsrc/account/account.entity.tssrc/account/account.repository.knex.integration.test.tssrc/account/account.repository.knex.tssrc/account/account.service.integration.test.tssrc/account/account.service.tssrc/account/account.service.unit.test.tssrc/account/types.tssrc/account/utils.tssrc/account/utils.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.unit.test.tssrc/feed/feed.service.tssrc/helpers/activitypub/activity.tssrc/http/api/account.controller.tssrc/http/api/account.controller.unit.test.tssrc/http/api/feed.controller.tssrc/http/api/feed.unit.test.tssrc/http/api/helpers/post.tssrc/http/api/notification.controller.tssrc/http/api/site.controller.tssrc/http/api/views/account.follows.view.tssrc/http/api/views/account.posts.view.tssrc/http/api/views/account.search.view.integration.test.tssrc/http/api/views/account.search.view.tssrc/http/api/views/account.view.tssrc/http/api/views/blocks.view.tssrc/http/api/views/explore.view.tssrc/http/api/views/recommendations.view.tssrc/http/api/views/reply.chain.view.tssrc/http/api/webfinger.controller.tssrc/http/api/webfinger.unit.test.tssrc/http/host-data-context-loader.tssrc/http/routing/route-registry.tssrc/http/routing/route-registry.unit.test.tssrc/notification/__snapshots__/get-notifications-data.jsonsrc/notification/notification.service.tssrc/post/post.entity.tssrc/post/post.repository.knex.ts
✅ Files skipped from review due to trivial changes (3)
- src/account/account.repository.knex.integration.test.ts
- src/dispatchers.unit.test.ts
- src/http/api/notification.controller.ts
JohnONolan
force-pushed
the
codex/alternate-webfinger-domain
branch
from
493a04a to
4f52fea
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/api/views/account.search.view.ts (1)
158-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDomain-block filtering misses custom WebFinger domains.
Line 158 still joins
domain_blocksonly viaaccounts.domain_hash. After introducingwebfinger_host-based handles, accounts blocked by their custom handle domain can leak into search results.Suggested fix
.leftJoin('domain_blocks', function () { - this.on( - 'domain_blocks.domain_hash', - 'accounts.domain_hash', - ).andOnVal( - 'domain_blocks.blocker_id', - '=', - viewerAccountId.toString(), - ); + this.on(function () { + this.on('domain_blocks.domain_hash', 'accounts.domain_hash').orOn( + 'domain_blocks.domain_hash', + 'accounts.webfinger_host_hash', + ); + }).andOnVal('domain_blocks.blocker_id', '=', viewerAccountId.toString()); })🤖 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/http/api/views/account.search.view.ts` around lines 158 - 167, The leftJoin on 'domain_blocks' currently only matches domain_blocks.domain_hash to accounts.domain_hash, which misses custom WebFinger domains stored in accounts.webfinger_host; update the join condition in the leftJoin callback so domain_blocks.domain_hash is matched against both accounts.domain_hash and accounts.webfinger_host (e.g., keep the existing this.on('domain_blocks.domain_hash','accounts.domain_hash').orOn('domain_blocks.domain_hash','accounts.webfinger_host')) while preserving the this.andOnVal('domain_blocks.blocker_id','=', viewerAccountId.toString()) condition so blocks by the viewer are correctly applied.
♻️ Duplicate comments (4)
src/http/api/views/recommendations.view.ts (1)
87-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign domain-block filtering with the effective handle domain.
You now expose
domainfromCOALESCE(accounts.webfinger_host, accounts.domain), but thedomain_blocksjoin (Line 126) still checksaccounts.domain_hash. That can let accounts with blocked custom WebFinger domains slip into recommendations.Suggested fix
.leftJoin('domain_blocks', function () { - this.on( - 'domain_blocks.domain_hash', - 'accounts.domain_hash', - ).andOnVal( + this.on( + 'domain_blocks.domain_hash', + this.db.raw( + 'COALESCE(accounts.webfinger_host_hash, accounts.domain_hash)', + ), + ).andOnVal( 'domain_blocks.blocker_id', '=', viewerAccountId.toString(), ); })🤖 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/http/api/views/recommendations.view.ts` around lines 87 - 91, The query exposes domain via COALESCE(accounts.webfinger_host, accounts.domain) but the domain_blocks join still filters on accounts.domain_hash, allowing blocked webfinger domains to bypass filtering; update the domain_blocks join condition to use the hash of the effective domain (i.e. hash(COALESCE(accounts.webfinger_host, accounts.domain))) instead of accounts.domain_hash so the same computed domain value used in the select is used for blocking checks; locate the COALESCE usage in the select, the domain_blocks join condition, and the accounts.webfinger_host/accounts.domain fields to implement the replacement.src/http/api/webfinger.controller.ts (2)
121-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t turn repository failures into 404s.
This catch-all converts any
getBySite()failure intonull, so DB/query outages become a misleading not-found response. Only explicit not-found conditions should map tonull; everything else should bubble.Suggested fix
private async getAccountBySiteOrNull(site: { id: number; host: string; webhook_secret: string; ghost_uuid: string | null; }) { - try { - return await this.accountRepository.getBySite(site); - } catch (_error) { - return null; - } + return await this.accountRepository.getBySite(site); }🤖 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/http/api/webfinger.controller.ts` around lines 121 - 125, The current try/catch around this.accountRepository.getBySite(site) in the webfinger controller swallows all errors and returns null; change it so only repository "not found" cases are converted to null and all other exceptions are rethrown. Specifically, update the getBySite call handling in the method that uses this.accountRepository.getBySite to inspect the thrown error (e.g., check for a NotFoundError, record not found sentinel, or error.code === 'NOT_FOUND') and return null only for that case; for any other error rethrow or propagate the original error so outages/DB errors are not masked.
67-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReuse
normalizedResourceHostfor the site fallback lookup.The controller normalizes the requested host, but this branch still queries
SiteServicewith the rawresourceHost. Requests like mixed-case hosts can pass validation and still false-404 here.Suggested fix
- const site = - (await this.siteService.getSiteByHost(resourceHost)) || - (await this.siteService.getSiteByHost(`www.${resourceHost}`)); + const site = + (await this.siteService.getSiteByHost(normalizedResourceHost)) || + (await this.siteService.getSiteByHost( + `www.${normalizedResourceHost}`, + ));🤖 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/http/api/webfinger.controller.ts` around lines 67 - 69, The site lookup uses the raw resourceHost for the fallback which can mismatch normalized casing; change the second getSiteByHost call to use the already-normalized normalizedResourceHost (i.e. call this.siteService.getSiteByHost(`www.${normalizedResourceHost}`)) so the fallback lookup and the initial lookup both use the normalized host; update the code around the WebfingerController's site resolution (variables: normalizedResourceHost, resourceHost, site, and method siteService.getSiteByHost) accordingly.src/http/api/account.controller.ts (1)
445-469: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove the current-user domain endpoints to top-level paths.
These handlers operate on the current account, so
account/domainbreaks the route namespace convention used elsewhere in this codebase.Suggested fix
- `@APIRoute`('GET', 'account/domain') + `@APIRoute`('GET', 'domain') @@ - `@APIRoute`('PUT', 'account/domain') + `@APIRoute`('PUT', 'domain')As per coding guidelines "HTTP routes that operate on the current user should live at top-level paths (e.g.,
aliases,blocks/accounts,notifications); routes that operate on a specific account should useaccount/:handlenamespace; don't nest current-user resources underaccount/...."🤖 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/http/api/account.controller.ts` around lines 445 - 469, The current-user domain endpoints are incorrectly namespaced under 'account/domain'; update the APIRoute decorators on handleGetAccountDomain and handleUpdateAccountDomain to use the top-level path (e.g., change '`@APIRoute`('GET', 'account/domain')' to '`@APIRoute`('GET', 'domain')' and similarly for the PUT) so they follow the project's convention for current-user resources, and ensure any route metadata/docs or tests referencing 'account/domain' are updated to the new 'domain' top-level path.
🤖 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.
Outside diff comments:
In `@src/http/api/views/account.search.view.ts`:
- Around line 158-167: The leftJoin on 'domain_blocks' currently only matches
domain_blocks.domain_hash to accounts.domain_hash, which misses custom WebFinger
domains stored in accounts.webfinger_host; update the join condition in the
leftJoin callback so domain_blocks.domain_hash is matched against both
accounts.domain_hash and accounts.webfinger_host (e.g., keep the existing
this.on('domain_blocks.domain_hash','accounts.domain_hash').orOn('domain_blocks.domain_hash','accounts.webfinger_host'))
while preserving the this.andOnVal('domain_blocks.blocker_id','=',
viewerAccountId.toString()) condition so blocks by the viewer are correctly
applied.
---
Duplicate comments:
In `@src/http/api/account.controller.ts`:
- Around line 445-469: The current-user domain endpoints are incorrectly
namespaced under 'account/domain'; update the APIRoute decorators on
handleGetAccountDomain and handleUpdateAccountDomain to use the top-level path
(e.g., change '`@APIRoute`('GET', 'account/domain')' to '`@APIRoute`('GET',
'domain')' and similarly for the PUT) so they follow the project's convention
for current-user resources, and ensure any route metadata/docs or tests
referencing 'account/domain' are updated to the new 'domain' top-level path.
In `@src/http/api/views/recommendations.view.ts`:
- Around line 87-91: The query exposes domain via
COALESCE(accounts.webfinger_host, accounts.domain) but the domain_blocks join
still filters on accounts.domain_hash, allowing blocked webfinger domains to
bypass filtering; update the domain_blocks join condition to use the hash of the
effective domain (i.e. hash(COALESCE(accounts.webfinger_host, accounts.domain)))
instead of accounts.domain_hash so the same computed domain value used in the
select is used for blocking checks; locate the COALESCE usage in the select, the
domain_blocks join condition, and the accounts.webfinger_host/accounts.domain
fields to implement the replacement.
In `@src/http/api/webfinger.controller.ts`:
- Around line 121-125: The current try/catch around
this.accountRepository.getBySite(site) in the webfinger controller swallows all
errors and returns null; change it so only repository "not found" cases are
converted to null and all other exceptions are rethrown. Specifically, update
the getBySite call handling in the method that uses
this.accountRepository.getBySite to inspect the thrown error (e.g., check for a
NotFoundError, record not found sentinel, or error.code === 'NOT_FOUND') and
return null only for that case; for any other error rethrow or propagate the
original error so outages/DB errors are not masked.
- Around line 67-69: The site lookup uses the raw resourceHost for the fallback
which can mismatch normalized casing; change the second getSiteByHost call to
use the already-normalized normalizedResourceHost (i.e. call
this.siteService.getSiteByHost(`www.${normalizedResourceHost}`)) so the fallback
lookup and the initial lookup both use the normalized host; update the code
around the WebfingerController's site resolution (variables:
normalizedResourceHost, resourceHost, site, and method
siteService.getSiteByHost) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 490bd08b-2f0b-4d0f-8c23-7646322366ec
📒 Files selected for processing (42)
migrate/migrations/000081_add-webfinger-host-to-accounts.down.sqlmigrate/migrations/000081_add-webfinger-host-to-accounts.up.sqlsrc/account/account.entity.tssrc/account/account.repository.knex.integration.test.tssrc/account/account.repository.knex.tssrc/account/account.service.integration.test.tssrc/account/account.service.tssrc/account/account.service.unit.test.tssrc/account/types.tssrc/account/utils.tssrc/account/utils.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.unit.test.tssrc/feed/feed.service.tssrc/helpers/activitypub/activity.tssrc/http/api/account.controller.tssrc/http/api/account.controller.unit.test.tssrc/http/api/feed.controller.tssrc/http/api/feed.unit.test.tssrc/http/api/helpers/post.tssrc/http/api/notification.controller.tssrc/http/api/site.controller.tssrc/http/api/views/account.follows.view.tssrc/http/api/views/account.posts.view.tssrc/http/api/views/account.search.view.integration.test.tssrc/http/api/views/account.search.view.tssrc/http/api/views/account.view.tssrc/http/api/views/blocks.view.tssrc/http/api/views/explore.view.tssrc/http/api/views/recommendations.view.tssrc/http/api/views/reply.chain.view.tssrc/http/api/webfinger.controller.tssrc/http/api/webfinger.unit.test.tssrc/http/host-data-context-loader.tssrc/http/routing/route-registry.tssrc/http/routing/route-registry.unit.test.tssrc/notification/__snapshots__/get-notifications-data.jsonsrc/notification/notification.service.tssrc/post/post.entity.tssrc/post/post.repository.knex.integration.test.tssrc/post/post.repository.knex.ts
✅ Files skipped from review due to trivial changes (1)
- src/helpers/activitypub/activity.ts
JohnONolan
force-pushed
the
codex/alternate-webfinger-domain
branch
from
4f52fea to
27d8de5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/http/api/webfinger.controller.ts (1)
117-128: 💤 Low valueBackend failures are still swallowed and converted to 404s.
The try/catch returns
nullfor any error fromgetBySite(), which masks backend failures (database outages, connection errors) as "not found" responses. Consider either letting unexpected errors propagate or only catching specific not-found conditions.🤖 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/http/api/webfinger.controller.ts` around lines 117 - 128, The helper getAccountBySiteOrNull currently swallows all errors from accountRepository.getBySite(), turning backend failures into not-found results; change it to only convert a "not found" condition to null (e.g., detect a specific NotFoundError/RowNotFound error from getBySite()) and rethrow any other errors so they propagate as server errors. Locate getAccountBySiteOrNull and accountRepository.getBySite and update the catch to inspect the error type/message and return null only for the not-found case, otherwise throw the error.
🤖 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/http/api/site.controller.ts`:
- Around line 33-45: When enriching the site payload in the block that calls
this.accountService.getAccountForSite(site), guard against a null/undefined
account before accessing account.username or account.apId.href; if account is
falsy, build responseBody using the original site (or sensible defaults) and
skip calling getAccountHandleHost/getAccountHandle and reading
account.apId.href, otherwise proceed with the current enrichment logic. Locate
the code around responseBody, the call to this.accountService.getAccountForSite,
and the helpers getAccountHandleHost/getAccountHandle to implement the
conditional check and fallback behavior.
In `@src/http/api/views/blocks.view.ts`:
- Around line 18-22: The code selects an effective domain using
COALESCE(accounts.webfinger_host, accounts.domain) but still joins domain_blocks
on accounts.domain, causing domainBlockedByMe to be computed against the raw
domain instead of the effective (webfinger) domain; update the domain_blocks
join condition and any references used to compute domainBlockedByMe to use the
same COALESCE(accounts.webfinger_host, accounts.domain) expression or the alias
produced by the select so both the selected "domain" and the join/filter use the
identical effective-domain value (look for the COALESCE(...) select and the join
against domain_blocks to change the join condition to use that COALESCE/alias).
In `@src/http/api/views/explore.view.ts`:
- Around line 27-31: The query selects the effective handle domain via
COALESCE(accounts.webfinger_host, accounts.domain) but the block join still
checks accounts.domain_hash, which can miss blocks for custom webfinger_host
values; update the block join used in the Explore query to compare against the
hash of the same COALESCE(accounts.webfinger_host, accounts.domain) expression
(i.e., compute the effective domain the same way the select does) instead of
accounts.domain_hash so blocked custom WebFinger domains are filtered
consistently (locate the join/filter that references accounts.domain_hash in
this file and replace its input with the hashed
COALESCE(accounts.webfinger_host, accounts.domain) expression).
---
Nitpick comments:
In `@src/http/api/webfinger.controller.ts`:
- Around line 117-128: The helper getAccountBySiteOrNull currently swallows all
errors from accountRepository.getBySite(), turning backend failures into
not-found results; change it to only convert a "not found" condition to null
(e.g., detect a specific NotFoundError/RowNotFound error from getBySite()) and
rethrow any other errors so they propagate as server errors. Locate
getAccountBySiteOrNull and accountRepository.getBySite and update the catch to
inspect the error type/message and return null only for the not-found case,
otherwise throw the error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31632d14-80b9-4053-9da5-12908d190a6e
📒 Files selected for processing (43)
migrate/migrations/000081_add-webfinger-host-to-accounts.down.sqlmigrate/migrations/000081_add-webfinger-host-to-accounts.up.sqlsrc/account/account.entity.tssrc/account/account.repository.knex.integration.test.tssrc/account/account.repository.knex.tssrc/account/account.service.integration.test.tssrc/account/account.service.tssrc/account/account.service.unit.test.tssrc/account/types.tssrc/account/utils.tssrc/account/utils.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.unit.test.tssrc/feed/feed.service.tssrc/helpers/activitypub/activity.tssrc/http/api/account.controller.tssrc/http/api/account.controller.unit.test.tssrc/http/api/feed.controller.tssrc/http/api/feed.unit.test.tssrc/http/api/helpers/post.tssrc/http/api/notification.controller.tssrc/http/api/site.controller.tssrc/http/api/views/account.follows.view.tssrc/http/api/views/account.posts.view.tssrc/http/api/views/account.search.view.integration.test.tssrc/http/api/views/account.search.view.tssrc/http/api/views/account.view.tssrc/http/api/views/blocks.view.tssrc/http/api/views/explore.view.tssrc/http/api/views/recommendations.view.integration.test.tssrc/http/api/views/recommendations.view.tssrc/http/api/views/reply.chain.view.tssrc/http/api/webfinger.controller.tssrc/http/api/webfinger.unit.test.tssrc/http/host-data-context-loader.tssrc/http/routing/route-registry.tssrc/http/routing/route-registry.unit.test.tssrc/notification/__snapshots__/get-notifications-data.jsonsrc/notification/notification.service.tssrc/post/post.entity.tssrc/post/post.repository.knex.integration.test.tssrc/post/post.repository.knex.ts
✅ Files skipped from review due to trivial changes (4)
- src/http/routing/route-registry.unit.test.ts
- src/http/api/views/account.follows.view.ts
- src/helpers/activitypub/activity.ts
- src/http/api/feed.unit.test.ts
JohnONolan
force-pushed
the
codex/alternate-webfinger-domain
branch
2 times, most recently
from
f1ad394 to
3ff9533
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/http/api/site.controller.ts (1)
33-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
accountbefore building the enriched response.
getAccountForSite(site)can return nullish; the current dereference path throws and turns this endpoint into a 500.Proposed minimal fix
if (this.accountService) { const account = await this.accountService.getAccountForSite(site); - responseBody = { - ...site, - domain: getAccountHandleHost(account), - handle: getAccountHandle( - getAccountHandleHost(account), - account.username, - ), - actorUrl: account.apId.href, - }; + if (account) { + const handleHost = getAccountHandleHost(account); + responseBody = { + ...site, + domain: handleHost, + handle: getAccountHandle( + handleHost, + account.username, + ), + actorUrl: account.apId.href, + }; + } }🤖 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/http/api/site.controller.ts` around lines 33 - 45, The code dereferences account returned from this.accountService.getAccountForSite(site) without guarding for null/undefined which can throw; update the block that builds responseBody so it checks that account is truthy before calling getAccountHandleHost, getAccountHandle, or accessing account.apId.href (methods: getAccountForSite, getAccountHandleHost, getAccountHandle, and the responseBody construction). If account is nullish, supply safe fallbacks (e.g., omit domain/handle/actorUrl or use empty strings/defaults) or return the original site object unchanged; ensure responseBody construction only uses account fields when account exists to avoid a 500.
🧹 Nitpick comments (1)
src/http/api/feed.unit.test.ts (1)
64-69: ⚡ Quick winAdd a focused assertion for WebFinger-host precedence.
These fixtures only cover
nullWebFinger hosts, so the new precedence path isn’t directly validated. Please add one test whereauthor_webfinger_host/reposter_webfinger_hostare set and assertbody.posts[n].author.handleandbody.posts[n].repostedBy.handleexplicitly.As per coding guidelines, “Assertions should target the specific field being tested, not stringify the whole object; prefer
expect(result.field).toEqual(...)...”.Also applies to: 108-113
🤖 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/http/api/feed.unit.test.ts` around lines 64 - 69, Add a focused test case in src/http/api/feed.unit.test.ts that includes non-null author_webfinger_host and reposter_webfinger_host values in the fixture rows (instead of null) and assert the computed handles directly using expect(body.posts[i].author.handle).toEqual(...) and expect(body.posts[i].repostedBy.handle).toEqual(...); target the specific post index you want to validate and avoid stringifying the whole post object—this verifies WebFinger-host precedence for author and reposter handling.
🤖 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/account/account.repository.knex.ts`:
- Around line 427-464: getByWebfingerHandle only matches accounts.username,
causing custom-domain lookups like acct:index@custom.example to fail after
display-name changes; update the query in getByWebfingerHandle to also allow
matching the final path segment of accounts.ap_id (the actor path username) in
addition to accounts.username when the webfinger_host_hash matches: add an OR
condition that compares LOWER(SUBSTRING_INDEX(accounts.ap_id, '/', -1)) (or
equivalent SQL to extract the last path segment) to LOWER(?) (the incoming
username) while preserving the existing webfinger_host_hash check and still
returning the same selected columns so mapRowToAccountEntity can be used
unchanged.
---
Duplicate comments:
In `@src/http/api/site.controller.ts`:
- Around line 33-45: The code dereferences account returned from
this.accountService.getAccountForSite(site) without guarding for null/undefined
which can throw; update the block that builds responseBody so it checks that
account is truthy before calling getAccountHandleHost, getAccountHandle, or
accessing account.apId.href (methods: getAccountForSite, getAccountHandleHost,
getAccountHandle, and the responseBody construction). If account is nullish,
supply safe fallbacks (e.g., omit domain/handle/actorUrl or use empty
strings/defaults) or return the original site object unchanged; ensure
responseBody construction only uses account fields when account exists to avoid
a 500.
---
Nitpick comments:
In `@src/http/api/feed.unit.test.ts`:
- Around line 64-69: Add a focused test case in src/http/api/feed.unit.test.ts
that includes non-null author_webfinger_host and reposter_webfinger_host values
in the fixture rows (instead of null) and assert the computed handles directly
using expect(body.posts[i].author.handle).toEqual(...) and
expect(body.posts[i].repostedBy.handle).toEqual(...); target the specific post
index you want to validate and avoid stringifying the whole post object—this
verifies WebFinger-host precedence for author and reposter handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edfac374-2b0b-4135-948a-a150a4135b1e
📒 Files selected for processing (43)
migrate/migrations/000081_add-webfinger-host-to-accounts.down.sqlmigrate/migrations/000081_add-webfinger-host-to-accounts.up.sqlsrc/account/account.entity.tssrc/account/account.repository.knex.integration.test.tssrc/account/account.repository.knex.tssrc/account/account.service.integration.test.tssrc/account/account.service.tssrc/account/account.service.unit.test.tssrc/account/types.tssrc/account/utils.tssrc/account/utils.unit.test.tssrc/app.tssrc/configuration/registrations.tssrc/dispatchers.unit.test.tssrc/feed/feed.service.tssrc/helpers/activitypub/activity.tssrc/http/api/account.controller.tssrc/http/api/account.controller.unit.test.tssrc/http/api/feed.controller.tssrc/http/api/feed.unit.test.tssrc/http/api/helpers/post.tssrc/http/api/notification.controller.tssrc/http/api/site.controller.tssrc/http/api/views/account.follows.view.tssrc/http/api/views/account.posts.view.tssrc/http/api/views/account.search.view.integration.test.tssrc/http/api/views/account.search.view.tssrc/http/api/views/account.view.tssrc/http/api/views/blocks.view.tssrc/http/api/views/explore.view.tssrc/http/api/views/recommendations.view.integration.test.tssrc/http/api/views/recommendations.view.tssrc/http/api/views/reply.chain.view.tssrc/http/api/webfinger.controller.tssrc/http/api/webfinger.unit.test.tssrc/http/host-data-context-loader.tssrc/http/routing/route-registry.tssrc/http/routing/route-registry.unit.test.tssrc/notification/__snapshots__/get-notifications-data.jsonsrc/notification/notification.service.tssrc/post/post.entity.tssrc/post/post.repository.knex.integration.test.tssrc/post/post.repository.knex.ts
✅ Files skipped from review due to trivial changes (2)
- src/dispatchers.unit.test.ts
- src/notification/snapshots/get-notifications-data.json
Allow Ghost ActivityPub sites hosted on subdomains to expose handles on an alternate domain while preserving the existing actor URL and keys. This adds nullable account-level WebFinger host storage, strict live validation, authenticated domain APIs, WebFinger lookup behavior before host-data loading, and handle rendering updates across API views. Validation remains synchronous so a custom handle is only saved after the requested domain resolves to the current actor.
JohnONolan
force-pushed
the
codex/alternate-webfinger-domain
branch
from
3ff9533 to
020e60f
Compare
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.
Summary
Adds support for alternate Social Web/WebFinger handle domains, so a Ghost site running ActivityPub from
blog.site.comcan expose the handle as@index@site.comwhile keeping the actor URL, keys, inbox/outbox URLs, and existing identity rooted at the original ActivityPub URL.Ref implementation in Mastodon
https://docs-p.joinmastodon.org/admin/config/#web_domain
So (eg) on your root domain, like
onolan.orgyou forward requests like:and they will resolve to the correct place.
Future PR to Ghost will add UI for making domain portion of your handle configurable, which will validate that the necessary redirect is in place before you can add it.
What changed
accounts.webfinger_hoststorage, generated hash lookup, and uniqueness for claimed custom handles.GET/PUT /.ghost/activitypub/v1/account/domainendpoints.