fix(store): a search value that is not a uuid reports bad input - #1952
Conversation
Every aggregate search runs its query error through checkPostgresError and answers ErrBadInput when the database could not read a value as a uuid. The organizations search handler gains the branch the others already had.
…d input One case per search against postgres, plus handler cases for the organizations search error mapping.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: raystack/frontier/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughSearch repositories now map PostgreSQL invalid UUID errors to bad-input errors. Organization billing, invoice, and project Connect search endpoints return ChangesInvalid UUID search handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The reviewed malformed-UUID search paths return invalid-argument responses; no actionable merge risk is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Malformed UUID searches now receive a bad-input response instead of a database error. The reviewed paths use a generic validation message, with no identified expansion of data access. Some endpoint controls were not independently verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 36410598822Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.6%) to 52.695%Details
Uncovered Changes
Coverage Regressions16 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
Two more
Either give them the same treatment, or add a line to the description saying why they are out of scope. Right now a reader cannot tell the difference between left out on purpose and missed. Separate note, on a handler this PR now makes reachable. |
The billing invoice search gains the same mapping as the others, and its two wraps use %w so the cause stays in the chain. The org invoices and org projects handlers pass the reason through instead of answering invalid argument with an internal server error message.
|
All three taken, and chasing the first two turned up a bug in each. The invoices handler. Fixed in 04fdedd, and it was two handlers rather than one:
While testing it I found its
field = "CAST(" + TABLE_USERS + "." + filter.Name + " AS TEXT)"so a malformed uuid compares as text and never reaches a uuid column. Nothing else in that search binds a caller value to one. I added the mapping first and then took it out again, because it was unreachable. That cast is itself broken, though. The string is handed to |
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
All three points are handled. I checked each one rather than taking the reply at face value.
BillingInvoiceRepository.Searchreturninginvoice.ErrBadInputis the right call, not the postgres one.billing/invoice/invoice.go:16defines it andinternal/api/v1beta1connect/billing_invoice.go:175is the handler that matches it.- The
%sto%wchange is a fix, not a style edit. Lines 181, 205 and 245 in that file already used%w, so 262 and 355 were the two outliers. Callers inbilling/invoice/service.goonly pass the error through, so nothing downstream changes shape. - Leaving
UserRepository.Searchout is right.addFilterhands thatCAST(...)string togoqu.I, which splits it on the dot, so the filter never reaches a uuid column. Worth its own issue, agreed.
Ran TestSearchInvalidUUID against a real postgres at 04fdeddc: all twelve cases pass. Also merged this with #1953 locally; no conflicts, and both docker suites pass together.
The duplicated seven-line block is still copied twelve times now. Not blocking, but a small searchQueryError helper would make the thirteenth search hard to get wrong.
|
Raised both of these as their own tickets, so they do not ride along here. The invoice filter one covers The user id filter one covers every operator, since they all go through the same identifier string. A valid uuid fails exactly like a malformed one. |
This branch had an error being deployed
3 participants
Follows #1942, which fixed this for the organization users search. This applies the same treatment to the rest.
Summary
Every aggregate search binds a caller supplied id to a uuid column. A value the database cannot read as a uuid reached the handler as a raw driver error, so the API answered internal and echoed the database text. It now answers invalid argument.
Changes
checkPostgresErrorand returnErrBadInputforErrInvalidTextRepresentation, matching the shape already used in the audit repositories. Searches that wrap their error keep their own wording in the default branch.ErrBadInputbranch. The other ten handlers already had it, because repositories already answerErrBadInputfor an unsupported filter or sort.org_users_repository.gomoves from the inlineifadded in fix(store): org users search reports a value that is not a uuid as bad input #1942 to the same switch, so all eleven read alike.search_invalid_uuid_pg_test.go: one case per search against a real postgres. Neworganization_billing_test.go: handler cases for the mapping above.Technical Details
checkPostgresErroris not automatic, it is a function each caller invokes. None of these searches called it, so the driver error travelled untouched to the handler anderrors.Is(err, ErrInvalidTextRepresentation)was false everywhere.The mapping sits after the transaction rather than inside its closure, so it does not depend on how the transaction wrapper reports a rollback.
The organizations search takes no id. Its bad value arrives through the
idfilter, which compares against the organizations primary key.BillingInvoiceRepository.Searchgets the same treatment. Its twoerrDBwraps used%s, which kept the cause out of the error chain, so nothing could have matched it; they now use%w. It returnsinvoice.ErrBadInputbecause that is the sentinel its own handler checks.UserRepository.Searchis left out. Itsidfilter is cast to text before comparison, so a malformed uuid never reaches a uuid column, and nothing else in that search binds a caller value to one.SearchOrganizationInvoicesandSearchOrganizationProjectsansweredCodeInvalidArgumentwith an internal server error message. Both now pass the reason through, like the other nine handlers.Test Plan
go test ./internal/store/postgres/ ./internal/api/v1beta1connect/passesmainfor ten of the eleven searches; organization users already passed there because of fix(store): org users search reports a value that is not a uuid as bad input #1942golangci-lint runon both packages reports no issuesmainand this branch on the same database with only the binary swappedSearchOrganizationProjectsSearchOrganizationServiceUserCredentialsSearchProjectUsersSearchOrganizationsSearchOrganizationUsersSearchOrganizationPATsSearchUserOrganizationsSearchUserProjectsSearchOrganizationServiceUsersSearchOrganizationInvoicesSearchOrganizationTokensFour RPCs change. Of the rest, organization PATs, user organizations and user projects validate the id as a uuid in proto so the request stops earlier, and organization service users, invoices and tokens resolve the organization before the search and answer not found. Their repositories are still covered, because the same path is reachable through a filter value.
SQL Safety
?placeholders,goqu.Ex{}, orgoqu.Record{}— neverfmt.Sprintfor+building a query that gets executed.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L.//nolint:forbidigoor// #nosec G20xannotations.