Skip to content

Relay tests discard inbox responses, hiding at least one 500 Internal Server Error #1046

Description

@dahlia

Several tests in packages/relay approve a Follow, throw away the Response that relay.fetch() returned, and assert only that the follower was written to the key–value store. I confirmed that handles public Follow activity in packages/relay/src/litepub.test.ts receives a 500 Internal Server Error while its store assertions still pass. I printed the status from inside the test:

PROBE public-follow status: 500

Reading the code from there: the follower's inbox is https://remote.example.com/users/alice/inbox, and remote.example.com does not resolve. After the change for GHSA-f59r-8gcj-68f2, outbound delivery validates its destination before calling fetch(), so validatePublicUrl() rejects that address and the UrlError escapes the inbox listener before the Accept is sent. The follower record is written earlier in the same path, which is why the store assertions hold.

What those assertions show is that the follower was persisted. They do not show that the Accept went out, or that the inbox request returned 202 Accepted. A regression that broke every outgoing Accept would leave them green.

I only measured that one test. Two counts bound the rest. Thirteen calls to relay.fetch(request) in litepub.test.ts and mastodon.test.ts discard the returned Response. Seventeen tests across the two files use alice's inbox on remote.example.com, though some of them only read from the store and never attempt delivery, so the affected subset is smaller than that. replaces malformed follower data on Follow and, in mastodon.test.ts, stores follower in KV when Follow is approved use the same fixture pattern, but I did not run them.

Three tests that do check delivery were repaired in c5c9d61 by moving their destinations onto the RFC 3849 documentation prefix 2001:db8::/32, which validatePublicUrl() accepts and which routes nowhere. Each test file owns a disjoint /48 and answers every request inside it from a module-scope fetch interceptor. Applying the same treatment to the module-level alice fixture should cover the remaining tests: move her inbox onto that prefix, then assert the response status wherever a test approves her Follow. That was left out of the earlier change because it reaches much further into both files.

I found no sign of broken relay behavior in production here. The hazard is in the tests: adding strictEqual(response.status, 202) to any of them fails today because of the fixture's hostname, not because of whatever change the contributor was making.

Activity

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

Metadata

Metadata

Assignees

Labels

Type

Fields

Priority

None yet

Effort

None yet

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions