Added custom webfinger handler #520
Conversation
|
""" WalkthroughA new route has been added to the application to handle requests at the endpoint Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/http/handler/webfinger.ts (1)
21-23: Consider adding more resource format validationThe current check only verifies if the resource starts with "acct:", but doesn't validate the complete format according to RFC 7033. This could lead to unexpected behavior with malformed inputs.
-if (!resource || resource.startsWith('acct:') === false) { +// Example regex for basic acct URI validation (RFC 7033) +const acctRegex = /^acct:([^@]+)@([^@]+)$/; +if (!resource || !acctRegex.test(resource)) { return next(); }src/http/handler/webfinger.unit.test.ts (1)
9-142: Add test for error handling from account repositoryThe current tests don't cover scenarios where
accountRepository.getBySite()throws an exception, which can happen in real-world conditions. Consider adding a test case for this scenario.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.ts(2 hunks)src/http/handler/webfinger.ts(1 hunks)src/http/handler/webfinger.unit.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/http/handler/webfinger.unit.test.ts (3)
src/site/site.service.ts (1)
SiteService(17-116)src/account/account.repository.knex.ts (1)
KnexAccountRepository(8-119)src/http/handler/webfinger.ts (1)
createWebFingerHandler(6-66)
src/app.ts (2)
src/instrumentation.ts (1)
spanWrapper(48-60)src/http/handler/webfinger.ts (1)
createWebFingerHandler(6-66)
🔇 Additional comments (7)
src/app.ts (1)
880-883: Clean integration of WebFinger handlerThe new route for WebFinger is appropriately positioned and follows the application's pattern of using the span wrapper for monitoring. Unlike other API routes, this endpoint correctly omits authentication requirements, which aligns with the WebFinger specification that requires public accessibility.
src/http/handler/webfinger.ts (1)
6-66: Handler effectively implements the WebFinger protocol with www fallbackThe implementation correctly follows WebFinger protocol requirements while adding the custom behavior to handle resources hosted on www subdomains. The function is well-documented and maintains proper separation of concerns.
src/http/handler/webfinger.unit.test.ts (5)
32-44: Well-structured test for fallback caseThe test effectively verifies that the handler falls back to the default implementation when the resource is missing.
46-58: Comprehensive test for non-acct URL caseThis test properly verifies that non-acct URLs are passed to the next handler in the chain.
60-77: Good coverage of site lookup behaviorThe test correctly verifies that when a site is found for the exact host, the request is passed to the default implementation.
79-96: Thorough testing of 404 response scenarioThis test properly verifies the 404 response when no site is found with www prefix.
98-141: Complete testing of successful response caseThe test comprehensively verifies the response structure, including subject, aliases, and links.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/http/handler/webfinger.unit.test.ts (1)
1-204: Consider adding tests for multi-level domainsThe current tests focus on simple domains like
example.com. Consider adding tests for multi-level domains (e.g.,example.co.uk) to ensure the handler works correctly with these more complex cases.You could add a test similar to the last one but with a multi-level domain:
it('should handle multi-level domains correctly', async () => { const handleWebFinger = createWebFingerHandler( accountRepository, siteService, ); const ctx = getCtx({ resource: 'acct:alice@example.co.uk' }); const next = vi.fn(); vi.mocked(siteService.getSiteByHost).mockImplementation((host) => { if (host === 'www.example.co.uk') { return Promise.resolve({ host: 'www.example.co.uk', } as Site); } return Promise.resolve(null); }); vi.mocked(accountRepository.getBySite).mockResolvedValue({ username: 'alice', url: 'https://www.example.co.uk', apId: new URL('https://www.example.co.uk/users/alice'), } as unknown as Account); const response = await handleWebFinger(ctx, next); expect(response?.status).toBe(200); expect(await response?.json()).toEqual({ subject: 'acct:alice@example.co.uk', aliases: ['https://www.example.co.uk/users/alice'], links: [ { rel: 'self', href: 'https://www.example.co.uk/users/alice', type: 'application/activity+json', }, { rel: 'http://webfinger.net/rel/profile-page', href: 'https://www.example.co.uk', }, ], }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.ts(2 hunks)src/http/handler/webfinger.ts(1 hunks)src/http/handler/webfinger.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app.ts
- src/http/handler/webfinger.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/http/handler/webfinger.unit.test.ts (3)
src/site/site.service.ts (1)
SiteService(17-116)src/account/account.repository.knex.ts (1)
KnexAccountRepository(8-119)src/http/handler/webfinger.ts (1)
createWebFingerHandler(10-87)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (10)
src/http/handler/webfinger.unit.test.ts (10)
1-8: Well-structured imports with proper type importsThe import statements are well organized, separating the testing framework imports from the application code imports, and correctly using type imports where appropriate.
9-31: Good test setup with well-factored mock creationThe test setup is clean and follows testing best practices:
- Using
beforeEachto reset mocks for each test- Creating utility function
getCtxto avoid repetition- Properly mocking the dependencies with TypeScript type casting
This approach makes the tests more maintainable and easier to read.
32-44: LGTM: Proper testing of falsy resource handlingThis test correctly verifies that the handler falls back to the default implementation when the resource query parameter is falsy, which is an important edge case to handle.
46-58: LGTM: Appropriate handling of non-acct resourcesThe test correctly verifies that non-acct resources (like https URLs) are delegated to the default WebFinger implementation, respecting the protocol standards.
60-73: LGTM: Robust handling of malformed acct resourcesGood test for malformed acct resources that don't contain the '@' symbol. The test ensures that:
- The default implementation is called
- The site service is not unnecessarily called
This helps maintain performance by avoiding unnecessary database lookups.
75-88: LGTM: Invalid acct resource detectionThis test properly verifies handling of acct resources with invalid host formats (e.g., missing TLD), ensuring they fall back to the default implementation without unnecessary service calls.
90-107: LGTM: Correct handling of direct site matchesThis test verifies an important behavior: when a site is found using the exact hostname from the resource (without www prefix), the handler correctly falls back to the default implementation since no special handling is needed.
109-126: LGTM: Proper 404 handling for missing www sitesThis test validates the case where neither the direct hostname nor the www-prefixed hostname resolves to a site, correctly returning a 404 response. The expectation checking is thorough, confirming both the response status and that the site service was called with the correct www-prefixed parameter.
128-157: LGTM: Error handling for missing accountsGood test for the scenario where a site exists but no account is found for it. The mock implementation is elaborate but clear, and the test properly verifies that a 404 is returned when the account repository throws an error.
159-202: LGTM: Validation of the happy path responseThis is a comprehensive test for the main feature of the custom handler. It verifies:
- The correct response status (200)
- The JSON structure matches the WebFinger standard
- The subject maintains the non-www domain in the WebFinger response
- All required links are present with correct relationships
The test adequately validates the core functionality of allowing www-hosted resources to be referenced without the www prefix.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/http/handler/webfinger.unit.test.ts (3)
60-73: Consider adding a comment explaining why this is a malformed resource.The test correctly handles malformed resources, but would benefit from documentation about why "acct:alice" is considered malformed (it's missing the '@' character that separates the username from the host).
const handleWebFinger = createWebFingerHandler( accountRepository, siteService, ); + // "acct:alice" is malformed because it's missing the '@' character + // that separates the username from the host const ctx = getCtx({ resource: 'acct:alice' }); const next = vi.fn();
75-88: Consider adding a comment explaining why this host is invalid.The test correctly verifies that invalid hosts are handled, but would benefit from documentation about why "example" is considered an invalid host (it doesn't match the HOST_REGEX pattern which likely requires a TLD).
const handleWebFinger = createWebFingerHandler( accountRepository, siteService, ); + // "example" is an invalid host because it doesn't match HOST_REGEX + // (likely missing a valid TLD like .com, .org, etc.) const ctx = getCtx({ resource: 'acct:alice@example' }); const next = vi.fn();
159-202: Consider verifying the Content-Type header in the successful response test.This test correctly verifies the status code and JSON body of the custom WebFinger response. However, it doesn't verify the Content-Type header, which according to the implementation should be set to 'application/jrd+json'.
expect(response?.status).toBe(200); + expect(response?.headers.get('Content-Type')).toBe('application/jrd+json'); expect(await response?.json()).toEqual({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.ts(2 hunks)src/http/handler/webfinger.ts(1 hunks)src/http/handler/webfinger.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app.ts
- src/http/handler/webfinger.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/http/handler/webfinger.unit.test.ts (3)
src/site/site.service.ts (1)
SiteService(17-116)src/account/account.repository.knex.ts (1)
KnexAccountRepository(8-119)src/http/handler/webfinger.ts (1)
createWebFingerHandler(10-87)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (8)
src/http/handler/webfinger.unit.test.ts (8)
1-8: Imports and setup look good.The imports provide all necessary dependencies for testing the WebFinger handler. The type imports ensure correct typing of mocked objects and parameters.
9-31: Well-structured test suite setup.The setup is clean and follows best practices:
- Mock implementations for dependencies
- Helper function for creating context objects
- Proper typing of mocked objects using TypeScript's
as unknown aspattern- Clean reset of mocks before each test
This structure will make the tests maintainable and readable.
32-44: Good test for falsy resource fallback.This test correctly verifies that the handler defers to the default implementation when no resource is provided in the query parameters, which is an important edge case to cover.
46-58: Good test for non-acct resource fallback.This test correctly verifies that the handler defers to the default implementation when the resource doesn't start with "acct:", which is an important feature of the handler.
90-107: Good test for direct site fallback.This test correctly verifies that when a site is found directly for the resource host, the handler defers to the default implementation, which is a key decision point in the handler.
109-126: Good test for www fallback with no site found.This test correctly verifies that when no site is found for either the direct host or the "www." prefixed host, a 404 response is returned. It checks both the response status and that the correct site lookup was performed.
128-157: Good test for account not found scenario.This test correctly verifies that when a site is found for the "www." prefixed host, but no account is found for that site, a 404 response is returned. The mock implementation is well-structured to test this specific scenario.
1-204: Overall excellent test coverage!The test suite thoroughly covers all the logic paths in the WebFinger handler:
- Falsy and non-acct resources
- Malformed and invalid resources
- Direct site scenarios
- www fallback scenarios
- Error handling cases
- Successful response generation
All code paths are well-tested, and the assertions verify both the behavior and the data returned. This will help ensure the WebFinger handler works as expected in all scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/http/handler/webfinger.unit.test.ts (1)
207-236: Consider enhancing the multi-level subdomain testWhile the test correctly verifies that multi-level subdomains are handled properly, it only checks the response status code. For consistency with the previous test case, consider also validating the response structure to ensure it's correctly formatted.
const response = await handleWebFinger(ctx, next); expect(response?.status).toBe(200); +expect(await response?.json()).toEqual({ + subject: 'acct:alice@sub.example.com', + aliases: ['https://www.sub.example.com/users/alice'], + links: [ + { + rel: 'self', + href: 'https://www.sub.example.com/users/alice', + type: 'application/activity+json', + }, + { + rel: 'http://webfinger.net/rel/profile-page', + href: 'https://www.sub.example.com', + }, + ], +}); +expect(response?.headers.get('Content-Type')).toBe( + 'application/jrd+json', +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.ts(2 hunks)src/http/handler/webfinger.ts(1 hunks)src/http/handler/webfinger.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app.ts
- src/http/handler/webfinger.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/http/handler/webfinger.unit.test.ts (3)
src/site/site.service.ts (1)
SiteService(17-116)src/account/account.repository.knex.ts (1)
KnexAccountRepository(8-119)src/http/handler/webfinger.ts (1)
createWebFingerHandler(10-87)
🔇 Additional comments (8)
src/http/handler/webfinger.unit.test.ts (8)
1-31: Well-structured test setup with comprehensive mockingThe test setup is clean and follows best practices with proper mocking of the
siteServiceandaccountRepositorydependencies. The utility functiongetCtxprovides a clean abstraction for creating context objects with query parameters.
32-58: Thorough testing of default implementation fallback scenariosThese tests properly verify that the handler falls back to the default implementation when the resource is either falsy or not an
acct:resource, which is critical for ensuring the custom handler doesn't interfere with other valid WebFinger formats.
60-88: Good coverage of edge cases for malformed resourcesThe tests appropriately handle malformed and invalid
acct:resources, ensuring that the handler properly validates input before attempting to process it. The assertions also verify thatgetSiteByHostis not called unnecessarily in these cases.
90-107: Correct handling of direct site resolutionThis test properly verifies that when a site is found directly (without the www prefix), the handler correctly falls back to the default implementation, which aligns with the PR objective of only providing custom handling when the www version needs to be used.
109-126: 404 handling for missing sites is correctly testedThe test correctly verifies that a 404 response is returned when no site is found for the www version of the host, and confirms that the www prefix was properly added in the lookup.
128-157: Appropriate error handling for missing accountsThe test properly verifies the case where a site is found with the www prefix but no account exists for that site, ensuring a 404 response is returned. The mock implementation of
getSiteByHostis particularly well done to simulate different responses based on the host parameter.
159-205: Thorough validation of successful WebFinger responseThis test comprehensively checks the successful case, verifying not only the response status but also the entire JSON structure and content type. The expected JSON structure matches the implementation in the handler, ensuring the response has all required WebFinger fields.
1-236: Comprehensive test coverage for the custom WebFinger handlerOverall, the test suite provides excellent coverage of the custom WebFinger handler's functionality, covering all normal paths, edge cases, and error scenarios. The tests align perfectly with the PR objective of handling WebFinger lookups for resources hosted on www domains without requiring the www prefix. The mocking approach is clean and effective, and the assertions are thorough.
src/http/handler/webfinger.ts
Outdated
| // If a site is found for the resource host, we don't need to | ||
| // do any custom handling, we can just defer to the default | ||
| // webfinger implementation | ||
| return next(); |
There was a problem hiding this comment.
I think we do want to override here, at least sometimes
I think we need/want the subject value to always be username@site.com without the www. like below:
| resource | site url is www.site.com | site url is site.com |
|---|---|---|
acct:username@www.site.com |
username@site.com |
username@site.com |
acct:username@site.com |
username@site.com |
username@site.com |
But I think by forwarding to the default implementation we get
| resource | site url is www.site.com | site url is site.com |
|---|---|---|
acct:username@www.site.com |
username@www.site.com |
username@site.com |
acct:username@site.com |
username@site.com |
username@site.com |
There was a problem hiding this comment.
I think it's gonna be a pain in the arse but we should add some cucumber tests for the functionality, because that way if we do forward to fedify still, we can catch any regressions from upgrades
There was a problem hiding this comment.
ok cool, so if we always want subject to contain no www then we shouldn't forward onto the default handler for this case. Will update 👍
There was a problem hiding this comment.
I think it's gonna be a pain in the arse
Your assessment is correct 😅
I'll look into getting some added though, I agree it would be nice to get some coverage at a higher level
There was a problem hiding this comment.
Discussed with @allouis on call - We are only going to fallback to the default implementation if we are not handling acct: to reduce impact of upstream changes and feel more confident covering just at the unit level as e2e is a bit of a pita here
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/http/handler/webfinger.unit.test.ts (2)
190-218: Test for multi-level subdomain handling.Good test that verifies the handler works with multi-level subdomains. Consider extending the assertions to also verify the response structure in this case.
expect(response?.status).toBe(200); + expect(await response?.json()).toEqual({ + subject: 'acct:alice@sub.example.com', + aliases: ['https://www.sub.example.com/users/alice'], + links: [ + { + rel: 'self', + href: 'https://www.sub.example.com/users/alice', + type: 'application/activity+json', + }, + { + rel: 'http://webfinger.net/rel/profile-page', + href: 'https://www.sub.example.com', + }, + ], + }); + expect(response?.headers.get('Content-Type')).toBe( + 'application/jrd+json', + );
1-267: Consider adding a test for www fallback behavior.The current tests verify that the handler can find sites with www prefix, but there's no test specifically for the fallback mechanism when a non-www domain isn't found but its www counterpart exists.
Add a test that verifies the www fallback behavior:
it('should fallback to the www version of the domain when the non-www is not found', async () => { const handleWebFinger = createWebFingerHandler( accountRepository, siteService, ); const ctx = getCtx({ resource: 'acct:alice@example.com' }); const next = vi.fn(); // Mock getSiteByHost to first return null for 'example.com', // then return a site for 'www.example.com' vi.mocked(siteService.getSiteByHost).mockImplementation((host) => { if (host === 'www.example.com') { return Promise.resolve({ host: 'www.example.com', } as Site); } return Promise.resolve(null); }); vi.mocked(accountRepository.getBySite).mockResolvedValue({ username: 'alice', url: 'https://www.example.com', apId: new URL('https://www.example.com/users/alice'), } as unknown as Account); const response = await handleWebFinger(ctx, next); expect(response?.status).toBe(200); expect(siteService.getSiteByHost).toHaveBeenCalledWith('www.example.com'); expect(await response?.json()).toEqual({ subject: 'acct:alice@example.com', aliases: ['https://www.example.com/users/alice'], links: [ { rel: 'self', href: 'https://www.example.com/users/alice', type: 'application/activity+json', }, { rel: 'http://webfinger.net/rel/profile-page', href: 'https://www.example.com', }, ], }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/http/handler/webfinger.ts(1 hunks)src/http/handler/webfinger.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/http/handler/webfinger.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (10)
src/http/handler/webfinger.unit.test.ts (10)
1-8: Clean and comprehensive imports.The imports are well-organized, separating the testing utilities from the application-specific types and the function under test.
13-21: Good test helper function.The
getCtxfunction effectively creates a mock Context object with configurable query parameters, which simplifies the test setup. This approach promotes DRY principles throughout the test file.
23-30: Well-structured test setup.The
beforeEachhook properly initializes the mocked dependencies needed for each test, ensuring a clean test environment for each case.
32-44: Test covers fallback behavior for missing resource.Good test for verifying that the handler falls back to default implementation when the resource query parameter is missing.
46-58: Test covers non-acct resource handling.This test correctly verifies the fallback behavior for non-acct resources, which ensures the handler only processes relevant requests.
60-73: Test covers malformed acct resource handling.Good test for verifying behavior with malformed acct resources (missing @ symbol).
92-109: Test verifies 404 response when site is not found.This test properly checks that the handler returns a 404 when no site is found for the given domain. It also verifies that the handler attempts to look up the site with the www prefix.
111-140: Test validates 404 when account is not found.Good test that simulates a scenario where a site exists but no account can be found for it, verifying the correct error handling.
142-188: Comprehensive test for successful response generation.This test thoroughly verifies the JSON structure of a successful webfinger response, including the correct content type header. It validates that all required fields (subject, aliases, links) are properly formatted.
220-266: Test ensures www prefix is removed from subject.Excellent test that validates the core functionality of this PR - ensuring that the www prefix is not included in the subject field of the response, even when it's present in the request.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/http/handler/webfinger.unit.test.ts (2)
188-216: Consider enhancing multi-level subdomain test with response body verification.The test successfully verifies the status code, but it would be more thorough to also check the response JSON body to ensure the subject correctly omits the www prefix while maintaining other subdomains (e.g.,
acct:alice@sub.example.comvsacct:alice@www.sub.example.com).const response = await handleWebFinger(ctx, next); expect(response?.status).toBe(200); +expect(await response?.json()).toEqual({ + subject: 'acct:alice@sub.example.com', + aliases: ['https://www.sub.example.com/users/alice'], + links: [ + { + rel: 'self', + href: 'https://www.sub.example.com/users/alice', + type: 'application/activity+json', + }, + { + rel: 'http://webfinger.net/rel/profile-page', + href: 'https://www.sub.example.com', + }, + ], +}); +expect(response?.headers.get('Content-Type')).toBe('application/jrd+json');
1-265: Consider adding a test for non-www subdomain handling.The test suite comprehensively covers www subdomain handling, but it would be beneficial to add a test case for a resource with a subdomain that doesn't begin with www (e.g.,
acct:alice@api.example.com). This would verify the handler correctly attemptswww.api.example.comfor site lookup while maintainingapi.example.comin the subject.it('should handle a non-www subdomain', async () => { const handleWebFinger = createWebFingerHandler( accountRepository, siteService, ); const ctx = getCtx({ resource: 'acct:alice@api.example.com' }); const next = vi.fn(); vi.mocked(siteService.getSiteByHost).mockImplementation((host) => { if (host === 'www.api.example.com') { return Promise.resolve({ host: 'www.api.example.com', } as Site); } return Promise.resolve(null); }); vi.mocked(accountRepository.getBySite).mockResolvedValue({ username: 'alice', url: 'https://www.api.example.com', apId: new URL('https://www.api.example.com/users/alice'), } as unknown as Account); const response = await handleWebFinger(ctx, next); expect(response?.status).toBe(200); expect(await response?.json()).toEqual({ subject: 'acct:alice@api.example.com', aliases: ['https://www.api.example.com/users/alice'], links: [ { rel: 'self', href: 'https://www.api.example.com/users/alice', type: 'application/activity+json', }, { rel: 'http://webfinger.net/rel/profile-page', href: 'https://www.api.example.com', }, ], }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/http/handler/webfinger.ts(1 hunks)src/http/handler/webfinger.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/http/handler/webfinger.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
ref https://linear.app/ghost/issue/AP-963 Added a custom `GET /.well-known/webfinger` handler that will ensure that a resource on a `www.` domain can be referenced without the `www.` (i.e a webfinger lookup can be done on `@mike@mike.blog` if `mike.blog` is hosted at `www.mike.blog`)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/flag/flag.service.unit.test.ts (1)
62-68: Test coverage could be more comprehensive.While this test correctly verifies that unregistered flags are considered disabled, it would be beneficial to add tests for registered flags in both enabled and non-enabled states to ensure complete coverage of the
isDisabledmethod's behavior.Consider adding these additional test cases:
it('should be able to check if a flag is disabled', () => { const flagService = new FlagService(['foo', 'bar']); flagService.runInContext(async () => { // Test unregistered flag expect(flagService.isDisabled('baz')).toBe(true); + // Test registered but not enabled flag + expect(flagService.isDisabled('foo')).toBe(true); + + // Test registered and enabled flag + flagService.enable('bar'); + expect(flagService.isDisabled('bar')).toBe(false); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app.ts(3 hunks)src/flag/flag.service.ts(1 hunks)src/flag/flag.service.unit.test.ts(1 hunks)src/http/handler/webfinger.ts(1 hunks)src/http/handler/webfinger.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app.ts
- src/http/handler/webfinger.unit.test.ts
- src/http/handler/webfinger.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/flag/flag.service.unit.test.ts (1)
src/flag/flag.service.ts (1)
FlagService(3-51)
🔇 Additional comments (1)
src/flag/flag.service.ts (1)
44-46: Well-implemented utility method for flag state checks.This is a clean implementation of the
isDisabledmethod that provides a more readable alternative to using negation withisEnabled. The method follows the established pattern in the service and will help make conditional checks more intuitive in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/http/handler/webfinger.unit.test.ts (3)
25-36: Consider moving handler creation to beforeEachThe handler creation is repeated in every test. Consider moving this to the beforeEach block to reduce duplication.
beforeEach(() => { siteService = { getSiteByHost: vi.fn(), } as unknown as SiteService; accountRepository = { getBySite: vi.fn(), } as unknown as KnexAccountRepository; flagService = { isDisabled: vi.fn().mockReturnValue(false), } as unknown as FlagService; + + handleWebFinger = createWebFingerHandler( + accountRepository, + siteService, + flagService, + ); });
200-229: Consider adding response body validation for multi-level subdomain testWhile the status code check is good, consider also validating the response body to ensure the subject is correctly formatted without the www prefix for multi-level subdomains.
const response = await handleWebFinger(ctx, next); expect(response?.status).toBe(200); + expect(await response?.json()).toEqual(expect.objectContaining({ + subject: 'acct:alice@sub.example.com', + }));
1-279: Add test for disabled feature flag behaviorThere's no test for what happens when the 'custom-webfinger' feature flag is disabled. Consider adding a test that verifies the handler falls back to the default implementation when flagService.isDisabled returns true.
it('should fallback to the default webfinger implementation if the feature flag is disabled', async () => { vi.mocked(flagService.isDisabled).mockReturnValue(true); const handleWebFinger = createWebFingerHandler( accountRepository, siteService, flagService, ); const ctx = getCtx({ resource: 'acct:alice@example.com' }); const next = vi.fn(); await handleWebFinger(ctx, next); expect(next).toHaveBeenCalled(); expect(flagService.isDisabled).toHaveBeenCalledWith('custom-webfinger'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app.ts(3 hunks)src/flag/flag.service.ts(1 hunks)src/flag/flag.service.unit.test.ts(1 hunks)src/http/handler/webfinger.ts(1 hunks)src/http/handler/webfinger.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/flag/flag.service.ts
- src/app.ts
- src/flag/flag.service.unit.test.ts
- src/http/handler/webfinger.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (5)
src/http/handler/webfinger.unit.test.ts (5)
1-36: Well-structured test setup with proper mockingThe test file has a clean setup with appropriate mocks for dependencies. The helper function
getCtxis a nice abstraction for creating the Hono Context in tests.
37-97: Good coverage of fallback and error casesThese tests thoroughly cover the scenarios where the handler should fall back to the default implementation or return an error status. The test for malformed and invalid acct resources is particularly important for robust error handling.
99-149: Thorough testing of 404 scenariosGood tests for the cases where the site or account is not found. The verification that
getSiteByHostis called with the www prefix added (line 115) is especially important for the core functionality of this PR.
151-198: Comprehensive test for successful responsesThis test thoroughly verifies the structure of the successful WebFinger response, including the subject, aliases, links, and content type. The detailed assertion of the JSON structure ensures the response format meets the WebFinger spec requirements.
231-278: Excellent test for www prefix normalizationThis test is crucial for the PR's objective - ensuring the www prefix is properly handled in the response subject. The detailed assertion of the expected response structure provides confidence that this core functionality works correctly.
ref https://linear.app/ghost/issue/AP-963 Added a custom `GET /.well-known/webfinger` handler that will ensure that a resource on a `www.` domain can be referenced without the `www.` (i.e a webfinger lookup can be done on `@mike@mike.blog` if `mike.blog` is hosted at `www.mike.blog`)
ref https://linear.app/ghost/issue/AP-963
Added a custom
GET /.well-known/webfingerhandler that will ensure that a resource on awww.domain can be referenced without thewww.(i.e a webfinger lookup can be done on@mike@mike.blogifmike.blogis hosted atwww.mike.blog)