mirror of
https://gh.wpcy.net/https://github.com/discourse/discourse.git
synced 2026-05-16 21:54:46 +08:00
This PR combines two security fixes for the discourse-subscriptions plugin. ## Commits ### 1. [`8f0a8e98e9`](8f0a8e98e9) — SECURITY: Fix unauthorized group access via subscription finalize The `/s/finalize` endpoint accepted client-supplied `plan` and `transaction` parameters that determined which group a user would be added to after completing a Stripe payment. During the 3D Secure authentication flow, an authenticated user could complete a cheap subscription in `SubscribeController#create` but supply a premium plan ID when calling `#finalize`, granting themselves access to a higher-tier group they never paid for. This commit fixes the vulnerability by linking the two endpoints server-side using `server_session`. When `#create` produces a transaction requiring 3D Secure authentication (`incomplete` or `open` status), it stores the transaction ID and plan ID in the server session. `#finalize` then reads exclusively from the session instead of accepting client parameters, and clears the entry after successful finalization. On the frontend, `Transaction.finalize()` no longer sends any parameters to the server. (from #434) --- ### 2. [`dcf80dda61`](dcf80dda61) — SECURITY: Use per-request Stripe API key instead of global state Replace `set_api_key` (which mutated global `::Stripe.api_key`) with `set_stripe_api_key` (which stores the key in an instance variable). All Stripe API calls now receive `{ api_key: @stripe_api_key }` as the per-request opts parameter, following the stripe gem's documented per-request configuration pattern. This prevents API key leakage across concurrent requests in multi-threaded environments. (from #590) --- **Security Advisory:** https://github.com/discourse/discourse/security/advisories/GHSA-f866-8fcp-fgvv --- **Security Advisory:** https://github.com/discourse/discourse/security/advisories/GHSA-9vg5-mp49-xghh
168 lines
5.6 KiB
Ruby
168 lines
5.6 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
RSpec.describe DiscourseSubscriptions::Admin::SubscriptionsController do
|
|
before { SiteSetting.discourse_subscriptions_enabled = true }
|
|
|
|
it "is a subclass of AdminController" do
|
|
expect(DiscourseSubscriptions::Admin::SubscriptionsController < ::Admin::AdminController).to eq(
|
|
true,
|
|
)
|
|
end
|
|
|
|
let(:user) { Fabricate(:user) }
|
|
let(:customer) do
|
|
Fabricate(:customer, user_id: user.id, customer_id: "c_123", product_id: "pr_34578")
|
|
end
|
|
|
|
before do
|
|
Fabricate(:subscription, external_id: "sub_12345", customer_id: customer.id)
|
|
Fabricate(:subscription, external_id: "sub_77777", customer_id: customer.id)
|
|
end
|
|
|
|
context "when unauthenticated" do
|
|
it "does nothing" do
|
|
::Stripe::Subscription.expects(:list).never
|
|
get "/s/admin/subscriptions.json"
|
|
expect(response.status).to eq(404)
|
|
end
|
|
|
|
it "does not destroy a subscription" do
|
|
::Stripe::Subscription.expects(:delete).never
|
|
patch "/s/admin/subscriptions/sub_12345.json"
|
|
end
|
|
end
|
|
|
|
context "when authenticated" do
|
|
let(:admin) { Fabricate(:admin) }
|
|
|
|
before do
|
|
SiteSetting.discourse_subscriptions_secret_key = "secret-key"
|
|
sign_in(admin)
|
|
end
|
|
|
|
describe "index" do
|
|
before { SiteSetting.discourse_subscriptions_public_key = "public-key" }
|
|
|
|
it "gets the subscriptions and products" do
|
|
::Stripe::Subscription
|
|
.expects(:list)
|
|
.with(
|
|
{ expand: ["data.plan.product"], limit: 10, starting_after: nil, status: "all" },
|
|
DiscourseSubscriptions::Stripe.request_opts,
|
|
)
|
|
.returns(has_more: false, data: [{ id: "sub_12345" }, { id: "sub_nope" }])
|
|
get "/s/admin/subscriptions.json"
|
|
subscriptions = response.parsed_body["data"][0]["id"]
|
|
|
|
expect(response.status).to eq(200)
|
|
expect(subscriptions).to eq("sub_12345")
|
|
end
|
|
|
|
it "handles starting at a different point in the set" do
|
|
::Stripe::Subscription
|
|
.expects(:list)
|
|
.with(
|
|
{ expand: ["data.plan.product"], limit: 10, starting_after: "sub_nope", status: "all" },
|
|
DiscourseSubscriptions::Stripe.request_opts,
|
|
)
|
|
.returns(has_more: false, data: [{ id: "sub_77777" }, { id: "sub_yepnoep" }])
|
|
get "/s/admin/subscriptions.json", params: { last_record: "sub_nope" }
|
|
subscriptions = response.parsed_body["data"][0]["id"]
|
|
|
|
expect(response.status).to eq(200)
|
|
expect(subscriptions).to eq("sub_77777")
|
|
end
|
|
end
|
|
|
|
describe "destroy" do
|
|
let(:group) { Fabricate(:group, name: "subscribers") }
|
|
|
|
before { group.add(user) }
|
|
|
|
it "deletes a customer" do
|
|
::Stripe::Subscription
|
|
.expects(:cancel)
|
|
.with("sub_12345", anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(plan: { product: "pr_34578" }, customer: "c_123")
|
|
|
|
# We don't want to delete the customer record. The webhook hook will update the status instead.
|
|
expect { delete "/s/admin/subscriptions/sub_12345.json" }.not_to change {
|
|
DiscourseSubscriptions::Customer.count
|
|
}
|
|
end
|
|
|
|
it "removes the user from the group" do
|
|
::Stripe::Subscription
|
|
.expects(:cancel)
|
|
.with("sub_12345", anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(
|
|
plan: {
|
|
product: "pr_34578",
|
|
metadata: {
|
|
group_name: "subscribers",
|
|
},
|
|
},
|
|
customer: "c_123",
|
|
)
|
|
|
|
expect { delete "/s/admin/subscriptions/sub_12345.json" }.to change {
|
|
user.groups.count
|
|
}.by(-1)
|
|
end
|
|
|
|
it "does not remove the user from the group" do
|
|
::Stripe::Subscription
|
|
.expects(:cancel)
|
|
.with("sub_12345", anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(
|
|
plan: {
|
|
product: "pr_34578",
|
|
metadata: {
|
|
group_name: "group_does_not_exist",
|
|
},
|
|
},
|
|
customer: "c_123",
|
|
)
|
|
|
|
expect { delete "/s/admin/subscriptions/sub_12345.json" }.not_to change {
|
|
user.groups.count
|
|
}
|
|
end
|
|
|
|
it "does not refund when refund param is the string 'false'" do
|
|
::Stripe::Subscription
|
|
.expects(:cancel)
|
|
.with("sub_12345", anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(plan: { product: "pr_34578" }, customer: "c_123")
|
|
::Stripe::Subscription
|
|
.expects(:retrieve)
|
|
.with("sub_12345", DiscourseSubscriptions::Stripe.request_opts)
|
|
.never
|
|
::Stripe::Refund.expects(:create).never
|
|
|
|
delete "/s/admin/subscriptions/sub_12345.json", params: { refund: "false" }
|
|
end
|
|
|
|
it "refunds if params[:refund] present" do
|
|
::Stripe::Subscription
|
|
.expects(:cancel)
|
|
.with("sub_12345", anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(plan: { product: "pr_34578" }, customer: "c_123")
|
|
::Stripe::Subscription
|
|
.expects(:retrieve)
|
|
.with("sub_12345", DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(latest_invoice: "in_123")
|
|
::Stripe::Invoice
|
|
.expects(:retrieve)
|
|
.with("in_123", DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(payment_intent: "pi_123")
|
|
::Stripe::Refund.expects(:create).with(
|
|
{ payment_intent: "pi_123" },
|
|
DiscourseSubscriptions::Stripe.request_opts,
|
|
)
|
|
|
|
delete "/s/admin/subscriptions/sub_12345.json", params: { refund: true }
|
|
end
|
|
end
|
|
end
|
|
end
|