Skip to content

show.html.erb #77 ダウンロードリンク追加 #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomya-n wants to merge 7 commits into
base: main
Choose a base branch
from
Open

show.html.erb #77 ダウンロードリンク追加 #84

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
19fde88
show.html.erb #77 ダウンロードリンク追加
tomya-n Oct 25, 2024
e7d0fac
show.html.erb #77 ダウンロードリンク修正
tomya-n Oct 27, 2024
e97249f
やっぱこれで動いた #77
tomya-n Nov 2, 2024
33724b7
Merge branch '77-add-download-link' into develop
tomya-n Nov 9, 2024
89007c7
Merge branch 'miminashi:main' into 77-add-download-link
tomya-n Nov 11, 2024
64a3d13
Merge branch '77-add-download-link' of https://github.com/tomya-n/ytd…
tomya-n Nov 11, 2024
888f1fd
Merge branch 'miminashi:main' into 77-add-download-link
tomya-n Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/views/archives/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
<div>追加日: <%= @archive.created_at.in_time_zone('Tokyo').strftime('%Y-%m-%d %H:%M') %></div>
<div>状態: <%= @archive.status %></div>
</div>
<div>
<%= link_to("ダウンロード", @archive.video, download: @archive.title ) %>

@coderabbitai coderabbitai Bot Oct 27, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider sanitizing the filename for download.

The archive title is used directly as the download filename, which could cause issues if it contains invalid characters.

Consider sanitizing the filename:

-<%= link_to("ダウンロード", @archive.video, download: @archive.title ) %>
+<%= link_to("ダウンロード", @archive.video, download: sanitize_filename(@archive.title)) %>

You'll need to implement a helper method like:

def sanitize_filename(filename)
  filename.strip.gsub(/[^0-9A-Za-z.\-]/, '_')
end

</div>
Comment on lines +21 to +24

@coderabbitai coderabbitai Bot Oct 27, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add a video presence check to prevent 404 errors.

The download link should only be displayed when a video is attached, similar to how the video player is conditionally rendered.

Consider wrapping the download link in a conditional check:

-      <div>
-        <%= link_to("ダウンロード", @archive.video, download: @archive.title ) %>
-      </div>
+      <% if @archive.video.attached? %>
+        <div>
+          <%= link_to("ダウンロード", @archive.video, download: @archive.title ) %>
+        </div>
+      <% end %>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<div>
<%= link_to("ダウンロード", @archive.video, download: @archive.title ) %>
</div>
<% if @archive.video.attached? %>
<div>
<%= link_to("ダウンロード", @archive.video, download: @archive.title ) %>
</div>
<% end %>

💡 Codebase verification

Authorization check required for video downloads

The codebase currently lacks any authentication or authorization mechanisms:

  • No authentication checks in ApplicationController
  • No authorization checks in ArchivesController for the show action
  • Direct video access through ActiveStorage without access control

This confirms the original review comment's concern about implementing proper authorization. The video downloads are currently accessible to anyone who has the URL.

Key locations requiring attention:

  • app/controllers/application_controller.rb: Add authentication mechanism
  • app/controllers/archives_controller.rb: Add authorization check in show action
  • app/views/archives/show.html.erb: Video download link should only be visible to authorized users
🔗 Analysis chain

Verify download authorization.

Ensure that only authorized users can download videos. Consider adding authorization checks.

Let's check if there are any existing authorization checks in the codebase:

Consider implementing authorization using one of these approaches:

  1. Add a before_action in ArchivesController
  2. Use a policy framework like Pundit
  3. Implement download tracking for security monitoring
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for authorization-related code
echo "Checking for authorization mechanisms..."
rg -l "before_action|authenticate|authorize|current_user" app/controllers/
rg -l "cancan|pundit|devise" Gemfile

Length of output: 245

Script:

#!/bin/bash
# Let's check the content of archives_controller.rb to see the authorization mechanisms
echo "Checking ArchivesController content..."
cat app/controllers/archives_controller.rb

# Let's also check for any application-wide authentication in application_controller
echo -e "\nChecking ApplicationController content..."
cat app/controllers/application_controller.rb

Length of output: 2239

</div>
<div class="article__log">
<h3>ログ</h3>
Expand Down