Corrige vulnerabilidades de segurança apontadas pelo CodeQL#9
Open
Rossi-Luciano wants to merge 2 commits into
Open
Corrige vulnerabilidades de segurança apontadas pelo CodeQL#9Rossi-Luciano wants to merge 2 commits into
Rossi-Luciano wants to merge 2 commits into
Conversation
- Substitui o uso do nome de arquivo fornecido pelo usuário como parte do
caminho temporário por um nome fixo ("package.zip")
- O nome original do arquivo continua sendo preservado na variável `filename`
e usado apenas para armazenamento no banco de dados (campo `package_name`),
sem nunca fluir para expressões de caminho do sistema de arquivos
- Elimina o vetor de path injection nos alertas CodeQL:
· alert 11: construção do zip_path a partir de input do usuário (linha 36)
· alert 10: abertura do arquivo com caminho derivado de input do usuário (linha 14)
· alert 9: verificação de existência de arquivo com caminho contaminado (linha 50)
Por quê: o CodeQL rastreava o dado do usuário (uploaded_file.filename) até
operações de sistema de arquivos, classificando como risco de path traversal.
A separação entre nome para exibição e nome para uso em I/O elimina
completamente esse fluxo de dados contaminado.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Adiciona a função auxiliar `_safe_redirect_target()` que valida o parâmetro
`next` antes de utilizá-lo como destino de redirecionamento
- A função aceita apenas caminhos relativos internos: deve começar com "/"
e não pode começar com "//" (que permitiria redirecionar para domínio externo)
- Qualquer valor inválido ou ausente resulta em redirecionamento seguro para
a rota principal (`web.index`)
- Substitui o uso direto de `request.args.get("next")` na rota `set_language`
pelo retorno validado de `_safe_redirect_target()`
Por quê: sem a validação, um atacante poderia construir uma URL como
`/language/en?next=https://site-malicioso.com` e redirecionar o usuário para
fora da aplicação após a troca de idioma — vulnerabilidade classificada pelo
CodeQL como "URL redirection from remote source" com severidade error.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| raise ValueError(zip_only_message or "Only SPS .zip files are supported.") | ||
| with tempfile.TemporaryDirectory(prefix="spsvalidator-") as temp_dir: | ||
| zip_path = os.path.join(temp_dir, Path(filename).name) | ||
| zip_path = os.path.join(temp_dir, "package.zip") |
Member
There was a problem hiding this comment.
@Rossi-Luciano não pode ser fixo. Recupere o nome do arquivo em uso
| if next_url and next_url.startswith("/") and not next_url.startswith("//"): | ||
| return next_url | ||
| return url_for("web.index") | ||
|
|
Member
There was a problem hiding this comment.
Use uma solução mais apropriada que é adoção da função urlparse
from urllib.parse import urlparse # No Python moderno, ou werkzeug.urls
def _safe_redirect_target(next_url: str | None) -> str:
if not next_url:
return url_for("web.index")
# Faz o parsing da URL
parsed_url = urlparse(next_url)
# Se o netloc (host) estiver vazio, significa que é uma URL relativa (interna e segura)
if not parsed_url.netloc and parsed_url.path.startswith("/"):
return next_url
return url_for("web.index")
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
2 participants
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.
O que esse PR faz?
Corrige duas vulnerabilidades de segurança identificadas pelo GitHub Advanced Security (CodeQL) na revisão
scieloorg/scielo-tools#15:nextera usado diretamente como destino de redirecionamento sem validação, permitindo redirecionar o usuário para URLs externas maliciosas.Os demais alertas (cookie injection e path injection nos testes) foram analisados e classificados como falsos positivos: o valor do cookie já é sanitizado por
normalize_language()com whitelist estrita, e os caminhos nos testes são controlados pelo pytest (tmp_path).Onde a revisão poderia começar?
src/spsvalidator/web/routes.py— função_safe_redirect_target()e uso na rotaset_languagesrc/spsvalidator/services/validation_service.py— substituição do nome de arquivo do usuário por"package.zip"no caminho temporárioComo este poderia ser testado manualmente?
pip install -e ".[dev]"pytest— todos os testes devem continuar passando/language/en?next=https://google.come confirmar que o redirecionamento vai para/e não para a URL externa.zipcom nome contendo caracteres especiais e confirmar que a validação ocorre normalmenteAlgum cenário de contexto que queira dar?
Os alertas foram gerados pelo CodeQL durante a revisão do PR
scieloorg/scielo-tools#15. Os dois alertas reais (py/url-redirectioncom severidade error epy/path-injectioncom severidade error) exigiam correção antes do merge. Os demais cinco alertas foram analisados e descartados como falsos positivos, com justificativa documentada na issue.Screenshots
N/A
Quais são tickets relevantes?
Closes #6
Referências
py/url-redirection: redirecionamento para URL não confiávelpy/path-injection: uso de dados do usuário em expressões de caminho