Spot the Tax · Card 5 of 20
Authorization belongs in the query, not after
Why Invoice.find(params[:id]) is a security hole, even with authenticate_user!.
The code
What will this cost you in six months?
class InvoicesController < ApplicationController
before_action :authenticate_user!
def show
@invoice = Invoice.find(params[:id])
end
endThe problem
Any logged-in user can view any invoice in the system just by changing the ID in the URL. The before_action :authenticate_user! ran successfully, but all that did was confirm that the request came from someone who is logged in. It never checked whether this particular user is allowed to see this particular invoice. So the request goes through, the wrong invoice gets rendered, and you have an information leak that's effectively impossible to detect from your logs.
Take a moment. Before revealing, think about how you'd actually close this hole. What's the shape of the safer code, and what would make the unsafe version impossible to write by accident?
The solution
Build the ownership check directly into the query, by going through the user's own invoices instead of looking up invoices globally. If the invoice doesn't belong to the user, the query simply matches nothing and find raises RecordNotFound — exactly the same response you'd get if the invoice didn't exist at all.
- The unsafe version stops being writable — you have to scope the query
- One enforcement point instead of remembering to authorize after every find
- A 404 reveals less information than a 403
def show
@invoice = current_user.invoices.find(params[:id])
endThe principle at play — Scoped queries
The web works on URLs, and anyone can change a URL. So when your server receives a request for /invoices/123, it has to decide what to send back. There are really two questions in that decision, and they need to be answered separately. Authentication asks "who is making this request?", which is what Devise or any other auth library handles. Authorization asks "is this person allowed to see this resource?", which is a completely different question and needs different code.
Invoice.find(params[:id]) only handles the first question. By writing that line, you've implicitly decided that any logged-in user can look at any invoice in the system. You can patch over that by adding an explicit authorization check after the find, but that approach depends on every developer remembering to add it on every action — and the first time someone forgets, you have a security incident on your hands.
current_user.invoices.find(params[:id]) takes a different approach: it builds the query against the user's own invoices, so if the invoice doesn't belong to them, the query matches nothing and the controller renders 404. Authorization stops being a separate step you can forget about and becomes part of the query itself. The general principle here is to design your APIs so the safe path is the easiest one to write, which makes the unsafe one harder to do accidentally.