fix(miner): reject a portfolio-queue identifier containing the '::' composite-id separator (#8857) - #8925
Conversation
|
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 #8925 +/- ##
==========================================
+ Coverage 90.56% 90.61% +0.05%
==========================================
Files 96 97 +1
Lines 22490 22611 +121
Branches 3884 3911 +27
==========================================
+ Hits 20367 20488 +121
Misses 1945 1945
Partials 178 178
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…omposite-id separator portfolio-queue-manager's queueItemId joins apiBaseUrl/repoFullName/identifier on '::' (ITEM_ID_SEPARATOR). repoFullName is '::'-free via isValidRepoSegment, but normalizeIdentifier had no such guard, so an identifier like 'issue::5' silently corrupted the parsed triple (e.g. queueItemId(base, 'acme/widgets', 'issue::5') parsed back as repoFullName 'issue', identifier '5'). Reject a '::'-containing identifier at construction (normalizeIdentifier) rather than encode-and-hope. Tests: round-trip a valid (single-colon) identifier; enqueue with 'issue::5' throws invalid_identifier while 'issue:5' is still accepted.
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-26 14:18:19 UTC
Review summary Nits — 3 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.
|
Problem
Closes #8857.
portfolio-queue-manager.ts'squeueItemIdjoinsapiBaseUrl::repoFullName::identifieron::(ITEM_ID_SEPARATOR), andparseQueueItemIdsplits from the last two::.repoFullNameis guaranteed::-free (isValidRepoSegment), butidentifier(normalizeIdentifier) had no such guard. SoqueueItemId('https://api.github.com', 'acme/widgets', 'issue::5')parses back as{ apiBaseUrl: 'https://api.github.com::acme/widgets', repoFullName: 'issue', identifier: '5' }— silently corrupted. Nothing enforced the invariant.Fix
Reject any identifier containing the literal
::substring innormalizeIdentifier, throwinginvalid_identifierat construction rather than allowing silent downstream corruption (the issue's preferred reject-the-invariant approach, not encoding).Tests
queueItemId/parseQueueItemIdround-trip an identifier that does not contain::(single-colonissue:5).issue::5throwsinvalid_identifier;issue:5is still accepted.Reverting the guard fails the rejection test.
git diff --checkclean.