From cd7012cca8f9d26bd3b348dd0d59049e723e3d72 Mon Sep 17 00:00:00 2001 From: akadusei Date: Fri, 26 Jul 2024 16:57:13 +0000 Subject: [PATCH] Limit description lengths to mitigate potential DoS --- CHANGELOG.md | 1 + docs/10-I18N.md | 1 + .../mixins/validate_credit_note_item_spec.cr | 29 +++++++++++++++++++ .../mixins/validate_credit_note_spec.cr | 17 +++++++++++ .../mixins/validate_invoice_item_spec.cr | 17 +++++++++++ .../mixins/validate_invoice_spec.cr | 19 ++++++++++++ .../mixins/validate_receipt_spec.cr | 19 ++++++++++++ .../mixins/validate_transaction_spec.cr | 18 ++++++++++++ .../operations/mixins/validate_credit_note.cr | 1 + .../mixins/validate_credit_note_item.cr | 2 ++ .../operations/mixins/validate_description.cr | 17 +++++++++++ .../operations/mixins/validate_invoice.cr | 1 + .../mixins/validate_invoice_item.cr | 2 ++ .../operations/mixins/validate_receipt.cr | 1 + .../operations/mixins/validate_transaction.cr | 1 + 15 files changed, 146 insertions(+) create mode 100644 src/bill/operations/mixins/validate_description.cr diff --git a/CHANGELOG.md b/CHANGELOG.md index 865ea33..76fd984 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Changed - Upgrade GitHub actions +- Limit description lengths to mitigate potential DoS ## [0.17.0] - 2024-06-02 diff --git a/docs/10-I18N.md b/docs/10-I18N.md index c912c36..a8b45c8 100644 --- a/docs/10-I18N.md +++ b/docs/10-I18N.md @@ -91,6 +91,7 @@ en: credit_note_not_found: Credit note does not exist credit_or_debit_required: Is this a credit or a debit? description_required: Description is required + description_too_long: Description cannot be longer than %{max} characters due_at_required: Due time is required inactive_at_earlier: Inactive time cannot be earlier than active time invoice_id_invalid: Invoice ID is invalid diff --git a/spec/bill/operations/mixins/validate_credit_note_item_spec.cr b/spec/bill/operations/mixins/validate_credit_note_item_spec.cr index 71aa4e3..e8526b4 100644 --- a/spec/bill/operations/mixins/validate_credit_note_item_spec.cr +++ b/spec/bill/operations/mixins/validate_credit_note_item_spec.cr @@ -199,4 +199,33 @@ describe Bill::ValidateCreditNoteItem do operation.id.should_not have_error end end + + it "rejects long description" do + invoice = CreateInvoice.create!( + params( + user_id: UserFactory.create.id, + description: "New invoice", + due_at: 3.days.from_now, + status: :open + ), + line_items: [{ + "description" => "Item 1", + "quantity" => "1", + "price" => "999" + }] + ) + + credit_note = CreditNoteFactory.create &.invoice_id(invoice.id) + + SaveCreditNoteItem.create(params( + description: "d" * 600, + credit_note_id: credit_note.id, + price: 1 + )) do |operation, credit_note_item| + credit_note_item.should be_nil + + operation.description + .should(have_error "operation.error.description_too_long") + end + end end diff --git a/spec/bill/operations/mixins/validate_credit_note_spec.cr b/spec/bill/operations/mixins/validate_credit_note_spec.cr index d807517..00cb3a3 100644 --- a/spec/bill/operations/mixins/validate_credit_note_spec.cr +++ b/spec/bill/operations/mixins/validate_credit_note_spec.cr @@ -98,4 +98,21 @@ describe Bill::ValidateCreditNote do operation.reference.should have_error("operation.error.reference_exists") end end + + it "rejects long description" do + user = UserFactory.create + invoice = InvoiceFactory.create &.user_id(user.id).status(:open) + + SaveCreditNote.create(params( + invoice_id: invoice.id, + description: "d" * 600, + reference: "123", + status: :open + )) do |operation, credit_note| + credit_note.should be_nil + + operation.description + .should(have_error "operation.error.description_too_long") + end + end end diff --git a/spec/bill/operations/mixins/validate_invoice_item_spec.cr b/spec/bill/operations/mixins/validate_invoice_item_spec.cr index 90812ad..f403079 100644 --- a/spec/bill/operations/mixins/validate_invoice_item_spec.cr +++ b/spec/bill/operations/mixins/validate_invoice_item_spec.cr @@ -92,4 +92,21 @@ describe Bill::ValidateInvoiceItem do .should have_error("operation.error.quantity_lte_zero") end end + + it "rejects long description" do + user = UserFactory.create + invoice = InvoiceFactory.create &.user_id(user.id) + + SaveInvoiceItem.create(params( + invoice_id: invoice.id, + description: "d" * 600, + quantity: 0, + price: 222 + )) do |operation, invoice_item| + invoice_item.should be_nil + + operation.description + .should(have_error "operation.error.description_too_long") + end + end end diff --git a/spec/bill/operations/mixins/validate_invoice_spec.cr b/spec/bill/operations/mixins/validate_invoice_spec.cr index b9318c7..f789ea1 100644 --- a/spec/bill/operations/mixins/validate_invoice_spec.cr +++ b/spec/bill/operations/mixins/validate_invoice_spec.cr @@ -131,4 +131,23 @@ describe Bill::ValidateInvoice do operation.reference.should have_error("operation.error.reference_exists") end end + + it "rejects long description" do + user = UserFactory.create + + SaveInvoice.create(params( + user_id: user.id, + business_details: "ACME Inc", + description: "d" * 600, + due_at: 1.day.from_now, + reference: "123", + status: :open, + user_details: "Mary Smith" + )) do |operation, invoice| + invoice.should be_nil + + operation.description + .should(have_error "operation.error.description_too_long") + end + end end diff --git a/spec/bill/operations/mixins/validate_receipt_spec.cr b/spec/bill/operations/mixins/validate_receipt_spec.cr index 4800a37..1755121 100644 --- a/spec/bill/operations/mixins/validate_receipt_spec.cr +++ b/spec/bill/operations/mixins/validate_receipt_spec.cr @@ -165,4 +165,23 @@ describe Bill::ValidateReceipt do operation.reference.should have_error("operation.error.reference_exists") end end + + it "rejects long description" do + user = UserFactory.create + + SaveReceipt.create(params( + user_id: user.id, + business_details: "ACME Inc", + description: "d" * 600, + amount: 90, + reference: "123", + status: :open, + user_details: "Mary Smith" + )) do |operation, receipt| + receipt.should be_nil + + operation.description + .should(have_error "operation.error.description_too_long") + end + end end diff --git a/spec/bill/operations/mixins/validate_transaction_spec.cr b/spec/bill/operations/mixins/validate_transaction_spec.cr index e568c48..b056497 100644 --- a/spec/bill/operations/mixins/validate_transaction_spec.cr +++ b/spec/bill/operations/mixins/validate_transaction_spec.cr @@ -120,4 +120,22 @@ describe Bill::ValidateTransaction do operation.reference.should have_error("operation.error.reference_exists") end end + + it "rejects long description" do + user = UserFactory.create + + SaveTransaction.create(params( + user_id: user.id, + description: "d" * 600, + amount: 33, + reference: "123", + status: :open, + type: :invoice, + )) do |operation, transaction| + transaction.should be_nil + + operation.description + .should(have_error "operation.error.description_too_long") + end + end end diff --git a/src/bill/operations/mixins/validate_credit_note.cr b/src/bill/operations/mixins/validate_credit_note.cr index d8e31ff..9b451d1 100644 --- a/src/bill/operations/mixins/validate_credit_note.cr +++ b/src/bill/operations/mixins/validate_credit_note.cr @@ -13,6 +13,7 @@ module Bill::ValidateCreditNote include Bill::ValidateStatusTransition include Bill::ValidateReference + include Bill::ValidateDescription private def validate_status_required validate_required status, diff --git a/src/bill/operations/mixins/validate_credit_note_item.cr b/src/bill/operations/mixins/validate_credit_note_item.cr index c674515..2ebd712 100644 --- a/src/bill/operations/mixins/validate_credit_note_item.cr +++ b/src/bill/operations/mixins/validate_credit_note_item.cr @@ -17,6 +17,8 @@ module Bill::ValidateCreditNoteItem validate_credit_lte_invoice end + include Bill::ValidateDescription + private def validate_credit_note_id_required validate_required credit_note_id, message: Rex.t(:"operation.error.credit_note_id_required") diff --git a/src/bill/operations/mixins/validate_description.cr b/src/bill/operations/mixins/validate_description.cr new file mode 100644 index 0000000..fba2971 --- /dev/null +++ b/src/bill/operations/mixins/validate_description.cr @@ -0,0 +1,17 @@ +module Bill::ValidateDescription + macro included + skip_default_validations + + before_save do + validate_description_length + end + + private def validate_description_length + max = 510 + + validate_size_of description, + max: max, + message: Rex.t(:"operation.error.description_too_long", max: max) + end + end +end diff --git a/src/bill/operations/mixins/validate_invoice.cr b/src/bill/operations/mixins/validate_invoice.cr index 61af4c0..31acf62 100644 --- a/src/bill/operations/mixins/validate_invoice.cr +++ b/src/bill/operations/mixins/validate_invoice.cr @@ -12,6 +12,7 @@ module Bill::ValidateInvoice include Bill::ValidateStatusTransition include Bill::ValidateReference + include Bill::ValidateDescription include Lucille::ValidateUserExists private def validate_business_details_required diff --git a/src/bill/operations/mixins/validate_invoice_item.cr b/src/bill/operations/mixins/validate_invoice_item.cr index 4b37856..621952a 100644 --- a/src/bill/operations/mixins/validate_invoice_item.cr +++ b/src/bill/operations/mixins/validate_invoice_item.cr @@ -15,6 +15,8 @@ module Bill::ValidateInvoiceItem validate_invoice_exists end + include Bill::ValidateDescription + private def validate_invoice_id_required validate_required invoice_id, message: Rex.t(:"operation.error.invoice_id_required") diff --git a/src/bill/operations/mixins/validate_receipt.cr b/src/bill/operations/mixins/validate_receipt.cr index 631b925..fb2e9be 100644 --- a/src/bill/operations/mixins/validate_receipt.cr +++ b/src/bill/operations/mixins/validate_receipt.cr @@ -14,6 +14,7 @@ module Bill::ValidateReceipt include Bill::ValidateStatusTransition include Bill::ValidateReference + include Bill::ValidateDescription include Lucille::ValidateUserExists private def validate_amount_required diff --git a/src/bill/operations/mixins/validate_transaction.cr b/src/bill/operations/mixins/validate_transaction.cr index 6b5bd97..1571d9b 100644 --- a/src/bill/operations/mixins/validate_transaction.cr +++ b/src/bill/operations/mixins/validate_transaction.cr @@ -15,6 +15,7 @@ module Bill::ValidateTransaction include Lucille::ValidateUserExists include Bill::ValidateStatusTransition include Bill::ValidateReference + include Bill::ValidateDescription private def validate_user_id_required validate_required user_id,