Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix security issue in order status endpoint #10573

Conversation

kshalot
Copy link
Contributor

@kshalot kshalot commented Nov 9, 2020

Issue

ensure_order_token in OrderStatusController (permalink) checks if order_token is truthy instead of checking if it's present?.

Outcome

Passing an empty string '' as the token allows to query any complete order without knowing it's token.

Description

Searching for complete orders forms a pipeline where the last step should filter out every order whose token doesn't match the token provided by the client:

def execute
orders = by_user(scope)
orders = by_number(orders)
orders = by_token(orders)
orders
end

by_token first checks whether token is present:

def token?
token.present?
end

def by_token(orders)
return orders unless token?
orders.where(token: token)
end

This completely skips filtering by token when the token is blank. In particular, it skips it when the token is an empty string ''. Since empty string evaluates to true, ensure_order_token would accept it as valid input. This makes it possible to query for any complete order without knowing it's token.

Steps to reproduce

  1. Complete an order <order_number>.
  2. Set the X-Spree-Order-Token header to an empty string ''.
  3. Query /api/v2/storefront/order_status/<order_number>.

You should now see the order's details.

@squash-labs
Copy link

squash-labs bot commented Nov 9, 2020

Manage this branch in Squash

Test this branch here: https://upsidelabfixorder-status-accep-8htfj.squash.io

@damianlegawiec damianlegawiec merged commit bc1b1ad into spree:master Nov 10, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants