fix(deep-links): guard percent-decoding against malformed input - #1373
Conversation
Deep link URLs are user-supplied: they can arrive from a hand-typed maestro:// link, an OS protocol handler, or a file path containing a literal '%'. All six decodeURIComponent calls in deep-link-urls.ts were unguarded, so malformed percent encoding threw URIError and the outer catch dropped the entire link (returning null) instead of degrading to the raw segment. Adds a shared safeDecodeURIComponent to src/shared/stringUtils.ts and routes all six call sites through it. Also fixes AttachmentImage.tsx, which declared a local safeDecodeURIComponent that called *itself* instead of decodeURIComponent. Every invocation recursed until the stack blew, and the try/catch swallowed the RangeError - so image srcs were never actually decoded. It now imports the shared helper. Closes #1372
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change adds a shared safe URI decoder. Deep-link parsing and attachment image handling now use it. Tests cover valid decoding, malformed input preservation, and continued parsing of deep-link segments. ChangesSafe URI decoding
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change safely handles malformed deep-link and attachment-path encoding while preserving valid decoding behavior; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR centralizes guarded percent-decoding in a shared helper, uses it to preserve malformed deep-link segments, and replaces AttachmentImage’s recursive local implementation.
Confidence Score: 4/5The PR appears safe to merge; the only issue is the non-blocking omission of the new helper from the canonical shared-utilities guide. The shared helper correctly catches malformed decoding, has no runtime dependencies, and is covered across direct and deep-link use cases; only repository-required utility documentation remains incomplete. Files Needing Attention: src/shared/stringUtils.ts Important Files Changed
Reviews (1): Last reviewed commit: "fix(deep-links): guard percent-decoding ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * safeDecodeURIComponent('100%'); // '100%' (would throw URIError) | ||
| * ``` | ||
| */ | ||
| export function safeDecodeURIComponent(value: string): string { |
There was a problem hiding this comment.
safeDecodeURIComponent is now a canonical utility used by shared and renderer code, but it is absent from docs/agent-guides/SHARED-UTILS.md; this makes the helper harder to discover and encourages future duplicate implementations.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 `@src/shared/stringUtils.ts`:
- Around line 66-74: Update safeDecodeURIComponent to catch only URIError from
decodeURIComponent, return the original value for that specific
malformed-encoding error, and allow all other exceptions to propagate to Sentry.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6593badf-e8f3-49ba-b465-8ca70239809a
📒 Files selected for processing (5)
src/__tests__/shared/deep-link-urls.test.tssrc/__tests__/shared/stringUtils.test.tssrc/renderer/components/AutoRun/AttachmentImage.tsxsrc/shared/deep-link-urls.tssrc/shared/stringUtils.ts
The bare catch swallowed everything, including failures that are real faults rather than 'this was not valid encoding' - keeping them out of Sentry, which is the silent-failure pattern CLAUDE.md warns against. Narrowed to URIError. This also matches the implementation already merged on rc, so the branches converge on this function instead of conflicting: both add the same symbol to the same file, and a lazy conflict resolution would have kept whichever side happened to be checked out.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Closes #1372
What
All six
decodeURIComponentcalls insrc/shared/deep-link-urls.tsnow route through a sharedsafeDecodeURIComponenthelper.Why
Deep link input is user-supplied - a hand-typed
maestro://link, an OS protocol-handler payload, or a file path containing a literal%. Malformed percent encoding throwsURIError. The parser's outertry/catchmeant that error didn't escape the function, but it did drop the entire link (returningnull) rather than degrading to the raw segment. Somaestro://file/sess/report%20100%#L12silently resolved to nothing.Note on the issue premise
The issue says to use "the existing
safeDecodeURIComponenthelper fromsrc/shared/stringUtils.ts". That helper did not exist onmain- this PR adds it.There was a broken local copy in
src/renderer/components/AutoRun/AttachmentImage.tsxthat called itself instead ofdecodeURIComponent, recursing until the stack overflowed while the barecatchswallowed the resultingRangeError- so it always returned the input undecoded, at the cost of thousands of stack frames per call. This PR deletes it and points the file at the shared helper.Correction to an earlier revision of this description: that broken copy was NOT introduced by #1310.
git blamedates it to1a1d5cf46e(2026-03-31), four months earlier, and #1310 targetedrc, whereAttachmentImage.tsxalready imports the shared helper and has no local copy. The original attribution was wrong on both counts.Changes
src/shared/stringUtils.ts- newsafeDecodeURIComponent(value), decodes or returns the original value.src/shared/deep-link-urls.ts- all six call sites use it.encodeURIComponenton the build side is untouched, so valid round-trips are unchanged.src/renderer/components/AutoRun/AttachmentImage.tsx- deletes the self-recursive local copy, imports the shared one.Tests
stringUtils.test.ts- valid decoding, malformed input (100%,%,%zz,%E0%A4%A, a Windows path), and pass-through cases.deep-link-urls.test.ts- malformed session / tab / group / file segments now resolve to the raw segment, including the mixed case where one segment decodes and its sibling does not.Verified the 3 new deep-link tests fail on
mainand pass here.npm run lint, eslint, prettier, and the fullsrc/__tests__/shared+src/__tests__/renderer/components/AutoRunsuites (1884 tests) all pass locally.Summary by CodeRabbit
Bug Fixes
Tests