Skip to content

fix(review): avoid overlapping screenshot table extraction#4993

Open
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-memory-exhaustion-vulnerability-in-matrix-gate
Open

fix(review): avoid overlapping screenshot table extraction#4993
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-memory-exhaustion-vulnerability-in-matrix-gate

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The matrix-mode table parser could repeatedly rescan overlapping header+separator candidates and materialize the same rows many times, creating a quadratic CPU/memory denial-of-service risk for attacker-controlled PR bodies.
  • The change prevents that excessive re-scanning while preserving the intended matrix detection behavior.

Description

  • Advance the outer scan index past a detected markdown table region in extractTableRows() so each table region is processed once instead of being re-scanned from every matching header line; implemented in src/review/screenshot-table-gate.ts.
  • Apply the same fix to the duplicated implementation in packages/gittensory-engine/src/review/screenshot-table-gate.ts for parity.
  • Add regression tests that assert a crafted separator-only table is extracted once (tests added to both test/unit/screenshot-table-gate.test.ts and test/unit/screenshot-table-gate-engine.test.ts).
  • Refresh generated Cloudflare Worker types (worker-configuration.d.ts) to satisfy the cf-typegen drift check.

Testing

  • Ran the targeted unit tests with npm test -- --run test/unit/screenshot-table-gate.test.ts test/unit/screenshot-table-gate-engine.test.ts, and the two test files passed (all included assertions succeeded).
  • Ran the targeted coverage command npm run test:coverage -- --run test/unit/screenshot-table-gate.test.ts test/unit/screenshot-table-gate-engine.test.ts, where the focused tests passed but the global coverage threshold check failed when v8 reported repository-wide coverage below the required threshold.
  • Attempted the full local gate with npm run test:ci; the run exercised many checks but did not complete cleanly in this environment due to external/tooling fallbacks and unrelated suite failures (network/DNS and environment-driven failures outside the scope of this patch).
  • Attempted npm audit --audit-level=moderate, which failed due to the registry audit endpoint returning 403 Forbidden in the environment; no package changes were made by this PR.

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 11, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui f9d9dd8 Commit Preview URL

Branch Preview URL
Jul 11 2026, 08:37 AM

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.30%. Comparing base (7bbf529) to head (f9d9dd8).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4993      +/-   ##
==========================================
- Coverage   94.31%   94.30%   -0.01%     
==========================================
  Files         454      454              
  Lines       39034    39036       +2     
  Branches    14234    14234              
==========================================
+ Hits        36813    36814       +1     
  Misses       1572     1572              
- Partials      649      650       +1     
Flag Coverage Δ
shard-1 46.90% <50.00%> (-0.01%) ⬇️
shard-2 33.23% <0.00%> (+<0.01%) ⬆️
shard-3 31.36% <50.00%> (-0.01%) ⬇️
shard-4 33.06% <50.00%> (+<0.01%) ⬆️
shard-5 33.60% <0.00%> (+0.06%) ⬆️
shard-6 45.15% <0.00%> (-0.08%) ⬇️

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

Files with missing lines Coverage Δ
...tensory-engine/src/review/screenshot-table-gate.ts 99.20% <100.00%> (-0.80%) ⬇️
src/review/screenshot-table-gate.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

gittensory-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - fixes required

Review updated: 2026-07-12 01:10:35 UTC

5 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI failing · dirty

🛑 Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This is a real, correctly-implemented fix for a quadratic rescan in extractTableRows(): by setting `i = j - 1` after consuming a detected table region, each header+separator block is processed once instead of being re-scanned from every subsequent matching line, and the new test (40 separator-only lines → exactly 38 rows) genuinely distinguishes fixed from unfixed behavior since without the fix the same body would produce a triangular-sum blowup rather than 38. However, the identical for-loop-with-inner-while pattern in hasImageBearingMarkdownTable (src/review/screenshot-table-gate.ts) is left untouched and still has no analogous skip, so the same crafted-separator-row DoS this PR claims to close is still fully reachable through the default presence-mode gate path.

Blockers

  • hasImageBearingMarkdownTable in src/review/screenshot-table-gate.ts (and its packages/gittensory-engine twin) has the exact same overlapping-rescan for-loop/while-loop shape as the pre-fix extractTableRows but was not given the `i = j - 1` skip, so a crafted PR body of many separator-only lines with no image markup still forces O(n²) work on the presence-mode path (the gate's non-matrix, default-config path), meaning the DoS this PR claims to fix is still reachable through evaluateScreenshotTableGate.
Nits — 4 non-blocking
  • No inline comment on the new `i = j - 1;` line explaining why the outer index jumps past the consumed table region — the surrounding function already carries heavy rationale comments, and a one-line note would keep future edits from accidentally reverting the skip.
  • The regression test only asserts the post-fix row count (38); consider also asserting on timing or explicitly documenting in the test comment that removing `i = j - 1` reproduces a triangular/quadratic row count, so a future refactor that reintroduces the bug fails obviously rather than just returning a different number.
  • Apply the same `i = j - 1;` (or equivalent early-exit-aware skip) to hasImageBearingMarkdownTable in both src/review/screenshot-table-gate.ts and packages/gittensory-engine/src/review/screenshot-table-gate.ts, with a matching crafted-body regression test for that function.
  • Consider factoring the shared header+separator-detection loop (duplicated across hasImageBearingMarkdownTable and extractTableRows) into one helper now that it needs the same fix twice — the existing doc comment on extractTableRows explicitly chose duplication for behavior-pinning reasons, but that tradeoff is exactly what let this second copy drift out of sync with the security fix.

Why this is blocked

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

CI checks failing

  • validate
  • validate-code
  • validate-tests-merge
  • validate
  • validate-tests (2)
  • validate-tests (6)
  • validate-tests (4)
  • validate-tests (1)
  • validate-tests (3)
  • validate-tests (5)
  • validate-code
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 45 registered-repo PR(s), 37 merged, 415 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 45 PR(s), 415 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, JavaScript, MDX, Shell, Solidity
  • Official Gittensor activity: 45 PR(s), 415 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

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

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added the manual-review Gittensor contributor context label Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex 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.

1 participant