Backport: ReDoS fix + NarrowedBy getter limitation

[UniquePixels]

Summary

Two framework issues discovered during the Unbound bot migration need upstream attention.

Issue 1: ReDoS risk in component.ts dynamic RegExp (CWE-1333)

Severity: Security
Found by: Semgrep pre-migration scan
Fix available: framework/fixes branch on Unbound repo (commits 36edc09, ea3db35)

parseComponentId() constructs a RegExp from user-provided component ID patterns without validating the input characters. A malicious or malformed pattern containing quantifiers, alternation, or nested groups could cause catastrophic backtracking.

Fix: Added SAFE_PATTERN allowlist (/^[a-zA-Z0-9_:.-]+$/) that validates the inner pattern before regex construction. Only alphanumerics, dashes, underscores, dots, and :param markers are permitted. Throws AppError with code ERR_COMPONENT_PATTERN for invalid characters.

Also added nosemgrep annotation on the new RegExp() call with a safety comment explaining why the dynamic regex is safe post-validation.

Files changed:

• src/core/sparks/component.ts — validation + safety comments
• src/core/sparks/component.test.ts — test.each for rejected characters (|, (, ), [, ], +, *)

Issue 2: NarrowedBy cannot narrow class getters

Severity: Type system limitation (no runtime impact)
No code fix — documentation/awareness only

TypeScript intersection types cannot override class getters. When inCachedGuild guard narrows BaseInteraction, the guild property (a getter on the class) produces Guild & (Guild | null) instead of the expected Guild. This means code after the guard still sees guild as potentially null.

Workaround: Use non-null assertion (interaction.guild!) with a biome-ignore comment referencing this issue.

Upstream options:

1. Document the limitation in guard/NarrowedBy docs
2. Consider runtime accessor pattern instead of type-level narrowing for class getters
3. Accept as known TS limitation

---

Found during the Unbound bot migration (Domains 0–10). Ready to cherry-pick from the framework/fixes branch for Issue 1.

[coderabbitai]

Coding Plan

Summary

• The ReDoS fix uses an allowlist approach rather than a blocklist, validating that component ID patterns contain only alphanumerics, dashes, underscores, dots, and colon-prefixed param markers before regex construction
• The Semgrep annotation (nosemgrep) documents why the dynamic RegExp is safe post-validation, satisfying static analysis tools
• The getter limitation is handled through documentation since it's a fundamental TypeScript behavior with no practical code-level fix
• Both changes align with existing patterns: AppError with ERR_COMPONENT_PATTERN code, test.each for parameterized test cases, and > [!NOTE] callouts in documentation

Implementation Steps

Phase 1: ReDoS Security Fix in parseComponentId

This phase addresses the CWE-1333 vulnerability by validating component ID pattern characters before regex construction, ensuring only safe characters can be interpolated into the dynamic RegExp.

Task 1: Add SAFE_PATTERN Validation

Introduce input validation in parseComponentId() to reject patterns containing potentially dangerous regex metacharacters before the dynamic RegExp is constructed.

• Define a SAFE_PATTERN constant regex (/^[a-zA-Z0-9_:.-]+$/) at module level in src/core/sparks/component.ts near the existing PARAM_REGEX constant (around line 126)
• Add validation check after extracting the inner string (after line 145) that tests inner against SAFE_PATTERN
• Throw AppError with code ERR_COMPONENT_PATTERN if validation fails, including the invalid pattern in metadata
• Add a nosemgrep comment annotation directly above the new RegExp() call (line 190) explaining that the input has been validated against the SAFE_PATTERN allowlist

Task 2: Add Test Coverage for Rejected Characters

Extend the test suite with parameterized tests verifying that dangerous regex metacharacters are properly rejected.

• Add a test.each block in src/core/sparks/component.test.ts within the parseComponentId describe block (after the existing error case tests around line 75)
• Test cases should include rejected characters: |, (, ), [, ], +, * embedded in otherwise valid patterns (e.g., {ban-:user|id}, {ban-:user(id)})
• Use the established try/catch assertion pattern: verify error is AppError instance and code is ERR_COMPONENT_PATTERN
• Include test case descriptions that explain what character is being rejected

🤖 Prompt for AI agents

Implement the ReDoS (CWE-1333) security fix in `parseComponentId()` by adding an
allowlist-based input validation step before dynamic RegExp construction, and
extend the test suite to cover rejected characters.

**Task 1: Add `SAFE_PATTERN` validation to `src/core/sparks/component.ts`**
- At module level near the existing `PARAM_REGEX` constant (around line 126),
define a new constant: `SAFE_PATTERN = /^[a-zA-Z0-9_:.-]+$/`
- After the `inner` string is extracted (after line 145), add a validation check
that tests `inner` against `SAFE_PATTERN`
- If validation fails, throw an `AppError` with code `ERR_COMPONENT_PATTERN`,
including the invalid pattern in the error metadata
- Directly above the `new RegExp()` call (line 190), add a `nosemgrep` comment
annotation explaining that the input has already been validated against the
`SAFE_PATTERN` allowlist

**Task 2: Add parameterized test coverage in
`src/core/sparks/component.test.ts`**
- Inside the `parseComponentId` describe block, after the existing error case
tests (around line 75), add a `test.each` block
- Include test cases for the following rejected metacharacters embedded in
otherwise valid patterns: `|`, `(`, `)`, `[`, `]`, `+`, `*` — e.g.,
`{ban-:user|id}`, `{ban-:user(id)}`
- Each test case should use the established try/catch assertion pattern: verify
the caught `error` is an `AppError` instance and that `code` equals
`ERR_COMPONENT_PATTERN`
- Each test case description should clearly identify which character is being
rejected

Phase 2: Document NarrowedBy Getter Limitation

This phase adds documentation explaining the TypeScript limitation where intersection types cannot override class getters, affecting how inCachedGuild narrows the guild property.

Add Getter Limitation Note to Guards Documentation

Document the limitation and workaround in the guards documentation where developers will encounter it.

• Add a > [!NOTE] callout in docs/guards.md within the inCachedGuild section (after line 45, near the existing channel-may-be-null caveat)
• Explain that TypeScript intersection types cannot override class getters: BaseInteraction.guild is a getter returning Guild | null, and intersecting with { guild: Guild } produces Guild & (Guild | null) rather than Guild
• Document the recommended workaround: use non-null assertion (interaction.guild!) with a biome-ignore comment referencing this known TypeScript limitation
• Optionally add a brief mention in the "Automatic Type Narrowing" section (around line 745) noting that this limitation applies specifically to class getter properties, not regular object properties

🤖 Prompt for AI agents

Add documentation to `docs/guards.md` explaining the TypeScript limitation where
`NarrowedBy` cannot narrow class getter properties like `guild` on Discord.js
interactions.

**Task: Update `docs/guards.md`**
- In the `inCachedGuild` section (after line 45, near the existing
channel-may-be-null caveat), insert a `> [!NOTE]` callout block
- Explain that TypeScript intersection types cannot override class getters:
`BaseInteraction.guild` is a getter returning `Guild | null`, and intersecting
with `{ guild: Guild }` produces `Guild & (Guild | null)` rather than `Guild`
- Document the recommended workaround: use a non-null assertion
(`interaction.guild!`) accompanied by a `biome-ignore` comment referencing this
known TypeScript limitation
- Optionally, add a brief mention in the "Automatic Type Narrowing" section
(around line 745) noting that this limitation applies specifically to class
getter properties, not to regular object properties

Research

The project is a Discord bot framework built on discord.js with a "spark" system for declarative handler registration. The parseComponentId() function in src/core/sparks/component.ts parses component ID patterns (exact or parameterized) and constructs RegExp objects for parameterized patterns. Error handling uses AppError with structured ERR_* codes. The guards system uses NarrowedBy<TBase, Guards> to intersect guard output types for automatic type narrowing. Documentation lives in docs/ with guards.md covering the guard system and components.md covering component patterns. Tests use Bun's test runner with test.each for parameterized cases.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement the ReDoS (CWE-1333) security fix in `parseComponentId()` by adding an
allowlist-based input validation step before dynamic RegExp construction, and
extend the test suite to cover rejected characters.

**Task 1: Add `SAFE_PATTERN` validation to `src/core/sparks/component.ts`**
- At module level near the existing `PARAM_REGEX` constant (around line 126),
define a new constant: `SAFE_PATTERN = /^[a-zA-Z0-9_:.-]+$/`
- After the `inner` string is extracted (after line 145), add a validation check
that tests `inner` against `SAFE_PATTERN`
- If validation fails, throw an `AppError` with code `ERR_COMPONENT_PATTERN`,
including the invalid pattern in the error metadata
- Directly above the `new RegExp()` call (line 190), add a `nosemgrep` comment
annotation explaining that the input has already been validated against the
`SAFE_PATTERN` allowlist

**Task 2: Add parameterized test coverage in
`src/core/sparks/component.test.ts`**
- Inside the `parseComponentId` describe block, after the existing error case
tests (around line 75), add a `test.each` block
- Include test cases for the following rejected metacharacters embedded in
otherwise valid patterns: `|`, `(`, `)`, `[`, `]`, `+`, `*` — e.g.,
`{ban-:user|id}`, `{ban-:user(id)}`
- Each test case should use the established try/catch assertion pattern: verify
the caught `error` is an `AppError` instance and that `code` equals
`ERR_COMPONENT_PATTERN`
- Each test case description should clearly identify which character is being
rejected
===============================================================================

Task: 2

Add documentation to `docs/guards.md` explaining the TypeScript limitation where
`NarrowedBy` cannot narrow class getter properties like `guild` on Discord.js
interactions.

**Task: Update `docs/guards.md`**
- In the `inCachedGuild` section (after line 45, near the existing
channel-may-be-null caveat), insert a `> [!NOTE]` callout block
- Explain that TypeScript intersection types cannot override class getters:
`BaseInteraction.guild` is a getter returning `Guild | null`, and intersecting
with `{ guild: Guild }` produces `Guild & (Guild | null)` rather than `Guild`
- Document the recommended workaround: use a non-null assertion
(`interaction.guild!`) accompanied by a `biome-ignore` comment referencing this
known TypeScript limitation
- Optionally, add a brief mention in the "Automatic Type Narrowing" section
(around line 745) noting that this limitation applies specifically to class
getter properties, not to regular object properties

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[coderabbitai]

⚠️ Possible Duplicate Issue(s)

• Guard narrowing doesn't override class getters (guild: Guild | null) #32

🔗 Related PRs

#6 - 🌈 improve(testing): consolidate test utilities [merged]
#8 - 🦠 fix(qa): detect all Biome finding severities as failures [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[UniquePixels]

Additional: Redundant double-call in component test (minor)

In src/core/sparks/component.test.ts on the framework/fixes branch, the unsafe character test calls parseComponentId twice — once in expect().toThrow() and again in a try/catch to check the error code:

expect(() => parseComponentId(pattern)).toThrow(AppError);
try {
    parseComponentId(pattern);  // redundant
} catch (error) {
    expect((error as AppError).code).toBe('ERR_COMPONENT_PATTERN');
}

Can be simplified to a single assertion, e.g.:

try {
    parseComponentId(pattern);
    expect.unreachable('should have thrown');
} catch (error) {
    expect(error).toBeInstanceOf(AppError);
    expect((error as AppError).code).toBe('ERR_COMPONENT_PATTERN');
}

Low priority — functional but redundant.