Conversation
|
|
transcental
left a comment
There was a problem hiding this comment.
Will test this in a bit, but from a quick look these should be fixed
|
There was a problem hiding this comment.
Pull request overview
This PR adds integration with the Inkthreadable print-on-demand service, enabling the shop to sell custom-printed merchandise through their API. The implementation includes a new shop item type, service layer for API communication, background jobs for order fulfillment and status synchronization, and database schema changes to store configuration.
Changes:
- Added
inkthreadable_configJSONB column toshop_itemstable for storing product configuration - Created
InkthreadableServicemodule to handle API communication with signature-based authentication - Implemented
ShopItem::InkthreadableItemmodel with fulfillment logic and configuration accessors - Added
Shop::SendInkthreadableOrderJobto submit orders to Inkthreadable API - Added
Shop::SyncInkthreadableStatusJob(runs hourly) to poll for order status updates and mark shipped orders as fulfilled
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| db/migrate/20260121015055_add_inkthreadable_config_to_shop_items.rb | Migration adding JSONB column for Inkthreadable product configuration |
| db/schema.rb | Schema update reflecting new inkthreadable_config column |
| app/models/shop_item.rb | Annotation update for new column |
| app/models/shop_item/inkthreadable_item.rb | New model class implementing fulfillment logic for Inkthreadable items |
| app/models/shop_item/*.rb | Annotation updates for new column across all ShopItem subclasses |
| app/services/inkthreadable_service.rb | Service module for Inkthreadable API integration with SHA1 signature authentication |
| app/jobs/shop/send_inkthreadable_order_job.rb | Background job to submit orders to Inkthreadable with address normalization |
| app/jobs/shop/sync_inkthreadable_status_job.rb | Recurring job to poll order statuses and sync fulfillment state |
| app/controllers/admin/shop_items_controller.rb | Added InkthreadableItem to available shop item types list |
| config/recurring.yml | Configured hourly sync job for Inkthreadable order status updates |
| test/models/shop_item_test.rb | Annotation update for new column |
| test/fixtures/shop_items.yml | Annotation update for new column |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| end | ||
|
|
||
| def list_orders(params = {}) | ||
| query_parts = params.map { |k, v| "#{k}=#{v}" }.sort.join("&") |
There was a problem hiding this comment.
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.
| 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, | ||
| 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, |
There was a problem hiding this comment.
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.
| class ShopItem::InkthreadableItem < ShopItem | ||
| def fulfill!(order) | ||
| Shop::SendInkthreadableOrderJob.perform_later(order.id) | ||
| order.queue_for_fulfillment! | ||
| end | ||
|
|
||
| def inkthreadable_config | ||
| super || {} | ||
| 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 |
There was a problem hiding this comment.
The new InkthreadableItem model lacks test coverage. Consider adding tests to verify the fulfill! method enqueues the correct job, and to test the configuration accessor methods (product_number, design_urls, shipping_method, brand_name) with various configurations including edge cases like missing or empty inkthreadable_config.
| if shipped_at.present? || tracking_number.present? | ||
| mark_as_fulfilled(order, tracking_number) |
There was a problem hiding this comment.
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.
| def handle_cancelled(order) | ||
| Rails.logger.warn "[InkthreadableSync] Order #{order.id} was refunded/cancelled in Inkthreadable" | ||
| end |
There was a problem hiding this comment.
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.
| def generate_signature(request_body) | ||
| Digest::SHA1.hexdigest("#{request_body}#{secret_key}") |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| payload[:brandName] = shop_item.brand_name if shop_item.brand_name.present? | ||
| payload[:comment] = "Flavortown order ##{order.id}" if Rails.env.production? |
There was a problem hiding this comment.
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.
| payload[:comment] = "Flavortown order ##{order.id}" if Rails.env.production? | |
| payload[:comment] = "Flavortown order ##{order.id}" |
| def send_slack_alert(order, status) | ||
| slack_id = order.assigned_to_user&.slack_id || ALERT_SLACK_ID | ||
| client = Slack::Web::Client.new(token: Rails.application.credentials.dig(:slack, :bot_token)) | ||
|
|
||
| client.chat_postMessage( | ||
| channel: slack_id, | ||
| text: "⚠️ Inkthreadable order needs attention!\n\nOrder ##{order.id} has unexpected status: *#{status}*\n\nPlease review: #{Rails.application.routes.url_helpers.admin_shop_order_url(order, host: "flavortown.hackclub.com")}" | ||
| ) | ||
| rescue Slack::Web::Api::Errors::SlackError => e | ||
| Rails.logger.error "[InkthreadableSync] Failed to send Slack alert: #{e.message}" | ||
| end | ||
|
|
There was a problem hiding this comment.
Let's move this to the shop_order.rb model and use it whenever issues come up! Also going to make it a channel instead of DMing me - invited you on Slack
| class AddInkthreadableConfigToShopItems < ActiveRecord::Migration[8.1] | ||
| def change | ||
| add_column :shop_items, :inkthreadable_config, :jsonb | ||
| end | ||
| end |
There was a problem hiding this comment.
We need to have a field on the admin page to set the config
|
@copilot ai ai on the wall, ai of ai review my code plz |
|
@NeonGamerBot-QK I've opened a new pull request, #936, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
|
| end | ||
|
|
||
| def generate_signature(request_body) | ||
| Digest::SHA1.hexdigest("#{request_body}#{secret_key}") |
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic hashing algorithm on sensitive data High
Show autofix suggestion
Hide autofix suggestion
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 thatgenerate_signatureusesOpenSSL::HMAC.hexdigest("SHA256", secret_key, request_body)instead ofDigest::SHA1.hexdigest("#{request_body}#{secret_key}"). - No other methods or definitions are needed; we are only changing the internal implementation of the existing method.
| @@ -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 |
|
>,<
adds everything requested in the gh issue