Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions docs/hardening/ONDA-7-DISCOUNT-FAIL-CLOSED.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
# Onda 7 — validate_quote_real_discount fail-closed em NULL

**Data:** 14 de maio de 2026
**PR alvo:** cleanup/onda-7-validate-discount-fail-closed
**Bloqueador resolvido:** B-4 da auditoria de 10/mai/2026
**Tempo de execução:** ~30 minutos
**Risco:** baixo (mudança conservadora que não quebra fluxos existentes)
**Impacto financeiro:** alto (evita desconto ilimitado para vendedor sem limite cadastrado)

## Contexto

A função trigger `public.validate_quote_real_discount` (chamada antes de INSERT/UPDATE em `quotes`) tinha um bypass NULL:

```sql
SELECT max_discount_percent INTO _max_allowed
FROM public.seller_discount_limits WHERE user_id = NEW.seller_id;

IF _max_allowed IS NOT NULL AND _real_pct > _max_allowed THEN -- ⚠️ BUG
-- bloqueia
END IF;
```

**O bug:** quando o vendedor não tinha linha em `seller_discount_limits`, `_max_allowed = NULL`. A condição `_max_allowed IS NOT NULL AND ...` era então FALSA, e o trigger deixava passar **qualquer desconto** sem checagem.

**Cenário real descrito na auditoria:**

1. Admin promove novo vendedor.
2. Esquece de cadastrar limite de desconto.
3. Vendedor cria orçamento com 99% de desconto.
4. Trigger não bloqueia.
5. Cliente fecha negócio. Sistema persiste. Margem evapora.

## Estado pré-fix no banco production

Query executada antes do fix:

| Role | Total | Com limite | Sem limite |
|---|---|---|---|
| `dev` | 1 | 1 | 0 ✅ |
| `admin` | 2 | 1 | **1** ⚠️ |
| `vendedor` | 5 | 5 | 0 ✅ |

Observações:
- Todos os 5 vendedores ativos hoje têm limite cadastrado — fix não quebra ninguém.
- 1 admin não tem limite, mas admins pegam bypass via `_is_admin` (lógica intocada).
- Vendedor novo entrando = vulnerável ao bug B-4.

## Mudanças aplicadas

```diff
SELECT max_discount_percent INTO _max_allowed
FROM public.seller_discount_limits WHERE user_id = NEW.seller_id;

+ -- Onda 7 (B-4): fail-CLOSED em NULL.
+ _max_allowed := COALESCE(_max_allowed, 0);
+
- IF _max_allowed IS NOT NULL AND _real_pct > _max_allowed THEN
+ IF _real_pct > _max_allowed THEN
IF NOT EXISTS (...) THEN
+ IF _max_allowed = 0 THEN
+ RAISE EXCEPTION 'Vendedor sem limite de desconto cadastrado. Solicite ao administrador o cadastro em seller_discount_limits, ou peca aprovacao para o desconto de %%%.', _real_pct
+ USING ERRCODE = 'check_violation';
+ ELSE
RAISE EXCEPTION 'Desconto real (%%%) excede o limite do vendedor (%%%). Solicite aprovacao antes de salvar.',
_real_pct, _max_allowed USING ERRCODE = 'check_violation';
+ END IF;
END IF;
END IF;
```

## Opções consideradas

A auditoria sugeriu 3 opções:

| Opção | Comportamento | Decisão |
|---|---|---|
| **A (RAISE EXCEPTION em NULL)** | Vendedor sem limite NÃO CONSEGUE criar quote nem com desconto 0% | Rejeitada — muito agressiva, quebra UX |
| **B (COALESCE NULL → 0)** ✅ | Vendedor sem limite não consegue dar desconto > 0% (mas pode criar quote sem desconto) | **Escolhida** |
| C (trigger ao promover) | Auto-criar linha default em seller_discount_limits ao promover papel | Adiada — vai numa Onda futura |

Opção B é a mais conservadora: não quebra fluxo existente (5 vendedores têm limite, continuam funcionando igual), mas fecha a porta pra desconto infinito.

## Escopo intencionalmente limitado

A mesma função tem outro problema mencionado em §3.2 da auditoria: o check de admin usa `role = 'admin'`, enquanto a hierarquia atual é `dev > supervisor > agente`. **Não foi alterado nesta Onda** porque:

1. Verificação no banco mostrou que **existem 2 usuários reais com `role = 'admin'`** (a auditoria assumia 0). O check funciona hoje.
2. Tema do "dual admin pattern" está **deferido** por decisão arquitetural separada (memória do PO sobre PR-A na fase F2 do plano).
3. Foco da Onda 7 é EXCLUSIVAMENTE B-4 (NULL bypass).

## Comparação antes/depois

| Cenário | Antes (com bug) | Depois (Onda 7) |
|---|---|---|
| Vendedor com limite cadastrado, desconto dentro do limite | ✅ Passa | ✅ Passa (idem) |
| Vendedor com limite cadastrado, desconto excede limite | ❌ Bloqueia (msg padrão) | ❌ Bloqueia (msg padrão, idem) |
| Vendedor SEM linha em seller_discount_limits, desconto 0% | ✅ Passa (sem desconto, sem trigger) | ✅ Passa (idem — nem entra no IF) |
| **Vendedor SEM linha, desconto > 0%** | ⚠️ **Passa qualquer desconto** | ✅ **Bloqueia com mensagem clara** |
| Admin (role='admin'), qualquer desconto | ✅ Bypass (idem) | ✅ Bypass (idem) |

## Validação empírica

1. ✅ SELECT em `pg_proc` confirma que `validate_quote_real_discount` tem `COALESCE(_max_allowed, 0)` no corpo
2. ✅ Teste SQL isolado confirma que `COALESCE((SELECT ... WHERE user_id não existe), 0) = 0`
3. ✅ Comment registrado: `Onda 7 (B-4): ... NULL agora trata como 0 (fail-closed em NULL bypass)`

## Próximos passos

- **Opção C** (trigger ao promover papel) pode entrar numa Onda futura, mas não é crítico depois deste fix — o admin agora recebe uma mensagem clara "Vendedor sem limite de desconto cadastrado" e sabe o que fazer.
- **Dual admin pattern** continua deferido para PR-A da fase F2.

## Aplicação em prod

Migration aplicada em prod (`doufsxqlfjyuvxuezpln`) em **14/mai/2026 16:52 UTC** via MCP `apply_migration`. Versão: `20260514165252`.

ADR 0006 respeitada: nenhum `supabase db push` foi executado.

## Riscos / rollback

- **Falso positivo mínimo:** se alguém apagar acidentalmente uma linha de `seller_discount_limits` de um vendedor ativo, o próximo quote com desconto vai bloquear. Mensagem clara guia o admin a recadastrar. Risco aceitável.
- **Rollback:** aplicar nova migration restaurando o código antigo (não recomendado — reabre B-4).
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
-- Onda 7 (B-4 da auditoria de 10/mai/2026): fail-CLOSED em NULL no validate_quote_real_discount.
--
-- CONTEXTO:
-- A funcao trigger validate_quote_real_discount tinha bypass NULL: quando o vendedor
-- nao tinha linha em seller_discount_limits, _max_allowed=NULL e a condicao
-- "_max_allowed IS NOT NULL AND _real_pct > _max_allowed" era FALSA, deixando passar
-- DESCONTO INFINITO. Risco real em producao: vendedor novo entra, admin esquece de
-- cadastrar limite, vendedor da 99% de desconto, margem evapora.
--
-- MUDANCA:
-- 1. COALESCE(_max_allowed, 0) apos o SELECT INTO — NULL vira 0% (default conservador).
-- 2. Mensagem de erro distinta no caso "sem cadastro" vs "estourou limite".
--
-- ESCOPO INTENCIONALMENTE LIMITADO:
-- O check de admin (_is_admin via role='admin') NAO foi alterado. Tema do "dual admin
-- pattern" esta deferido para decisao arquitetural separada (memoria do PO).
--
-- APLICADA EM PROD em 14/mai/2026 via MCP apply_migration (ADR 0006).
-- Este arquivo registra a migration no repo para historico/auditabilidade.

CREATE OR REPLACE FUNCTION public.validate_quote_real_discount()
RETURNS trigger
LANGUAGE plpgsql
SECURITY DEFINER
SET search_path TO 'public'
AS $function$
DECLARE
_markup NUMERIC := COALESCE(NEW.negotiation_markup_percent, 0);
_apparent_pct NUMERIC := COALESCE(NEW.discount_percent, 0);
_presented NUMERIC := COALESCE(NEW.subtotal, 0);
_real_sub NUMERIC;
_final NUMERIC;
_real_pct NUMERIC;
_max_allowed NUMERIC;
_is_admin BOOLEAN;
BEGIN
IF _markup > 0 THEN _real_sub := _presented / (1 + _markup / 100);
ELSE _real_sub := _presented; END IF;
_final := _presented * (1 - _apparent_pct / 100);
IF _real_sub > 0 THEN
_real_pct := ROUND(((_real_sub - _final) / _real_sub) * 100, 2);
ELSE _real_pct := 0; END IF;
NEW.real_subtotal := ROUND(_real_sub, 2);
NEW.real_discount_percent := _real_pct;

IF NEW.status IN ('draft', 'pending') AND NEW.seller_id IS NOT NULL AND _real_pct > 0 THEN
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce the fail-closed check outside draft/pending

This fail-closed logic only runs for draft and pending, but I checked the active quote RLS and quotes_insert_scope/quotes_update_scope allow a seller to insert or update their own quote without a status restriction, while pending_approval/sent are valid quote statuses. A seller with no discount-limit row can therefore submit the same discounted quote as pending_approval or sent and skip the _max_allowed check entirely; move the positive-discount validation to all seller-controlled statuses, or require an approved request before any status can bypass it.

Useful? React with 👍 / 👎.

SELECT EXISTS (
SELECT 1 FROM public.user_roles
WHERE user_id = NEW.seller_id AND role = 'admin'
) INTO _is_admin;

IF NOT _is_admin THEN
SELECT max_discount_percent INTO _max_allowed
FROM public.seller_discount_limits WHERE user_id = NEW.seller_id;

-- Onda 7 (B-4): fail-CLOSED em NULL. Vendedor sem linha em seller_discount_limits
-- nao tem mais desconto ilimitado — agora trata como 0% (precisa aprovacao para qualquer desconto).
_max_allowed := COALESCE(_max_allowed, 0);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat null limits as exceeded in the quote UI

With this migration, a non-admin seller who has no seller_discount_limits row now has an effective limit of 0%, so any positive real discount on a draft/pending quote raises here. I checked the quote builder path: useQuoteBuilderState currently returns false for isDiscountExceeded when maxDiscountPercent === null, and it only creates approval requests when maxDiscountPercent !== null, so the UI still shows the normal create/save action and then hits this database exception instead of offering the approval flow. Either the frontend needs to treat a missing limit as 0 or the migration needs a compatible path for creating approval requests before blocking these saves.

Useful? React with 👍 / 👎.


IF _real_pct > _max_allowed THEN
IF NOT EXISTS (
SELECT 1 FROM public.discount_approval_requests
WHERE quote_id = NEW.id AND status = 'approved'
AND requested_discount_percent >= _real_pct
) THEN
-- Mensagens distintas: "sem cadastro" (Onda 7) vs "estourou limite" (comportamento original)
IF _max_allowed = 0 THEN
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A condição _max_allowed = 0 não distingue “sem cadastro” de “limite cadastrado em 0%”, então pode exibir a mensagem errada para vendedores com limite real de 0%.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260514165252_onda7_validate_quote_real_discount_fail_closed_null.sql, line 67:

<comment>A condição `_max_allowed = 0` não distingue “sem cadastro” de “limite cadastrado em 0%”, então pode exibir a mensagem errada para vendedores com limite real de 0%.</comment>

<file context>
@@ -0,0 +1,87 @@
+            AND requested_discount_percent >= _real_pct
+        ) THEN
+          -- Mensagens distintas: "sem cadastro" (Onda 7) vs "estourou limite" (comportamento original)
+          IF _max_allowed = 0 THEN
+            RAISE EXCEPTION
+              'Vendedor sem limite de desconto cadastrado. Solicite ao administrador o cadastro em seller_discount_limits, ou peca aprovacao para o desconto de %%%.',
</file context>
Fix with Cubic

RAISE EXCEPTION
'Vendedor sem limite de desconto cadastrado. Solicite ao administrador o cadastro em seller_discount_limits, ou peca aprovacao para o desconto de %%%.',
Comment on lines +53 to +69
Comment on lines +53 to +69
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Diferenciação de erro pode ficar incorreta para limite explícito 0%.

Em Line 67, a regra usa _max_allowed = 0 para inferir “sem cadastro”. Se existir linha em seller_discount_limits com max_discount_percent = 0, a mensagem “sem limite cadastrado” fica errada (há cadastro, com limite zero).

💡 Ajuste sugerido
 DECLARE
   _markup       NUMERIC := COALESCE(NEW.negotiation_markup_percent, 0);
   _apparent_pct NUMERIC := COALESCE(NEW.discount_percent, 0);
   _presented    NUMERIC := COALESCE(NEW.subtotal, 0);
   _real_sub     NUMERIC;
   _final        NUMERIC;
   _real_pct     NUMERIC;
+  _max_allowed_raw NUMERIC;
   _max_allowed  NUMERIC;
+  _has_limit    BOOLEAN := FALSE;
   _is_admin     BOOLEAN;
 BEGIN
@@
-      SELECT max_discount_percent INTO _max_allowed
+      SELECT max_discount_percent INTO _max_allowed_raw
       FROM public.seller_discount_limits WHERE user_id = NEW.seller_id;
+      _has_limit := FOUND;
 
-      _max_allowed := COALESCE(_max_allowed, 0);
+      _max_allowed := COALESCE(_max_allowed_raw, 0);
@@
-          IF _max_allowed = 0 THEN
+          IF NOT _has_limit OR _max_allowed_raw IS NULL THEN
             RAISE EXCEPTION
               'Vendedor sem limite de desconto cadastrado. Solicite ao administrador o cadastro em seller_discount_limits, ou peca aprovacao para o desconto de %%%.',
               _real_pct
               USING ERRCODE = 'check_violation';
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@supabase/migrations/20260514165252_onda7_validate_quote_real_discount_fail_closed_null.sql`
around lines 53 - 69, The current logic coalesces NULL to 0 and later uses
"_max_allowed = 0" to infer "no cadastro", which misclassifies an explicit 0
limit as "no cadastro"; change the query flow to detect whether a row existed
(use the SELECT ... INTO followed by checking FOUND or assign a boolean like
_has_row) and keep _max_allowed as-is (or COALESCE to 0 only for arithmetic) but
use the row-existence flag to decide the "sem cadastro" message; update the
conditional around _max_allowed and the RAISE EXCEPTION paths (referencing
_max_allowed, the SELECT INTO from public.seller_discount_limits for
NEW.seller_id, and the subsequent IF NOT EXISTS check against
public.discount_approval_requests) so explicit max_discount_percent = 0 produces
the "estourou limite" message while a missing row triggers "sem cadastro".

_real_pct
USING ERRCODE = 'check_violation';
ELSE
RAISE EXCEPTION
'Desconto real (%%%) excede o limite do vendedor (%%%). Solicite aprovacao antes de salvar.',
_real_pct, _max_allowed
USING ERRCODE = 'check_violation';
END IF;
END IF;
END IF;
END IF;
END IF;
RETURN NEW;
END;
$function$;

COMMENT ON FUNCTION public.validate_quote_real_discount IS
'Onda 7 (B-4): valida real_discount_percent vs seller_discount_limits.max_discount_percent. NULL agora trata como 0 (fail-closed em NULL bypass). Admins (role=admin) tem bypass mantido.';
Loading