fix(ios): prevent crash on iOS 18.6+ when loading HTTP/HTTPS image URLs #6913
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughThis patch enhances URL conversion logic in expo-modules-core's Convertibles.swift by adding HTTP(S) URL detection with fallback parsing, introducing iOS 18.6+ file path validation with comprehensive error handling for empty strings, invalid characters, and path format requirements. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/UIKit/Image.tsx (1)
37-41: Normalize the URL inMediafor consistency and robustness.
MediapassesimageUrldirectly toImageContainerwithout normalization, whileThumbnormalizes its URL usingnormalizeImageUrl. The downstreamformatAttachmentUrlandMessageImagedo not perform the same validation. SincenormalizeImageUrlspecifically prevents iOS crashes with expo-image by rejecting invalid protocols (file://, data:) and validating HTTP/HTTPS URLs, this inconsistency could leaveMediavulnerable to crashes ifelement?.imageUrlcontains invalid or malformed URLs. ApplynormalizeImageUrltoimageUrlinMediabefore passing it toImageContainer.
🤖 Fix all issues with AI agents
In @app/containers/message/Components/Attachments/Reply.tsx:
- Around line 169-177: The protocol check using image.includes('http') is too
permissive and misclassifies relative paths; change it to a proper starts-with
or regex check (e.g., test for 'http://' or 'https://' via
image.startsWith('http://') || image.startsWith('https://') or
/^https?:\/\//.test(image)) when constructing fullImageUrl so that only true
absolute URLs are used, keeping the rest of the logic that builds fullImageUrl
and calls normalizeImageUrl(user.id, user.token) unchanged.
🧹 Nitpick comments (1)
app/lib/methods/helpers/normalizeImageUrl.ts (1)
24-32: Consider case-insensitive checks for URL scheme prefixes.The checks use
startsWithwhich is case-sensitive. While uncommon, URLs likeFILE://orData:would bypass these guards and fail later at the URL parsing stage. The current behavior is safe (parsing would reject them), but explicit handling would be clearer.Optional: Case-insensitive prefix checks
// Reject file:// URLs - these should be handled separately - if (trimmedUrl.startsWith('file://')) { + if (trimmedUrl.toLowerCase().startsWith('file://')) { return null; } // Reject data: URLs - these are handled differently by expo-image - if (trimmedUrl.startsWith('data:')) { + if (trimmedUrl.toLowerCase().startsWith('data:')) { return null; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
app/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
app/containers/UIKit/Image.tsxapp/containers/message/Components/Attachments/Reply.tsxapp/containers/message/Urls.tsxapp/lib/methods/helpers/normalizeImageUrl.ts
🧰 Additional context used
🧬 Code graph analysis (3)
app/containers/message/Urls.tsx (3)
app/lib/methods/helpers/normalizeImageUrl.ts (1)
normalizeImageUrl(12-68)app/containers/UIKit/index.tsx (2)
image(124-126)image(185-187)app/containers/UIKit/Image.tsx (1)
Image(54-54)
app/containers/message/Components/Attachments/Reply.tsx (1)
app/lib/methods/helpers/normalizeImageUrl.ts (1)
normalizeImageUrl(12-68)
app/containers/UIKit/Image.tsx (2)
app/containers/UIKit/interfaces.ts (1)
IThumb(210-213)app/lib/methods/helpers/normalizeImageUrl.ts (1)
normalizeImageUrl(12-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (5)
app/containers/message/Components/Attachments/Reply.tsx (1)
179-179: LGTM!The normalized URL is correctly passed to the
Imagecomponent, ensuring expo-image receives a validated HTTP/HTTPS URL.app/containers/UIKit/Image.tsx (1)
27-35: LGTM!The
Thumbcomponent correctly normalizes the URL and gracefully handles invalid/null URLs by returningnull.app/containers/message/Urls.tsx (2)
72-90: LGTM!The URL normalization is correctly integrated:
- The hook runs unconditionally (respecting React's rules of hooks)
- The
loadAsynccall is properly guarded withnormalizedUrl && maxSize- The early return after the hook prevents rendering invalid URLs
123-123: LGTM!The
Imagesource correctly uses the normalized URL.app/lib/methods/helpers/normalizeImageUrl.ts (1)
12-67: Well-designed defensive helper.The function handles edge cases thoroughly:
- Null/undefined/empty inputs
- Whitespace trimming
- Unsupported schemes (file://, data:)
- Protocol-relative URLs
- Malformed URLs
The docstring clearly explains the iOS crash scenario this prevents.
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108048 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @patches/expo-modules-core+2.3.12.patch:
- Around line 7-68: The patch accidentally adds two IDE-generated Eclipse
.project files (expo-modules-core/.project and
expo-modules-core/expo-module-gradle-plugin/.project); remove these files from
the patch and regenerate the patch so it contains only the intended runtime code
changes (the Swift/URL validation edits), ensuring no IDE/project descriptor
files are included.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
patches/expo-modules-core+2.3.12.patch
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (1)
patches/expo-modules-core+2.3.12.patch (1)
73-82: Good defensive fix to prevent crashes on empty file paths.The guard correctly prevents
URL(fileURLWithPath:)from receiving an empty string, which would create an invalid URL and could cause crashes downstream in expo-image. This aligns well with the documented intent in commit 6566c7b ("fix: expo url validation") and the project's pattern of handling edge cases in the expo ecosystem.Minor note:
UrlContainsInvalidCharactersExceptionis semantically imprecise for an empty value (empty ≠ invalid characters), but reusing the existing exception type is a pragmatic choice that avoids adding new exception types for edge cases.
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108105 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/views/ShareView/Preview.tsx:
- Around line 101-116: The IconPreview call passes item?.filename which can be
undefined; update the title prop in the IconPreview invocation (inside
Preview.tsx where IconPreview is rendered) to provide a guaranteed string
fallback (e.g., item?.filename ?? '' or 'Untitled') so the title prop always
receives a string; leave other props (description using prettyBytes(item?.size
?? 0), theme, width, height) unchanged.
🧹 Nitpick comments (1)
app/containers/message/Components/Attachments/Image/Image.tsx (1)
15-15: Dual URL validation is redundant but acceptable.The component now uses both
isValidUrl(line 24) andisValidImageUri(line 70). SinceisValidImageUriis more comprehensive and specifically designed for expo-image safety, consider consolidating to use onlyisValidImageUriinshowImageto avoid redundant checks.♻️ Optional consolidation
-import { isValidUrl } from '../../../../../lib/methods/helpers/isValidUrl'; // ... -const showImage = isValidUrl(uri) && imageDimensions.width && status === 'downloaded'; +const showImage = isValidImageUri(uri) && imageDimensions.width && status === 'downloaded'; // ... -const isValidUri = isValidImageUri(uri); // ... -{showImage && isValidUri ? ( +{showImage ? (Also applies to: 24-24, 70-74
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
app/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
app/containers/ImageViewer/ImageViewer.tsxapp/containers/message/Components/Attachments/Image/Image.tsxapp/lib/methods/helpers/normalizeImageUrl.tsapp/views/ShareView/Preview.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/containers/message/Components/Attachments/Image/Image.tsx (1)
app/lib/methods/helpers/normalizeImageUrl.ts (1)
isValidImageUri(10-56)
app/containers/ImageViewer/ImageViewer.tsx (1)
app/lib/methods/helpers/normalizeImageUrl.ts (1)
isValidImageUri(10-56)
app/views/ShareView/Preview.tsx (1)
app/lib/methods/helpers/normalizeImageUrl.ts (1)
isValidImageUri(10-56)
🔇 Additional comments (5)
app/containers/message/Components/Attachments/Image/Image.tsx (1)
27-37: LGTM: Proper guard added before async image loading.The
isValidImageUricheck correctly preventsImage.loadAsyncfrom being called with invalid URIs, which aligns with the PR's goal of preventing expo-image crashes.app/containers/ImageViewer/ImageViewer.tsx (1)
130-137: LGTM: Early validation prevents expo-image crashes.The early return pattern correctly guards against invalid URIs. The fallback maintains layout dimensions, which prevents UI shifts.
Consider whether a placeholder icon or error indicator would improve user experience, though the silent fallback may be intentional for this component's use case.
app/lib/methods/helpers/normalizeImageUrl.ts (3)
10-56: Well-structured URI validation with comprehensive coverage.The function properly handles all common URI schemes (data:, file://, http/https, absolute paths) with appropriate fallbacks. The use of try-catch around
new URL()prevents exceptions from propagating.
67-123: Solid HTTP/HTTPS URL normalization with edge case handling.The function correctly:
- Rejects non-HTTP schemes (data:, file://) as documented
- Handles protocol-relative URLs (
//example.com) by prefixinghttps:- Returns normalized URL strings via
urlObj.toString()Note: The behavioral difference between
isValidImageUri(acceptsdata:) andnormalizeImageUrl(rejectsdata:) is intentional per the documentation, but consumers should be aware of this distinction.
1-1: Import is properly configured.The
react-native-url-polyfillpackage is installed as a dependency (version 2.0.0), so the import statement will resolve correctly.
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @patches/expo-modules-core+2.3.12.patch:
- Around line 1-68: The patch accidentally includes IDE-generated
Eclipse/Buildship files—remove the added
node_modules/expo-modules-core/android/.project and
node_modules/expo-modules-core/expo-module-gradle-plugin/.project entries from
the patch (they contain machine-specific IDs like 1767968821913/1910) and
regenerate the patch so only the URL normalization fix remains; optionally add
node_modules/**/.project to .gitignore before regenerating to prevent
recurrence.
🧹 Nitpick comments (1)
patches/expo-modules-core+2.3.12.patch (1)
85-92: Verbose logging may impact performance in production.The extensive
os_logcalls with emoji prefixes and multiple parameters will execute on every URL conversion in production builds. WhileOSLogis designed to be efficient, the frequency of URL conversions in an image-heavy app could accumulate overhead.Consider:
- Using
#if DEBUGpreprocessor directives to limit logging to debug builds- Using
.debuglog type instead of.defaultfor non-error cases (system may optimize these away in production)♻️ Suggested approach using debug type
- os_log("🔍 [URL.convert] Attempting to convert URI: %{public}@ (length: %d)", log: logger, type: .default, value, value.count) + os_log("🔍 [URL.convert] Attempting to convert URI: %{public}@ (length: %d)", log: logger, type: .debug, value, value.count) // First we try to create a URL without extra encoding, as it came. if let url = convertToUrl(string: value) { - os_log("✅ [URL.convert] Successfully converted via convertToUrl: %{public}@", log: logger, type: .default, value) + os_log("✅ [URL.convert] Successfully converted via convertToUrl: %{public}@", log: logger, type: .debug, value) return url }Keep
.errortype for actual error cases to ensure they're captured.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
patches/expo-modules-core+2.3.12.patch
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (2)
patches/expo-modules-core+2.3.12.patch (2)
151-159: Error handling and fallback path looks correct.The percent-encoding fallback and final error handling maintain consistent behavior:
- Failed conversions are logged with diagnostic details before throwing
UrlContainsInvalidCharactersExceptionis used uniformly, which provides predictable error handling for callers
98-149: Crash prevention logic is well-structured.The defensive validation for iOS 18.6+ stricter URL handling is comprehensive:
- Empty path guard prevents assertion failures
- Whitespace trimming handles edge cases
- Format validation ensures absolute paths or
file://URLs- Control character detection catches invalid input
The inline format validation (distinguishing absolute paths from
file://URLs) is defensive and necessary even ifisFileUrlPathalready identified this as a file path, since it validates the trimmed path conforms to acceptable formats before URL creation.
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108128 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108131 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @patches/expo-modules-core+2.3.12.patch:
- Around line 54-66: The block adds a new isHttpUrl that shadows an earlier
variable and incorrectly uses value when constructing the URL; rename the new
boolean (e.g., isHttpUrlLowercased) to avoid shadowing the earlier isHttpUrl and
base it on trimmedLowercased, and when attempting to parse an HTTP URL in the
guard branch use URL(string: trimmedPath) (not URL(string: value)); update the
guard condition to use the renamed boolean and ensure
UrlContainsInvalidCharactersException is thrown only after attempting
URL(string: trimmedPath).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
patches/expo-modules-core+2.3.12.patch
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (3)
patches/expo-modules-core+2.3.12.patch (3)
9-27: LGTM! Early HTTP URL detection prevents iOS 18.6 crash.The guard against calling
fileURLWithPathon HTTP URLs is critical for iOS 18.6+ compatibility. The fallback toURLComponentsfor RFC 3986 compliance is a sensible addition.
1-4: Monitor expo-modules-core for upstream iOS 18 URL handling fixes.This patch targets
expo-modules-core@2.3.12, which is outdated (current version is 3.0.29 as of January 2026). GitHub issues confirm that iOS 18/Xcode 16 URL fileURLWithPath crashes remain unresolved in the package, with users currently downgrading or applying similar patches as workarounds. Monitor the Expo GitHub repository for an upstream fix before upgrading to newer versions. If upgrading becomes necessary, verify whether the patch still applies or if the issue has been addressed.
38-49: Consider the consistency betweenisFileUrlPathcheck and trimmed path usage.The
isFileUrlPath(value)check on line 38 uses the original untrimmed value, but subsequent code trims whitespace (line 46) and usestrimmedPathfor validation and URL creation (line 88). While the patch includes defensive re-validation of the trimmed path (checkingisAbsolutePathandisFileURLagainsttrimmedPath), this guards against the inconsistency. However, ifisFileUrlPathdoesn't handle leading/trailing whitespace correctly, the initial branch decision might rely on characteristics that disappear after trimming. The current defensive checks mitigate this, but consider documenting whetherisFileUrlPathhandles whitespace or trimming the value earlier in the flow.
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108133 |
OtavioStasiak
changed the title
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Missing PR to expo (so we can remove this patch in the future) and an example URL to understand the issue/solution better
OtavioStasiak
requested a deployment
to
official_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
Proposed changes
In iOS 18.6, Apple introduced stricter validation for
URL(fileURLWithPath:)that causes fatal crashes in production TestFlight builds when HTTP/HTTPS URLs are incorrectly passed to this API.The
isFileUrlPath()function inexpo-modules-corewas incorrectly identifying HTTP/HTTPS URLs as file paths. This happened because:URL(string:)nil(e.g., malformed URLs), it assumes it's a file pathURL(fileURLWithPath:)on HTTP URLs, triggering iOS 18.6's strict validationSolution:
Added explicit HTTP/HTTPS URL detection before any file path checks to prevent HTTP URLs from being passed to
URL(fileURLWithPath:).http://orhttps://prefixes at the start of the conversion processURL(string:)orURLComponentsand return earlyIssue(s)
https://rocketchat.atlassian.net/browse/SUP-962
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.