Skip to content

docs(security): correct the October review and sign it off, finding 8 included - #1655

Merged
jmrplens merged 2 commits into
bp-1031/4-badgesfrom
bp-1031/5-review-signoff
Oct 31, 2026
Merged

jmrplens merged 2 commits into
bp-1031/4-badgesfrom
bp-1031/5-review-signoff

Conversation

@jmrplens

@jmrplens jmrplens commented Oct 31, 2026 •

Copy link
Copy Markdown
Owner

The October security review record still read as if finding 8 were held back, and a review of the rows #1650 added found places where they said more, or less, than the code and the advisory do. This corrects them and closes the sign-off, for the OpenSSF Best Practices gold criterion security_review, which asks for a security review a person performed as well as tools.

The boundary 1 path row said EscapePathSegment and EscapePathSegments escape every argument it lists. The model registry download uses neither: its arguments are escaped with url.PathEscape, by client-go's own route under a directory and by rootFilePath at the package root. The row now says so, and says what its model registry test holds.

The record said every requirement was traced at 8e959c3d8, but the path row describes code that arrived with 39b7093af, the commit 3.4.0 was released from. The introduction, the scope, the method and the Sign-off table now name that commit for the path row.

Finding 8 now names the harm its advisory gives: the request ran under the operator's token, or the caller's in HTTP mode, past the server's per-action controls. It states which GitLab releases refuse the path and with which flags, and its status records the release, that the fix was merged from the advisory's private fork, and that the advisory is not yet in the GitHub Advisory Database or OSV and its CVE is unassigned.

Findings 3, 4 and 5 still waited for a release that has shipped. Each keeps the status signed off on 27 October and gains a dated update: #1560 corrected the policy, and #1554 and Go 1.27.3 shipped in 3.4.0, before the 2 November deadline finding 4 named. The dependencies row says the same.

I read the boundary 1 path row and finding 8 on 31 October and sign the review as of that date, every finding included. The record now answers the question it left open on finding 8: of the rows recorded on 27 October it moves only input validation, whose untraced "String parameters are sanitized where applicable" did not hold for a path argument until 3.4.0, and the others keep their results, since every request stayed on the instance under the token's own authority, read-only mode removed every write it reaches, safe mode previewed each, and a read stayed a GET.

Two dated updates follow from the layers below:

  • The input validation row now names the stdio test that holds the SDK's validation on the individual surface, which the record had left untraced.
  • Finding 8 records my decision of 31 October: SECURITY.md now names the request-path traversal as a kind of vulnerability, and the per-action controls as a requirement.

SECURITY.md's advisory count and disclosure steps, and the assurance case, are corrected for finding 8 in the first two layers of this stack.

Signed-off-by: José M. Requena Plens mail@jmrp.io

…indings to 3.4.0

The boundary 1 path row said EscapePathSegment and EscapePathSegments
escape every argument it lists, the model registry download's included.
That handler uses neither: its arguments are escaped with url.PathEscape,
by client-go's own route under a directory and by rootFilePath at the
package root. The row now says so, and names the model registry test
for what it holds.

The record said every requirement was traced at 8e959c3, where the
path row describes code that arrived with 39b7093, the commit 3.4.0
was released from. The introduction, the scope and the method now name
that commit for the path row.

Finding 8 now says the request ran under the operator's token, or the
caller's in HTTP mode, past the server's per-action controls, which is
the harm its advisory names; it states which GitLab releases refuse the
path and with which flags; and its status records the release, that
the fix was merged from the advisory's private fork, and that the
advisory is not yet in the GitHub Advisory Database or OSV, with its
CVE unassigned.

Findings 3, 4 and 5 waited for a release that has shipped. Each keeps
the status signed off on 27 October and gains a dated update: #1560
corrected the policy, #1554 and Go 1.27.3 shipped in 3.4.0, before the
2 November deadline finding 4 named. The dependencies row says the same.

Signed-off-by: José M. Requena Plens <mail@jmrp.io>
The record still read as if finding 8 were held back until its advisory
is published, and asked whether it changes the result of any row
without answering. GHSA-rxjw-584q-hxjr was published on 31 October
2026 with 3.4.0, and I read the boundary 1 path row and finding 8 that
day.

The status line, the introduction and the Sign-off table now say the
review is signed off as of 31 October 2026, every finding included,
with finding 8 held back from the sign-off of 27 October until its
advisory was published, and the path row read at 39b7093, the commit
3.4.0 was released from.

What the maintainer's review covers now answers the question on finding
8: of the rows recorded on 27 October it moves only input validation,
whose untraced "String parameters are sanitized where applicable" did
not hold for a path argument until 3.4.0, and that row now says so.
The others keep their results, because every request stayed on the
instance under the token's own authority, read-only mode removed every
write it reaches, safe mode previewed each, and a read stayed a GET.

Signed-off-by: José M. Requena Plens <mail@jmrp.io>
@sourcery-ai

sourcery-ai Bot commented Oct 31, 2026

Copy link
Copy Markdown

Sorry @jmrplens, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 2 days and 19 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@jmrplens
jmrplens added this pull request to stack #1657 October 31, 2026 08:44
@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Files selected but had no reviewable changes: 1
  • docs/development/security-review-2026-10.md
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 59847f1 and 73ade1d.

Files selected for processing: 1
  • docs/development/security-review-2026-10.md

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@jmrplens jmrplens added documentation Improvements or additions to documentation security Security-related issue labels Oct 31, 2026
@jmrplens jmrplens added this to the 3.5.0 milestone Oct 31, 2026
@jmrplens jmrplens changed the title bp 1031/5 review signoff docs(security): correct the October review and sign it off, finding 8 included Oct 31, 2026
@jmrplens jmrplens added the v3.5.0 Targeted at the 3.5.0 release label Oct 31, 2026
@sonarqubecloud

Copy link
Copy Markdown

@jmrplens
jmrplens merged commit a3eeb22 into main Oct 31, 2026
40 checks passed
@jmrplens
jmrplens deleted the bp-1031/5-review-signoff branch October 31, 2026 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation security Security-related issue v3.5.0 Targeted at the 3.5.0 release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant