mirror of
https://gh.wpcy.net/https://github.com/discourse/discourse.git
synced 2026-05-13 17:44:56 +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
187 lines
5.9 KiB
Ruby
187 lines
5.9 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
RSpec.describe DiscourseSubscriptions::User::SubscriptionsController do
|
|
before do
|
|
SiteSetting.discourse_subscriptions_enabled = true
|
|
SiteSetting.discourse_subscriptions_secret_key = "secret-key"
|
|
end
|
|
|
|
def create_price(id, product)
|
|
price = { id: id, product: product }
|
|
def price.id
|
|
self[:id]
|
|
end
|
|
price
|
|
end
|
|
|
|
it "is a subclass of ApplicationController" do
|
|
expect(DiscourseSubscriptions::User::SubscriptionsController < ::ApplicationController).to eq(
|
|
true,
|
|
)
|
|
end
|
|
|
|
context "when not authenticated" do
|
|
it "does not get the subscriptions" do
|
|
::Stripe::Customer.expects(:list).never
|
|
get "/s/user/subscriptions.json"
|
|
end
|
|
|
|
it "does not destroy a subscription" do
|
|
::Stripe::Subscription.expects(:delete).never
|
|
patch "/s/user/subscriptions/sub_12345.json"
|
|
end
|
|
|
|
it "doesn't update payment method for subscription" do
|
|
::Stripe::Subscription.expects(:update).never
|
|
::Stripe::PaymentMethod.expects(:attach).never
|
|
put "/s/user/subscriptions/sub_12345.json", params: { payment_method: "pm_abc123abc" }
|
|
end
|
|
end
|
|
|
|
context "when authenticated" do
|
|
let(:user) { Fabricate(:user, email: "beanie@example.com") }
|
|
let(:customer) do
|
|
Fabricate(:customer, user_id: user.id, customer_id: "cus_23456", product_id: "prod_123")
|
|
end
|
|
|
|
before do
|
|
sign_in(user)
|
|
Fabricate(:subscription, customer_id: customer.id, external_id: "sub_10z")
|
|
end
|
|
|
|
describe "index" do
|
|
plans_json =
|
|
File.read(
|
|
Rails.root.join(
|
|
"plugins",
|
|
"discourse-subscriptions",
|
|
"spec",
|
|
"fixtures",
|
|
"json",
|
|
"stripe-price-list.json",
|
|
),
|
|
)
|
|
|
|
it "gets subscriptions" do
|
|
::Stripe::Price
|
|
.stubs(:list)
|
|
.with(anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(JSON.parse(plans_json, symbolize_names: true))
|
|
|
|
subscriptions_json =
|
|
File.read(
|
|
Rails.root.join(
|
|
"plugins",
|
|
"discourse-subscriptions",
|
|
"spec",
|
|
"fixtures",
|
|
"json",
|
|
"stripe-subscription-list.json",
|
|
),
|
|
)
|
|
|
|
::Stripe::Subscription
|
|
.stubs(:list)
|
|
.with(anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.returns(JSON.parse(subscriptions_json, symbolize_names: true))
|
|
|
|
get "/s/user/subscriptions.json"
|
|
|
|
subscription = JSON.parse(response.body, symbolize_names: true).first
|
|
|
|
expect(subscription[:id]).to eq("sub_10z")
|
|
expect(subscription[:items][:data][0][:plan][:id]).to eq("price_1OrmlvEYXaQnncShNahrpKvA")
|
|
expect(subscription[:product][:name]).to eq("Exclusive Access")
|
|
end
|
|
|
|
it "aggregates prices from multiple pages using pagination logic" do
|
|
subscription_data = { id: "sub_10z", items: { data: [{ price: { id: "price_200" } }] } }
|
|
::Stripe::Subscription
|
|
.stubs(:list)
|
|
.with(
|
|
{ customer: "cus_23456", status: "all" },
|
|
DiscourseSubscriptions::Stripe.request_opts,
|
|
)
|
|
.returns({ data: [subscription_data] })
|
|
|
|
# Build the first page of 100 prices that do NOT include the desired price.
|
|
prices_page_1 =
|
|
(1..100).map do |i|
|
|
create_price("price_#{i}", { id: "prod_dummy", name: "Dummy Product #{i}" })
|
|
end
|
|
|
|
# Second page containing the desired price.
|
|
prices_page_2 = [create_price("price_200", { id: "prod_200", name: "Matching Product" })]
|
|
|
|
::Stripe::Price
|
|
.expects(:list)
|
|
.with(
|
|
has_entries(limit: 100, expand: ["data.product"]),
|
|
DiscourseSubscriptions::Stripe.request_opts,
|
|
)
|
|
.returns({ data: prices_page_1, has_more: true })
|
|
|
|
::Stripe::Price
|
|
.expects(:list)
|
|
.with(
|
|
has_entries(limit: 100, expand: ["data.product"], starting_after: "price_100"),
|
|
DiscourseSubscriptions::Stripe.request_opts,
|
|
)
|
|
.returns({ data: prices_page_2, has_more: false })
|
|
|
|
get "/s/user/subscriptions.json"
|
|
result = JSON.parse(response.body, symbolize_names: true)
|
|
subscription = result.first
|
|
|
|
expect(subscription[:id]).to eq("sub_10z")
|
|
expect(subscription[:plan][:id]).to eq("price_200")
|
|
expect(subscription[:product][:id]).to eq("prod_200")
|
|
expect(subscription[:product][:name]).to eq("Matching Product")
|
|
end
|
|
end
|
|
|
|
describe "update" do
|
|
it "updates the payment method for subscription" do
|
|
::Stripe::Subscription
|
|
.expects(:update)
|
|
.with(anything, anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.once
|
|
::Stripe::PaymentMethod
|
|
.expects(:attach)
|
|
.with(anything, anything, DiscourseSubscriptions::Stripe.request_opts)
|
|
.once
|
|
put "/s/user/subscriptions/sub_10z.json", params: { payment_method: "pm_abc123abc" }
|
|
end
|
|
end
|
|
end
|
|
|
|
context "when authenticated as another user" do
|
|
fab!(:user_1, :user)
|
|
fab!(:user_2, :user)
|
|
fab!(:customer) { Fabricate(:customer, user_id: user_1.id, customer_id: "001") }
|
|
fab!(:subscription) { Fabricate(:subscription, customer_id: customer.id, external_id: "abc") }
|
|
|
|
before { sign_in(user_2) }
|
|
|
|
describe "destroy" do
|
|
it "does not allow user to cancel a subscription that is not theirs" do
|
|
::Stripe::Subscription.expects(:update).never
|
|
|
|
delete "/s/user/subscriptions/abc.json"
|
|
|
|
expect(response.status).to eq(404)
|
|
end
|
|
end
|
|
|
|
describe "update" do
|
|
it "does not allow user to update a subscription that is not theirs" do
|
|
::Stripe::PaymentMethod.expects(:attach).never
|
|
::Stripe::Subscription.expects(:update).never
|
|
|
|
put "/s/user/subscriptions/abc.json", params: { payment_method: "x" }
|
|
|
|
expect(response.status).to eq(404)
|
|
end
|
|
end
|
|
end
|
|
end
|