fix(engine): validate the repo and PR number behind buildResultsPayload's customer link - #9632
Conversation
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 02:22:23 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9632 +/- ##
==========================================
+ Coverage 90.12% 90.13% +0.01%
==========================================
Files 891 891
Lines 112306 112329 +23
Branches 26629 26635 +6
==========================================
+ Hits 101216 101248 +32
+ Misses 9760 9747 -13
- Partials 1330 1334 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…ad's customer link (JSONbored#9611) buildResultsPayload composes the customer-facing prLink and summary by raw interpolation of repoFullName and prNumber, neither validated: - repoFullName like "acme/widgets/../../evil" produced a link the browser resolves to github.com/evil. It's now treated as valid only when it splits into exactly two path-safe segments (the same isValidRepoSegment guard governor-ledger.ts keeps locally); otherwise prLink is null and the summary renders "unknown repository" instead of the raw value. - prNumber only had a non-null check, so 0/-3/2.5 took the has-PR branch and rendered ".../pull/0" etc. hasPr now additionally requires Number.isInteger && > 0. - additions/deletions now normalize with the finiteNonNegativeInt rule the sibling Rent-a-Loop modules use, so a negative or fractional count can't reach totals or the diff. title's redactSecrets scrub, MAX_DIFF_PREVIEW_FILES, the diff slice, the status default, and diffPreview ordering are unchanged; the function still never throws. Tests cover every branch in both suites: the engine's own node:test suite (packages/loopover-engine/test/results-payload.test.ts, the source the engine Codecov flag grades) at 100% on the changed code, plus the root vitest suite's required regression cases. Closes JSONbored#9611 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes #9611
What
buildResultsPayload(packages/loopover-engine/src/results-payload.ts) builds the customer-facingprLinkandsummaryby raw interpolation, validating neither value — even thoughresult.titleright beside it is scrubbed for the documented "public-safe" contract.repoFullName—"acme/widgets/../../evil"yieldedhttps://github.com/acme/widgets/../../evil/pull/1, which browsers resolve togithub.com/evil. It's now valid only when it splits into exactly two path-safe segments (theisValidRepoSegmentguardgovernor-ledger.tskeeps locally for the same reason); otherwiseprLinkisnulland the summary renders the literalunknown repositoryinstead of the raw value.prNumber— only a non-null check, so0/-3/2.5produced.../pull/0etc.hasPrnow additionally requiresNumber.isInteger(prNumber) && prNumber > 0; anything else takes the no-PR branch.additions/deletions— normalized with theNumber.isFinite(v) ? Math.max(0, Math.floor(v)) : 0rule the sibling Rent-a-Loop modules use, so a negative/fractional count can't reachtotalsor the diff preview.redactSecrets(title),MAX_DIFF_PREVIEW_FILES, the diff slice, thestatusdefault, anddiffPreviewordering are unchanged; the function still never throws.Tests
Every changed branch is covered in the engine's own node:test suite (
packages/loopover-engine/test/results-payload.test.ts— the suite theengineCodecov flag grades this source with) at 100% on the changed code, plus the DoD's required regression cases in the roottest/unit/results-payload.test.ts. Verified: engine suite 6/6, root suite 12/12 (existing cases unchanged), non-vacuous (4/6 fail with the fix reverted),tsc --noEmitclean,git diff --checkclean.