Repository navigation
Fix the security audit findings: promo caps, keepalive anon access, error text, import DNS pinning - #230
Merged
Conversation
share's outer catch copied the thrown message (PostgREST or Postgres detail) into the 500 body. create-portal copied Stripe's message, which can name the customer id or the rejected parameter. Both now answer with a fixed error and log the detail server-side. create-portal sends code portal_failed, which the browser translates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keepalive_events and keepalive_heartbeat had TO anon policies and GRANT ALL to anon, so the publishable key could insert unbounded event rows, read them, and overwrite the heartbeat without a session. Revoke client grants on both tables and the event sequence, drop the anon policies, and keep keepalive_touch() service-role only where it exists. The external monitor's VPS timer already uses the service-role key and keeps working. A monitor run with the publishable key now gets 401/403. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
redeem_promo_code_atomic returned early for discount codes, before user_promos was written and redemptions_count incremented, so max_redemptions and single use never applied. create-checkout then set allow_promotion_codes, which lets Stripe accept any promotion code on the account. A discount row now carries its Stripe promotion-code id in config.stripe_promotion_code. Claiming it records the user and counts against the cap; asking again for a held code returns the same id without counting. create-checkout claims the code as the user with p_discount_only (so a non-discount code typed there grants nothing) and sends only discounts[0].promotion_code. Every refusal is one promo_invalid answer. The pricing dialog keeps a claimed discount code in the field so the upgrade sends it. Admin requires the promo_ id when creating a discount code. Deploy order: the migration before create-checkout. Existing discount rows need a stripe_promotion_code before they work again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fetch() resolves the hostname again when it connects, so a name whose DNS answer changed between validateResolvedPublicHost and the connection (rebinding) could reach a private range. import-source now dials one of the checked addresses directly (TCP, then TLS with SNI and certificate checks for the hostname) and reads the answer with a small HTTP/1.1 client: Content-Length, chunked and close-delimited bodies, gzip and deflate, 1xx skipped, a 32 KB head cap, and anything else refused. Each redirect hop is checked and pinned the same way. Checked in the pinned edge-runtime image (v1.77.4) against real HTTPS sites, an http-to-https redirect, and a certificate for the wrong name. Closes the recorded exception in docs/standards/security.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rade to Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 1: one address that never answered spent the whole import deadline, so a healthy second address was never tried, and a TLS handshake that stalled was not cancelled when the deadline aborted. Each address now gets 3 s to connect and finish TLS before the next is tried, and an abort closes the connection mid-handshake. Deno still holds the TCP socket of a cancelled handshake until the worker exits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ndings Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed.
Note 🎁 Summarized by CodeRabbit Free
Comment |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the findings from the security audit in draft #226 (
SECURITY-AUDIT.mdoncursor/security-audit-ccaa). I checked each one against currentmainbefore fixing it.Warning
Keepalive monitor, Pi backup. After the keepalive migration reaches a project, the Pi's GitHub Actions copy of
supabase-keepalive-monitorgets 401/403 on its REST and RPC steps for DebateLab staging and production, because itsSUPABASE_KEEPALIVE_PROJECTS_B64entries for those two projects use the publishable key. Its last run already got 401 on/rest/v1/for both. The VPS timer, which the monitor's README calls the primary scheduler, uses the service-role key and keeps working. To fix the Pi, switch those two entries toserviceRoleKey, or setrestRead,restWrite,rpcandstoragetofalsefor them. Staging applies migrations on its own after merge, so the Pi goes red for staging at merge, not at the production deploy.What changed
1. Discount promo caps (Medium). Confirmed on
main. Thediscountbranch ofredeem_promo_code_atomicreturned beforeuser_promoswas written andredemptions_countwas incremented, andcreate-checkoutsetallow_promotion_codes: true.20261024120100_discount_promo_claims.sqlreplaces the function. A discount row must carry its Stripe promotion-code id inconfig.stripe_promotion_code(^promo_[A-Za-z0-9]+$). Claiming one records the user and counts againstmax_redemptions. A user asking again for a code they already hold gets the same id back without counting again, even after the cap is reached. A discount row without a valid id answers like an unknown code.p_discount_onlyargument (defaultfalse). Checkout passestrue, so a non-discount code typed into the Checkout path is refused and grants nothing. Adding the argument changes the signature, so the migration drops the old one-argument function and creates the new one in the same transaction. If both existed, PostgREST would refuse every call as ambiguous.redeem-promostill sends onlyp_codeand resolves to the new function.create-checkoutclaims the code as the user, with the user-scoped client and after the open-subscription check, and sends onlydiscounts: [{ promotion_code }]. It never setsallow_promotion_codes. Every refusal, including the lockout, is onepromo_invalid400. The service-role promo lookup anddiscountPromoRefusalare gone. If Stripe fails after the claim, the claim stays counted, which errs on the side of the cap.promo_id, in the form and in a server-side check in theadminfunction.2. Anon keepalive tables (Medium). Confirmed. The tables had
TO anoninsert/select/update policies andGRANT ALLto anon. Before locking anything down I found the only writer,Programcode/supabase-keepalive-monitor. A VPS systemd timer runs it as the primary scheduler with the service-role key, and a backup workflow on the Pi runs it with the publishable key for DebateLab (see the warning above). Nothing in this repo reads or writes these tables.20261024120000_keepalive_server_only.sqlrevokes all client grants on both tables and the event sequence, drops the five anon policies, and keepskeepalive_touch(text, jsonb)service-role only when it exists. That function exists only in the hosted projects, where the monitor's bootstrap SQL creates it, so the revoke is guarded. No rows or tables are dropped.publishable-access.sqlalso adds anon policies onstorage.objectsforkeepalive-monitor/checks/*. Those policies live in the hosted projects, not this repo. They have the same problem, since the publishable key can read and overwrite those objects, and should be removed on the hosted side.3.
shareerror text (Low). Onmainthe quota failure the audit cites is already caught with a fixed message. The outercatchstill copied any other thrown message into the 500 body. It now answersFailed to create share linkand logs the detail, truncated.4.
create-portalerror text (Low). Confirmed. Stripe failures now answer with a fixedportal_failederror, which the browser translates with the existingserverErrors.portalFailedtext. Stripe's type, code, request id and message go to the log.5.
import-sourceDNS rebinding (unranked in the audit). Confirmed.validateResolvedPublicHostresolved the name, thenfetchresolved it again. I ran a spike in the pinned edge-runtime image,supabase/edge-runtime:v1.77.4.Deno.connectto an IP plusDeno.startTlswith the hostname works there, so I implemented the pinned dial.validateResolvedPublicHostnow returns the addresses it checked, and newimport-source/pinned-fetch.tsconnects only to one of them. It opens TCP to the address, then TLS with SNI and certificate checks for the hostname, and finishes the handshake before sending anything. It reads the answer with a small HTTP/1.1 client that handles one GET per connection, Content-Length, chunked and close-delimited bodies, gzip and deflate, 1xx skipped, a 32 KB head cap. Anything else is refused. Socket and TLS errors reach the user as fixed messages. Each redirect hop gets the same check and pin.docs/standards/security.mddrops the recorded exception and states the rule.Open
discounts, or a rebinding DNS server. Unit tests show the pin, and so does a connection to an unresolvable hostname in the runtime.storage.objectsanon policies on the hosted projects are still open (see finding 2).Deploy later (not done here)
No migration, function deploy or production change was made.
20261024120000_keepalive_server_only.sqland20261024120100_discount_promo_claims.sql.create-checkout,create-portal,share,import-sourceandadmin. Deploycreate-checkoutonly after the promo migration. The new function calls the RPC withp_discount_only, which the old database function does not accept, so every promo checkout would fail until the migration lands.redeem-promoitself is unchanged and works with both.UPDATE public.promo_codes SET config = config || '{"stripe_promotion_code": "promo_..."}' WHERE code = '...' AND type = 'discount';Tests
./.superset/db-gate.sh --db-only): all 31 files, 541 tests passed, including the newdiscount_promo_claims_test.sql(cap reached by the last distinct user, the next user refused with the unknown-code message,redemptions_countequal to the cap, a holder's repeat call not counted, the Checkout path refusing a non-discount code and granting nothing, a single function overload) andkeepalive_server_only_test.sql(anon and authenticated hold no privilege and no policy; anon insert, select and heartbeat update fail with 42501; the service role can still upsert the heartbeat and insert an event).legacy_table_grants_test.sqlnow asserts that anon has no keepalive sequence access.npm run test:edge): 482 passed. New: Checkout sendsdiscounts[0][promotion_code]and noallow_promotion_codes, and claims as the user; a refused claim creates no session; a Stripe portal failure answers with the fixed body (the old handler leaked the customer id);pinnedGetconnects to the given address without resolving the hostname, skips 103, decodes chunked gzip, reads close-delimited bodies, refuses malformed answers, falls through to the next checked address (including one that never answers, after a 3 s per-address budget), and closes on abort, including mid-TLS-handshake; the import handler dials exactly the addresses each hop checked across a redirect. I ran the checkout and portal tests against the old handlers, and they failed.pinnedGetfetched example.com, en.wikipedia.org (http to https redirect), bbc.com, gov.uk and github.com. A certificate for the wrong name was refused with the fixed message. A hostname that cannot resolve still connected to the given address, which shows no second lookup happens.localtest.me(127.0.0.1) is refused.npm run verify: passed (vitest 651, edge 480 at that point, Playwright smoke 47).verify-debatelab, fake provider), desktop and mobile Chromium:recipes/billingandrecipes/billing-states, 18 tests, all passed (16 in the first run, where the newbill-discountfailed on my selector for a trial account; after the fix,bill-discountandbill-promopassed on both viewports).bill-discountshows the code claimed once, a second Apply not counted again, the code kept in the dialog, and Upgrade sending it tocreate-checkout. Alsorecipes/library12/12, includinglib-import, which imports a live Wikipedia page through the local Edge runtime and so goes through the pinned client;e2e/full-stack/billing4/4.pinned-fetch.ts, both fixed incad752f9: a stalled first address used up the whole deadline, and a stalled TLS handshake wasn't cancelled on abort. Round 2 oncad752f9found nothing.GitHub Actions is billing-blocked, so CI here is red without running. The results above are local runs, not a CI pass. After merging the latest
maininto the branch I re-ran docs check, lint, typecheck,npm test(651),npm run test:edge(482) and the build at the pushed head; all passed.🤖 Generated with Claude Code