discourse/plugins/discourse-subscriptions/spec/requests/admin/subscriptions_controller_spec.rb
Alan Guo Xiang Tan c34f2aac8d SECURITY: Fixes for discourse-subscriptions
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
2026-03-31 15:12:45 +01:00

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