fix(sec): enforce base32 charset regex on isValidTaskId with regression tests (#176)

[jihadMo]

Resolves #176.

Enforces base32 RFC 4648 regex /^[A-Z2-7]{25}$/ validation in backend/src/bounties/taskid.ts to prevent arbitrary string injection, along with regression test suite in backend/src/bounties/taskid.test.ts.

[devasign-app]

AI Review: All Acceptance Criteria Met

1 advisory finding below — review before merging.

All acceptance criteria are satisfied. isValidTaskId now enforces the RFC 4648 base32 character set [A-Z2-7] and exact length of 25 using /^[A-Z2-7]{25}$/, and regression tests verify rejection of non-base32 characters and acceptance of valid derived task IDs. One warning suggestion is provided to restore tests for upper length boundary and determinism.

Acceptance Criteria

Met

• ✅ isValidTaskId returns false for 25-character strings that contain characters outside the RFC 4648 base32 charset [A-Z2-7], including strings like '!!!!!!!!!!!!!!!!!!!!!!!!!'. — In backend/src/bounties/taskid.ts line 34, isValidTaskId uses /^[A-Z2-7]{25}$/.test(taskId) to validate the task ID. This restricts valid strings to exactly 25 RFC 4648 base32 characters [A-Z2-7], causing non-conforming 25-character strings like '!!!!!!!!!!!!!!!!!!!!!!!!!' to return false.

  backend/src/bounties/taskid.ts:33

  33 | export function isValidTaskId(taskId: string): boolean {
  34 |   return typeof taskId === "string" && /^[A-Z2-7]{25}$/.test(taskId);
  35 | }

• ✅ isValidTaskId returns true for valid 25-character RFC 4648 base32 task IDs, such as those generated by taskIdForBounty. — In backend/src/bounties/taskid.ts line 34, isValidTaskId returns true for 25-character base32 strings matching /^[A-Z2-7]{25}$/. Task IDs produced by taskIdForBounty format 'BNTY' followed by 21 base32 characters, yielding a 25-character base32 string that satisfies the pattern.

  backend/src/bounties/taskid.ts:33

  33 | export function isValidTaskId(taskId: string): boolean {
  34 |   return typeof taskId === "string" && /^[A-Z2-7]{25}$/.test(taskId);
  35 | }

• ✅ Regression tests in backend/src/bounties/taskid.test.ts assert that isValidTaskId rejects 25-character strings with illegal base32 characters and accepts valid generated task IDs. — In backend/src/bounties/taskid.test.ts lines 5-12, the test suite asserts isValidTaskId('!!!!!!!!!!!!!!!!!!!!!!!!!') === false and isValidTaskId(validId) === true where validId is generated by taskIdForBounty('bounty_test_123').

  backend/src/bounties/taskid.test.ts:5

  5 | test("isValidTaskId enforces base32 charset and 25-char length", () => {
  6 |   assert.equal(isValidTaskId("!!!!!!!!!!!!!!!!!!!!!!!!!"), false, "Illegal charset must return false");
  7 |   assert.equal(isValidTaskId("lower-case-invalid-base32"), false, "Lowercase characters must return false");
  8 |   assert.equal(isValidTaskId("SHORT"), false, "Short taskId must return false");
  9 | 
  10 |   const validId = taskIdForBounty("bounty_test_123");
  11 |   assert.equal(isValidTaskId(validId), true, "Valid base32 derived taskId must return true");
  12 | });

Additional Findings

Warnings

• Possible regression (backend/src/bounties/taskid.test.ts:5)
  The PR removes existing test cases for taskIdForBounty determinism, distinctness across bounty IDs, input length upper-bound validation (e.g. 26 characters), and property checks across multiple bounty ID formats. Removing these tests reduces test coverage and risks masking regressions in core bounty ID derivation logic.

  Suggested Change: line 3

  -  3 | import { isValidTaskId, taskIdForBounty } from "./taskid.js";
  +  3 | import { isValidTaskId, taskIdForBounty, TASK_ID_LENGTH } from "./taskid.js";
  
  +  5 | test("always produces exactly 25 base32 chars", () => {
  +  6 |   for (const id of ["a", "b1e-uuid-value", crypto.randomUUID(), crypto.randomUUID(), "x".repeat(200)]) {
  +  7 |     const t = taskIdForBounty(id);
  +  8 |     assert.equal(t.length, TASK_ID_LENGTH);
  +  9 |     assert.match(t, /^[A-Z2-7]+$/);
  + 10 |     assert.equal(isValidTaskId(t), true);
  + 11 |   }
  + 12 | });
  + 13 | 
  + 14 | test("is deterministic and distinct per bounty id", () => {
  + 15 |   const a = crypto.randomUUID();
  + 16 |   const b = crypto.randomUUID();
  + 17 |   assert.equal(taskIdForBounty(a), taskIdForBounty(a));
  + 18 |   assert.notEqual(taskIdForBounty(a), taskIdForBounty(b));
  + 19 | });
  + 20 | 
  
  + 25 |   assert.equal(isValidTaskId("X".repeat(26)), false, "Overlong taskId must return false");

Nitpicks

• Possible regression (backend/src/bounties/taskid.ts:34)
  Hardcoding {25} in /^[A-Z2-7]{25}$/ decouples isValidTaskId from the exported TASK_ID_LENGTH constant (export const TASK_ID_LENGTH = 25;), duplicating the length value as a magic number. Checking taskId.length === TASK_ID_LENGTH && /^[A-Z2-7]+$/.test(taskId) preserves TASK_ID_LENGTH as the single source of truth.

  Suggested Change: line 33

  - 34 |   return typeof taskId === "string" && /^[A-Z2-7]{25}$/.test(taskId);
  + 34 |   return typeof taskId === "string" && taskId.length === TASK_ID_LENGTH && /^[A-Z2-7]+$/.test(taskId);

📋 Copy Review for AI Agent

Copy the prompt below and paste it into your AI coding assistant to apply all findings.

Apply the following code review findings to the codebase. For each item, make the described change at the specified file and line. Use the fix instruction when provided, otherwise implement the fix based on the issue description.

1. [WARN] backend/src/bounties/taskid.test.ts:5
   Issue: Possible regression — The PR removes existing test cases for `taskIdForBounty` determinism, distinctness across bounty IDs, input length upper-bound validation (e.g. 26 characters), and property checks across multiple bounty ID formats. Removing these tests reduces test coverage and risks masking regressions in core bounty ID derivation logic.
   Fix: Restore the deleted test coverage in `backend/src/bounties/taskid.test.ts` for `taskIdForBounty` determinism, distinctness, format validation, and overlong length checks while keeping the new base32 charset tests.

2. [NIT] backend/src/bounties/taskid.ts:34
   Issue: Possible regression — Hardcoding `{25}` in `/^[A-Z2-7]{25}$/` decouples `isValidTaskId` from the exported `TASK_ID_LENGTH` constant (`export const TASK_ID_LENGTH = 25;`), duplicating the length value as a magic number. Checking `taskId.length === TASK_ID_LENGTH && /^[A-Z2-7]+$/.test(taskId)` preserves `TASK_ID_LENGTH` as the single source of truth.
   Fix: In `backend/src/bounties/taskid.ts`, update `isValidTaskId` to check `taskId.length === TASK_ID_LENGTH && /^[A-Z2-7]+$/.test(taskId)` so that `TASK_ID_LENGTH` remains the single source of truth for length validation.

📊 Review metadata

• Processing time: 65s
• Completed: 2026-08-07T21:04:14.877Z

🤖 This review was generated by AI. While we strive for accuracy, please use your judgment when applying suggestions.

[vercel]

@jihadMo is attempting to deploy a commit to the devasign Team on Vercel.

A member of the Team first needs to authorize it.