Skip to content

Avoid duplicate response headers on Elysia 1.4.18 and earlier - #972

Open
dktsudgg wants to merge 7 commits into
fedify-dev:2.0-maintenancefrom
dktsudgg:970-elysia-http-header-deduplication
Open

Avoid duplicate response headers on Elysia 1.4.18 and earlier#972
dktsudgg wants to merge 7 commits into
fedify-dev:2.0-maintenancefrom
dktsudgg:970-elysia-http-header-deduplication

Conversation

@dktsudgg

Copy link
Copy Markdown
Contributor

Fixes #970.

@fedify/elysia's fedify() plugin copies the federation response headers into set.headers and then returns the same Response object.

When the response has a body, that Response already carries its own headers,
so on Elysia 1.4.18 and earlier(where Elysia merges set.headers into the returned response by appending unconditionally) every response header ends up duplicated.
(for example : Content-Type: application/activity+json, application/activity+json)

Elysia 1.4.19 changed the merge to skip headers the response already has, which masks the bug, but the package still supports Elysia ^1.3.6, so users on 1.4.18 and earlier receive malformed federation responses that a strict ActivityPub peer can reject.

This change moves the early return response (the case where the response has a body) above the header copy,
so the copy only runs on the bodyless path where a fresh new Response(null, …) needs the headers.
When there is a body the Response is returned untouched, so there is nothing for Elysia to merge back in, and the headers are no longer duplicated on any Elysia version.

See #970 for the details.

Assisted-by: Claude Code:claude-fable-5

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b49efb35-c920-4b8e-b8cb-c0bc2f59652a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dahlia dahlia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a changelog entry to CHANGES.md? Thanks!

@dktsudgg

dktsudgg commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

@dahlia I added a changelog entry!

@dktsudgg
dktsudgg requested a review from dahlia August 2, 2026 15:30
Comment thread CHANGES.md Outdated
@dktsudgg
dktsudgg requested a review from dahlia August 3, 2026 11:44
@dahlia

dahlia commented Aug 4, 2026

Copy link
Copy Markdown
Member

@dktsudgg Could you change the target of this pull request to 2.0-maintenance? 1.8.x is now end-of-life.

@dktsudgg
dktsudgg changed the base branch from 1.8-maintenance to 2.0-maintenance August 9, 2026 02:16
@dktsudgg
dktsudgg force-pushed the 970-elysia-http-header-deduplication branch from dd52ce7 to ab95f06 Compare August 9, 2026 02:18
@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dktsudgg

dktsudgg commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@dahlia I changed target branch to 2.0-maintenance

@dahlia dahlia added integration/elysia Elysia integration (@fedify/elysia) component/integration Web framework integration labels Aug 12, 2026
Comment thread changes.d/elysia/duplicate-response-headers.md Outdated
@dktsudgg
dktsudgg requested a review from dahlia August 14, 2026 09:09
@dahlia

dahlia commented Aug 14, 2026

Copy link
Copy Markdown
Member

Could you rebase your commits on the latest 2.0-maintenance?

@dktsudgg
dktsudgg force-pushed the 970-elysia-http-header-deduplication branch from 9d0061a to d58b040 Compare August 14, 2026 15:35
@dahlia

dahlia commented Aug 14, 2026

Copy link
Copy Markdown
Member

Sorry, the fix I made on 2.0-maintenance was half-baked. Could you make a rebase once again?

When a response has a body, the plugin returns the `response` object directly,
so there is no need to copy the response's headers into Elysia's `set.headers`.
Elysia already includes `response.headers` in the HTTP response on its own.

In fact, on Elysia 1.4.18 and earlier, copying `response.headers` into `set.headers`
and then returning the response causes the HTTP response headers to be duplicated.
(This was fixed on Elysia's side in 1.4.19.)

This change therefore moves the early return of the `response` object, for the case where a body exists, above the header copy so that the HTTP response headers are no longer duplicated.

While this problem is partly due to the bug in Elysia 1.4.18 and earlier,
this fix is expected to resolve it sufficiently, so it does not raise the project's minimum Elysia version, keeping the current Elysia version support range intact.

Assisted-by: Claude Code:claude-fable-5
Assisted-by: Claude Code:claude-fable-5
@dktsudgg
dktsudgg force-pushed the 970-elysia-http-header-deduplication branch from 76d87fc to 655f2dc Compare August 14, 2026 15:42
@dktsudgg

dktsudgg commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Sorry, and I found that I mistyped like https://github.com/fedify-dev/fedify/issues/972,
I fixed it to https://github.com/fedify-dev/fedify/pull/972
(Addressed in 655f2dc)

CHANGES.md links the `fedify-dev#972` reference to the pull request URL, but the
fragment declared no link URLs, so sacho compiled its default issue URL
and `sacho check` reported the materialized changelog as out of sync.
Add a front matter links block so the compiled entry matches
CHANGES.md.

Assisted-by: Claude Code:claude-fable-5
Hongdown 0.5.3 requires a blank line after the front matter closing
delimiter while sacho's normal form forbids one, so a fragment with a
links block cannot satisfy both checkers. Exclude changes.d/ from
Hongdown as main already does, leaving the fragments to sacho alone.

Assisted-by: Claude Code:claude-fable-5
@dktsudgg

Copy link
Copy Markdown
Contributor Author

I added two commits with AI assistance to make mise run check pass.
please check e233750 and 9cc1d9e.
I confirmed it works locally, but the commit messages are AI-generated, so please give them a read.

It looks like there is a real incompatibility between sacho and hongdown rather than a one-off formatting issue.
hongdown 0.5.3 requires a blank line after the front matter's closing ---, while sacho's normal form forbids one,
so a changelog fragment with a links block cannot satisfy both checkers at once.
main branch works around this by excluding changes.d/ from Hongdown in .hongdown.toml, which is what 9cc1d9e backports.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/integration Web framework integration integration/elysia Elysia integration (@fedify/elysia)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants