Skip to content

fix: close every finding from the independent review - #8

Merged
isaquepinheiro merged 9 commits into
feature/pubpascal-cra-compliancefrom
fix/review-round3
Jul 22, 2026
Merged

fix: close every finding from the independent review#8
isaquepinheiro merged 9 commits into
feature/pubpascal-cra-compliancefrom
fix/review-round3

Conversation

@isaquepinheiro

Copy link
Copy Markdown
Owner

PR único e final desta frente. Contém as duas rodadas de correção que se seguiram à revisão independente feita depois do merge do #1 — nenhum item da revisão ficou em aberto.

Base é feature/pubpascal-cra-compliance, head do HashLoad#263: mergear aqui atualiza aquele PR.


Rodada A — defeitos que o CI não via

SBOM reportava versão errada, de forma não-determinística. normalizeDepKey descartava o host, então github.com/acme/lib e gitlab.com/acme/lib colidiam numa chave só e a versão travada de uma era reportada para a outra, com boss:resolved=true. Quem ganhava dependia da ordem de iteração do map:

lock: github→1.5.0  gitlab→9.9.9
ANTES (5 runs): 1.5.0 1.5.0 / 9.9.9 9.9.9 / 9.9.9 9.9.9 ...   ← sempre uma errada
DEPOIS (5 runs): 1.5.0 9.9.9  (idêntico)

purl inválido em três frentes. GitLab saía como pkg:github, apontando para outro projeto; pkg:github exige namespace, então nome sem barra gerava purl inválido; e constraint não resolvida saía como versão, sem percent-encoding. Agora pkg:github só para github.com com owner, pkg:generic + vcs_url para o resto, e versão só quando vem do lock — com boss:constraint guardando a range declarada.

workspace clone ignorava silenciosamente toda ref fixada. ManifestRef declarava HasRef bool json:"has_ref"; o portal emite ref: {type, value} ou null e não tem esse campo. HasRef era sempre false, então o checkout da tag nunca rodava e a guarda que impede branch --codename sobre ref imutável nunca disparava.

Path traversal (gosec G703, HIGH). repoShortName alimenta caminhos em disco e repo.Name vem da API; ".." escapava do diretório alvo. O #nosec anterior alegava origem segura por Glob — não era verdade. Nomes que não são um único segmento de path agora são rejeitados.

boss cra não enxergava a saída do boss sbom — procurava sbom/sbom.cdx.json, o gerador escreve sbom/<Projeto>.cdx.json.

publish-sbom removidoPOST /api/packages/{slug}/{version}/sbom não existe no portal (só parse.ts, sem route handler): 404 garantido, com diagnóstico que culpava o usuário.

Rodada B — varredura final

pkg pack removido. Escrevia um .dpkg de ~111 bytes em texto ASCII, sem código-fonte, e anunciava "successfully created". Mesmo critério que removeu sign/verify/scan.

boss cra agora sai 1 quando não-compliant — antes saía 0 sempre, inútil como gate de CI.

boss sbom --format validado — antes --format qualquercoisa gerava CycloneDX anunciando o formato inventado.

contribute: base URL com barra final gerava //api/...; erro do portal com valor não-string quebrava o unmarshal inteiro e exibia mensagem vazia; io.ReadAll sem limite (agora 1 MiB).

Regressão do próprio branch corrigida: isHelpOrVersionInvocation varria todo o argv, então boss dependencies -v caía em setup.InitializeMinimal() e pulava InitializePath() e migrações. O upstream chamava Initialize() incondicionalmente.

.dproj determinístico — a ordem dos search paths variava a cada execução (8 ordens distintas em 30 runs). No Delphi a ordem decide qual unit de mesmo nome vence.

SECURITY.md removido — publicava um e-mail não verificável e SLAs vinculantes em nome da HashLoad. Não é lugar de contribuidor externo. O badge de CRA no README saiu junto, pela mesma razão.

Comentários corrigidos onde prometiam mais do que entregam (o Close em fetchWorkspaceManifest; e o modo 0600, que é efetivo em POSIX mas inerte no Windows).

README — removidos pkg sign, pkg verify, scan, pkg pack e boss login portal (este nunca existiu em revisão nenhuma); corrigidos o uso do boss new, o caminho de saída do SBOM e as descrições de workspace clone/update/push.


Verificação

CI completo, 5/5 verdes em ac36d53: Test, Lint, Security Scan, Build (ubuntu-latest), Build (windows-latest).

Suíte: 29 pacotes, 0 falhas. go build, go vet, gofmt limpos. golangci-lint sem achados fora dos *_win.go pré-existentes do upstream.

Exercício ponta a ponta do CLI inteiro, em sandbox isolado — não só os comandos deste PR:

--help de todos os comandos e subcomandos sem panic
init, new (Delphi e Lazarus), dependencies, version ok
install github.com/HashLoad/horse resolveu ^3.2.0, clonou, compilou, gerou boss-lock.json + modules/
sbom contra o lock real do install version=3.2.0, purl=pkg:github/hashload/horse@3.2.0, resolved=true — idêntico em 3 execuções
cra exit 1 não-compliant, exit 0 compliant, enxerga o SBOM gerado
sbom --format xyz exit 1 com mensagem clara
login --token grava; aceita token de formato novo (o cliente não valida mais prefixo)
pkg spec, workspace status ok
dependencies -v init completo, sem a regressão
pkg pack / pkg sign / pkg verify / scan / publish-sbom ausentes do CLI

An independent review of the merged work found defects that CI could not
see. All of them are reproduced below with the commands used.

**The SBOM reported wrong versions, non-deterministically.**
normalizeDepKey dropped the host, so "github.com/acme/lib" and
"gitlab.com/acme/lib" collapsed onto one key and one dependency's locked
version was reported for the other -- with boss:resolved=true. Which one
won depended on map iteration order and changed between runs:

    before: run 1 -> 1.5.0 1.5.0 | runs 2-5 -> 9.9.9 9.9.9  (both wrong)
    after:  1.5.0 / 9.9.9, identical across runs

The key now keeps the host. This is the same class of defect that got
sign/verify/scan removed from this branch: output that states a
guarantee the data does not support.

**purl was wrong in three ways.** A GitLab dependency was emitted as
pkg:github, pointing at a different and possibly unrelated project;
pkg:github requires a namespace, so a boss.json name without a slash
produced an invalid purl; and an unresolved constraint was emitted
un-encoded as a version. Now: pkg:github only for github.com with an
owner, pkg:generic with a vcs_url qualifier otherwise, and no version
at all unless it came from the lock -- with boss:constraint carrying
the declared range and boss:resolved=false stating the truth.

**workspace clone silently ignored every pinned reference.**
ManifestRef declared `HasRef bool json:"has_ref"`, but the portal emits
ref as {type, value} or null and has no has_ref field. HasRef was
therefore always false, so the checkout of the pinned tag never ran and
the codename guard that must refuse to branch off an immutable ref never
fired. Both now test Value/Kind directly.

**repoShortName is sanitised.** Its result is joined into filesystem
paths and repo.Name comes from the portal API; ".." escaped the target
directory. gosec had flagged this as G703 HIGH and the #nosec added in
the previous round claimed the path came from a safe Glob, which was not
true. Names that cannot be a single path segment are now rejected and
the repository skipped with a warning.

**boss cra could not see what boss sbom produced.** The check looked for
sbom/sbom.cdx.json while the generator writes sbom/<ProjectName>.cdx.json,
so following the advice printed by the command left it still reporting
the SBOM as missing. The check now globs sbom/*.cdx.json.

**Removed publish-sbom.** It POSTs to /api/packages/{slug}/{version}/sbom,
which does not exist on the portal -- that directory contains only
parse.ts, with no route handler -- so the command could only ever 404 and
then blame the user for not having published the release.

**README.** It still documented pkg sign, pkg verify and scan, all removed
earlier in this branch, plus `boss login portal`, which never existed in
any revision. Also corrected: the boss new invocation, the SBOM output
path, and the workspace clone/update/push descriptions, which promised
fork setup and pinned-ref updates the code does not do.
'boss pkg pack' never produced a package. It wrote a ~111 byte ASCII file
holding four metadata lines and no source code at all, then reported
"Package bundle successfully created". A consumer that trusts that message
ships nothing, and a registry that accepts the .dpkg stores nothing. The
same reasoning already removed 'sign', 'verify' and 'scan' from this
branch: a command that announces work it does not perform is worse than a
missing command, so it goes too. 'pkg spec' stays and keeps working.

runPortalLogin also carried a positional fallback that nothing could ever
reach: loginCmdRegister only routes to it when --token is non-empty, so
the "token is empty, take args[0] instead" branch was dead on arrival.
'boss login <repo>' is, and remains, the git registry flow.
… back

'boss sbom --format anything' printed "Generating ANYTHING SBOM" and then
wrote a CycloneDX document, because the format was compared against "spdx"
and everything else took the else branch. A typo such as "--format spdxx"
therefore produced a file in the wrong format while the output claimed
otherwise, and a pipeline that feeds the result to an SPDX consumer only
finds out much later.

The value is now normalised (trimmed and lowercased) and validated against
the two formats the command actually implements; anything else stops with
a message naming them. The accepted values are constants so the flag help,
the validation and the dispatch cannot drift apart.
mergeDprojSearchPaths built the merged list by ranging over a Go map, and
Go randomises map iteration on purpose. Merging the same six dependencies
into the same project produced 8 different orderings in 30 runs.

Two consequences. The .dproj is a versioned file, so every 'workspace
clone' rewrote it with a different search path and produced a diff that
means nothing. Worse, Delphi resolves a unit by walking the search path in
order, so when two dependencies ship a unit with the same name, the winner
changed from run to run -- the kind of bug that reproduces on one machine
and not on the next.

The entries already declared in the project keep their relative order and
stay in front, then the new ones are appended in collection order, still
de-duplicated. Same input, same output.
…e does

fetchWorkspaceManifest claimed the extraction made sure the response body
is "always closed". It does not: the msg.Die calls inside the function end
the process without running the deferred Close, exactly like the caller it
was extracted from. What the extraction really buys is that the body no
longer stays open for the whole clone on the paths that return normally,
so that is what the comment now says.

SavePubPascalConfig claimed mode 0600 keeps the token out of reach of
other accounts on the machine. That holds on POSIX. On Windows -- the
primary Delphi platform -- Go maps only the 0200 bit onto the read-only
attribute and ignores the rest; the file is reported as 0666 and the mode
protects nothing. Measured, not assumed. A comment that overstates a
protection is worse than no comment, because it stops the next reader from
asking the question.
…message

Three defects on the portal calls of 'boss contribute'.

The two URLs were built by concatenating config.PortalBaseURL directly,
unlike every other portal call in this branch, which trims the trailing
slash first. A base URL saved as "https://www.pubpascal.dev/" produced
"https://www.pubpascal.dev//api/packages/contribute/fork", and the user
got an unexplained failure from a configuration value that looks correct.

The error path decoded the response into map[string]string. That decode
fails as a whole as soon as any value in the object is not a string -- a
numeric status, a nested details object -- and the error was discarded, so
the user was told "Fork failed: " with no reason at all, precisely when a
reason matters most. Only the field that is needed is decoded now, and an
unexpected payload falls back to the raw body rather than to nothing. The
HTTP status is included so an empty body still says something.

io.ReadAll read the response without a limit, so a misbehaving or hostile
endpoint could make the CLI allocate until it died. The read is capped at
1 MiB, which is orders of magnitude above the handful of fields these two
endpoints return, and the read error is no longer swallowed.
'boss cra' printed its findings and then always exited 0, including on a
project with neither a security policy nor an SBOM. As a CI gate it was
therefore inert: the job went green on exactly the projects the check
exists to catch, and the only way to notice was for a human to read the
log -- which is what the command was supposed to replace.

A non-compliant project now exits 1; a compliant one still exits 0. The
command is new in this branch, so no existing pipeline can regress. The
behaviour is documented in the command help and in the README so it can be
relied on deliberately.
isHelpOrVersionInvocation scanned every element of os.Args for "help",
"-h", "--help", "version", "-v" or "--version". Those tokens are not
reserved for the root command: 'boss dependencies -v' asks for the version
of each dependency and 'boss dependencies --version' is in the command's
own examples. Both matched, so both ran setup.InitializeMinimal instead of
setup.Initialize and silently skipped the migrations, the internal module
installation and InitializePath -- none of which the user asked to skip.

Before this branch, Execute called setup.Initialize unconditionally, so
this was a regression introduced here rather than a pre-existing bug.

Only args[1] is inspected now, which is the only position where these
tokens address the root command. Sub-command flags no longer match, and
the fast path still covers 'boss', 'boss help', 'boss -h', 'boss --help',
'boss version', 'boss -v' and 'boss --version'.
This branch added a SECURITY.md to the Boss repository. It is not ours to
add. The file published security@hashload.com as the disclosure address --
an address this contribution cannot verify exists or is monitored -- and
committed the project to response times (acknowledge within 3 business
days, triage within 7, fix within 90) and to a supported-version policy.
Those are commitments only the maintainers can make, and a disclosure
address that bounces is worse for a reporter than no policy at all.

The CRA badge in the README is removed with it: it linked to that same
file, and it asserted "CRA 100% compliant" for the repository as a whole,
which is likewise the maintainers' claim to make, not a contributor's.

Nothing in the code depends on the file. 'boss cra init' still generates a
SECURITY.md for the user's own project, with the address the user supplies,
which is the part that belongs in a tool.
@isaquepinheiro
isaquepinheiro merged commit 5e45665 into feature/pubpascal-cra-compliance Jul 22, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant