Security: Empty catch block in decodeNpub swallows all errors silently#862
Security: Empty catch block in decodeNpub swallows all errors silently#862tomaioo wants to merge 1 commit into
Conversation
The `decodeNpub` function in `bot/modules/nostr/lib.ts` has an empty catch block that silently swallows all errors, including potential security issues with malformed input. This makes debugging difficult and could mask attacks attempting to exploit the NIP-19 decoding. Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
WalkthroughThe ChangesdecodeNpub Error Handling
Estimated code review effort: 1 (Trivial) | ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bot/modules/nostr/lib.ts (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent falsy return values across failure paths.
Decode failure now explicitly returns
null(Line 8), but a successful decode withtype !== 'npub'still implicitly returnsundefined(Line 6). Both are treated the same downstream via truthy checks currently, but the function's contract remains ambiguous. Consider making both invalid-input paths returnnullexplicitly for consistency.♻️ Proposed fix
export const decodeNpub = (npub: string) => { try { const { type, data } = nip19.decode(npub); - if (type === 'npub') return data; + if (type === 'npub') return data; + return null; } catch (err) { return null; } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/modules/nostr/lib.ts` around lines 3 - 9, The decodeNpub function currently returns null on decode errors but falls through to an implicit undefined when nip19.decode succeeds with a non-npub type, so make both invalid paths explicit and consistent. Update decodeNpub in lib.ts so the nip19.decode result only returns data when type === 'npub', and otherwise returns null just like the catch branch, keeping the function’s failure contract uniform.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bot/modules/nostr/lib.ts`:
- Around line 7-9: The catch block in the Nostr parsing path discards the thrown
error, so the promised logging is missing. Update the error handling in the
function containing the catch block to log the caught `err` before returning
`null`, using the existing logger or equivalent observability mechanism, while
keeping the clear null return path intact.
---
Nitpick comments:
In `@bot/modules/nostr/lib.ts`:
- Around line 3-9: The decodeNpub function currently returns null on decode
errors but falls through to an implicit undefined when nip19.decode succeeds
with a non-npub type, so make both invalid paths explicit and consistent. Update
decodeNpub in lib.ts so the nip19.decode result only returns data when type ===
'npub', and otherwise returns null just like the catch branch, keeping the
function’s failure contract uniform.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef77b1b2-a45e-4e01-b5dd-89c52abe12e3
📒 Files selected for processing (1)
bot/modules/nostr/lib.ts
| } catch (err) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Missing logging despite PR objective.
The PR description states this change adds "logging and a clear error return path," but the catch block only returns null — the caught error is discarded without any log call. If observability of malformed npub input was the intent, add a log statement here.
🐛 Proposed fix
} catch (err) {
+ console.error('Failed to decode npub:', err);
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| return null; | |
| } | |
| } catch (err) { | |
| console.error('Failed to decode npub:', err); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bot/modules/nostr/lib.ts` around lines 7 - 9, The catch block in the Nostr
parsing path discards the thrown error, so the promised logging is missing.
Update the error handling in the function containing the catch block to log the
caught `err` before returning `null`, using the existing logger or equivalent
observability mechanism, while keeping the clear null return path intact.
1 participant
Summary
Security: Empty catch block in decodeNpub swallows all errors silently
Problem
Severity:
Medium| File:bot/modules/nostr/lib.ts:L4The
decodeNpubfunction inbot/modules/nostr/lib.tshas an empty catch block that silently swallows all errors, including potential security issues with malformed input. This makes debugging difficult and could mask attacks attempting to exploit the NIP-19 decoding.Solution
Add error logging and return explicit error indication. Consider:
catch (err) { logger.error('NIP-19 decode failed:', err); return null; }and ensure callers handle null/undefined returns.Changes
bot/modules/nostr/lib.ts(modified)Summary by CodeRabbit