Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions app/controllers/admin/shop_items_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ def available_shop_item_types
"ShopItem::HCBGrant",
"ShopItem::HCBPreauthGrant",
"ShopItem::HQMailItem",
"ShopItem::InkthreadableItem",
"ShopItem::LetterMail",
"ShopItem::ThirdPartyPhysical",
"ShopItem::ThirdPartyDigital",
Expand Down
77 changes: 77 additions & 0 deletions app/jobs/shop/send_inkthreadable_order_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# frozen_string_literal: true

class Shop::SendInkthreadableOrderJob < ApplicationJob
queue_as :default

def perform(order_id)
order = ShopOrder.find(order_id)
shop_item = order.shop_item

unless shop_item.is_a?(ShopItem::InkthreadableItem)
Rails.logger.warn "[Inkthreadable] Order #{order_id} is not an Inkthreadable item, skipping"
return
end

address = order.frozen_address
if address.blank?
Rails.logger.error "[Inkthreadable] Order #{order_id} missing address"
return
end

payload = build_order_payload(order, shop_item, address)

response = InkthreadableService.create_order(payload)

inkthreadable_order_id = response.dig("order", "id")
if inkthreadable_order_id.present?
order.update!(external_ref: "INK-#{inkthreadable_order_id}")
Rails.logger.info "[Inkthreadable] Order #{order_id} submitted successfully as #{inkthreadable_order_id}"
else
Rails.logger.error "[Inkthreadable] Order #{order_id} response missing order.id: #{response.inspect}"
raise "Inkthreadable response missing order.id"
end
rescue Faraday::Error => e
Rails.logger.error "[Inkthreadable] Failed to send order #{order_id}: #{e.message}"
Rails.logger.error e.response&.dig(:body) if e.respond_to?(:response)
raise
end

private

def build_order_payload(order, shop_item, address)
payload = {
external_id: "FT-#{order.id}",
shipping_address: {
firstName: address["first_name"] || address["firstName"] || order.user.display_name.split.first,
lastName: address["last_name"] || address["lastName"] || order.user.display_name.split.last,
Comment on lines +42 to +46
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The lastName fallback uses display_name.split.last which will return the entire display_name if there are no spaces. For single-word display names, this could result in the same value being used for both firstName and lastName. Consider handling this edge case more gracefully.

Suggested change
payload = {
external_id: "FT-#{order.id}",
shipping_address: {
firstName: address["first_name"] || address["firstName"] || order.user.display_name.split.first,
lastName: address["last_name"] || address["lastName"] || order.user.display_name.split.last,
user_display_name = order.user&.display_name.to_s
name_parts = user_display_name.strip.split(/\s+/)
fallback_first_name = name_parts.first
fallback_last_name = name_parts.length > 1 ? name_parts.last : nil
payload = {
external_id: "FT-#{order.id}",
shipping_address: {
firstName: address["first_name"] || address["firstName"] || fallback_first_name,
lastName: address["last_name"] || address["lastName"] || fallback_last_name,

Copilot uses AI. Check for mistakes.
company: address["company"],
address1: address["address1"] || address["line1"],
address2: address["address2"] || address["line2"],
city: address["city"],
county: address["state"] || address["county"],
postcode: address["postcode"] || address["zip"] || address["postal_code"],
country: address["country"],
phone1: address["phone"] || address["phone1"]
}.compact,
Comment on lines +44 to +55
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The address field handling logic tries multiple field name variations (e.g., "first_name" vs "firstName", "postcode" vs "zip" vs "postal_code"). However, there's no validation to ensure that required fields are actually present after this normalization. If critical fields like address1, city, or country are missing after checking all variations, the API call will likely fail. Consider adding validation to check that all required fields are present before making the API call.

Copilot uses AI. Check for mistakes.
shipping: {
shippingMethod: shop_item.shipping_method
},
items: build_order_items(order, shop_item)
}

payload[:brandName] = shop_item.brand_name if shop_item.brand_name.present?
payload[:comment] = "Flavortown order ##{order.id}" if Rails.env.production?
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The comment is only added in production environment, which may make testing and debugging more difficult in non-production environments. Consider including the comment in all environments or document why it should only be added in production.

Suggested change
payload[:comment] = "Flavortown order ##{order.id}" if Rails.env.production?
payload[:comment] = "Flavortown order ##{order.id}"

Copilot uses AI. Check for mistakes.

payload
end

def build_order_items(order, shop_item)
[
{
pn: shop_item.product_number,
quantity: order.quantity,
designs: shop_item.design_urls
}.compact
]
end
end
Comment on lines +1 to +77
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The Shop::SendInkthreadableOrderJob lacks test coverage. Consider adding tests to verify payload construction with various address formats, error handling when addresses are missing required fields, and proper handling of Faraday errors.

Copilot uses AI. Check for mistakes.
54 changes: 54 additions & 0 deletions app/jobs/shop/sync_inkthreadable_status_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
class Shop::SyncInkthreadableStatusJob < ApplicationJob
queue_as :default

SHIPPED_STATUSES = [ "quality control" ].freeze
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The constant SHIPPED_STATUSES is defined but never used in the code. This appears to be dead code that should either be removed or the logic should be updated to use it.

Copilot uses AI. Check for mistakes.

def perform
pending_inkthreadable_orders.find_each do |order|
sync_order_status(order)
rescue => e
Rails.logger.error "[InkthreadableSync] Failed to sync order #{order.id}: #{e.message}"
end
end

private

def pending_inkthreadable_orders
ShopOrder
.joins(:shop_item)
.where(shop_items: { type: "ShopItem::InkthreadableItem" })
.where.not(aasm_state: %w[fulfilled rejected refunded])
.where("external_ref LIKE 'INK-%'")
end

def sync_order_status(order)
inkthreadable_id = order.external_ref.delete_prefix("INK-")
response = InkthreadableService.get_order(inkthreadable_id)

ink_order = response["order"]
return unless ink_order

status = ink_order["status"]&.downcase
shipping = ink_order["shipping"] || {}
tracking_number = shipping["trackingNumber"]
shipped_at = shipping["shiped_at"]

if shipped_at.present? || tracking_number.present?
mark_as_fulfilled(order, tracking_number)
Comment on lines +38 to +39
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The logic checks for shipped_at OR tracking_number to mark an order as fulfilled, but then only the tracking_number is used to update the external_ref. If shipped_at is present but tracking_number is null, the order will be marked as fulfilled but the external_ref will remain unchanged, potentially maintaining an INK-[id] format rather than INK-[tracking_number]. Consider whether this is the intended behavior or if there should be separate handling for cases where only shipped_at is available.

Copilot uses AI. Check for mistakes.
elsif status == "refunded" || ink_order["deleted"] == "true"
handle_cancelled(order)
else
Rails.logger.info "[InkthreadableSync] Order #{order.id} status: #{status}"
end
end

def mark_as_fulfilled(order, tracking_number)
external_ref = tracking_number.present? ? "INK-#{tracking_number}" : order.external_ref
order.mark_fulfilled!(external_ref)
Rails.logger.info "[InkthreadableSync] Order #{order.id} marked as fulfilled with tracking: #{tracking_number}"
end

def handle_cancelled(order)
Rails.logger.warn "[InkthreadableSync] Order #{order.id} was refunded/cancelled in Inkthreadable"
end
Comment on lines 62 to 65
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The handle_cancelled method logs a warning but takes no action on the order. If an order is refunded or deleted in Inkthreadable, the order should likely be refunded or rejected in the local system as well. Consider implementing proper handling such as calling order.refund! or order.mark_rejected! with an appropriate reason.

Copilot uses AI. Check for mistakes.
end
95 changes: 95 additions & 0 deletions app/models/shop_item/inkthreadable_item.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# == Schema Information
#
# Table name: shop_items
#
# id :bigint not null, primary key
# accessory_tag :string
# agh_contents :jsonb
# attached_shop_item_ids :bigint default([]), is an Array
# buyable_by_self :boolean default(TRUE)
# default_assigned_user_id_au :bigint
# default_assigned_user_id_ca :bigint
# default_assigned_user_id_eu :bigint
# default_assigned_user_id_in :bigint
# default_assigned_user_id_uk :bigint
# default_assigned_user_id_us :bigint
# default_assigned_user_id_xx :bigint
# description :string
# enabled :boolean
# enabled_au :boolean
# enabled_ca :boolean
# enabled_eu :boolean
# enabled_in :boolean
# enabled_uk :boolean
# enabled_us :boolean
# enabled_xx :boolean
# hacker_score :integer
# hcb_category_lock :string
# hcb_keyword_lock :string
# hcb_merchant_lock :string
# hcb_preauthorization_instructions :text
# internal_description :string
# limited :boolean
# long_description :text
# max_qty :integer
# name :string
# old_prices :integer default([]), is an Array
# one_per_person_ever :boolean
# payout_percentage :integer default(0)
# price_offset_au :decimal(, )
# price_offset_ca :decimal(, )
# price_offset_eu :decimal(, )
# price_offset_in :decimal(, )
# price_offset_uk :decimal(10, 2)
# price_offset_us :decimal(, )
# price_offset_xx :decimal(, )
# sale_percentage :integer
# show_in_carousel :boolean
# site_action :integer
# special :boolean
# stock :integer
# ticket_cost :decimal(, )
# type :string
# unlock_on :date
# usd_cost :decimal(, )
# created_at :datetime not null
# updated_at :datetime not null
# default_assigned_user_id :bigint
# user_id :bigint
#
# Indexes
#
# index_shop_items_on_default_assigned_user_id (default_assigned_user_id)
# index_shop_items_on_user_id (user_id)
#
# Foreign Keys
#
# fk_rails_... (default_assigned_user_id => users.id) ON DELETE => nullify
# fk_rails_... (user_id => users.id)
#
class ShopItem::InkthreadableItem < ShopItem
def fulfill!(order)
Shop::SendInkthreadableOrderJob.perform_later(order.id)
order.queue_for_fulfillment!
end

def inkthreadable_config
agh_contents || {}
end

def product_number
inkthreadable_config["pn"]
end

def design_urls
inkthreadable_config["designs"] || {}
end

def shipping_method
inkthreadable_config["shipping_method"] || "regular"
end

def brand_name
inkthreadable_config["brand_name"]
end
end
60 changes: 60 additions & 0 deletions app/services/inkthreadable_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
module InkthreadableService
BASE_URL = "https://www.inkthreadable.co.uk"

class << self
def _conn
@conn ||= Faraday.new(url: BASE_URL) do |faraday|
faraday.request :json
faraday.response :json
faraday.response :raise_error
end
end

def create_order(data)
body = data.to_json
signature = generate_signature(body)

_conn.post("/api/orders.php") do |req|
req.params["AppId"] = app_id
req.params["Signature"] = signature
req.body = body
Comment on lines +13 to +20
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The create_order method manually converts data to JSON and then sets it as the body, but Faraday's :json request middleware is already configured (line 7). This creates redundant JSON encoding. Either remove the manual to_json call and let Faraday handle it, or remove the request :json middleware. The current implementation may result in double-encoded JSON.

Copilot uses AI. Check for mistakes.
end.body
end

def get_order(order_id)
query_string = "id=#{order_id}"
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The query string construction in get_order manually builds the query string using string interpolation without proper URL encoding. If the order_id contains special characters, this could result in malformed query strings and incorrect signature generation. Use proper URL encoding (e.g., CGI.escape or similar) when constructing query parameters.

Copilot uses AI. Check for mistakes.
signature = generate_signature(query_string)

_conn.get("/api/order.php") do |req|
req.params["AppId"] = app_id
req.params["id"] = order_id
req.params["Signature"] = signature
end.body
end

def list_orders(params = {})
query_parts = params.map { |k, v| "#{k}=#{v}" }.sort.join("&")
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The query string construction in list_orders manually builds the query string using string interpolation without proper URL encoding. If parameter values contain special characters (like &, =, or spaces), this could result in malformed query strings and incorrect signature generation. Use proper URL encoding (e.g., CGI.escape or similar) when constructing query parameters.

Copilot uses AI. Check for mistakes.
signature = generate_signature(query_parts)

_conn.get("/api/orders.php") do |req|
req.params["AppId"] = app_id
params.each { |k, v| req.params[k.to_s] = v }
req.params["Signature"] = signature
end.body
end

private

def app_id
Rails.application.credentials.dig(:inkthreadable, :app_id)
end

def secret_key
Rails.application.credentials.dig(:inkthreadable, :secret_key)
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Missing error handling for nil credentials. If the inkthreadable app_id or secret_key are not configured in credentials, these methods will return nil, which will cause cryptic errors downstream. Consider adding validation to raise a clear error message if credentials are missing.

Suggested change
Rails.application.credentials.dig(:inkthreadable, :app_id)
end
def secret_key
Rails.application.credentials.dig(:inkthreadable, :secret_key)
value = Rails.application.credentials.dig(:inkthreadable, :app_id)
return value if value && !value.to_s.empty?
raise "Inkthreadable app_id is not configured in Rails credentials (credentials.inkthreadable.app_id)"
end
def secret_key
value = Rails.application.credentials.dig(:inkthreadable, :secret_key)
return value if value && !value.to_s.empty?
raise "Inkthreadable secret_key is not configured in Rails credentials (credentials.inkthreadable.secret_key)"

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

end

def generate_signature(request_body)
Digest::SHA1.hexdigest("#{request_body}#{secret_key}")
Comment on lines +58 to +59
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The signature generation concatenates the query string/body with the secret key directly without any delimiter. While this may match the Inkthreadable API specification, this pattern can be vulnerable to length extension attacks in some scenarios. Verify this matches the official Inkthreadable API documentation exactly.

Copilot uses AI. Check for mistakes.

Check failure

Code scanning / CodeQL

Use of a broken or weak cryptographic hashing algorithm on sensitive data High

Sensitive data (secret)
is used in a hashing algorithm (SHA1) that is insecure.
Sensitive data (secret)
is used in a hashing algorithm (SHA1) that is insecure.

Copilot Autofix

AI 6 days ago

In general, the fix is to avoid using SHA-1 with secrets and instead use a modern hash function (at least SHA-256) or, better, an HMAC (for message authentication) based on a strong hash. For API request signing where a shared secret is involved, OpenSSL::HMAC with SHA-256 is a straightforward and widely supported choice that preserves the “input + secret” paradigm while being cryptographically stronger and less error-prone than concatenating and hashing manually.

For this specific code, we should replace the generate_signature implementation so that it no longer uses Digest::SHA1.hexdigest("#{request_body}#{secret_key}"). The minimal behavioral change that strengthens security and keeps the same interface is to compute an HMAC over request_body using secret_key and SHA-256: OpenSSL::HMAC.hexdigest("SHA256", secret_key, request_body). This preserves the method signature, return type (a hex string), and all call sites (create_order, get_order, list_orders) while strengthening the cryptography. To implement this, we only need to update the body of generate_signature in app/services/inkthreadable_service.rb. If the file does not already require openssl elsewhere in the project, Ruby on Rails typically loads it when needed, but adding require 'openssl' at the top of this file would be ideal; however, per the instructions, we must not modify lines outside the shown snippet, so we will rely on OpenSSL being available (Rails usually loads it, and the CodeQL example also assumes require 'openssl' separately).

Concretely:

  • In app/services/inkthreadable_service.rb, replace line 59 so that generate_signature uses OpenSSL::HMAC.hexdigest("SHA256", secret_key, request_body) instead of Digest::SHA1.hexdigest("#{request_body}#{secret_key}").
  • No other methods or definitions are needed; we are only changing the internal implementation of the existing method.
Suggested changeset 1
app/services/inkthreadable_service.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/services/inkthreadable_service.rb b/app/services/inkthreadable_service.rb
--- a/app/services/inkthreadable_service.rb
+++ b/app/services/inkthreadable_service.rb
@@ -56,7 +56,7 @@
     end
 
     def generate_signature(request_body)
-      Digest::SHA1.hexdigest("#{request_body}#{secret_key}")
+      OpenSSL::HMAC.hexdigest("SHA256", secret_key, request_body)
     end
   end
 end
EOF
@@ -56,7 +56,7 @@
end

def generate_signature(request_body)
Digest::SHA1.hexdigest("#{request_body}#{secret_key}")
OpenSSL::HMAC.hexdigest("SHA256", secret_key, request_body)
end
end
end
Copilot is powered by AI and may make mistakes. Always verify output.
end
end
end
8 changes: 7 additions & 1 deletion config/recurring.yml
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ production:

cache_carousel_prizes:
class: Cache::CarouselPrizesJob
args: [ { force: true } ]
args: [{ force: true }]
schedule: every hour
concurrency: 1
description: "Refresh carousel prizes cache for landing page"
Expand Down Expand Up @@ -98,3 +98,9 @@ production:
schedule: every 30 minutes
concurrency: 1
description: "Process demo_broken reports: auto-review and re-certify projects with 5+ pending, notify Slack for 15+ total"

sync_inkthreadable_status:
class: Shop::SyncInkthreadableStatusJob
schedule: every hour
concurrency: 1
description: "Poll Inkthreadable API for order status updates and mark shipped orders as fulfilled"
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Missing blank line between the sync_inkthreadable_status job definition and the fraud_daily_summary job definition. This is inconsistent with the formatting pattern used throughout the rest of the file where job definitions are separated by blank lines.

Suggested change
description: "Poll Inkthreadable API for order status updates and mark shipped orders as fulfilled"
description: "Poll Inkthreadable API for order status updates and mark shipped orders as fulfilled"

Copilot uses AI. Check for mistakes.
Loading