Fix incorrect handling of relative path URIs in link alternate headers #155
Conversation
WalkthroughThe changes involve updates to the test suite in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DocumentLoader
participant DocumentService
Client->>DocumentLoader: fetchDocumentLoader(url)
DocumentLoader->>DocumentService: GET /link-obj-relative
DocumentService-->>DocumentLoader: 200 OK (empty response, relative link)
DocumentLoader->>Client: Return document structure with resolved relative link
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
@dahlia this bug affects Hollo and prevents federation with GoToSocial v0.16.0 & v0.16.0+git-f1cbf6f |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/runtime/docloader.test.ts (2)
122-143: Improve test structure and remove duplication.The test step has the following issues:
- The first assertion (lines 123-131) is duplicated from the previous test step and doesn't test relative URL handling.
- The test name could be more specific about what's being tested.
Consider applying these improvements:
- await t.step("Link header relative url", async () => { - assertEquals(await fetchDocumentLoader("https://example.com/link-ctx"), { - contextUrl: "https://www.w3.org/ns/activitystreams", - documentUrl: "https://example.com/link-ctx", - document: { - id: "https://example.com/link-ctx", - name: "Fetched object", - type: "Object", - }, - }); - + await t.step("resolves relative URLs in Link headers", async () => { assertEquals(await fetchDocumentLoader("https://example.com/link-obj-relative"), { contextUrl: null, documentUrl: "https://example.com/object", document: { "@context": "https://www.w3.org/ns/activitystreams", id: "https://example.com/object", name: "Fetched object", type: "Object", }, }); });
86-97: Add test cases for additional relative path scenarios.The current test only covers a simple relative path case. Consider adding test cases for:
- Nested paths (e.g., '/docs/jsonldcontext.jsonld')
- Parent directory references (e.g., '../docs/schema.json')
- Same directory references (e.g., './schema.json')
This would provide better coverage for the relative path handling fix described in the PR objectives.
Example mock for nested paths:
mf.mock("GET@/link-obj-nested", (_req) => new Response( "", { status: 200, headers: { "Content-Type": "text/html; charset=utf-8", Link: '</docs/jsonldcontext.jsonld>; rel="alternate"; ' + 'type="application/activity+json"', }, }, ));Would you like me to generate the complete set of test cases for these scenarios?
src/runtime/docloader.ts (1)
136-148: Consider expanding test coverage for URI resolution edge cases.While the changes look good, consider adding test cases for:
- Different relative path formats (./path, ../path, /path)
- Protocol-relative URLs (//example.com)
- Error cases with malformed URIs
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/runtime/docloader.test.ts (2 hunks)
- src/runtime/docloader.ts (1 hunks)
🔇 Additional comments (2)
src/runtime/docloader.test.ts (1)
86-97: LGTM! Mock response correctly simulates relative path scenario.The mock response accurately represents the use case where a Link header contains a relative path URI, which is essential for testing the fix described in the PR objectives.
src/runtime/docloader.ts (1)
136-148: LGTM! The changes correctly handle relative URIs in link alternate headers.The introduction of
altUriproperly resolves relative URIs against the base URL (docUrl), fixing the issue where relative paths in link alternate headers were not being correctly handled. This change aligns with the existing HTML link handling approach in the codebase.Let's verify that the HTML link handling uses the same URL resolution approach:
✅ Verification successful
The changes correctly align with existing HTML link handling approach
The verification confirms that HTML link handling in the same file uses identical URL resolution logic:
- Line 186:
new URL(attribs.href, docUrl).href !== docUrl.hreffor comparison- Line 192:
fetch(new URL(attribs.href, docUrl).href)for fetchingThe PR's changes to handle relative URIs in link alternate headers perfectly mirror this established pattern, ensuring consistent behavior across both HTML and alternate link processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify HTML link handling uses similar URL resolution # Test: Search for URL resolution in HTML link handling rg -A 5 "new URL\(.+, docUrl\)"Length of output: 1122
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Thank you! Everything looks good, except that it should be merged into previous versions. Instead of merging this pull request, I'm cherry-picking your commit to the 1.0-maintenance branch.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
=======================================
Coverage 79.31% 79.32%
=======================================
Files 42 42
Lines 7925 7926 +1
Branches 1119 1119
=======================================
+ Hits 6286 6287 +1
Misses 1618 1618
Partials 21 21 ☔ View full report in Codecov by Sentry. |
|
This patch was applied to 0.15.4, 1.0.6, and 1.1.2. |
2 participants
I think I've gotten this change right, essentially the code would try to resolve
http://schema.org/be redirected tohttps://schema.org/and get back a link alternate with:Which Fedify would correctly discover, however, it would then try to fetch
/docs/jsonldcontext.jsonldinstead ofhttps://schema.org/docs/jsonldcontext.jsonld, this fetch would result in an invalid URL error.This change should fix that by resolving alternate URIs relative to the docUrl, as is done in HTML response handling.
The test change appears to assert this but I'm not 100% sure this is correct.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation