test: adiciona suíte de testes, e no caminho corrige o escape de views e a precisão monetária - #2822
Merged
Merged
Conversation
O `esc_url()` aceitava qualquer esquema, o que abria `javascript:` em qualquer atributo `href`. Ele passa a aceitar apenas http, https, mailto, tel, ftp e ftps, rejeitando controles e nao-ASCII. `esc_img_src()` nasce separado, e nao como opcao do `esc_url()`: as duas funcoes respondem a perguntas diferentes. Clicar num link `data:text/html` executa o conteudo, entao `href` o rejeita; ja o `<img src>` de um QR Code e um `data:image/...` legitimo, e `esc_url()` o devolveria vazio, fazendo a imagem sumir silenciosamente. Por isso `data:` so passa em `img src`, e so com o prefixo `data:image/`. O gate de XSS ganha o parser fail-closed, operador virgula, `echo $x;` solto, corte correto de statement, funcoes desconhecidas sem argumento, casts, literais e ternarios. Os 42 `<img src>` de QR Code e logo nas 27 views passaram a usar `esc_img_src()`. Corrige tambem a resolucao de linha do relatorio: a linha vinha de um `strpos()` que achava a primeira ocorrencia da expressao no arquivo, e nao do offset do regex. Com `esc_scalar($result->idOs)` em `os/visualizarOs.php` o gate apontou a linha 8, um `echo` de markup legitimo, em vez da 411. O offset do regex ja era a posicao exata.
O create_base declarava as colunas monetarias assim:
'preco' => [
'type' => 'DECIMAL',
'constraint' => 10, 2,
],
O PHP nao aplica o operador virgula ali dentro de um array: le
'constraint' => 10 e descarta o 2 como elemento posicional. O dbforge so
usa o CONSTRAINT, entao a coluna saia como DECIMAL(10), que o MySQL le como
DECIMAL(10,0), sem nenhuma casa decimal.
O create_base foi corrigido no lugar, o que so ajuda quem ainda nao rodou
essa migration. Quem rodou entre 2012 e agora tem as quatro colunas sem
centavos, e nenhuma migration posterior as repara. A 20220320173741 toca
lancamentos, os, vendas, cobrancas, produtos_os, servicos_os e
itens_de_vendas, mas nao contas, produtos e servicos.
O estrago real: produtos.precoVenda e produtos.precoCompra alimentam
Relatorios_model::produtosCustom(), que faz
SUM(produtos.estoque * produtos.precoVenda) e filtra por precoVenda
BETWEEN. A avaliacao de estoque em dinheiro era arredondada para reais
inteiros. servicos.preco e contas.saldo tiveram o mesmo destino.
A migration so amplia a precisao. Os centavos que o DECIMAL(10,0) ja
descartou na escrita nao voltam: o dado foi perdido na hora em que foi
gravado, e nenhuma alteracao de schema consegue recupera-lo.
A seed Configuracoes tambem passou a pular as chaves ja existentes. Sete
delas vem das migrations com o mesmo idConfig que a seed usaria, e
configuracoes.config tem UNIQUE em unique_valor, entao um insert cego
abortava com 1062 na primeira linha e tornava o Tools::seed()
impossivel de reexecutar em qualquer instalacao.
O banco.sql acompanha as migrations para o check:parity continuar fechando.
O verificarLogin() montava a resposta com echo e, em varios caminhos, com exit(). Isso torna o controller intestavel: o exit() mata o processo inteiro em vez de encerrar a requisicao, entao nao ha como afirmar nada sobre o comportamento depois dele num teste. O payload agora e montado e devolvido pelo $this->output. Os caminhos de erro foram unificados em respondFailure(). Eram tres variantes quase iguais, e as tres devolviam o token CSRF; um quarto caminho (o email inexistente) ficava de fora por estar escrito de outra forma, o que e exatamente o tipo de divergencia que o teste precisa pegar. O cabecalho CORS passou a sair pelo set_header() do CI_Output em vez do header() do PHP. O CI3 so emite os cabecalhos no fim da requisicao, o que mantem o cabecalho correto sem acordar os avisos de "Cannot modify header information" do CLI, que o Whoops converte em excecao. Nao existe um set_headers() plural no CI3, entao cada cabecalho sai pelo set_header(). O redirect de destino invalido saiu de redirect() para respond_redirect(), que escolhe o status a partir do metodo e do protocolo: 307 e 303 preservam o metodo, que e o que se quer num POST, e fora de HTTP/1.1 nao ha informacao confiavel e o redirect() cai em 302. A regra saiu do controller para um helper proprio porque o status sai por header() nativo, que em CLI nao tem getter: como funcao pura, ela fica coberta de verdade em vez de replicada na documentacao.
A suíte roda in-process: o app sobe uma vez no bootstrap e cada teste instancia o controller direto, porque reiniciar o ciclo de vida do CI3 por teste nao e possivel. Isso expoe bugs de reentrancia do framework que o ControllerTestCase contorna, e TestApplication::resetSharedState() tem de rodar antes de cada controller: CI_Controller::__construct() percorre is_loaded() e chama load_class() com o nome cru, que so procura APPPATH/libraries/<nome>.php e erra a Session (que e CI_Session), a Permission (sem prefixo CI_) e a Form_validation (que o Loader injeta, e chega nula via load_class()). Loader::_ci_models tambem precisa ser limpo, porque Loader::model() retorna cedo em in_array() antes de acoplar o modelo, e do segundo controller em diante $this->Some_model seria null. O banco de teste e montado pela cadeia de migrations, nao pelo banco.sql. O dump e todo CREATE TABLE IF NOT EXISTS e nao tem um unico DROP, entao importa-lo nunca exercita uma linha de migration, e uma instalacao montada assim nunca seria pega antes de chegar a um usuario. O setup-db.php recria o banco, roda Tools::migrate() e carrega as seeds canonicas. Como essa recriaacao acontece dentro da raiz do documento, o acesso a application/tests/ e bloqueado em dois lugares: .htaccess para o Apache e location ^~ /application/ no nginx. O ^~ e obrigatorio, porque sem ele a location ~* \.php$ e avaliada antes — as regex tem prioridade sobre prefixo comum — e o acesso passaria. O nginx nao le .htaccess, entao mexer num sem o outro reabre a pasta em silencio. O check-schema-parity.php monta um segundo banco a partir do banco.sql e compara com o do migrations, tabela a tabela e coluna a coluna. E a barreira que mantem o dump do instalador e a cadeia de migrations de divergirem, ja que nada mais exercita os dois. Compara nomes e tipos, nao indices, foreign keys, charset ou collation, e exclui a tabela de controle. O CI passa a rodar test, format:check, xss:check e check:parity. O xss:check falha o build em vez de so imprimir, e o baseline continua vazio.
…ller O CI3 carrega o banco pelo autoloader, que roda dentro de CI_Controller::__construct() — ou seja, uma vez por controller construído. Loader::database() deveria devolver a conexão já aberta, mas a guarda dele testa isset($CI->db) com $CI = get_instance(), e no meio do construtor get_instance() é o controller que está nascendo, que ainda não tem a propriedade $db. A guarda falha, o Loader abre uma conexão nova e guarda essa, trocando a que o processo inteiro usava. Na web é inofensivo: um request, um controller, uma conexão. Na suíte, não: a conexão é a mesma do processo todo, e é nela que qualquer transação de caso é aberta. Trocá-la no meio do teste deixa a transação órfã num objeto que ninguém mais referencia, e cada escrita do teste é commitada na hora. Havia um segundo defeito no mesmo lugar. CI_Controller::$instance é private static e continua apontado para o último controller construído, de modo que get_instance() devolvia um controller já jogado fora para quem viesse depois. É por ele que Loader::database() resolve o $CI, então o problema se reforçava a cada chamada. resetSharedState() passa a devolver o singletão ao objeto super antes de cada controller, e o harness guarda essa referência em TestApplication::superObject() em vez de confiar em get_instance(). Com isso, callControllerRaw() reaponta o controller para a conexão do processo e fecha a sobra, em vez de deixá-la vazando. Os 76 casos existentes continuam verdes.
A suíte escrevia no banco de verdade e por isso exigia conserto à mão. O `logs` era o exemplo mais visível: cada login autenticado insere uma linha pelo log_info(), e o total subia de 5 em 5 a cada execução, porque nada desfazia essas escritas. A trait é opt-in por classe, como no Laravel: `use TransactsDatabase;`. Ela abre a transação num gancho `#[Before]` e a desfaz num `#[After]`, e nada mais precisa lembrar de restaurar o que o caso mexeu. login/verificarLogin é o exemplo do ganho. O caso da conta sem expiração salvava o `dataExpiracao`, escrevia null e devolvia o valor num `finally` — mas o `finally` era pulado por qualquer `fail()` antes dele, e aí o `dataExpiracao` ficava null para os testes seguintes, um vazamento que só apareceria como um "conta expirada" inexplicável em outro caso. Agora o caso grava o null e não restaura nada. Três detalhes do CI3 que a trait teve de respeitar, todos registrados no código porque são armadilhas silenciosas: - trans_begin() aninhado não abre SAVEPOINT: só incrementa _trans_depth, e trans_rollback() só fala com o banco quando o depth é exatamente 1. Por isso o fechamento é um laço que desce até zero. - _trans_depth é protected e não tem getter, então é lido por closure amarrada ao driver. - _trans_rollback() e _trans_commit() ligam de novo o autocommit, e é por isso que `@@autocommit` é a fonte do detector: o `_trans_depth` é um contador em PHP e pode dizer 1 com o banco sem transação nenhuma. Como o MySQL 8.4 não tem @@in_transaction e information_schema.INNODB_TRX não é confiável, o autocommit é o que separa de fato uma transação viva de uma que já acabou. Se o código sob testar commitou ou reverteu, o tearDown acusa em vez de devolver o banco silenciosamente sujo. A detecção não enxerga commit implícito de DDL, e a trait não deve ser usada em caso que rode migration: TestApplicationTest, que é justamente o que migra, fica de fora. Os casos de TransactsDatabaseTest interrogam a conexão de dentro do teste, mas isso não prova o descarte: uma trait que fechasse com trans_complete() passaria em todos eles. O par final é o que prova — um caso grava uma linha em `logs` e confirma que ela está visível dentro da transação, e o caso seguinte, que só roda depois, exige que ela tenha desaparecido. Verifiquei que o par falha de verdade trocando o rollback por commit na trait. `logs` fica em 0 depois de execuções repetidas, e a suíte vai a 82 casos.
O setup-db.php derrubava e recriava o banco a cada execução, e isso custava ~9,3s dos ~10s de uma execução. A cadeia de migrations é a parte cara: rodar Tools::migrate() contra um schema em dia leva 2ms. Medido antes e depois, `composer test` sai de ~10s para ~1,06s. A divisão passa a ser *mantém o schema, limpa os dados*, e saber em qual metade uma mudança cai é o ponto. O schema é reaproveitado quando TestDatabase::isSchemaCurrent() concorda, o que exige três coisas: o banco existe, `migrations.version` é a da migration mais recente, e uma impressão digital bate. As três precisam valer, e a terceira não é redundante com a segunda: editar o *conteúdo* de uma migration que já rodou, ou de uma seed, não muda o número do arquivo, então a versão continua em dia e um banco velho passaria pelo teste. A impressão digital cobre toda migration, as seeds e o TestFixtures.php, e mora em sys_get_temp_dir()/mapos-test-schema/<banco>.hash — num arquivo ao lado do banco e não numa tabela dentro dele, porque o check-schema-parity.php compara toda BASE TABLE exceto `migrations` e uma tabela a mais aqui leria como drift. No caminho reaproveitado nenhuma seed roda, e é a única coisa ali que não é óbvia: a seed Usuarios grava um idUsuarios explícito, então repetir a inserção numa tabela que já tem a linha aborta com 1062. No lugar das seeds entra a conferência das três contas, que era o que o script já fazia ao final. `Tools::migrate()` continua rodando nos dois caminhos: é um no-op de 2ms quando o schema está em dia, e é o que faz um banco meio montado se consertar sozinho. `composer test:fresh` (setup-db.php --fresh) força a remontagem, e ele apaga a impressão digital junto com o banco: uma montagem que falha no meio deixaria o arquivo de uma versão que não é a do banco, e a execução seguinte acreditaria nele. Os dados são limpos por caso, pela TransactsDatabase, que já existia. Uma classe pede a reinstalação da linha de base sobrescrevendo `resetsBaselineData()` para devolver true; antes de cada caso, dentro da transação que a trait acabou de abrir, ela apaga `usuarios` e reinstala as três contas. DELETE, nunca TRUNCATE, porque TRUNCATE é DDL e faria commit implícito da transação que a trait abriu um instante antes. É opt-in, e não automático, por dois motivos. O primeiro é de custo: uma classe que não escreve no banco não tem o que ganhar aqui, e paga o preço em todo caso. O segundo é o TransactsDatabaseTest, que precisa que `logs` NÃO seja limpa entre os seus dois casos: é a linha que sobrou ou não que prova que a trait descarta em vez de commitar, e limpar `logs` ali tornaria a prova uma tautologia. Por isso resetBaselineData() falha alto quando `logs` não está vazia, em vez de limpar, e aponta o `composer test:fresh`. Uma linha ali é indistinguível de uma transação commitada, que é justamente o que a trait existe para pegar. `configuracoes` não entra na reinstalação: 13 das suas 14 linhas vêm da seed, mas a `email_automatico` vem de uma migration e nenhuma seed a recria — apagar a tabela e rodar a seed de novo deixaria o banco sem essa linha para sempre, e a montagem seguinte nem perceberia, porque reconstrói tudo do zero e voltaria a tê-la. O quality.yml ganhou um passo, e não uma barra de tempo. O runner é limpo e paga a migration inteira de qualquer jeito, então o ganho de CI é zero. O passo existe porque a única forma de o reaproveitamento degradar é silenciosa: ele voltaria a remontar tudo e a suíte passaria igual, só mais devagar. Por isso o passo exige a mensagem de reuso, e não se limita a rodar o script. Um achado de verificação que vale mais que o ganho: a primeira versão do teste da reinstalação sujava a linha de base num caso e conferia no seguinte que a sujeira não tinha chegado. Desligando a reinstalação de propósito, a suíte continuava verde — o rollback do caso anterior já desfaz a sujeira, então o segundo caso vê a linha de base de qualquer jeito. O teste era uma tautologia, e um teste que passa com o recurso desligado não está testando o recurso. Ele foi trocado por um que chama a reinstalação diretamente e verifica as quatro formas de ela quebrar. O que a suíte não consegue observar, e está escrito no código como tal, é se a reinstalação roda dentro ou fora da transação: o estado final é o mesmo nas duas ordens.
…nada O gate de escape reprovava 0 achados com código 0 quando não lia nenhuma view, porque o caminho de `--update-baseline` transformava uma lista vazia em um arquivo de baseline vazio, sem erro. A partir dali o gate aprovava tudo: as 234 entradas de decisão sumiam e nenhuma voltava a ser conferida. A guarda de contagem de arquivos entra ANTES do caminho de escrita, e sai com 2 — distinto do 1, que pede correção numa view. "Não achei nada" e "não havia nada" são respostas diferentes, e só a primeira é um erro. O texto do relatório e o cabeçalho do baseline saem do script de linha de comando para `Report`, e passam a ser gerados a partir do próprio scan: as contagens que o cabeçalho afirma não podem divergir das entradas que ele descreve. A data original é preservada, porque ela é registro e contagem de execução não é. `--update-baseline` passa a mesclar em vez de sobrescrever, o que era a instrução que se anulava sozinha: o AGENTS.md mandava rodar o comando e depois explicar as entradas, e a primeira metade apagava a segunda. O relatório também deixa de ler a convenção de volta a partir da chave. `ViewScanner` devolve o agrupamento por conferência como dado, e ninguém mais precisa saber que o prefixo mora no meio da string.
…e schema O diretório Support/ era um plano só: oito classes de 200 a 350 linhas lado a lado, sem que a estrutura do diretório dissesse nada sobre o que dependia do que. Passa a ser App/, Clone/, Database/, Transaction/, ViewEscaping/ e Infra/, e a divisão é por ciclo de vida, não por tamanho. O ganho concreto é o leitor de metadados. Seis consultas de information_schema escritas à mão em cinco arquivos respondiam perguntas que já tinham dono, e a fifth era o leitor de nome de tabela reimplementado à mão — literalmente o que `SchemaReader` existe para substituir. A divergência entre duas leituras da mesma pergunta não dá erro: dá um número que alguém confia. `SchemaReader` é agora a única leitura, e o motivo está no arquivo: uma lista de tabelas vinda de SHOW e outra de information_schema podem discordar sobre o que existe sem ninguém perceber. `assertDatabaseNameIsSafe()` passa a exigir o sufixo `_test` E identificador seguro. A checagem do sufixo sozinha recusava `mapos_x_test; DROP DATABASE producao`; a do identificador sozinha recusava o banco errado. As duas juntas fecham o caminho, e um nome de banco que entra num DROP tem que passar pelas duas. `closeDriverTransaction()` se recusa a zerar o contador de um driver que não conhece. A versão anterior pulava o autocommit e zerava assim mesmo, para qualquer driver não-mysqli, e devolvia sem erro — o estado que o método existe para desfazer, criado em silêncio dentro de um tearDown. Agora um driver não suportado falha alto, e a exceção sai antes do contador, porque zerar o contador junto com a exceção faria o próximo caso achar que não há transação para desfazer. O TestSchemaCloneWorkerParityTest não paga mais por um banco que não usa. Ele herdava o trait de origem sintética, cujos hooks BeforeClass/AfterClass montavam e derrubavam dois bancos ANTES de o markTestSkipped() dentro de cada caso rodar — em toda execução da série, para zero asserções. Troca pelo trait compartilhado, que dá o describe() de que o caso precisa sem os hooks de banco.
Duas metades da mesma prova. Por dentro, a suíte do gate: precedência das regras, a guarda fail-closed, a tokenização das formas de saída, o agrupamento por conferência e o cabeçalho do baseline. Por fora, ZeroViewGuardTest executa o CLI de verdade numa árvore temporária com application/views vazio. A segunda metade é a que não dá para escrever contra a biblioteca. O defeito que a guarda existe para impedir é a ORDEM no executável — a checagem de cobertura precisa vir antes de --update-baseline escrever — e um teste da biblioteca não enxerga onde o exit() está. Um caminho de views configurável por variável de ambiente resolveria o teste e criaria exatamente o buraco que a guarda fecha: apontar o gate para uma pasta vazia e obter um verde. O contra-teste importa tanto quanto os outros: com uma view de verdade o caminho de escrita tem que funcionar, senão a guarda dispara sempre e o gate fica verde para sempre sem conferir nada. `ServedPathsTest` confere que /application/ e /tools/ estão bloqueados nos DOIS arquivos de nginx. Eles são quase-cópias sem ligação entre si, e o compose renderiza o template por cima do default.conf — mudar um sem o outro reabre a pasta sem nenhum aviso. O .htaccess do Apache não ajuda: nginx não o lê.
…o está escapado application/ já estava bloqueada e tools/ ficou de fora pelo motivo errado: o gate em si recusa execução por SAPI, então a pasta parecia inofensiva. O que mora ali não é código, é tools/xss-baseline.txt — o inventário de cada valor que chega à página sem escaper, com arquivo, expressão e linha. Servir isso é publicar um mapa do que não está escapado, que é a mesma informação que um XSS entrega um exploit por vez. O ^~ é obrigatório pelo mesmo motivo da regra acima: sem ele a ~* \.php$ é avaliada antes e o acesso passaria. Nos dois arquivos. O .htaccess do Apache cobre o caso dele, mas nginx não lê .htaccess, então a regra precisa existir dos dois lados — mudar num e não no outro reabre a pasta sem aviso. ServedPathsTest confere a paridade.
A execução paralela existia por um motivo real: TransactsDatabase faz rollback por caso, e LoginControllerTest e BaselineDataResetTest fazem DELETE e re-INSERT em usuarios, então em um banco COMPARTILHADO os dois processos tomam X-lock nas mesmas linhas do InnoDB e serializam. Dois processos que deveriam rodar juntos param de rodar juntos. O ParaTest não isola banco, então o clone faz: cada worker recebe o seu antes do boot do CI3, porque o autoloader `database` conecta enquanto index.php sobe e o banco precisa existir antes disso. O nome vem do token antes do sufixo _test, e TestDatabase::workerDatabaseName() é a função pura que deriva esse nome — qualquer nome escrito à mão é uma corrida, e a corrida custava 7,1s com 4 processos em vez de 1,5s. `test:parallel` fica com --processes=4 fixado e CI NÃO roda. O custo frio é linear nos workers porque cada um paga o próprio clone e o MySQL serializa DDL: 9,88s contra 2,32s do serial, para economizar 0,95s uma vez que os workers existam. Isso é infraestrutura para uma suíte que ainda não cresceu até ela. O grep do CI deixa de hardcodar "3 usuários disponíveis". O número é TestFixtures::USER_COUNT, e escrevê-lo no workflow criaria uma segunda fonte para a mesma verdade — que é a divergência que o workflow existe para pegar. O bootstrap também resolve as credenciais de MAPOS_TEST_DB_* e publica as cinco chaves DB_*, não três: a suíte abre duas conexões e config/database.php só conhece $_ENV. Sem o par de credenciais o CI passa localmente (o .env preenche) e falha no CI, onde não há .env.
…ontrollerTestCase LoginControllerTest mantinha a própria tabela de status por location, e ela já existe em GeneralHelperTest. Duas tabelas do mesmo dado divergem no dia em que uma linha muda em uma delas, e o teste passa conferindo o mapa errado — o que ocupa o lugar de uma conferência que não existe. O controller não é o dono da regra de redirect, o helper é, e o teste passa a ser do helper. No ControllerTestCase, os cabeçalhos acumulados são zerados em vez de apendados. Apendar o Location que o caso espera pareceria resolver, porque get_header() varre de trás para frente e devolve a última ocorrência — mas isso só funciona por acidente, e um caso que afirma a AUSÊNCIA de um header leria o valor antigo. Zerar é o que deixa as duas direções certas.
tools/ entra no mesmo regime de application/ e pelo mesmo motivo de fundo: a pasta fica na raiz do documento, e o que mora ali é o inventário do que não está escapado. O script se recusa a rodar fora de CLI, então nada ali é executável por HTTP, e mesmo assim o arquivo de baseline é dado que não deve ser servido. A seção do ParaTest entra com a assimetria medida, e não com a promessa. O custo frio é linear nos workers porque cada um paga o próprio clone e o MySQL serializa DDL: 9,88s contra 2,32s do serial para economizar 0,95s uma vez que os workers existam. O que decide se a ferramenta vale alguma coisa é o número frio, e hoje ele perde do serial outright. Por isso --processes=4 fixado e CI fora. Também entra a instrução do baseline na ordem correta, que antes se anulava sozinha: o comando primeiro, a explicação das entradas depois. A primeira metade da instrução apagava a segunda.
O gate de XSS estava verde sobre uma divida real: a quinta forma de
saida so casava um `echo` cujo valor comecava com `$`, entao uma linha
que embrulhasse um valor do banco numa concatenacao - a forma mais
comum de este projeto emitir um - nunca era lida por nenhuma conferencia.
A guarda fail-closed (EscapingChecks::unrecognizedOutput()) foi o que
expôs isso.
Cada valor agora passa pelo escaper do seu contexto: `esc()` em texto e
atributo entre aspas, `rawurlencode()` + `esc_url()` em `href`, `esc_css()`
dentro de `style`, `esc_img_src()` em `<img src>`. `esc()` dentro de um
atributo continua transparente para o JavaScript, que le o valor com
`.attr()` e escreve com `.val()`. Os campos WYSIWYG, que ja usavam
`printSafeHtml()`, foram deixados como estavam.
A divida esta paga: o baseline cai de 234 para 13 entradas, e nenhuma
delas e saida sem escape. Dez sao a palavra `print` de `@media print` e
`window.print()`, que nao e PHP. As outras tres sao
`views/errors/cli/*.php`, que o CodeIgniter so carrega quando `is_cli()`
e true - nesse ramo a mensagem nao ganha `<p>` e vira texto puro, entao
`esc()` mostraria `<` para quem le o erro no console.
Tres defeitos apareceram no caminho, e um deles muda o que a tela mostra:
- `conecte/visualizar_os.php`: `'R$ ' . $p->preco ?: $p->precoVenda`
associava como `('R$ ' . $p->preco) ?: $p->precoVenda`, entao o
fallback nunca acontecia e um item sem preco de venda aparecia como
`R$ 0`. Passou a espelhar o padrao da linha de servico.
- `tema/topo.php`: `saudacao()` era declarada sem o guard de
`function_exists()` que `mapos/login.php` usa, que e o "Cannot
redeclare" que a propria view documenta; o parametro que a funcao
nunca lia foi removido.
- `financeiro/lancamentos.php`: um modal de ~78 linhas dentro de um
comentario HTML, mais o binding de `$("#formDespesa").validate()`
apontando para um elemento que nunca chegava ao DOM. Ambos mortos.
Um ponto em aberto, deliberadamente nao tocado: `conecte/cobrancas.php`
renderiza o link do boleto fora da checagem de permissao `eCobranca`,
duplicando a linha de dentro. Isso e autorizacao, nao escape, e merece a
revisao propria.
`composer test` 302 testes, `xss:check` 13 achados e 0 novos,
`check:parity` 27 tabelas, `format:check` 0 de 315.
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
1 participant
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.
Descrição
Sete commits, e a ordem deles importa: a suíte de testes é o que fecha o trabalho, e ela só rendeu porque dois bugs independentes apareceram no caminho. Os dois últimos (conexão do processo e trait de transação) só existem porque a suíte passou a rodar de verdade — são defeitos do harness que ninguém encontraria sem ele.
fix(security): escapa saídas em views e endurece o gate de XSSO
esc_url()aceitava qualquer esquema, o que abriajavascript:em qualquerhref. Passa a aceitar apenas http, https, mailto, tel, ftp e ftps, rejeitando controles e não-ASCII.esc_img_src()nasce separado, e não como opção doesc_url(). As duas funções respondem a perguntas diferentes: clicar num linkdata:text/htmlexecuta o conteúdo, entãohrefo rejeita; mas o<img src>de um QR Code é umdata:image/...legítimo, eesc_url()o devolveria vazio, fazendo a imagem sumir silenciosamente.data:só passa emimg src, e só com o prefixodata:image/. Os 41<img src>de QR Code e logo em 26 views passaram a usaresc_img_src().O
esc_js()foi removido: era idêntico aoesc_json(), e o nome sugerindo o contrário era a razão pela qual as duas armadilhas doesc_json()— aspas manuais em volta eJSON.parse()— já tinham sido cometidas. Oesc_scalar()centraliza a única pergunta que todos os escapers fazem ("isto é um valor de texto?"), para que um sétimo escaper não precise inventar a sétima política.O gate de XSS ganha parser fail-closed, operador vírgula,
echo $x;solto, corte correto de statement, funções desconhecidas sem argumento, casts, literais e ternários — e passa a falhar o build em vez de só imprimir. O baseline continua vazio.Corrige também a resolução de linha do relatório: a linha vinha de um
strpos()que achava a primeira ocorrência da expressão no arquivo, e não do offset do regex. Comesc_scalar($result->idOs)emos/visualizarOs.phpo gate apontava a linha 8 — umechode markup legítimo — em vez da 411.fix: repara a precisão das colunas DECIMAL criadas pelo create_baseO
create_basedeclarava as colunas monetárias assim:O PHP não aplica o operador vírgula ali dentro de um array: lê
'constraint' => 10e descarta o2como elemento posicional. O dbforge só usa o CONSTRAINT, então a coluna saía comoDECIMAL(10), que o MySQL lê comoDECIMAL(10,0)— dinheiro sem nenhuma casa decimal.Quem rodou essa migration entre 2012 e hoje continua com as quatro colunas assim, e nenhuma migration posterior as repara: a
20220320173741toca lancamentos, os, vendas, cobrancas, produtos_os, servicos_os e itens_de_vendas, mas nãocontas,produtoseservicos. O estrago real está emprodutos.precoVenda/precoCompra, que alimentamRelatorios_model::produtosCustom()—SUM(produtos.estoque * produtos.precoVenda)e filtro porprecoVenda BETWEEN; a avaliação de estoque em dinheiro era arredondada para reais inteiros.A migration
20260927111019_repair_decimal_precisionsó amplia a precisão: os centavos que oDECIMAL(10,0)já descartou na escrita não voltam, e nenhuma alteração de schema recupera dado perdido na gravação.A seed
Configuracoespassou a pular as chaves já existentes. Sete delas vêm das migrations com o mesmoidConfigque a seed usaria, econfiguracoes.configtem UNIQUE emunique_valor— um insert cego abortava com 1062 na primeira linha e tornava oTools::seed()impossível de reexecutar em qualquer instalação, inclusive numa freshly migrada.O
banco.sqlacompanha as migrations (vendas.garantia,cobrancas.payment_method,cobrancas.total) para o gate de paridade continuar fechando.refactor: torna o Login verificável e unifica as respostas de erroverificarLogin()montava a resposta comechoe, em vários caminhos, comexit(). Isso torna o controller intestável: oexit()mata o processo inteiro em vez de encerrar a requisição, então não há como afirmar nada sobre o que vem depois dele. O payload agora é montado e devolvido pelo$this->output.Os caminhos de erro foram unificados em
respondFailure(). Eram três variantes quase iguais, e as três devolviam o token CSRF; um quarto caminho (o e-mail inexistente) ficava de fora por estar escrito de outra forma — exatamente o tipo de divergência que o teste precisa pegar. Oform_validationsaiu:validation_errors()devolve HTML, e a view escreve com$('#message').text(), então as tags apareceriam como texto visível, em inglês, uma por linha.O cabeçalho CORS passou a sair pelo
set_header()do CI_Output em vez doheader()do PHP — o Output só emite no fim da requisição, o que mantém o cabeçalho correto sem acordar os avisos de "Cannot modify header information" do CLI, que o Whoops converte em exceção. E o redirect de destino inválido saiu deredirect()para orespond_redirect(), com a regra de status extraída como função puraredirect_status_for()— em CLI o status sai porheader()nativo e não tem getter, então como função ela fica coberta de verdade em vez de replicada na documentação.test: adiciona suíte de testes, gate de paridade e CI de qualidadeA suíte roda in-process: o app sobe uma vez no bootstrap e cada teste instancia o controller direto, porque reiniciar o ciclo de vida do CI3 por teste não é possível. Isso expõe bugs de reentrância do framework que o
ControllerTestCasecontorna —TestApplication::resetSharedState()roda antes de cada controller (oCI_Controller::__construct()percorreis_loaded()e chamaload_class()com o nome cru, que só procuraAPPPATH/libraries/<nome>.phpe erra aSession, aPermissione aForm_validation), eLoader::_ci_modelstambém é limpo, senão$this->Some_modelchega nulo do segundo controller em diante.O banco de teste é montado pela cadeia de migrations, não pelo
banco.sql. O dump é todoCREATE TABLE IF NOT EXISTSe não tem um únicoDROP, então importá-lo nunca exercita uma linha de migration, e uma instalação montada assim nunca seria pega antes de chegar a um usuário. Osetup-db.phprecria o banco, rodaTools::migrate()e carrega as seeds canônicas. O nome do banco precisa terminar em_test; ambos os scripts abortam caso contrário.Como essa recriação acontece dentro da raiz do documento, o acesso a
application/tests/é bloqueado em dois lugares:.htaccesspara o Apache elocation ^~ /application/no nginx. O^~é obrigatório — sem ele alocation ~* \.php$é avaliada antes (regex tem prioridade sobre prefixo comum) e o acesso passaria. O nginx não lê.htaccess, então mexer num sem o outro reabre a pasta em silêncio.O
check-schema-parity.phpmonta um segundo banco a partir dobanco.sqle compara com o das migrations, tabela a tabela e coluna a coluna. É a barreira que mantém o dump do instalador e a cadeia de migrations de divergirem, já que nada mais exercita os dois caminhos — e é o que teria pegado osDECIMAL(10,0).O CI passa a rodar
test,format:check,xss:checkecheck:parity, num job só com MySQL 8.4 de serviço — os passos sem MySQL e os com MySQL dividiam a mesma instalação de dependências e a mesma configuração de PHP, então dois jobs pagavam duas vezes o download do Composer e podiam divergir na versão do PHP sem ninguém perceber.Também entra
DB_PORT(opcional, vazio usa 3306) emdatabase.phpe.env.example, o helperredirectno autoload, a seção de testes noAGENTS.mde o guia de escape deCONTRIBUTING.mdatualizado paraesc_img_src().fix: ignore em arquivos de uploadTrês entradas no
.gitignore:.phpunit.cache(o cache de resultado do PHPUnit),/assets/arquivos/*(uploads, que antes entravam no status do git como untracked) e.ai-jail(marcador de ambiente do tooling local, que é mountpoint e não arquivo do projeto). Ouploadssem barra, que já existia, não cobreassets/arquivos.fix(tests): mantém a conexão do processo em vez de reabrir por controllerO CI3 carrega o banco pelo autoloader, que roda dentro de
CI_Controller::__construct()— ou seja, uma vez por controller construído.Loader::database()deveria devolver a conexão já aberta, mas a guarda dele testaisset($CI->db)com$CI = get_instance(), e no meio do construtorget_instance()é o controller que está nascendo, que ainda não tem a propriedade$db. A guarda falha, o Loader abre uma conexão nova e guarda essa, trocando a que o processo inteiro usava.Na web é inofensivo: um request, um controller, uma conexão. Na suíte, não — a conexão é a mesma do processo todo, e é nela que a transação de cada caso é aberta. Trocá-la no meio do teste deixa a transação órfã num objeto que ninguém mais referencia, e cada escrita do teste é commitada na hora.
Havia um segundo defeito no mesmo lugar.
CI_Controller::$instanceéprivate statice continua apontando para o último controller construído, de modo queget_instance()devolvia um controller já jogado fora para quem viesse depois. É por ele queLoader::database()resolve o$CI, então o problema se reforçava a cada chamada.resetSharedState()passa a devolver o singletão ao objeto super antes de cada controller, e o harness guarda essa referência emTestApplication::superObject()em vez de confiar emget_instance().test: isola cada caso numa transação com a trait TransactsDatabaseA suíte escrevia no banco de verdade e por isso exigia conserto à mão. O
logsera o exemplo mais visível: cada login autenticado insere uma linha pelolog_info(), e o total subia de 5 em 5 a cada execução, porque nada desfazia essas escritas. Hojelogsfica em 0 depois de execuções repetidas.A trait é opt-in por classe, como no Laravel:
use TransactsDatabase;. Ela abre a transação num gancho#[Before]e a desfaz num#[After], e nada mais precisa lembrar de restaurar o que o caso mexeu.login/verificarLoginé o exemplo do ganho. O caso da conta sem expiração salvava odataExpiracao, escrevia null e devolvia o valor numfinally— mas ofinallyera pulado por qualquerfail()antes dele, e aí odataExpiracaoficava null para os testes seguintes, um vazamento que só apareceria como um "conta expirada" inexplicável em outro caso. Agora o caso grava o null e não restaura nada.Três detalhes do CI3 que a trait teve de respeitar, todos registrados no código porque são armadilhas silenciosas:
trans_begin()aninhado não abre SAVEPOINT: só incrementa_trans_depth, etrans_rollback()só fala com o banco quando o depth é exatamente 1. Por isso o fechamento é um laço que desce até zero._trans_depthéprotectede não tem getter, então é lido por closure amarrada ao driver._trans_rollback()e_trans_commit()religam o autocommit. Se o fechamento não rodar, a conexão fica com autocommit desligado e todo teste seguinte escreve dentro de uma transação que ninguém fecha.O par de testes que prova o isolamento é o final: um caso grava uma linha em
logse confirma que ela está visível dentro da transação, e o caso seguinte, que só roda depois, exige que ela tenha desaparecido. Verificado que o par falha de verdade trocando o rollback por commit na trait.fix(tests): publica as credenciais do banco em $_ENV(pendente — no working tree, ainda não commitado)Este é o que fazia o
composer testdo CI morrer antes do primeiro teste, e é o único item desta lista que não está commitado ainda.TestDatabase::fromEnvironment()resolvia usuário e senha do ambiente e guardava os dois em propriedades privadas. SóDB_HOSTNAME,DB_PORTeDB_DATABASEeram publicados em$_ENV— econfig/database.phpnão conheceMAPOS_TEST_DB_*: ele lê$_ENV['DB_USERNAME']e cai no placeholderenter_db_usernamequando a chave não existe.A suíte abre duas conexões para o mesmo banco: o PDO do
TestDatabase, que cria e apaga o banco, e a do autoloaderdatabasedoindex.php, que o CI3 abre durante o boot. Com as credenciais só nas propriedades, as duas discordavam. O sintoma no CI era:O
.envde desenvolvimento mascara o defeito: ele preenche$_ENV['DB_USERNAME']por fora, então localmente o placeholder nunca aparece. No CI não existe.env(é gitignored), e o boot morre.A correção resolve as credenciais depois do
safeLoad()— o fallback delas é lido de$_ENV, que é o Dotenv quem popula — e publica as cinco chaves em$_ENV. O invariante de "um.envde desenvolvimento não redireciona a suíte" continua valendo: o valor final é a sobreposição do ambiente ou o próprio valor do.env, nunca os dois misturados.check-schema-parity.phptinha o mesmo defeito e é corrigido junto, por vir do mesmo lugar.O
TestDatabaseTestnovo compara oconfig/database.phpefetivo com o que foi publicado e rejeita qualquer placeholderenter_*. O que este teste pega, e o que não pega: ele só tem o que comparar onde o.envfalta, ou seja, no CI. Localmente ele passa mesmo no estado quebrado, porque o.envpreenche a chave — a barreira real contra essa regressão é ocomposer testno CI sem.env, e o teste existe para documentar e fixar o invariante que a fez passar.Issue relacionada
(vazio)
Tipo de mudança
fix) —DECIMAL(10,0),esc_url()aceitandojavascript:, seedConfiguracoesnão reexecutável, conexões divergentes no harness, upload no status do gitrefactor) —Login::verificarLogin(); exceção: o texto das mensagens de erro do login muda (ver "Como testar")chore) — PHPUnit 12.5, gate de paridade, CI,AGENTS.md/CONTRIBUTING.md,.gitignorefeat)docs)Como testar
Conexão vem do ambiente, via
MAPOS_TEST_DB_*(default127.0.0.1:8989/mapos_test);application/.envé opcional e não redireciona a suíte.O teste que importa para o último commit — é a condição exata do CI, e é a que reprodovia o
Access denied:mv application/.env /tmp/env.bak MAPOS_TEST_DB_HOSTNAME=127.0.0.1 MAPOS_TEST_DB_PORT=8989 \ MAPOS_TEST_DB_DATABASE=mapos_test MAPOS_TEST_DB_USERNAME=root MAPOS_TEST_DB_PASSWORD=root \ composer test mv /tmp/env.bak application/.envSem
.env, a suíte tem que passar. Antes da correção, ela morre nosetup-db.php:44com o usuárioenter_db_username. Vale rodar nos dois sentidos: com o.envde volta também, porque o caminho do fallback para o.envtambém mudou de ordem.Verificações manuais que valem a pena, em ordem de risco:
<img src>trocaram deesc()/esc_url()paraesc_img_src(). Se a imagem sumir, é exatamente o modo de falha que o commit descreve. Abra um QR de cobrança e confirme que renderiza.<h5 id="message">do modal mudou: antes vinha o HTML devalidation_errors()escrito via.text()(mostrando as tags), agora vem um texto único em português. Confirme que aparece uma linha só, sem<ul>/<li>visível.composer testsó cobre a instalação nova (migration chain do zero). O caminho de atualização de uma base de 2012 não é exercido por nada aqui. Se você tiver um dump comprodutos.precoVendapreenchido, rode a migration e confiradecimal(10,2).logsdepois de rodar a suíte duas vezes. Deve continuar em 0 linhas. Se crescer, a traitTransactsDatabasenão está isolando o caso que escreve.Limites conhecidos da suíte:
show_error(403), que encerra o processo.$_SESSION; nada vai paraci_sessions.TransactsDatabasenão cobre DDL (faz commit implícito no MySQL e derruba a transação sem aviso) nem tabelas MyISAM, nem grupos de conexão além dodefault. O docblock da trait lista os três; o ponto sensível é o DDL, porqueTestApplicationTestrodaTools::migrate()sem a trait e só funciona enquanto todas as migrations já estiverem aplicadas.check:paritycompara nomes e tipos, não índices, foreign keys, charset ou collation, e exclui a tabelamigrations. Por isso oSchemaTestafirma valores absolutos (decimal(10,2)) em vez de comparar as duas pontas — um par de edições convergentes passaria pela comparação.TestDatabaseTestsó pega a regressão de credenciais onde não existe.env, isto é, no CI. Localmente ele passa mesmo no estado quebrado..htaccesse nginx). Instalações fora do Docker precisam do.htaccessou de uma regra equivalente. A nova regra do nginx cobreapplication/inteira, o que também passa a valer paraapplication/views/errors/.Capturas de tela
Sem mudança visual intencional. Duas alterações podem mexer no que se vê, e ambas valem antes/depois:
validation_errors().img srcmudou de função; a confirmação é justamente de que não mudou.Checklist
composer format. —format:checkpassa, 0 de 278 arquivos..env, credenciais, dumps de banco ou arquivos de IDE no diff. — sóapplication/.env.example(placeholders) ebanco.sql(arquivo já versionado, só DDL).application/vendor/não foi commitada.xss:checkpassa com baseline vazio.$this->db->query()são DDL literal (ALTER TABLE), sem entrada de usuário; oSchemaTestusa bindings?.Banco de dados
banco.sql). — Parcial, e precisa de decisão sua. A mudança está na migration20260927111019_repair_decimal_precision, e obanco.sqltambém foi editado (3 colunas:vendas.garantia,cobrancas.payment_method,cobrancas.total— estas três já divergiam e só apareceram agora porque o gate de paridade passa a comparar as duas pontas). Editar o dump é proibido peloAGENTS.md, mas ocheck:parityfalha enquanto ele não acompanhar. Vale a decisão explícita do revisor: ou o dump é gerado a partir das migrations e oAGENTS.mdmuda, ou o gate ganha uma lista de exceções.up()edown(). —down()é um no-op deliberado, com comentário: reverter reintroduz o arredondamento e perde centavos que ainda existiam. O template pededown(); a migration tem o método, mas vazio. Se a casa exigirdown()funcional, isso precisa ser uma decisão consciente, não um checkbox.setup-db.phpmonta do zero pela cadeia de migrations,SchemaTestafirma os tipos). A atualização de uma base existente não foi testada, e a migration mexe justamente nas colunas que uma base de 2012 tem erradas.