Repository navigation
Verify notification signatures before actions run - #76
Conversation
ParamsDecoder decodes params[:transloadit] without authenticating it, and replaces the raw field that a signature covers. Add an opt-in NotificationVerifier for notify_url controllers. It verifies the raw form field with the configured Auth Secret through Transloadit#verify_notification! and answers invalid notifications with 401 before any action runs. ParamsDecoder stays decoding-only, because browser form submissions carry no notification signature, and the README now says its data is unauthenticated. Closes #73. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Ruby 2.2 and 2.3 reject ensure inside do blocks, and Rails 5.0 on JRuby passes quirks_mode to a json gem that no longer accepts it, so the ordering test now stubs ParamsDecoder's decoding. Skip LFS smudging when Bundler checks out ruby-sdk for the Rails 8.0 job. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72bf3c491c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ```ruby | ||
| class TransloaditNotificationsController < ApplicationController | ||
| include Transloadit::Rails::NotificationVerifier | ||
| skip_forgery_protection |
There was a problem hiding this comment.
For applications using the Rails 3.x or 4.x versions still included in this repository's CI matrix, copying this example raises NoMethodError while defining the controller because those releases do not provide skip_forgery_protection. Consequently, users on supported versions cannot use the documented notification endpoint as written; document the appropriate version-specific skip_before_filter/skip_before_action :verify_authenticity_token alternative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the example now uses skip_before_action :verify_authenticity_token (Rails 4.0+) and names skip_before_filter for Rails 3.
skip_forgery_protection only exists from Rails 5.2, so the README now uses skip_before_action and names skip_before_filter for Rails 3. Pin the Rails 8.0 job to the ruby-sdk commit that adds verify_notification! until a released transloadit gem includes it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Why
ParamsDecoderdecodesparams[:transloadit]without checkingparams[:signature], and it replaces the raw field that the signature covers. A controller receivingnotify_urlcallbacks could therefore act on a forged Assembly status.Change
Add
Transloadit::Rails::NotificationVerifier, an opt-in concern for the controller that receives notifications:transloaditform field (request.request_parameters) with the Auth Secret fromconfig/transloadit.yml, throughTransloadit#verify_notification!from Add constant-time Assembly notification verification ruby-sdk#85.401before the action runs.MissingSecret) and authenticated but invalid JSON propagate as server errors, since they aren't authentication failures.transloadit_notification, with indifferent access.ParamsDecoderdecodedparams[:transloadit]first.transloadit_notification_clientcan be overridden when notifications are signed with another Auth Key.Compatibility:
ParamsDecoderkeeps decoding without verifying. Browser form submissions carry the request's sha384signature, not a notification signature, so verifying there would reject every upload form. The README now says its data is unauthenticated and points notification handlers to the verifier.Dependency: the gemspec still allows
transloadit >= 1.1.1, so the legacy Ruby 2.2/2.3 jobs keep resolving. With a transloadit gem older than 4.1.0, the verifier raisesNotImplementedErrornaming the required version instead of skipping verification.The README covers multiple Auth Keys, CSRF (
skip_before_action :verify_authenticity_token, orskip_before_filteron Rails 3), retries and replays, and idempotent handling.Testing
test/lib/test_notification_verifier.rbcovers:ParamsDecoderdecodedparamssha1:signature, tampered payload, missing signature) runs whenever the resolved transloadit gem supports verification.REQUIRE_NOTIFICATION_VERIFICATION=1, which fails if the resolved gem cannot verify, so the real-HMAC test cannot silently drop out. Until a released transloadit gem includes Add constant-time Assembly notification verification ruby-sdk#85, the job pins ruby-sdk to #85's head commit (d85aa53); switch it to the released version once that ships.Closes #73.
🤖 Generated with Claude Code