fix(meteor): add error handling for external avatar provider failures#38526
fix(meteor): add error handling for external avatar provider failures#38526Piyush0631 wants to merge 4 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughAdds robust proxy logic for external avatar fetches: encodes Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as AvatarProxy (Server)
participant Provider as ExternalProvider
Client->>Server: GET /avatar/:username
Server->>Provider: fetch(encodedURL)
alt fetch throws
Provider-->>Server: error
Server-->>Client: 502
else fetch succeeds
Provider-->>Server: response (status, headers, body stream)
Server->>Server: validate response.ok && body && pipeable
alt validation fails
Server-->>Client: 502
else validation passes
Server->>Server: filter/sanitize response headers
Server->>Server: attach body 'error' listener
Server-->>Client: write filtered headers
Provider-->>Client: pipe body stream
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
🤖 Fix all issues with AI agents
In `@apps/meteor/server/routes/avatar/user.ts`:
- Line 13: The code interpolates raw username into externalProviderUrl, which is
unsafe because username comes from decodeURIComponent(req.url.slice(1)...); fix
by URL-encoding the username before substitution (e.g., use
encodeURIComponent(username)) so special characters (#, ?, /, spaces) cannot
change the URL structure; update the line that builds url (the one using
externalProviderUrl.replace('{username}', username)) to replace with the encoded
username and ensure you don't double-decode the original input.
- Around line 20-26: The if guard beginning with "if (!ok || !body || typeof
(body as any).pipe !== 'function'" is missing the closing parenthesis and
opening brace, causing a parse error and making the rest of the try block
unreachable; fix it by adding the missing ")" and "{" so the statement becomes a
proper block (and ensure the corresponding closing "}" remains after the
res.end() and return); locate this in apps/meteor/server/routes/avatar/user.ts
where variables ok, body and res are used and adjust braces so the try block and
subsequent code are syntactically balanced.
🧹 Nitpick comments (4)
apps/meteor/server/routes/avatar/user.ts (4)
28-33: Consider also filteringcontent-encoding.When you strip
content-lengthandtransfer-encoding, the downstream client loses framing context. If the upstream returned acontent-encoding: gzipbody, piping the raw compressed stream while advertisingcontent-encoding: gzipbut without the originalcontent-lengthcan confuse clients. Either stripcontent-encodingas well or ensure the response is decompressed before piping.
35-44: Headers are set viasetHeaderbefore piping, but no explicit status code is written.
res.setHeader()on lines 29-33 doesn't flush headers. When(body as any).pipe(res)starts writing, Node will implicitly callwriteHead(200). This works, but it means the upstream's HTTP status (e.g., 301 redirect) is silently converted to 200. If intentional (treat anyokresponse as 200), this is fine—just confirming it's deliberate.
72-72:handleExternalProvideris still fire-and-forget (void).The PR objective states that calling the handler without awaiting was part of the original problem. The fix still uses
void(noawait), meaning any rejection after the initialreturninuserAvatarByUsername/userAvatarByIdis silently swallowed. While the internaltry/catchnow handles most errors, an unexpected throw outside it would become an unhandled rejection. Considerawaiting instead, consistent with the PR's stated goal.♻️ Suggested change (both call sites)
- void handleExternalProvider(externalProviderUrl, requestUsername, res); + await handleExternalProvider(externalProviderUrl, requestUsername, res);- void handleExternalProvider(externalProviderUrl, user.username, res); + await handleExternalProvider(externalProviderUrl, user.username, res);Also applies to: 144-144
17-18: Remove unnecessaryas anycasts;serverFetchreturns a properly-typed Response with.okand.body.
serverFetchreturns node-fetch'sResponseobject with explicitly typed properties:.ok: booleanand.body: ReadableStream<Uint8Array> | null. Theas anyandas unknowncasts bypass compile-time checking unnecessarily. Sincebodyis used with.pipe()and.on(), cast it narrowly toReadable | nullinstead:Suggested fix
- const ok = typeof (response as any).ok === 'boolean' ? (response as any).ok : true; - const body = (response as any).body as unknown; + const { ok, body } = response;If stricter typing for stream operations is needed:
import type { Readable } from 'stream'; const { ok, body } = response; const bodyStream = body as Readable | null;
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/server/routes/avatar/user.ts">
<violation number="1" location="apps/meteor/server/routes/avatar/user.ts:13">
P1: The `username` is interpolated into the URL without encoding. If the username contains special characters like `#`, `?`, `/`, or spaces, it could alter the URL structure or cause the fetch to target an unintended path. Use `encodeURIComponent(username)` to safely encode the value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/routes/avatar/user.ts`:
- Around line 17-18: The code currently assigns ok with a default of true and
uses broad "as any" casts which hides type errors; change the ok default to
false (i.e., treat non-boolean/missing response.ok as failed) and remove the
unnecessary "as any" casts by typing the response as the proper Response type
returned by serverFetch so you can read response.ok (boolean) and response.body
(stream) directly; update the assignments that create ok and body to use the
typed response (no "as any") and ensure a 502 is returned when ok is false.
🧹 Nitpick comments (2)
apps/meteor/server/routes/avatar/user.ts (2)
28-33: Consider promotingunsafeHeadersto a module-level constant.This set is static and will be re-allocated on every call. Moving it outside the function (e.g., as a
Setfor O(1) lookup) is a minor but clean improvement.♻️ Suggested refactor
+const UNSAFE_HEADERS = new Set(['host', 'content-length', 'connection', 'set-cookie', 'transfer-encoding']); + const handleExternalProvider = async (externalProviderUrl: string, username: string, res: ServerResponse): Promise<void> => { ... - const unsafeHeaders = ['host', 'content-length', 'connection', 'set-cookie', 'transfer-encoding']; response.headers.forEach((value, key) => { - if (!unsafeHeaders.includes(key.toLowerCase())) { + if (!UNSAFE_HEADERS.has(key.toLowerCase())) { res.setHeader(key, value); } });
15-50: Remove unnecessaryas anycasts fromresponseobject — use proper types from node-fetch.Lines 17, 18, 20, 35, 36, and 44 cast through
as any. Thefetchresponse from@rocket.chat/server-fetch(node-fetch v2.7.0 with types) properly exposes.ok: booleanand.body: ReadableStream. Other parts of the codebase (e.g.,removeLicense.ts,AfterSaveOEmbed.ts) access these properties directly without casting. Remove the casts and rely on the library's types.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/routes/avatar/user.ts`:
- Around line 27-32: The unsafeHeaders list currently misses "content-encoding",
so a decompressed body from node-fetch can be forwarded with a misleading
Content-Encoding header; update the unsafeHeaders array (the variable named
unsafeHeaders used in the response.headers.forEach(...) block) to include
"content-encoding" (in lowercase) so that the header is not proxied to the
client when response bodies have already been decompressed.
2 participants
Problem
When Accounts_AvatarExternalProviderUrl is enabled, the avatar route fetches the upstream avatar and streams it to the client. The current implementation has no error handling and no validation.
Current behavior (problematic code)-
const handleExternalProvider = async (externalProviderUrl: string, username: string, res: ServerResponse): Promise => {
const response = await fetch(externalProviderUrl.replace('{username}', username));
// DANGEROUS: Blindly copies all headers and pipes without validation
response.headers.forEach((value, key) => res.setHeader(key, value));
response.body.pipe(res);
};
// called without awaiting/handling
void handleExternalProvider(externalProviderUrl, requestUsername, res);
Impact
Uncontrolled failures: If the external provider is unreachable (timeout/DNS/network), fetch() rejects and the route may not return a controlled error response to the client.
Invalid responses: We don’t check response.ok. If the upstream returns a 404/HTML error page, we just pipe it into an![]()
tag.
Security risk: Copying all upstream headers blindly can transfer headers like set-cookie, content-length, etc.
Proof & Testing Setup (Reproduction)
To isolate and prove this issue, created a focused testing setup for the avatar route.
The files are local testing helpers used to verify the fix and are not included in this PR.
'use strict';
/**
*/
const base = require('./.mocharc.base.json');
module.exports = {
...base,
exit: true,
spec: ['server/routes/avatar/**/*.spec.ts'],
};
This reuses the base Mocha setup but restricts specs to the avatar route.
"scripts": {
".testunit:definition": "mocha --config ./.mocharc.definition.js",
".testunit:jest": "TZ=UTC TS_NODE_COMPILER_OPTIONS='{"allowJs": false}' jest",
".testunit:server": "mocha --config ./.mocharc.js",
".testunit:server:cov": "nyc -r text -r lcov mocha --config ./.mocharc.js",
".testunit:avatar": "nyc -r text -r lcov mocha --config ./.mocharc.avatar.js",
// ... existing scripts ...
}
This script runs only the avatar tests (via the config above) with coverage.
it('should respond with 502 when external provider fetch fails', async () => {
const userId = 'user123';
const request = { url:
/${userId}, headers: {} };});
This simulates the external provider being down and asserts that the route should respond with 502 and end the response.
cd apps/meteor
yarn .testunit:avatar
Result (before fix):
61 passing
1 failing
should respond with 502 when external provider fetch fails:
AssertionError: expected false to be true
expect(response.writeHead.calledWith(502)).to.be.true
This shows that when the external provider fetch fails, we currently do not send the expected 502 response.
Solution->
Hardened the handleExternalProvider flow:
Try/catch wrapper: catches network/DNS errors and responds with 502.
Validation: checks a best-effort response.ok and ensures body is present and pipeable before piping.
Header security: uses a blocklist to strip unsafe headers (host, set-cookie, etc.).
Stream handling: listens for body.on('error') to avoid crashes/hangs during streaming.
New code->
const handleExternalProvider = async (externalProviderUrl: string, username: string, res: ServerResponse): Promise => {
const url = externalProviderUrl.replace('{username}', encodeURIComponent(username));
};
Result (after fix):
62 passing
0 failing
Summary by CodeRabbit