fix: attachments with # in the filename fail to download - #7563
fix: attachments with # in the filename fail to download#7563OtavioStasiak wants to merge 4 commits into
Conversation
WalkthroughThe attachment URL formatter escapes raw ChangesAttachment URL formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/methods/helpers/formatAttachmentUrl.ts`:
- Around line 33-35: Update the rc_token branch in formatAttachmentUrl to avoid
encodeURI entirely, escaping only raw # characters so existing percent-encoded
sequences such as %20 remain unchanged. Remove the subsequent encodeURI calls in
AttachmentView.tsx when consuming the formatted URL, and add coverage for an
attachment URL containing both %20 and a raw #.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b272ae6f-5d57-4942-b151-a4c18e0e152c
📒 Files selected for processing (2)
app/lib/methods/helpers/formatAttachmentUrl.test.tsapp/lib/methods/helpers/formatAttachmentUrl.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: E2E Shard Preflight
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/helpers/formatAttachmentUrl.test.tsapp/lib/methods/helpers/formatAttachmentUrl.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/methods/helpers/formatAttachmentUrl.test.tsapp/lib/methods/helpers/formatAttachmentUrl.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/lib/methods/helpers/formatAttachmentUrl.test.tsapp/lib/methods/helpers/formatAttachmentUrl.ts
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/helpers/formatAttachmentUrl.test.tsapp/lib/methods/helpers/formatAttachmentUrl.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/helpers/formatAttachmentUrl.test.ts
🔇 Additional comments (1)
app/lib/methods/helpers/formatAttachmentUrl.ts (1)
13-15: 📐 Maintainability & Code Quality | ⚡ Quick winAdd explicit types to the new helper functions.
app/lib/methods/helpers/formatAttachmentUrl.ts#L13-L15: DeclareescapeFragmentDelimiterwith: string.app/lib/methods/helpers/formatAttachmentUrl.test.ts#L16-L17: Define an interface for the settings fixture, annotate the parameter, and declare thevoidreturn type.[recommend_recommended_refactor]
Source: Coding guidelines
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/views/AttachmentView.tsx`:
- Around line 62-63: Update the isAnimated detection in AttachmentView to
recognize GIF URLs ending with either a query string or the encoded fragment
marker %23 after the .gif extension, while preserving the image_type check. Add
a regression test covering a formatted clip.gif#draft URL when
attachment.image_type is missing or inaccurate.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd24f031-655e-44e5-9f9c-40325b54821e
📒 Files selected for processing (3)
app/lib/methods/helpers/formatAttachmentUrl.test.tsapp/lib/methods/helpers/formatAttachmentUrl.tsapp/views/AttachmentView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/lib/methods/helpers/formatAttachmentUrl.test.ts
- app/lib/methods/helpers/formatAttachmentUrl.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: E2E Shard Preflight
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/AttachmentView.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/views/AttachmentView.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/views/AttachmentView.tsx
🧠 Learnings (3)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/AttachmentView.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/AttachmentView.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/views/AttachmentView.tsx
🔇 Additional comments (1)
app/views/AttachmentView.tsx (1)
76-76: LGTM!
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Waiting
GitHub Actions
Waiting
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/views/AttachmentView.test.tsx (1)
53-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return types to the new TypeScript functions.
Add an explicit React element return type to the mocked
ImageViewerand: booleantorenderWithImageUrl.As per coding guidelines, TypeScript functions must have explicit type annotations for parameters and return types.
Also applies to: 135-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/AttachmentView.test.tsx` around lines 53 - 57, Add explicit return type annotations to the mocked ImageViewer function, using the appropriate React element type, and to renderWithImageUrl with : boolean. Keep the existing parameter annotations and function behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/views/AttachmentView.test.tsx`:
- Around line 53-57: Add explicit return type annotations to the mocked
ImageViewer function, using the appropriate React element type, and to
renderWithImageUrl with : boolean. Keep the existing parameter annotations and
function behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd2deb3d-4200-4708-865e-eef41df17390
📒 Files selected for processing (2)
app/views/AttachmentView.test.tsxapp/views/AttachmentView.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/AttachmentView.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/AttachmentView.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/views/AttachmentView.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/views/AttachmentView.test.tsx
🧠 Learnings (4)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/AttachmentView.test.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/AttachmentView.test.tsx
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/views/AttachmentView.test.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/views/AttachmentView.test.tsx
🔇 Additional comments (2)
app/views/AttachmentView.test.tsx (2)
12-19: LGTM!Also applies to: 79-79, 112-112
142-162: LGTM!
| return urlObj.toString(); | ||
| } | ||
|
|
||
| // The server encodes the path with `encodeURI(file.name)`, which leaves `#` raw and truncates the url at the fragment. |
There was a problem hiding this comment.
The server does that? Are you sure? What version started doing that?
| const uri = encodeURI(url); | ||
| const isAnimated = attachment.image_type === 'image/gif' || /\.gif(\?|$)/i.test(url); | ||
| const uri = formatAttachmentUrl(attachment.title_link || attachment.image_url, user.id, user.token, baseUrl); | ||
| // `#` is escaped to `%23` before it reaches here, so both spellings can follow the extension. |
| } | ||
|
|
||
| // The server encodes the path with `encodeURI(file.name)`, which leaves `#` raw and truncates the url at the fragment. | ||
| const escapeFragmentDelimiter = (url: string) => url.replace(/#/g, '%23'); |
There was a problem hiding this comment.
What other symbols should be escaped as well?
2 participants
Proposed changes
An attachment whose filename contains
#couldn't be downloaded or played — the request reached the server with a truncated path and returned the wrong resource or a 404.The server builds the attachment path with
encodeURI(file.name)(sendFileMessage.ts#L57),and
encodeURIdoesn't escape#. So a file nameda video #2.movarrives as:Every consumer downstream reads that
#as the fragment delimiter:setParamInUrl'snew URL()splits it intopath = /file-upload/<id>/a%20video%20+hash = #2.movand re-emits the tail after the auth params, and HTTP drops the fragment before connecting. The server only ever seesa video.formatAttachmentUrlnow escapes#→%23on server-origin attachment urls, before anything parses them. Attachment urls never carry a meaningful fragment, so this is unambiguous.Notes on placement:
rc_tokenbranch the escape runs afterencodeURI, not before —encodeURIleaves#raw but does escape%, so pre-escaping would have produced%2523._originalUrlearly return is untouched: that's not a file-upload path, so a#there can be a genuine fragment.?in a filename breaks the same way, but is deliberately left alone — the server does emit real query strings (signed-url expiry), so there's no safe string-level rule to tell the two apart.Covered by a new
formatAttachmentUrl.test.ts(relative, absolute,FileUpload_ProtectFiles, pre-authed and CDN-prefixed urls, plus already-escaped / external / base64 /file://passthroughs).Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1486
How to test or reproduce
a video #2.movto a channel.setParamInUrlpath.Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
#characters so images and videos load correctly.