Encode file path in url #1661
Encode file path in url #1661
Conversation
📝 WalkthroughWalkthroughIntroduces a new encode_path helper to URL-encode each path segment and updates ListFiles.php to apply encode_path to non-directory file URLs for record, view, edit, and download actions. Directory handling remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant UI as ListFiles (UI)
participant H as encode_path()
participant R as Router
participant C as Files Controller
U->>UI: Click "Download" on file with special chars
UI->>H: encode_path(join_paths(path, filename))
H-->>UI: Encoded path
UI->>R: GET /files/download/{encoded path}
R->>C: Route to download action
C-->>U: File response
note over UI,R: Same encoding applied for record, view, and edit URLs
Assessment against linked issues
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
rmartinoscar
changed the title
Boy132
changed the title
![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 (9)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (9)
70-76: Encode breadcrumb links when building URLsBreadcrumb links currently pass raw path (Line 73). If a segment contains “{}”, the generated URL can still break. Encode before passing to getUrl.
Update Line 73:
$breadcrumbs[self::getUrl(['path' => encode_path(ltrim($previousParts, '/'))])] = $part;
170-178: Encode path on redirects back to ListFilesSeveral redirects construct ListFiles::getUrl(['path' => $this->path]) without encoding; this can reintroduce the bug for “{}”. Encode in these spots:
- Line 177
return redirect(ListFiles::getUrl(['path' => encode_path($this->path)]));
300-308: Same redirect encoding needed after archiveEncode the ‘path’ parameter in the redirect.
- Line 307
return redirect(ListFiles::getUrl(['path' => encode_path($this->path)]));
326-328: Same redirect encoding needed after unarchive
- Line 327
return redirect(ListFiles::getUrl(['path' => encode_path($this->path)]));
396-401: Same redirect encoding needed after bulk archive
- Line 399
return redirect(ListFiles::getUrl(['path' => encode_path($this->path)]));
520-523: Same redirect encoding needed after upload/pull
- Line 522
return redirect(ListFiles::getUrl(['path' => encode_path($this->path)]));
446-449: Encode path when redirecting after “file already exists”
- Line 448
$this->redirect(self::getUrl(['path' => encode_path(dirname($path))]));
486-491: Encode path when redirecting after “folder already exists”
- Line 489
$this->redirect(self::getUrl(['path' => encode_path(dirname($path))]));
566-569: Encode path for SearchFiles URL as wellSearch page receives a ‘path’ parameter too; encode it to keep behavior consistent.
- Lines 566–569
->action(fn ($data) => redirect(SearchFiles::getUrl([ 'searchTerm' => $data['searchTerm'], 'path' => encode_path($this->path), ])))
🧹 Nitpick comments (2)
app/helpers.php (1)
114-119: encode_path: correct per-segment encoding; add docblock + tests for edge casesGood choice using rawurlencode on each segment; this preserves “/” while safely encoding “{}”, spaces, etc. Please add a brief docblock (usage: route param building, not filesystem ops) and unit tests covering: "{}", spaces, "#", "+", "%", leading/trailing slashes, and multi-segment inputs.
I can draft the docblock and PHPUnit tests if you want.
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (1)
108-118: DRY: centralize route-path encodingTo prevent future misses, add a tiny helper on this page and use it wherever building URLs:
private function routePath(string $path): string { return encode_path(ltrim($path, '/')); }Then replace array keys like ['path' => encode_path(join_paths(...))] with ['path' => $this->routePath(join_paths(...))].
Also applies to: 125-131, 180-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php(2 hunks)app/helpers.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (3)
app/Filament/Server/Resources/FileResource/Pages/EditFiles.php (2)
EditFiles(40-259)getUrl(244-247)app/helpers.php (2)
encode_path(115-118)join_paths(53-60)app/Filament/Server/Resources/FileResource/Pages/DownloadFiles.php (1)
DownloadFiles(19-71)
🔇 Additional comments (3)
app/Filament/Server/Resources/FileResource/Pages/ListFiles.php (3)
125-125: LGTM: directory “view” action now URL-safeUsing encode_path here ensures “{}” and other specials don’t break navigation.
130-130: LGTM: edit action path encoding is correctThis aligns with EditFiles route expecting an encoded ‘path’ segment.
184-184: LGTM: download action path encoding matches DownloadFiles::mount(rawurldecode())This prevents curly-brace routes from failing while remaining compatible with rawurldecode in DownloadFiles.
| } | ||
|
|
||
| return $file->canEdit() ? EditFiles::getUrl(['path' => join_paths($this->path, $file->name)]) : null; | ||
| return $file->canEdit() ? EditFiles::getUrl(['path' => encode_path(join_paths($this->path, $file->name))]) : null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encode directory record URL too to avoid inconsistent behavior and possible 500s
You’ve encoded file record URLs; do the same for directories (Line 110) so clicking the row for a directory named with “{}” can’t trip the router.
Change on Line 110:
return self::getUrl(['path' => encode_path(join_paths($this->path, $file->name))]);🤖 Prompt for AI Agents
In app/Filament/Server/Resources/FileResource/Pages/ListFiles.php around line
117, the URL returned for directory rows is not being passed through
encode_path, which can cause router errors for names with special characters;
update the directory branch to wrap join_paths($this->path, $file->name) with
encode_path before calling self::getUrl (i.e., call self::getUrl(['path' =>
encode_path(join_paths(...))]) ) so directories are encoded the same way as
files.
Closes #1553