Skip to content

fix(api): reject a non-positive-integer installation id on the seven id routes - #9740

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/installation-id-integer-validation-9716
Jul 29, 2026
Merged

fix(api): reject a non-positive-integer installation id on the seven id routes#9740
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/installation-id-integer-validation-9716

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

Closes #9716.

Seven routes parsed an installation id out of the path and validated it with Number.isFinite alone,
which accepts fractions and exponent notation — Number("1.5")1.5, and 0/negative all passed
and were bound straight into the D1 lookup. A GitHub installation id is always a positive integer, and
the repo's own sibling routes already guard with !Number.isInteger(id) || id <= 0 (the
dead-letter-queue admin routes at routes.ts:2031/:2057, and the chat-qa :number param). A client
sending a malformed id also got an undocumented status.

Change

  • All seven routes — GET /v1/installations/:id/{health,repair}, POST .../repair/refresh, and the
    four /v1/app/installations/:id/* siblings (health, repair, repair/refresh, agent/bulk-settings) —
    now use !Number.isInteger(installationId) || installationId <= 0, returning the existing
    { error: "invalid_installation_id" } body with 400 unchanged. The two /v1/internal/orb/* routes
    that parse installationId from a JSON body are out of scope and untouched.
  • Every one of the seven operations now declares 400 in the published OpenAPI document (the four app
    routes in orb-and-control-route-specs.ts, the three legacy registerPath blocks in spec.ts);
    bulk-settings' existing 400 description is widened to cover the id cause too. openapi.json is
    regenerated.

Validation

  • New test: each of the seven routes rejects a fractional (1.5), zero, and negative id with
    400 invalid_installation_id, while a valid positive integer still reaches the handler (200).
    (1e31000 is a genuine integer and is deliberately still accepted.)
  • Bug-catch verified: reverting the guards to Number.isFinite fails the new test.
  • 100% patch coverage on the changed guard lines and the OpenAPI declarations.
  • ui:openapi:check drift-check clean.

…id routes

Seven routes parsed an installation id and validated it with Number.isFinite
alone, which accepts fractions and exponent notation -- Number("1.5") -> 1.5 and
0/negative all passed and were bound straight into the D1 lookup. A GitHub
installation id is always a positive integer, and the repo's own sibling routes
(the dead-letter-queue admin routes, the chat-qa :number param) already guard with
!Number.isInteger(id) || id <= 0.

Switch all seven -- GET/`/v1/installations/:id/{health,repair}`,
POST `.../repair/refresh`, and their four `/v1/app/installations/:id/*` siblings
(health, repair, repair/refresh, agent/bulk-settings) -- to that guard, returning
the existing { error: "invalid_installation_id" } body with 400 unchanged. The two
`/v1/internal/orb/*` routes that parse installationId from a JSON body are out of
scope and untouched.

Every one of the seven operations now declares 400 in the published OpenAPI
document (the four app routes in orb-and-control-route-specs.ts, the three legacy
routes' registerPath blocks in spec.ts); bulk-settings' existing 400 description is
widened to cover the id cause too. openapi.json is regenerated.

Adds a test asserting each of the seven routes rejects a fractional/zero/negative
id with 400 invalid_installation_id, while a valid positive integer still reaches
the handler.

Closes JSONbored#9716
@shin-core
shin-core requested a review from JSONbored as a code owner July 29, 2026 05:42
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 29, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-29 05:52:27 UTC

5 files · 1 AI reviewer · no blockers · CI green · unstable

⏸️ Suggested Action - Manual Review

Review summary
This PR tightens the installation-id guard on seven routes from Number.isFinite to Number.isInteger + positive check, closing the gap where fractions, zero, and negatives bound straight into the D1 lookup, and adds matching 400 responses to the OpenAPI spec (both the hand-authored specs and the regenerated openapi.json). The fix is applied at the correct layer (the guard itself, right before the value is used), matches the existing sibling pattern cited in the description (routes.ts DLQ admin routes), and is covered by a new integration test that exercises all seven routes with fractional/zero/negative ids plus a positive-integer control case. The bulk-settings OpenAPI description was correctly widened rather than just appended to, since that route already had a 400 for a different cause.

Nits — 3 non-blocking
  • The magic literal `400` is repeated across all seven guard sites and three OpenAPI files without a shared constant, which is consistent with the existing pattern in the file but slightly duplicative.
  • The FAILED 'Contributor trust' check has no detail provided in what was given here, and this PR's branch is 1 commit behind the default branch, so the failure's cause could not be verified from what's available.
  • Consider extracting the `!Number.isInteger(id) || id <= 0` check into a small shared helper (e.g. `isValidInstallationId`) since it's now duplicated seven times across routes.ts.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9716
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 38 registered-repo PR(s), 26 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 38 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff applies the required !Number.isInteger(id) || id<=0 guard verbatim to all seven handlers without reordering relative to scope checks, adds 400 to all seven OpenAPI declarations (three legacy spec.ts blocks and four orb-and-control-route-specs.ts blocks, including widening bulk-settings' description), and regenerates openapi.json accordingly. New integration tests cover the fractional (1.5

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is registered but has no active allocation in the current snapshot.
  • Public profile languages: TypeScript, JavaScript, Solidity, Dart, Python, CSS, PHP, Rust
  • Official Gittensor activity: 38 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Await review-lane availability.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: hold · clause: success
  • config: 9343aed68db0a3a522c23462ba34d3089eaffc24006e67ab7e5eb62faa23e727 · pack: oss-anti-slop · ci: passed
  • record: 71289cdc7fadd23f5479500a8dd3167dcaadbc5932f8382aada28d3ca553eb09 (schema v5, head 91684a3)
Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop before /
before /
after /
after /
/ mobile before / (mobile)
before / (mobile)
after / (mobile)
after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

Scroll preview
Route Before (production) After (this PR's preview)
/ before / (scroll)
before / (scroll)
after / (scroll)
after / (scroll)

A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show.

🟩 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.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent did not find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.46%. Comparing base (0c67c5e) to head (91684a3).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9740       +/-   ##
===========================================
- Coverage   90.27%   77.46%   -12.81%     
===========================================
  Files         904      285      -619     
  Lines      113176    62234    -50942     
  Branches    26839     7354    -19485     
===========================================
- Hits       102170    48210    -53960     
- Misses       9676    13651     +3975     
+ Partials     1330      373      -957     
Flag Coverage Δ
backend 95.98% <100.00%> (+0.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/api/routes.ts 95.58% <100.00%> (ø)
src/openapi/orb-and-control-route-specs.ts 100.00% <ø> (ø)
src/openapi/spec.ts 99.30% <ø> (ø)

... and 753 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 29, 2026
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Held for manual review: the gate and required CI are green, but GitHub reports this pull request's mergeable state as unstable because a non-required check or status is not passing, so LoopOver will not auto-merge. A maintainer can resolve the failing check or review and merge manually. This is an automated maintenance action.

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 29, 2026
@JSONbored
JSONbored merged commit a9ac8a7 into JSONbored:main Jul 29, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

api(installations): the installation routes accept a non-integer id and never publish their 400

2 participants