Repository navigation
Fix CSP blocking the PDF viewer when files are stored in a remote storage service - #134
antopalidi wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPDF controllers now add external Active Storage origins to ChangesPDF storage CSP support
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DocumentsController
participant NeedsPdfDocument
participant ActiveStorageBlob
DocumentsController->>NeedsPdfDocument: append_storage_host_to_csp
NeedsPdfDocument->>ActiveStorageBlob: resolve PDF blob URL
ActiveStorageBlob-->>NeedsPdfDocument: blob origin
NeedsPdfDocument-->>DocumentsController: update connect-src CSP
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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.
🧹 Nitpick comments (1)
spec/controllers/decidim/participatory_documents/admin/documents_controller_spec.rb (1)
24-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the promised admin local-storage and no-file cases.
These specs only cover remote storage. Add contexts for local storage and for a document without an attachment for both
pdf_viewerandedit_pdf. Assert that local storage keepsconnect-src 'self'and that no attachment does not append a storage host.🤖 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 `@spec/controllers/decidim/participatory_documents/admin/documents_controller_spec.rb` around lines 24 - 52, Add local-storage and no-attachment contexts to both the GET pdf_viewer and GET edit_pdf specs. Stub a local service URL and assert the CSP connect-src remains 'self'; create a document without an attachment and assert no storage host is appended, preserving the existing remote-storage coverage.
🤖 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
`@spec/controllers/decidim/participatory_documents/admin/documents_controller_spec.rb`:
- Around line 24-52: Add local-storage and no-attachment contexts to both the
GET pdf_viewer and GET edit_pdf specs. Stub a local service URL and assert the
CSP connect-src remains 'self'; create a document without an attachment and
assert no storage host is appended, preserving the existing remote-storage
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b48cd521-c3c1-4184-8f0a-eb1533f2f337
📒 Files selected for processing (5)
app/controllers/concerns/decidim/participatory_documents/needs_pdf_document.rbapp/controllers/decidim/participatory_documents/admin/documents_controller.rbapp/controllers/decidim/participatory_documents/documents_controller.rbspec/controllers/decidim/participatory_documents/admin/documents_controller_spec.rbspec/controllers/decidim/participatory_documents/documents_controller_spec.rb
There was a problem hiding this comment.
Pull request overview
This PR addresses a CSP issue where the PDF viewer (pdf.js) cannot fetch PDFs when the underlying ActiveStorage blob redirects to a remote storage/CDN origin (e.g., S3-compatible), by allowing that storage origin in the connect-src CSP directive for the PDF viewer/editor responses.
Changes:
- Add
before_actionhooks to adjust CSP for the publicpdf_viewerand adminpdf_viewer/edit_pdfactions. - Implement origin detection in
NeedsPdfDocumentto append the remote storage origin toconnect-srcwhen applicable. - Add controller specs validating CSP behavior (remote/local/no-file in public; remote cases in admin).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/controllers/decidim/participatory_documents/documents_controller_spec.rb | Adds public controller specs covering remote/local/no-file CSP behavior for pdf_viewer. |
| spec/controllers/decidim/participatory_documents/admin/documents_controller_spec.rb | Adds admin controller specs for remote-storage CSP behavior in pdf_viewer and edit_pdf. |
| app/controllers/decidim/participatory_documents/documents_controller.rb | Runs CSP adjustment before rendering the public pdf_viewer. |
| app/controllers/decidim/participatory_documents/admin/documents_controller.rb | Runs CSP adjustment before rendering admin pdf_viewer and edit_pdf. |
| app/controllers/concerns/decidim/participatory_documents/needs_pdf_document.rb | Adds logic to compute storage origin and append it to the connect-src CSP directive. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
606328e to
70cd420
Compare
microstudi
left a comment
microstudi
left a comment
There was a problem hiding this comment.
nice work, I have just a couple of questions
| env: | ||
| RUBY_VERSION: 3.2.6 | ||
| NODE_VERSION: 18.17.1 | ||
| NODE_OPTIONS: "--experimental-global-webcrypto" |
There was a problem hiding this comment.
Curious about this? what is it?
|
|
||
| - name: Install imagemagick and wkhtmltopdf | ||
| run: sudo apt install imagemagick wkhtmltopdf | ||
| run: sudo apt-get update && sudo apt-get install -y imagemagick wkhtmltopdf |
There was a problem hiding this comment.
| run: sudo apt-get update && sudo apt-get install -y imagemagick wkhtmltopdf | |
| run: sudo apt-get update && sudo apt-get install -y imagemagick libu2f-udev |
We don't need that anymore right?
With remote storage (S3-compatible / CDN) the PDF is fetched from the storage host, which is not allowed by the default CSP, so the browser blocks the request and the viewer stays empty. Local disk storage is not affected.
connect-srcCSP directive in the PDF viewer responses (publicpdf_viewer, adminpdf_viewer/edit_pdf).Summary by CodeRabbit
Bug Fixes
Tests