CP-13579 - Add page limit guards to PDF uploads - #90
Conversation
jewls618
left a comment
jewls618
left a comment
There was a problem hiding this comment.
Automated pass: a couple non-blocking warnings and suggestions, nothing blocking.
|
|
||
| def valid_event_params?(page_count, file_size) | ||
| return false if page_count.blank? || page_count <= 0 | ||
| return false unless params[:bucket].in?(BUCKETS) && params[:surface].in?(SURFACES) |
There was a problem hiding this comment.
[non-blocking] page_count and bucket are validated independently here, nothing checks that bucket actually matches page_count. A request with page_count: 5 and bucket: '201+' would still be logged as valid, which skews the size-bucket metrics this endpoint exists to produce. Consider deriving bucket from page_count server side instead of trusting the client value.
| BUCKETS = %w[121-150 151-200 201+].freeze | ||
| SURFACES = %w[builder dashboard].freeze | ||
|
|
||
| def create |
There was a problem hiding this comment.
[non-blocking] This endpoint has no rate limiting. Since verify_authenticity_token is skipped for create, any signed in user can call it repeatedly to flood the logs with fabricated events. Worth a throttle given its only job is metrics.
There was a problem hiding this comment.
Good callout. There's currently no throttle infra in the app (no Rack::Attack; cache is per-process memory_store, null_store in test), so a hand-rolled throttle here would be weak and untestable. The endpoint requires auth and stamps every hit with account/user id, so abuse is attributable. Happy to add Rack::Attack as a follow-up if you want it.
| return head :unauthorized if current_user.blank? | ||
|
|
||
| page_count = Integer(params[:page_count], exception: false) | ||
| file_size = params[:file_size].present? ? Integer(params[:file_size], exception: false) : nil |
There was a problem hiding this comment.
[non-blocking] page_count and file_size are parsed with Integer(..., exception: false) with no length limit on the input string first. Low risk since this is authenticated and log only, but a simple length check before parsing would be cheap insurance against a pathologically long digit string.
| // Best-practices copy for blocked dashboard uploads. The dashboard has no t() | ||
| // i18n, so the copy is held here (overridable per element via a | ||
| // data-page-limit-message attribute); mirrors the builder page_limit_body copy. | ||
| const PAGE_LIMIT_MESSAGE = 'This PDF has {page_count} pages, which exceeds the 120-page limit. Split it into smaller documents under 120 pages and add fields to each document.' |
There was a problem hiding this comment.
[non-blocking] This message string is duplicated in file_dropzone.js and again as a literal data-page-limit-message attribute in _upload_button.html.erb. None of them go through the template builder's i18n system, so a future copy change is likely to update one spot and drift from the others. Worth extracting to a single shared constant.
| // Best-practices copy for blocked dashboard uploads. The dashboard has no t() | ||
| // i18n, so the copy is held here (overridable per element via a | ||
| // data-page-limit-message attribute); mirrors the builder page_limit_body copy. | ||
| const PAGE_LIMIT_MESSAGE = 'This PDF has {page_count} pages, which exceeds the 120-page limit. Split it into smaller documents under 120 pages and add fields to each document.' |
There was a problem hiding this comment.
[non-blocking] Same message string is duplicated in dashboard_dropzone.js and hardcoded again as a data-page-limit-message attribute in _upload_button.html.erb. Worth extracting to one shared constant so future copy edits do not drift across the three locations.
| </button> | ||
| <input type="hidden" name="form_id" value="<%= form_id %>"> | ||
| <input id="upload_template" name="files[]" class="hidden" onchange="this.form.requestSubmit()" type="file" accept="image/*, application/pdf<%= ', .docx, .doc, .xlsx, .xls, .odt, .rtf' if Docuseal.advanced_formats? %>" multiple> | ||
| <input id="upload_template" name="files[]" class="hidden" onchange="this.form.requestSubmit()" type="file" data-page-limit-guard="true" data-page-limit-message="This PDF has {page_count} pages, which exceeds the 120-page limit. Split it into smaller documents under 120 pages and add fields to each document." accept="image/*, application/pdf<%= ', .docx, .doc, .xlsx, .xls, .odt, .rtf' if Docuseal.advanced_formats? %>" multiple> |
There was a problem hiding this comment.
[non-blocking] This data-page-limit-message value duplicates the PAGE_LIMIT_MESSAGE constant in dashboard_dropzone.js and file_dropzone.js. Since it matches the JS default already used as a fallback, this attribute may not even be needed. Worth extracting the copy to one shared source.
| @@ -0,0 +1,166 @@ | |||
| // Client-side page-count guard for PDF uploads (CP-13579). | |||
There was a problem hiding this comment.
[non-blocking] No unit tests cover this file directly, coverage comes only from the Capybara system specs that build real PDFs through HexaPDF. The project has no JS unit test framework yet so this is not a regression, but the regex based parsing (Count fallback, chunk boundary overlap) has enough branches that a few direct tests would help once JS unit testing exists.
There was a problem hiding this comment.
Agreed the regex/chunk-overlap branches deserve direct tests. The repo has no JS unit framework (no jest/vitest; package.json scripts are eslint-only), so coverage comes from the Capybara system specs using real HexaPDF-generated PDFs. Adding a JS framework felt out of scope for this PR — open to a follow-up to introduce one.
jewls618
left a comment
jewls618
left a comment
There was a problem hiding this comment.
Looks good 👍 a few non-blocking/optional comments
2 participants
CP-13579 - Add page limit guards to PDF uploads
What
Front-end guards that block PDF uploads over 120 pages on both upload surfaces, with best-practices guidance and blocked-attempt metrics:
upload()choke point (covers add-document, dropzone, preview, controls, replace). Over-limit uploads are blocked before any POST and open a best-practices modal advising users to split the file into documents under 120 pages and add fields to each document.uploadFileschoke point covers main, folder-card, and template-card drops), thefile-dropzoneelement (opt-in attribute, template uploads only), and the header upload button. Blocked uploads show an inline message with the same guidance.POST /page_limit_eventsendpoint (204, one structured log line with page count, size bucket 121-150/151-200/201+, surface, and stamped account/user) for counting oversize attempts by size. Records only, never enforces.Why
Uploading a very large PDF eagerly generates a preview image per page at upload time, which is slow and expensive. The guard steers users toward splitting large documents before upload, and the metrics show how often oversize uploads are attempted and at what sizes.
How to test
Manual: in the template builder attach a PDF with more than 120 pages and confirm the upload is blocked with the best-practices modal; repeat on the dashboard dropzone and the header upload button and confirm the inline message; confirm a small PDF and an encrypted PDF still upload normally.
Automated (all run in the branch worktree, each stable across repeated runs):
bundle exec rspec spec/system/template_builder_spec.rb— 6 examples, 0 failures (block + allow + encrypted-allow)bundle exec rspec spec/system/dashboard_spec.rb— 11 examples, 0 failures, 4 pending (pre-existing skips; both upload affordances + drop path)bundle exec rspec spec/requests/page_limit_events_spec.rb— 6 examples, 0 failures (204, log stamp, 422 validation, token auth, 401 incl. invalid token)bundle exec rspec spec/system/esign_spec.rb— 2 examples, 0 failures (eSign verify still accepts large PDFs)Mobile: no mobile-specific branch — mobile file pickers flow through the same guarded upload paths; the suite has no mobile emulation so this was not exercised separately. Screenshots of the new modal/message are still needed.
Screenshots