Conversation
Enable Google OAuth2 login for self-hosted DocuSeal when GOOGLE_CLIENT_ID env var is present. Adds GitHub Actions workflow to build amd64 images and push to Google Artifact Registry via Workload Identity Federation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove all "Unlock with DocuSeal Pro" placeholder gates from views - Enable previously gated features: editor/viewer roles, email reminders, reply-to fields, esign preferences, SMTP security options - Remove enterprise path blocking from errors controller - Remove Rollbar error tracking (JS client, meta tags, all Ruby calls) and replace with Rails.logger equivalents - Gut newsletter and enquiries phone-home controllers - Gut console redirect controller and remove upgrade/plans links - Remove "Powered by DocuSeal" attribution and demo signup links - Remove pricing links from Vue components (fields, formulas, conditions) - Remove upgrade button from navbar and Plans/Pro badge from settings - Clean up docuseal.rb constants (NEWSLETTER_URL, ENQUIRIES_URL) - Remove docuseal.com domain handling from application controller Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add COMPANY_LOGO_URL_KEY to AccountConfig model - Add logo URL to PersonalizationSettingsController ALLOWED_KEYS - Implement logo URL form in personalization settings with preview - Display custom logo in signing form banner when configured - Display custom logo in submission detail view when configured - Falls back to default DocuSeal logo when no URL is set Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Make omniauth_callbacks route conditional on GOOGLE_CLIENT_ID presence to match the User model's conditional devise :omniauthable. Fix erblint offenses: self-closing img tags, rescue modifier, and safe navigation chain length. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…etry Remove Pro feature gates, subscription code, and Rollbar telemetry
Use only short SHA and semver tags for immutable image references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Every outgoing webhook request is now signed with HMAC-SHA256 using a per-webhook signing key. Receivers can verify authenticity via three new headers: X-Webhook-Signature (sha256=<digest>), X-Webhook-Timestamp, and X-Webhook-Request-Id. Signing keys are auto-generated for new webhooks and lazily provisioned for existing ones. The webhook secret UI now displays the signing key with a verification example and regenerate option. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace DocuSeal orange quill logo with Kencove fence-post icon in dark blue (#1E3A5F). Update product name from DocuSeal to Kencove eSign. Applied across shared logo partial, public logo.svg, Vue logo component, and navbar title. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire up the existing SMS plumbing with actual Twilio delivery: SendSms module, SendSubmitterInvitationSmsJob, SMS settings form, re-send SMS button, and automatic dispatch alongside email in send_signature_requests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Google OAuth2 sign-in, Twilio SMS invitations and settings, HMAC-SHA256-signed webhooks, removes Rollbar client/server integration, updates branding to "Kencove eSign", simplifies promotional/permission UI, adds Google Artifact Registry Docker CI, and multiple related controller/view/model updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Devise as Devise/OmniAuth
participant Google as Google OAuth2
participant OmniauthCB as OmniauthCallbacksController
participant UsersLib as Users Module
participant DB as Database
User->>Devise: Click "Sign in with Google"
Devise->>Google: Redirect to Google auth
Google->>User: Authorize app
Google->>Devise: Callback with OAuth data
Devise->>OmniauthCB: Route to google_oauth2
OmniauthCB->>UsersLib: Users.from_omniauth(oauth)
UsersLib->>DB: Find or create user by email (domain checks)
DB-->>UsersLib: user or nil
UsersLib-->>OmniauthCB: user or nil
alt user found & active
OmniauthCB->>Devise: sign_in(user)
OmniauthCB-->>User: Redirect to dashboard
else
OmniauthCB-->>User: Redirect to sign_in with alert
end
sequenceDiagram
participant Submitter
participant Controller as SubmittersSendSmsController
participant DB as Database
participant Job as SendSubmitterInvitationSmsJob
participant SMS as SendSms
participant Twilio as Twilio API
Submitter->>Controller: POST /submitters/:slug/send_sms
Controller->>DB: Load submitter & check recent send_sms events
alt recent send exists (<=10h)
DB-->>Controller: recent event found
Controller-->>Submitter: Redirect back with alert
else
DB-->>Controller: no recent event
Controller->>Job: Enqueue job (submitter_id)
Controller->>DB: update submitter.sent_at if nil
Controller-->>Submitter: Redirect back with notice
Job->>DB: Load submitter & account SMS config
Job->>SMS: SendSms.call(...)
SMS->>Twilio: POST /Messages (Basic Auth)
Twilio-->>SMS: 200/4xx
alt Success
SMS-->>Job: success
else Failure
SMS-->>Job: raise SmsError
end
Job->>DB: Log SubmissionEvent(type: send_sms) and update sent_at
end
sequenceDiagram
participant App
participant Job as SendTestWebhookRequestJob
participant Webhook as WebhookUrl Model
participant Signer as SendWebhookRequest
participant Endpoint as External Webhook
App->>Job: Enqueue with webhook_url_id & payload
Job->>Webhook: Load webhook_url (includes signing_key)
Job->>Signer: call(webhook_url, payload)
Signer->>Signer: build JSON body
Signer->>Signer: compute timestamp and request-id
Signer->>Signer: compute HMAC-SHA256("timestamp.body", signing_key)
Signer->>Endpoint: POST body with headers X-Webhook-Signature, X-Webhook-Timestamp, X-Webhook-Request-Id
Endpoint-->>Signer: response
alt success
Signer-->>Job: success
else
Signer-->>Job: raise / retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/javascript/submission_form/signature_step.vue (1)
290-300:⚠️ Potential issue | 🟠 Major
href="#"withtarget="_blank"opens a useless blank tab.The disclosure link now points to
"#"but still carriestarget="_blank", so clicking it opens a new browser tab at the current page URL—confusing for users expecting to read an e-signature disclosure.Beyond the UX issue, if this disclosure link is required for e-signature legal compliance (ESIGN Act / UETA), rendering a non-functional link may not satisfy the disclosure obligation.
Consider either:
- Pointing to an actual disclosure page hosted in-app, or
- Removing the link (and
target="_blank") entirely if no disclosure is needed.Proposed fix (option 2 — remove dead link)
- {{ t('by_clicking_you_agree_to_the').replace('{button}', buttonText.charAt(0).toUpperCase() + buttonText.slice(1)) }} <a - href="#" - target="_blank" - > - <span class="inline md:hidden"> - {{ t('esignature_disclosure') }} - </span> - <span class="hidden md:inline"> - {{ t('electronic_signature_disclosure') }} - </span> - </a> + {{ t('by_clicking_you_agree_to_the').replace('{button}', buttonText.charAt(0).toUpperCase() + buttonText.slice(1)) }} + {{ t('electronic_signature_disclosure') }}app/views/shared/_settings_nav.html.erb (1)
26-30:⚠️ Potential issue | 🔴 CriticalUse
EncryptedConfig::SMS_KEYconstant instead of hardcoded string'submitter_invitation_sms'.The code checks for
'submitter_invitation_sms', which is anAccountConfigconstant used for message templates, not the actual SMS configuration storage key. PerEncryptedConfigmodel, SMS settings are stored underEncryptedConfig::SMS_KEY('sms_configs'). Replace with the correct constant to ensure the permission check validates against the actual SMS configuration.Also note: Line 68 has the same hardcoding issue with
'saml_configs'which lacks a defined constant.
🤖 Fix all issues with AI agents
In @.github/workflows/docker-gar.yml:
- Around line 25-31: The "Create .version file" step is vulnerable because
github.ref_name is interpolated directly into the shell; change the step to pass
github.ref_name into an environment variable (e.g., REF_NAME="${{
github.ref_name }}" ) and then write that variable to .version using a safe
write (e.g., printf '%s' "$REF_NAME" > .version) and similarly handle github.sha
via an env var (e.g., SHA="${{ github.sha }}" ) so the shell never directly
evaluates untrusted tag names; update the logic in the step (the block that
references github.ref_name and github.sha) to use these env vars and safe
printing instead of direct interpolation.
In `@app/controllers/sessions_controller.rb`:
- Around line 27-31: The after_sign_in_path_for method currently returns
params[:redir] unvalidated which allows open redirect; fix by validating
params[:redir] before returning: only accept a safe relative path (e.g. begins
with "/" but not "//" or contains a scheme/host) or parse it with URI.parse and
ensure its host is blank or matches request.host/your trusted host list; if
validation fails, fall back to super. Update the logic in after_sign_in_path_for
to perform this whitelist check on params[:redir] (referencing params[:redir]
and after_sign_in_path_for) and return super when the value is unsafe.
In `@app/controllers/sms_settings_controller.rb`:
- Around line 16-19: The rescue block in SmsSettingsController currently rescues
StandardError (rescue StandardError => e) and exposes e.message to the user;
change this to rescue only the expected exceptions (for example
Twilio::REST::RestError and ActiveRecord::RecordInvalid) or replace the flashed
e.message with a generic user-facing message (e.g., "An error occurred while
updating SMS settings") while logging the real error internally (use
Rails.logger.error or similar) before rendering :index with status
:unprocessable_content; update the rescue clause to list specific exception
classes or add a fallback rescue that logs the exception and sets a generic
flash[:alert] instead of e.message.
In `@app/controllers/submitters_send_sms_controller.rb`:
- Around line 7-17: The job is enqueued before persisting `@submitter` so if
`@submitter.save`! fails the SMS still sends; move the persistence step before
enqueuing (set `@submitter.sent_at` ||= Time.current and call `@submitter.save`!
first), then enqueue with
SendSubmitterInvitationSmsJob.perform_async('submitter_id' => `@submitter.id`)
only after a successful save, or alternatively use an after_commit/transaction
hook to enqueue the job so the job runs only if the record was committed.
In `@app/javascript/template_builder/payment_settings.vue`:
- Around line 197-203: The anchor rendered in payment_settings.vue when
!isConnected uses href="#" with target="_blank" and data-turbo="false", which
opens a confusing blank tab; either remove the dead link or make it inert:
change the v-if block that renders the anchor (the element using
v-if="!isConnected") to not render an anchor at all (replace with a plain
span/button or remove the block) or, if you must keep it visible, remove
target="_blank" and data-turbo="false" and set href to a real URL or to
"javascript:void(0)"/null so it doesn't open a new tab -- update the template
where the anchor is declared accordingly.
In `@app/jobs/send_submitter_invitation_sms_job.rb`:
- Line 7: The job currently calls Submitter.find(params['submitter_id']) which
will raise ActiveRecord::RecordNotFound if the record was deleted and cause
repeated Sidekiq retries; change this to use Submitter.find_by(id:
params['submitter_id']) and return early when nil (mirroring
SendTestWebhookRequestJob's pattern) so the job gracefully exits when the
submitter no longer exists.
In `@app/views/personalization_settings/_logo_form.html.erb`:
- Around line 13-15: Replace the hardcoded "Current logo" string in the span
inside the _logo_form.html.erb partial with a call to the Rails I18n helper (use
t()) so it follows the same translation pattern as the rest of the form; update
the locale YAML with an appropriate key (e.g.
views.personalization_settings._logo_form.current_logo or use the view-scoped
key via t('.current_logo')) and ensure the span text uses that translation.
In `@app/views/shared/_logo.html.erb`:
- Around line 2-3: ERB linter flags "No space detected where there should be a
single space" for the two long SVG <path> elements in _logo.html.erb (the <path
fill="currentColor" d="M45.92..." /> and the second <path fill="currentColor"
d="M47.19..." />); fix by either inserting the expected single-space(s) in the
offending attribute value(s) so the linter rule is satisfied, or wrap only those
two <path> lines with a targeted erb-lint disable/enable comment for the
specific rule (i.e., add an erb-lint disable before the first <path> and
re-enable after the second) to avoid changing SVG data.
In `@app/views/submissions/_send_sms_button.html.erb`:
- Line 3: The SMS button currently uses submitter.sent_at? to choose its label
which is incorrect because sent_at is set for any message type; update the view
logic that builds the button label (the call to button_title in
_send_sms_button.html.erb) to check SubmissionEvent records for this submitter
with event_type == 'send_sms' (e.g., SubmissionEvent.exists?(submitter_id:
submitter.id, event_type: 'send_sms')) and use that to decide between
t('re_send_sms') and t('send_sms'); keep the existing path helper
submitter_send_sms_index_path and disabled_with behavior unchanged.
In `@app/views/submissions/_send_sms.html.erb`:
- Around line 9-23: Replace the three hardcoded English strings in the
_send_sms.html.erb partial ("SMS not configured", "Configure SMS settings to
send text messages.", "Go to SMS settings") with i18n lookups using t(...) and
unique keys (e.g. use keys like submissions.sms.not_configured,
submissions.sms.configure_prompt, submissions.sms.go_to_settings) so the view
uses t('...') consistently; also add those keys and English values to the locale
file to avoid missing translation errors and ensure the link text uses the
t(...) result inside the anchor (refer to the partial and existing uses of t()
to mirror style).
In `@config/locales/i18n.yml`:
- Around line 356-357: Remove the duplicate yaml key by deleting the earlier
`sms_has_been_sent` entry (the one paired with `sms_has_been_sent_already`) and
keep the existing `sms_has_been_sent` definition that appears later; ensure only
a single `sms_has_been_sent` key remains and verify `sms_has_been_sent_already`
is still present and correctly placed to avoid YAML key collisions.
- Around line 649-650: The YAML file contains duplicate locale keys re_send_sms
and send_sms (previously defined earlier), which causes YAML linting errors;
remove the duplicate entries (or rename them if they are meant for a different
context) and ensure any code/view references use the single canonical key
(re_send_sms, send_sms) or the new name you choose so translations remain
consistent; update any tests/templates that referenced the removed duplicates to
point at the retained key.
In `@lib/send_sms.rb`:
- Around line 13-20: The Faraday connection in send_sms.rb (the conn created by
Faraday.new and used to post the Twilio request) lacks timeouts; update the
Faraday client construction to set reasonable open and read timeouts (e.g.
open_timeout: 8, read_timeout: 15) to mirror SendWebhookRequest, so the Sidekiq
worker won’t block indefinitely when Twilio is slow or unresponsive; modify the
Faraday.new options passed to conn (or its request options) to include these
timeout values before calling conn.basic_auth and conn.post.
- Line 18: The call to conn.basic_auth is invalid for Faraday 2.x; remove the
conn.basic_auth(account_sid, auth_token) call and instead configure basic auth
in the Faraday connection setup by adding the authorization middleware inside
the Faraday.new block (e.g. use conn.request :authorization, :basic,
account_sid, auth_token when building the Faraday connection object named conn
in lib/send_sms.rb) so authentication is applied via middleware rather than
calling basic_auth.
In `@lib/submissions/generate_result_attachments.rb`:
- Line 280: Replace the bare Rails.logger.error(e) calls with logging that
includes the full exception class, message and backtrace so you don't lose stack
traces; for example, change the Rails.logger.error(e) occurrences in
generate_result_attachments.rb (the error handlers around the PDF generation
paths and the Rollbar→Rails.logger replacements at the locations noted) to log
something like "#{e.class}: #{e.message}\n#{e.backtrace.join("\n")}" (or use
e.full_message if available) and apply the same replacement for all other
instances referenced (the other Rails.logger.error(e) occurrences in this file).
🧹 Nitpick comments (26)
app/controllers/console_redirect_controller.rb (1)
3-9: Consider whether this controller is still needed.With the action reduced to a bare
redirect_to root_path, this controller is essentially a no-op redirect. If the route can be replaced with a simple route-level redirect (e.g.,get '/console', to: redirect('/')inroutes.rb), the controller could be removed entirely.Not blocking — the current code is correct and harmless.
lib/submissions/timestamp_handler.rb (1)
56-59: Include the backtrace when logging the exception.
Rails.logger.error(e)only logse.message(viato_s), discarding the backtrace. Since thisrescuecatches allStandardErrors and silently falls back to a local timestamp, losing the stack trace will make production debugging difficult.♻️ Proposed fix
rescue StandardError => e - Rails.logger.error(e) + Rails.logger.error("#{e.class}: #{e.message}\n#{e.backtrace&.join("\n")}")lib/action_mailer_events_observer.rb (1)
29-32: Log the full backtrace, not just the exception message.
Rails.logger.error(e)callse.to_s, which logs only the message string and discards the backtrace. Since this is arescue StandardErrorin a mail delivery callback, you'll want the full stack trace for debugging production failures.🔧 Suggested fix
- Rails.logger.error(e) + Rails.logger.error("#{e.class}: #{e.message}\n#{e.backtrace&.join("\n")}")package.json (1)
36-37: Stray blank line left after removingrollbar.Nit: there's a trailing blank line at line 36 between
"qr-creator"and"sass".Suggested fix
"qr-creator": "^1.0.0", - "sass": "^1.62.1",lib/templates/process_document.rb (1)
158-161: Consider logging the backtrace along with the exception message.
Rails.logger.warn(e)only logse.to_s(the message). The backtrace is lost, which can make debugging Vips/Pdfium rendering failures difficult. Since thisrescuereturnsniland silently skips the page, having the backtrace would be valuable.This applies to all the Rollbar → Rails.logger replacements across the PR.
Suggested improvement
- Rails.logger.warn(e) + Rails.logger.warn("#{e.class}: #{e.message}\n#{e.backtrace&.first(5)&.join("\n")}")app/views/accounts/show.html.erb (1)
219-237: Remove deadif trueconditionals instead of leaving them as no-ops.Lines 219 and 238 use
<% if true %>which is functionally meaningless. If the intent is to make these blocks unconditional, remove theif/endwrapper entirely for clarity.♻️ Suggested fix (line 219 example)
- <% if true %> <% account_config = AccountConfig.find_or_initialize_by(account: current_account, key: AccountConfig::ENFORCE_SIGNING_ORDER_KEY) %> ... - <% end %>Apply the same pattern at line 238.
lib/params/base_validator.rb (1)
17-18: Consider logging the backtrace for unexpectedStandardError.
Rails.logger.error(e)only logse.to_s(the message). Unlike Rollbar, no backtrace is captured, making production debugging harder for unexpected errors.♻️ Suggested fix
- Rails.logger.error(e) + Rails.logger.error("#{e.class}: #{e.message}\n#{e.backtrace&.join("\n")}")app/views/email_smtp_settings/index.html.erb (1)
38-50: Sameif truedead conditional — remove the wrapper entirely.Same pattern as in
accounts/show.html.erb. Remove the no-op<% if true %>/<% end %>to keep the template clean.lib/docuseal.rb (1)
5-14: Stale branding constants — several URLs and identifiers still reference DocuSeal.
PRODUCT_NAMEwas updated to'Kencove eSign', butPRODUCT_URL,SUPPORT_EMAIL,GITHUB_URL,DISCORD_URL,TWITTER_URL, andTWITTER_HANDLEstill point to DocuSeal resources. If this is a white-label deployment, these should be updated or made configurable (e.g., via ENV vars) to avoid exposing end-users to upstream branding.app/views/shared/_settings_nav.html.erb (1)
68-68: Extract'saml_configs'to a constant for consistency.Lines 16, 21, and 37 use
EncryptedConfig::*_KEYconstants for configuration keys. Line 68 should follow the same pattern to reduce risk of typos and ensure maintainability. Note: This requires creating theEncryptedConfig::SAML_CONFIGS_KEYconstant, which is also referenced inapp/controllers/sso_settings_controller.rbas a hardcoded string.app/views/personalization_settings/_documents_copy_email_form.html.erb (2)
17-22: Remove deadif trueconditional.
<% if true %>is dead code — the block always renders. Strip theif/endwrapper entirely rather than leaving a no-op guard.♻️ Suggested cleanup
- <% if true %> <div class="form-control"> <%= ff.label :reply_to, t('reply_to'), class: 'label' %> <%= ff.email_field :reply_to, class: 'base-input', dir: 'auto', placeholder: t(:email) %> </div> - <% end %>
44-51: Same deadif trueconditional — remove the wrapper.♻️ Suggested cleanup
- <% if true %> <div class="flex items-center justify-between mx-1"> <span> <%= t('send_emails_automatically_on_completion') %> </span> <%= ff.check_box :enabled, { checked: ff.object.enabled != false, class: 'toggle' }, 'true', 'false' %> </div> - <% end %>app/views/personalization_settings/_logo_form.html.erb (1)
6-6: No server-side URL validation on the logo value.The
url_fieldprovides client-side validation only. The controller (PersonalizationSettingsController#create) accepts any string value for this key. Consider adding a server-side format check (e.g., must start withhttps://) to prevent storing invalid or potentially abusive URIs (e.g.,http://internal addresses).app/views/templates_preferences/_recipients.html.erb (1)
83-96: Remove deadif trueconditional — same pattern as in the other modified views.♻️ Suggested cleanup
- <% if true %> <%= form_for template, url: template_preferences_path(template), method: :post, html: { autocomplete: 'off', class: 'mt-2' }, data: { close_on_submit: false } do |f| %> ... <% end %> - <% end %>app/views/templates_preferences/_submitter_documents_copy_email_form.html.erb (1)
33-38: Remove deadif trueconditional — same cleanup as flagged in the other views..github/workflows/docker-gar.yml (1)
3-8: Tag pattern*.*.*is very permissive.This matches any tag containing two dots (e.g.,
foo.bar.baz). If the intent is to trigger only on semver tags, consider tightening the pattern (e.g.,v[0-9]*.[0-9]*.[0-9]*or[0-9]*.[0-9]*.[0-9]*).app/controllers/embed_scripts_controller.rb (1)
4-4:.freezewas removed from theDUMMY_SCRIPTconstant.The previous version froze this heredoc string. Freezing string constants is a Ruby best practice — it enables string deduplication and prevents accidental mutation. Consider restoring it.
♻️ Suggested fix
- DUMMY_SCRIPT = <<~JAVASCRIPT + DUMMY_SCRIPT = <<~JAVASCRIPT.freezeapp/jobs/send_test_webhook_request_job.rb (1)
39-48: Duplicated signing logic — consider reusingSendWebhookRequest.sign_request.The HMAC signing (key retrieval, timestamp, signature computation, header assignment) is duplicated between here and
lib/send_webhook_request.rblines 60–68. If the signing scheme changes, both sites must be updated in lockstep.Consider extracting a shared helper that returns the signing headers hash, usable by both the block-style and positional-argument-style Faraday calls:
♻️ Suggested refactor
In
lib/send_webhook_request.rb, add a method that returns headers:def signing_headers(body, webhook_url) key = webhook_url.ensure_signing_key! timestamp = Time.current.to_i.to_s signature = OpenSSL::HMAC.hexdigest('SHA256', key, "#{timestamp}.#{body}") { 'X-Webhook-Signature' => "sha256=#{signature}", 'X-Webhook-Timestamp' => timestamp, 'X-Webhook-Request-Id' => SecureRandom.uuid } endThen in this job:
- key = webhook_url.ensure_signing_key! - timestamp = Time.current.to_i.to_s - signature = OpenSSL::HMAC.hexdigest('SHA256', key, "#{timestamp}.#{body}") - Faraday.post(webhook_url.url, body, 'Content-Type' => 'application/json', 'User-Agent' => USER_AGENT, - 'X-Webhook-Signature' => "sha256=#{signature}", - 'X-Webhook-Timestamp' => timestamp, - 'X-Webhook-Request-Id' => SecureRandom.uuid, + **SendWebhookRequest.signing_headers(body, webhook_url), **webhook_url.secret.to_h)app/views/esign_settings/show.html.erb (1)
137-144: Redundant authorization check — Line 144 is always true inside the Line 137 guard.The outer
if can?(:manage, account_config)on Line 137 already ensures authorization. The identical check on Line 144 is redundant sinceaccount_confighasn't changed between the two checks.♻️ Simplification
- <% if can?(:manage, account_config) %> + <%# Authorization already checked on line 137 %> <%= form_for account_config, url: account_configs_path, method: :post do |f| %>Or simply remove the inner
if/endwrapper (lines 144 and 156) entirely.app/views/submissions/_logo.html.erb (1)
1-6: Consider moving the DB query out of the view partial.The
find_byquery on Line 1 runs a DB hit each time this partial is rendered. If the account configs are already eager-loaded elsewhere or this partial is rendered in a loop (e.g., listing multiple submissions), this becomes an N+1. Consider passinglogo_urlas a local variable from the controller or parent view.app/views/submit_form/_docuseal_logo.html.erb (1)
1-9: Duplicated logo-fetching logic across partials.The logo URL retrieval pattern (query
AccountConfig::COMPANY_LOGO_URL_KEY, conditional render) is duplicated between this file andapp/views/submissions/_logo.html.erb. Consider extracting a shared helper (e.g.,company_logo_url_for(account)) or a single partial to keep this DRY.app/controllers/application_controller.rb (1)
30-34: Consider preserving structured error context when logging.
Rails.logger.error(e)only logse.to_s(the message). You lose the backtrace and any Rollbar-style metadata. For rate-limit errors surfaced to users this is likely fine, but worth being intentional about.lib/submitters.rb (1)
170-190: Asymmetric guard-clause style between email and SMS paths.The email path uses an
ifblock (lines 172-178) while the SMS path usesnextstatements (lines 180-182). Thenextcalls will skip any future code appended after the SMS block for the current iteration. This is fine today but fragile if more per-submitter logic is added later. Consider wrapping the SMS path in anifblock to match the email pattern.Suggested refactor
- next if submitter.phone.blank? - next if submitter.declined_at? - next if submitter.preferences['send_sms'] == false - - if delay_seconds - SendSubmitterInvitationSmsJob.perform_in((delay_seconds + index).seconds, 'submitter_id' => submitter.id) - else - SendSubmitterInvitationSmsJob.perform_async('submitter_id' => submitter.id) - end + if submitter.phone.present? && !submitter.declined_at? && submitter.preferences['send_sms'] != false + if delay_seconds + SendSubmitterInvitationSmsJob.perform_in((delay_seconds + index).seconds, 'submitter_id' => submitter.id) + else + SendSubmitterInvitationSmsJob.perform_async('submitter_id' => submitter.id) + end + endapp/views/webhook_secret/show.erb (1)
36-36: Struct construction from potentially nil secret is safe but brittle.
@webhook_url.secret.to_a.firstyieldsnilwhen secret is empty, and*nilsplats to nothing, giving a Struct with nil fields. This works but is non-obvious. A brief comment or explicit fallback (|| ['', '']) would improve clarity.app/controllers/webhook_secret_controller.rb (1)
6-8: Side-effectfulshowaction performs a write on GET request.
ensure_signing_key!callsupdate!on the model when no key exists. While this is a pragmatic lazy-initialization pattern, a GET action with a database write can be surprising and isn't idempotent from a caching/proxy perspective. Consider moving the initialization to theupdateaction or a dedicatedcreate-style endpoint if feasible.app/controllers/submitters_send_sms_controller.rb (1)
7-12: Minor TOCTOU window on the rate-limit check.The
SubmissionEvent.exists?check and the actual job enqueue are not atomic. Concurrent requests could both pass the check and enqueue duplicate SMS jobs. The practical risk is low (duplicate SMS, not data corruption), but worth noting if this endpoint is exposed to automated triggers.
| def after_sign_in_path_for(...) | ||
| if params[:redir].present? | ||
| return console_redirect_index_path(redir: params[:redir]) if params[:redir].starts_with?(Docuseal::CONSOLE_URL) | ||
|
|
||
| return params[:redir] | ||
| end | ||
| return params[:redir] if params[:redir].present? | ||
|
|
||
| super | ||
| end |
There was a problem hiding this comment.
Open redirect vulnerability — params[:redir] is used without validation.
An attacker can craft a login link like ?redir=https://evil.com and redirect users to a malicious site after authentication. This is a classic phishing vector (CWE-601).
Validate that the redirect target is a relative path or belongs to a trusted host before using it.
🛡️ Proposed fix
def after_sign_in_path_for(...)
- return params[:redir] if params[:redir].present?
+ if params[:redir].present?
+ uri = URI.parse(params[:redir])
+ return params[:redir] if uri.relative? || uri.host == request.host
+ end
super
end🤖 Prompt for AI Agents
In `@app/controllers/sessions_controller.rb` around lines 27 - 31, The
after_sign_in_path_for method currently returns params[:redir] unvalidated which
allows open redirect; fix by validating params[:redir] before returning: only
accept a safe relative path (e.g. begins with "/" but not "//" or contains a
scheme/host) or parse it with URI.parse and ensure its host is blank or matches
request.host/your trusted host list; if validation fails, fall back to super.
Update the logic in after_sign_in_path_for to perform this whitelist check on
params[:redir] (referencing params[:redir] and after_sign_in_path_for) and
return super when the value is unsafe.
| rescue StandardError => e | ||
| flash[:alert] = e.message | ||
|
|
||
| render :index, status: :unprocessable_content |
There was a problem hiding this comment.
Rescuing StandardError is too broad and may leak internal error details.
This catches all exceptions including programming errors (e.g., NoMethodError, NameError) and exposes e.message directly to the user via flash. Narrow the rescue to the specific exception types you expect (e.g., Twilio API errors or validation errors), or use a generic user-facing message instead of e.message.
Suggested fix
- rescue StandardError => e
- flash[:alert] = e.message
+ rescue Twilio::REST::TwilioError, ActiveRecord::RecordInvalid => e
+ Rails.logger.error(e)
+ flash[:alert] = I18n.t('unable_to_save_sms_settings')📝 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.
| rescue StandardError => e | |
| flash[:alert] = e.message | |
| render :index, status: :unprocessable_content | |
| rescue Twilio::REST::TwilioError, ActiveRecord::RecordInvalid => e | |
| Rails.logger.error(e) | |
| flash[:alert] = I18n.t('unable_to_save_sms_settings') | |
| render :index, status: :unprocessable_content |
🤖 Prompt for AI Agents
In `@app/controllers/sms_settings_controller.rb` around lines 16 - 19, The rescue
block in SmsSettingsController currently rescues StandardError (rescue
StandardError => e) and exposes e.message to the user; change this to rescue
only the expected exceptions (for example Twilio::REST::RestError and
ActiveRecord::RecordInvalid) or replace the flashed e.message with a generic
user-facing message (e.g., "An error occurred while updating SMS settings")
while logging the real error internally (use Rails.logger.error or similar)
before rendering :index with status :unprocessable_content; update the rescue
clause to list specific exception classes or add a fallback rescue that logs the
exception and sets a generic flash[:alert] instead of e.message.
| if SubmissionEvent.exists?(submitter: @submitter, | ||
| event_type: 'send_sms', | ||
| created_at: 10.hours.ago..Time.current) | ||
| return redirect_back(fallback_location: submission_path(@submitter.submission), | ||
| alert: I18n.t('sms_has_been_sent_already')) | ||
| end | ||
|
|
||
| SendSubmitterInvitationSmsJob.perform_async('submitter_id' => @submitter.id) | ||
|
|
||
| @submitter.sent_at ||= Time.current | ||
| @submitter.save! |
There was a problem hiding this comment.
Job is enqueued before the model is persisted — if save! raises, the SMS is still sent.
If @submitter.save! on line 17 fails (e.g., validation error), the job enqueued on line 14 will still execute and send the SMS. Consider reordering so the model is saved first, or wrapping both in a transaction with the job enqueued after commit.
Proposed fix: reorder to save before enqueue
- SendSubmitterInvitationSmsJob.perform_async('submitter_id' => `@submitter.id`)
-
`@submitter.sent_at` ||= Time.current
`@submitter.save`!
+
+ SendSubmitterInvitationSmsJob.perform_async('submitter_id' => `@submitter.id`)📝 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.
| if SubmissionEvent.exists?(submitter: @submitter, | |
| event_type: 'send_sms', | |
| created_at: 10.hours.ago..Time.current) | |
| return redirect_back(fallback_location: submission_path(@submitter.submission), | |
| alert: I18n.t('sms_has_been_sent_already')) | |
| end | |
| SendSubmitterInvitationSmsJob.perform_async('submitter_id' => @submitter.id) | |
| @submitter.sent_at ||= Time.current | |
| @submitter.save! | |
| if SubmissionEvent.exists?(submitter: `@submitter`, | |
| event_type: 'send_sms', | |
| created_at: 10.hours.ago..Time.current) | |
| return redirect_back(fallback_location: submission_path(`@submitter.submission`), | |
| alert: I18n.t('sms_has_been_sent_already')) | |
| end | |
| `@submitter.sent_at` ||= Time.current | |
| `@submitter.save`! | |
| SendSubmitterInvitationSmsJob.perform_async('submitter_id' => `@submitter.id`) |
🤖 Prompt for AI Agents
In `@app/controllers/submitters_send_sms_controller.rb` around lines 7 - 17, The
job is enqueued before persisting `@submitter` so if `@submitter.save`! fails the
SMS still sends; move the persistence step before enqueuing (set
`@submitter.sent_at` ||= Time.current and call `@submitter.save`! first), then
enqueue with SendSubmitterInvitationSmsJob.perform_async('submitter_id' =>
`@submitter.id`) only after a successful save, or alternatively use an
after_commit/transaction hook to enqueue the job so the job runs only if the
record was committed.
| <a | ||
| v-if="!isConnected" | ||
| class="block link text-center mt-1" | ||
| href="https://www.docuseal.com/blog/accept-payments-and-request-signatures-with-ease" | ||
| href="#" | ||
| target="_blank" | ||
| data-turbo="false" | ||
| >{{ t('learn_more') }}</a> |
There was a problem hiding this comment.
Dead "Learn More" link with stale target="_blank" will open a confusing blank tab.
href="#" combined with target="_blank" opens a new browser tab pointing at the current page. Either hide/remove the link entirely until a real destination is available, or at minimum drop target="_blank" and data-turbo="false".
Suggested fix
Option A – remove the link:
- <a
- v-if="!isConnected"
- class="block link text-center mt-1"
- href="#"
- target="_blank"
- data-turbo="false"
- >{{ t('learn_more') }}</a>Option B – keep the link but strip stale attributes:
<a
v-if="!isConnected"
class="block link text-center mt-1"
href="#"
- target="_blank"
- data-turbo="false"
>{{ t('learn_more') }}</a>📝 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.
| <a | |
| v-if="!isConnected" | |
| class="block link text-center mt-1" | |
| href="https://www.docuseal.com/blog/accept-payments-and-request-signatures-with-ease" | |
| href="#" | |
| target="_blank" | |
| data-turbo="false" | |
| >{{ t('learn_more') }}</a> |
| <a | |
| v-if="!isConnected" | |
| class="block link text-center mt-1" | |
| href="https://www.docuseal.com/blog/accept-payments-and-request-signatures-with-ease" | |
| href="#" | |
| target="_blank" | |
| data-turbo="false" | |
| >{{ t('learn_more') }}</a> | |
| <a | |
| v-if="!isConnected" | |
| class="block link text-center mt-1" | |
| href="#" | |
| >{{ t('learn_more') }}</a> |
🤖 Prompt for AI Agents
In `@app/javascript/template_builder/payment_settings.vue` around lines 197 - 203,
The anchor rendered in payment_settings.vue when !isConnected uses href="#" with
target="_blank" and data-turbo="false", which opens a confusing blank tab;
either remove the dead link or make it inert: change the v-if block that renders
the anchor (the element using v-if="!isConnected") to not render an anchor at
all (replace with a plain span/button or remove the block) or, if you must keep
it visible, remove target="_blank" and data-turbo="false" and set href to a real
URL or to "javascript:void(0)"/null so it doesn't open a new tab -- update the
template where the anchor is declared accordingly.
| sms_has_been_sent: SMS has been sent. | ||
| sms_has_been_sent_already: SMS has been sent already. |
There was a problem hiding this comment.
Remove duplicate sms_has_been_sent key.
There is already an sms_has_been_sent entry later in the same locale (Line 594). Duplicate YAML keys can override or fail lint; please consolidate to a single key.
🧹 Suggested fix (keep the existing definition later)
- sms_has_been_sent: SMS has been sent.
sms_has_been_sent_already: SMS has been sent already.📝 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.
| sms_has_been_sent: SMS has been sent. | |
| sms_has_been_sent_already: SMS has been sent already. | |
| sms_has_been_sent_already: SMS has been sent already. |
🤖 Prompt for AI Agents
In `@config/locales/i18n.yml` around lines 356 - 357, Remove the duplicate yaml
key by deleting the earlier `sms_has_been_sent` entry (the one paired with
`sms_has_been_sent_already`) and keep the existing `sms_has_been_sent`
definition that appears later; ensure only a single `sms_has_been_sent` key
remains and verify `sms_has_been_sent_already` is still present and correctly
placed to avoid YAML key collisions.
| re_send_sms: Re-send SMS | ||
| send_sms: Send SMS |
There was a problem hiding this comment.
YAML key duplication (re_send_sms, send_sms).
These keys already exist earlier in the same locale (Line 417-418), and YAMLlint flags duplicates here. Keep a single definition (or rename for different contexts).
🧹 Suggested fix (remove duplicates)
- re_send_sms: Re-send SMS
- send_sms: Send SMS📝 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.
| re_send_sms: Re-send SMS | |
| send_sms: Send SMS |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 649-649: duplication of key "re_send_sms" in mapping
(key-duplicates)
[error] 650-650: duplication of key "send_sms" in mapping
(key-duplicates)
🤖 Prompt for AI Agents
In `@config/locales/i18n.yml` around lines 649 - 650, The YAML file contains
duplicate locale keys re_send_sms and send_sms (previously defined earlier),
which causes YAML linting errors; remove the duplicate entries (or rename them
if they are meant for a different context) and ensure any code/view references
use the single canonical key (re_send_sms, send_sms) or the new name you choose
so translations remain consistent; update any tests/templates that referenced
the removed duplicates to point at the retained key.
| conn = Faraday.new do |f| | ||
| f.request :url_encoded | ||
| f.adapter Faraday.default_adapter | ||
| end | ||
|
|
||
| conn.basic_auth(account_sid, auth_token) | ||
|
|
||
| response = conn.post(url, { 'To' => to, 'From' => from_number, 'Body' => body }) |
There was a problem hiding this comment.
No timeouts configured on the Faraday connection.
Unlike SendWebhookRequest which sets read_timeout: 15 and open_timeout: 8, this Twilio call has no timeouts. A slow or unresponsive Twilio endpoint could block the Sidekiq worker indefinitely.
🛡️ Proposed fix
conn = Faraday.new do |f|
f.request :url_encoded
f.adapter Faraday.default_adapter
+ f.options.open_timeout = 10
+ f.options.read_timeout = 15
end🤖 Prompt for AI Agents
In `@lib/send_sms.rb` around lines 13 - 20, The Faraday connection in send_sms.rb
(the conn created by Faraday.new and used to post the Twilio request) lacks
timeouts; update the Faraday client construction to set reasonable open and read
timeouts (e.g. open_timeout: 8, read_timeout: 15) to mirror SendWebhookRequest,
so the Sidekiq worker won’t block indefinitely when Twilio is slow or
unresponsive; modify the Faraday.new options passed to conn (or its request
options) to include these timeout values before calling conn.basic_auth and
conn.post.
| f.adapter Faraday.default_adapter | ||
| end | ||
|
|
||
| conn.basic_auth(account_sid, auth_token) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -maxdepth 3 -name "Gemfile*" 2>/dev/null | head -5Repository: kencove/docuseal
Length of output: 42
🏁 Script executed:
cd /tmp && git ls-files | grep -E "Gemfile|gemspec" | head -10Repository: kencove/docuseal
Length of output: 128
🏁 Script executed:
cat Gemfile 2>/dev/null || cat Gemfile.lock 2>/dev/null | head -100Repository: kencove/docuseal
Length of output: 1928
🏁 Script executed:
cat lib/send_sms.rbRepository: kencove/docuseal
Length of output: 831
🌐 Web query:
Faraday basic_auth method removed Faraday 2.0 API changes
💡 Result:
In Faraday 2.0, the Faraday::Connection auth helper methods (notably #basic_auth and #token_auth) were dropped as part of the 2.0 API cleanup/refactor. The project consolidated the old auth middlewares into a single Authorization request middleware. [1], [2]
What to change
Old (Faraday 1.x helper; removed in 2.x):
conn = Faraday.new(url: base_url)
conn.basic_auth(username, password) # removed in Faraday 2.xFaraday 2.x (use Authorization middleware):
conn = Faraday.new(url: base_url) do |f|
f.request :authorization, :basic, username, password
endThis uses Faraday::Request::Authorization, which builds the Authorization header; for :basic it expects username, password, otherwise it generally expects a single value (and can take a Proc/lambda). [3]
Why you saw “use #request(:basic_auth, ...) instead”
In late Faraday 1.x, Connection#basic_auth emitted a deprecation warning suggesting middleware usage; however Faraday 2.x ultimately standardized on :authorization, not :basic_auth. The Faraday maintainers explicitly note that Faraday 2.x will only allow the :authorization middleware for these auth cases. [1], [2]
Sources:
[1] Faraday 2.0 discussion (deprecations + “Faraday 2.x will only allow :authorization”) — GitHub Discussion #1358. (github.com)
[2] PR removing Connection auth helpers + merging auth middleware — GitHub PR #1308. (github.com)
[3] Faraday::Request::Authorization API (params for :basic, Proc support, etc.) — RubyDoc for faraday 2.x. (rubydoc.info)
Citations:
- 1: Faraday 2.0 lostisland/faraday#1358
- 2: Drop deprecated auth helpers from Connection and refactor auth middleware lostisland/faraday#1308
- 3: https://www.rubydoc.info/gems/faraday/Faraday/Request/Authorization
Faraday::Connection#basic_auth was removed in Faraday 2.x.
If this project installs Faraday 2.0 or later, line 18 will raise NoMethodError at runtime. Faraday 2.x consolidates authentication into the :authorization middleware. Move basic auth to the middleware block instead:
Recommended fix
conn = Faraday.new do |f|
f.request :url_encoded
+ f.request :authorization, :basic, account_sid, auth_token
f.adapter Faraday.default_adapter
end
- conn.basic_auth(account_sid, auth_token)
-
response = conn.post(url, { 'To' => to, 'From' => from_number, 'Body' => body })🤖 Prompt for AI Agents
In `@lib/send_sms.rb` at line 18, The call to conn.basic_auth is invalid for
Faraday 2.x; remove the conn.basic_auth(account_sid, auth_token) call and
instead configure basic auth in the Faraday connection setup by adding the
authorization middleware inside the Faraday.new block (e.g. use conn.request
:authorization, :basic, account_sid, auth_token when building the Faraday
connection object named conn in lib/send_sms.rb) so authentication is applied
via middleware rather than calling basic_auth.
| page.flatten_annotations | ||
| rescue StandardError => e | ||
| Rollbar.error(e) if defined?(Rollbar) | ||
| Rails.logger.error(e) |
There was a problem hiding this comment.
Rails.logger.error(e) loses the backtrace — log the full exception.
When you pass an exception object to Rails.logger.error, Ruby calls .to_s on it, which yields only the message — the backtrace is silently dropped. This applies to all the Rollbar→Rails.logger replacements in this file (lines 280, 734, 749, 834, 856). Since these are error-recovery paths in PDF generation, losing stack traces will make production debugging significantly harder.
Proposed fix (apply the same pattern at lines 734, 749, 834, 856)
- Rails.logger.error(e)
+ Rails.logger.error("#{e.class}: #{e.message}\n#{e.backtrace&.join("\n")}")📝 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.
| Rails.logger.error(e) | |
| Rails.logger.error("#{e.class}: #{e.message}\n#{e.backtrace&.join("\n")}") |
🤖 Prompt for AI Agents
In `@lib/submissions/generate_result_attachments.rb` at line 280, Replace the bare
Rails.logger.error(e) calls with logging that includes the full exception class,
message and backtrace so you don't lose stack traces; for example, change the
Rails.logger.error(e) occurrences in generate_result_attachments.rb (the error
handlers around the PDF generation paths and the Rollbar→Rails.logger
replacements at the locations noted) to log something like "#{e.class}:
#{e.message}\n#{e.backtrace.join("\n")}" (or use e.full_message if available)
and apply the same replacement for all other instances referenced (the other
Rails.logger.error(e) occurrences in this file).
Add QEMU setup and linux/arm64 platform to the GAR build workflow so the image runs natively on Apple Silicon for local dev. Bump timeout to 60m for cross-platform QEMU builds. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/docker-gar.yml (2)
64-72:docker/build-push-action@v6addsprovenance=mode=maxby default for public repositories, exposing build argument values.If the GitHub repository is public, provenance attestations with
mode=maxare automatically added to the image. If you're usingdocker/build-push-actionto build images for code in a public GitHub repository, the provenance attestations attached to your image by default contains the values of build arguments. If the Dockerfile passes any secrets or internal identifiers viaARG, they will be included in the publicly readable provenance manifest.Additionally, for multi-platform builds, the provenance attestations create extra manifest entries that appear as
unknown/unknownarchitecture entries alongsidelinux/arm64andlinux/amd64in the GAR UI.Consider explicitly configuring provenance to avoid unintentional data exposure:
🛠️ Proposed fix
with: context: . push: true platforms: linux/amd64,linux/arm64 tags: ${{ steps.meta.outputs.tags }} + provenance: mode=min cache-from: type=gha cache-to: type=gha,mode=max
mode=minretains SLSA compliance without including build arg values or rich layer metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-gar.yml around lines 64 - 72, The GitHub Actions step using docker/build-push-action@v6 ("Build and push Docker image") currently relies on the default provenance behavior which can leak ARG values for public repos; update that step's with: block to explicitly set provenance to a safer value (e.g., provenance: mode=min) to avoid including build-arg and rich layer metadata in public attestations (especially important for multi-platform builds that create extra unknown/unknown entries).
20-23: All third-party actions are pinned to mutable version tags, not immutable commit SHAs.If an attacker gains access to a GitHub account that publishes actions, they can commit malicious code and then update existing Git tags to point to a new malicious commit SHA. This exact scenario happened in March 2025 when
tj-actions/changed-fileswas compromised — more than 350 Git tags were retargeted, affecting over 23,000 repositories."Pinning an action to a full-length commit SHA is currently the only way to use an action as an immutable release."
♻️ Proposed fix (add SHA pins; populate the SHAs for the exact versions in use)
- uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4- uses: google-github-actions/auth@v2 + uses: google-github-actions/auth@<SHA> # v2- uses: docker/login-action@v3 + uses: docker/login-action@<SHA> # v3- uses: docker/metadata-action@v5 + uses: docker/metadata-action@<SHA> # v5- uses: docker/setup-qemu-action@v3 + uses: docker/setup-qemu-action@<SHA> # v3- uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@<SHA> # v3- uses: docker/build-push-action@v6 + uses: docker/build-push-action@<SHA> # v6Populate each
<SHA>with the full 40-character commit SHA for the pinned version. Tools like Renovate (helpers:pinGitHubActionDigestsToSemver) orcaarlos0/pinataautomate this and keep the human-readable tag in a trailing comment for Dependabot/Renovate auto-updates.Also applies to: 35-35, 42-42, 50-50, 59-59, 62-62, 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-gar.yml around lines 20 - 23, The workflow uses third-party actions pinned to mutable tags (e.g., uses: actions/checkout@v4); replace each mutable tag reference with the corresponding full 40-character commit SHA (for every "uses:" entry such as actions/checkout@v4 and the other third-party actions referenced on the workflow lines called out) so the action references are immutable — keep the human-readable tag as a trailing comment if desired and populate each <SHA> with the exact commit SHA for the pinned version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/docker-gar.yml:
- Around line 53-56: The `type=raw,value=latest,enable=${{ github.ref ==
'refs/heads/kencove' }}` condition only enables the latest tag for branch
pushes, so pushing a semver tag (e.g. refs/tags/v1.2.3) won't update :latest;
change the enable expression to also allow tag pushes (for example: enable when
github.ref == 'refs/heads/kencove' || startsWith(github.ref, 'refs/tags/'), or
use github.ref_type == 'tag') so the `latest` raw tag is published on
release/tag pushes as well as the kencove branch.
- Around line 5-6: The tags glob "*.*.*" is too broad and matches non-semver
tags; update the tags entry so it only matches semantic versions (for example
replace "*.*.*" with a stricter pattern like "v?\\d+\\.\\d+\\.\\d*(?:[-+].*)?"
or explicit "v?\\d+\\.\\d+\\.\\d" depending on your allowed variants), or remove
the manual filter and use the docker/metadata-action semver filter instead so
the tags: field only triggers for valid semver releases.
---
Duplicate comments:
In @.github/workflows/docker-gar.yml:
- Around line 25-31: The Create .version file step is vulnerable because it
writes unescaped github.ref_name/github.sha into the shell causing possible
command substitution; update the step to avoid shell interpretation by quoting
the values and using a safe printer (e.g., use printf '%s' with the variable
expansions) instead of unquoted echo, and apply this change for both branches
(when github.ref_type == "tag" and the else branch) so github.ref_name and
github.sha cannot execute as shell commands.
---
Nitpick comments:
In @.github/workflows/docker-gar.yml:
- Around line 64-72: The GitHub Actions step using docker/build-push-action@v6
("Build and push Docker image") currently relies on the default provenance
behavior which can leak ARG values for public repos; update that step's with:
block to explicitly set provenance to a safer value (e.g., provenance: mode=min)
to avoid including build-arg and rich layer metadata in public attestations
(especially important for multi-platform builds that create extra
unknown/unknown entries).
- Around line 20-23: The workflow uses third-party actions pinned to mutable
tags (e.g., uses: actions/checkout@v4); replace each mutable tag reference with
the corresponding full 40-character commit SHA (for every "uses:" entry such as
actions/checkout@v4 and the other third-party actions referenced on the workflow
lines called out) so the action references are immutable — keep the
human-readable tag as a trailing comment if desired and populate each <SHA> with
the exact commit SHA for the pinned version.
| tags: | ||
| - "*.*.*" |
There was a problem hiding this comment.
Overly broad tag glob may fire on non-semver tags.
The pattern *.*.* matches any tag with two dots (e.g., release.candidate.1, hotfix.pr.42). Restrict to semver if that is the intent:
🛠️ Tighter semver pattern
tags:
- - "*.*.*"
+ - "v[0-9]+.[0-9]+.[0-9]+"Or use the semver filter from docker/metadata-action and drop this restriction entirely.
📝 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.
| tags: | |
| - "*.*.*" | |
| tags: | |
| - "v[0-9]+.[0-9]+.[0-9]+" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docker-gar.yml around lines 5 - 6, The tags glob "*.*.*"
is too broad and matches non-semver tags; update the tags entry so it only
matches semantic versions (for example replace "*.*.*" with a stricter pattern
like "v?\\d+\\.\\d+\\.\\d*(?:[-+].*)?" or explicit "v?\\d+\\.\\d+\\.\\d"
depending on your allowed variants), or remove the manual filter and use the
docker/metadata-action semver filter instead so the tags: field only triggers
for valid semver releases.
.github/workflows/docker-gar.yml
Outdated
| tags: | | ||
| type=semver,pattern={{version}} | ||
| type=sha,prefix=,format=short | ||
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/kencove' }} |
There was a problem hiding this comment.
latest tag is never updated when a semver release tag is pushed.
The condition enable=${{ github.ref == 'refs/heads/kencove' }} means latest is only published on branch pushes, not on tag pushes. When a v1.2.3 tag is pushed, the workflow runs but the refs/heads/kencove condition is false, so latest is never refreshed to point at the new versioned image. This is likely unintentional — consumers pulling :latest will get the branch-tip image, not the most recent tagged release.
If the intent is for latest to track the newest tagged release, update the condition:
🛠️ Proposed fix
tags: |
type=semver,pattern={{version}}
type=sha,prefix=,format=short
- type=raw,value=latest,enable=${{ github.ref == 'refs/heads/kencove' }}
+ type=raw,value=latest,enable=${{ github.ref_type == 'tag' }}If branch-tip tracking is the intent (e.g., latest == current kencove state), document this clearly to avoid consumer confusion.
📝 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.
| tags: | | |
| type=semver,pattern={{version}} | |
| type=sha,prefix=,format=short | |
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/kencove' }} | |
| tags: | | |
| type=semver,pattern={{version}} | |
| type=sha,prefix=,format=short | |
| type=raw,value=latest,enable=${{ github.ref_type == 'tag' }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docker-gar.yml around lines 53 - 56, The
`type=raw,value=latest,enable=${{ github.ref == 'refs/heads/kencove' }}`
condition only enables the latest tag for branch pushes, so pushing a semver tag
(e.g. refs/tags/v1.2.3) won't update :latest; change the enable expression to
also allow tag pushes (for example: enable when github.ref ==
'refs/heads/kencove' || startsWith(github.ref, 'refs/tags/'), or use
github.ref_type == 'tag') so the `latest` raw tag is published on release/tag
pushes as well as the kencove branch.
- Add configurable auto-provisioning for Google OAuth users (GOOGLE_AUTO_CREATE, GOOGLE_ALLOWED_DOMAIN, GOOGLE_AUTO_CREATE_ROLE) - Replace hd: 'kencove.com' with env-driven GOOGLE_ALLOWED_DOMAIN - Users.from_omniauth now creates accounts on first login when enabled - Switch Docker image build from GitHub Actions QEMU to Cloud Build E2_HIGHCPU_32 (18 min vs 60+ min timeout on GH Actions) - GitHub Actions workflow now triggers Cloud Build instead of building directly Co-authored-by: Cursor <cursoragent@cursor.com>
Registry cache avoids rebuilding unchanged layers across builds. Increase timeout from 1h to 2h for uncached cold builds. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
config/initializers/devise.rb (1)
7-7:Rails.logger.warnlog message lacks non-PII diagnostic context.
'Invalid password'is a static string with no request metadata (e.g., remote IP,request.request_id). This makes it hard to correlate entries during a brute-force investigation. Addingrequest.remote_ipor Rack'sX-Request-Idwould improve usefulness without introducing PII.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/initializers/devise.rb` at line 7, The log call that currently does Rails.logger.warn('Invalid password') when warden_message == :invalid is missing non-PII diagnostic context; update the warning to include request-scoped non-PII identifiers (for example request.remote_ip and/or request.request_id or the Rack X-Request-Id header) along with a short message so entries can be correlated during investigations—modify the conditional around warden_message == :invalid and ensure the Rails.logger.warn invocation includes those request values (but not user-identifying data).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/docker-gar.yml:
- Around line 42-48: The gcloud invocation uses --async (in gcloud builds
submit) which hides build failures and also passes an unquoted --substitutions
value using the GitHub Actions expression steps.tag.outputs.img_tag that can
break gcloud parsing; remove --async (or replace with logic that waits/polls for
the build result) so the workflow blocks and fails on build errors, and wrap the
substitutions value in quotes (i.e. quote the --substitutions argument) so the
expanded ${ { steps.tag.outputs.img_tag } } is passed safely even if it contains
commas/spaces/equals.
In `@cloudbuild.yaml`:
- Around line 31-32: Remove the unconditional
'--tag=us-central1-docker.pkg.dev/$PROJECT_ID/docuseal/docuseal:latest' argument
in cloudbuild.yaml and make pushing the :latest tag conditional: add a new
substitution (e.g., _PUSH_LATEST) with a default false and change the build step
that currently uses '${_IMG_TAG}' to only include the :latest tag when
_PUSH_LATEST=true; then update the caller (.github/workflows/docker-gar.yml) to
pass _PUSH_LATEST=true only for tagged/release builds (leave it false for branch
pushes) so consumers pulling :latest only get release images.
In `@config/initializers/devise.rb`:
- Around line 341-344: The omniauth configuration calls
ENV.fetch('GOOGLE_CLIENT_SECRET') which raises at boot if the secret is missing;
update the guard around config.omniauth (the block that sets config.omniauth
:google_oauth2, ENV.fetch('GOOGLE_CLIENT_ID'),
ENV.fetch('GOOGLE_CLIENT_SECRET'), oauth_opts) so it only runs when both
GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET are present (or alternatively use
ENV['GOOGLE_CLIENT_SECRET'] with a nil-safe fallback and delay failure until
sign-in); ensure you reference the GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET
environment variables and the config.omniauth :google_oauth2 entry when applying
the change.
---
Duplicate comments:
In @.github/workflows/docker-gar.yml:
- Around line 5-6: The workflow's tag trigger uses an overly broad pattern
("*.*.*") under the tags key which matches unintended image tags; update the
tags pattern to a more specific, intentional matcher (for example use a prefix
like "v*.*.*" or a constrained regex-equivalent) so only release-version tags
trigger this workflow and replace the current "*.*.*" value in the tags list
accordingly.
- Around line 36-40: The workflow directly interpolates github.ref_name into a
shell echo which allows shell injection; instead write the output value to
GITHUB_OUTPUT using the safe delimiter/heredoc form (write a delimiter marker,
then the raw github.ref_name, then end delimiter) rather than inline echo
interpolation, and do the same for the img_tag derived from github.sha; update
the block that currently uses echo "img_tag=${{ github.ref_name }}" and echo
"img_tag=$(echo ${{ github.sha }} | cut -c1-8)" to use the GITHUB_OUTPUT heredoc
pattern so the raw values are written without being interpreted by the shell.
---
Nitpick comments:
In `@config/initializers/devise.rb`:
- Line 7: The log call that currently does Rails.logger.warn('Invalid password')
when warden_message == :invalid is missing non-PII diagnostic context; update
the warning to include request-scoped non-PII identifiers (for example
request.remote_ip and/or request.request_id or the Rack X-Request-Id header)
along with a short message so entries can be correlated during
investigations—modify the conditional around warden_message == :invalid and
ensure the Rails.logger.warn invocation includes those request values (but not
user-identifying data).
| - name: Trigger Cloud Build | ||
| run: | | ||
| gcloud builds submit \ | ||
| --config=cloudbuild.yaml \ | ||
| --project=kencove-prod \ | ||
| --substitutions=_IMG_TAG=${{ steps.tag.outputs.img_tag }} \ | ||
| --async |
There was a problem hiding this comment.
--async silences build failures; unquoted substitution value can break parsing.
Two distinct issues in this block:
-
No build-failure signal (Line 44–48):
--asynccausesgcloud builds submitto return immediately after queuing the job. If the Cloud Build run fails (e.g., Buildx error, registry push failure, OOM on the worker), the GitHub Actions job exits with ✅, the commit gets a green check, and the broken image is silently never produced. Consider removing--asyncand letting the job block until the build completes, or poll for the result. Thetimeout-minutes: 5on the job is consistent with--async(submission itself is fast), but that timeout would need to be increased (or removed) if the job blocks. -
Unquoted substitution value (Line 47):
--substitutionsuses comma-separatedkey=valuepairs — a value containing a comma causes a parse error. The expression${{ steps.tag.outputs.img_tag }}is interpolated by GitHub Actions before the shell runs, so any special character in the resolved value (comma, space, equals) will corrupt thegcloudcommand.
🛠️ Proposed fix — block on result and quote the substitution
- name: Trigger Cloud Build
+ timeout-minutes: 120
run: |
gcloud builds submit \
--config=cloudbuild.yaml \
--project=kencove-prod \
- --substitutions=_IMG_TAG=${{ steps.tag.outputs.img_tag }} \
- --async
+ --substitutions="_IMG_TAG=${{ steps.tag.outputs.img_tag }}"
-
- timeout-minutes: 5
+ timeout-minutes: 130If keeping async is a deliberate choice (e.g., to avoid long-running Actions minutes), at minimum add the quote and document that build status must be monitored in Cloud Build console.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docker-gar.yml around lines 42 - 48, The gcloud invocation
uses --async (in gcloud builds submit) which hides build failures and also
passes an unquoted --substitutions value using the GitHub Actions expression
steps.tag.outputs.img_tag that can break gcloud parsing; remove --async (or
replace with logic that waits/polls for the build result) so the workflow blocks
and fails on build errors, and wrap the substitutions value in quotes (i.e.
quote the --substitutions argument) so the expanded ${ {
steps.tag.outputs.img_tag } } is passed safely even if it contains
commas/spaces/equals.
| - '--tag=us-central1-docker.pkg.dev/$PROJECT_ID/docuseal/docuseal:${_IMG_TAG}' | ||
| - '--tag=us-central1-docker.pkg.dev/$PROJECT_ID/docuseal/docuseal:latest' |
There was a problem hiding this comment.
:latest is always overwritten on every build, including branch pushes.
Both a semver tag push (e.g., 1.2.3) and a branch push to kencove will overwrite :latest with whatever was just built, because --tag=...docuseal:latest is unconditionally present. If a branch push lands a work-in-progress state, consumers pulling :latest will get the non-release image with no warning. Consider conditionally pushing :latest only for tagged builds by factoring the latest tag into the substitution or by removing the hard-coded tag and relying on ${_IMG_TAG} alone when the caller is not a release tag.
🛠️ Conditional latest via extra substitution
Add a _PUSH_LATEST substitution in cloudbuild.yaml:
substitutions:
_IMG_TAG: 'latest'
+ _PUSH_LATEST: 'false'And use it in the build step (Cloud Build supports bash-style conditionals in args via a shell wrapper, or you can split the step):
- - '--tag=us-central1-docker.pkg.dev/$PROJECT_ID/docuseal/docuseal:latest'Then in .github/workflows/docker-gar.yml, pass _PUSH_LATEST=true only when building from a tag:
- --substitutions=_IMG_TAG=${{ steps.tag.outputs.img_tag }} \
+ --substitutions=_IMG_TAG=${{ steps.tag.outputs.img_tag }},_PUSH_LATEST=${{ github.ref_type == 'tag' }} \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cloudbuild.yaml` around lines 31 - 32, Remove the unconditional
'--tag=us-central1-docker.pkg.dev/$PROJECT_ID/docuseal/docuseal:latest' argument
in cloudbuild.yaml and make pushing the :latest tag conditional: add a new
substitution (e.g., _PUSH_LATEST) with a default false and change the build step
that currently uses '${_IMG_TAG}' to only include the :latest tag when
_PUSH_LATEST=true; then update the caller (.github/workflows/docker-gar.yml) to
pass _PUSH_LATEST=true only for tagged/release builds (leave it false for branch
pushes) so consumers pulling :latest only get release images.
| config.omniauth :google_oauth2, | ||
| ENV.fetch('GOOGLE_CLIENT_ID'), | ||
| ENV.fetch('GOOGLE_CLIENT_SECRET'), | ||
| oauth_opts |
There was a problem hiding this comment.
Missing guard on GOOGLE_CLIENT_SECRET can crash app boot.
Line 343 calls ENV.fetch('GOOGLE_CLIENT_SECRET') which raises KeyError at initialization time if the key is absent from the environment — even though no corresponding presence check exists for it. If an operator sets GOOGLE_CLIENT_ID but omits GOOGLE_CLIENT_SECRET, the Rails app will fail to start.
Fix: extend the outer guard to require both variables, or use a nil-safe fallback and fail at sign-in time rather than boot time.
🛡️ Proposed fix — guard both credentials
- if ENV['GOOGLE_CLIENT_ID'].present?
+ if ENV['GOOGLE_CLIENT_ID'].present? && ENV['GOOGLE_CLIENT_SECRET'].present?
oauth_opts = {}
oauth_opts[:hd] = ENV['GOOGLE_ALLOWED_DOMAIN'] if ENV['GOOGLE_ALLOWED_DOMAIN'].present?
config.omniauth :google_oauth2,
ENV.fetch('GOOGLE_CLIENT_ID'),
ENV.fetch('GOOGLE_CLIENT_SECRET'),
oauth_opts
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/initializers/devise.rb` around lines 341 - 344, The omniauth
configuration calls ENV.fetch('GOOGLE_CLIENT_SECRET') which raises at boot if
the secret is missing; update the guard around config.omniauth (the block that
sets config.omniauth :google_oauth2, ENV.fetch('GOOGLE_CLIENT_ID'),
ENV.fetch('GOOGLE_CLIENT_SECRET'), oauth_opts) so it only runs when both
GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET are present (or alternatively use
ENV['GOOGLE_CLIENT_SECRET'] with a nil-safe fallback and delay failure until
sign-in); ensure you reference the GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET
environment variables and the config.omniauth :google_oauth2 entry when applying
the change.
1 participant
Summary by CodeRabbit
New Features
Improvements
Infrastructure