Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds NIP‑51 (Lists): feature flag and build/test wiring, new public nostr_list API and implementation (dynamic items, metadata, serialize/deserialize to nostr_event), optional NIP‑44 private-item encryption, tests, and feature-query registration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant List as nostr_list
participant Crypto as NIP44_Encryption
participant Event as nostr_event
Client->>List: nostr_list_create(kind)
Client->>List: set metadata (d_tag,title,description,image)
Client->>List: add items (pubkey/event/relay/hashtag/word/reference/etc.)
Client->>List: nostr_list_to_event(keypair)
alt Private items present and NIP44 available
List->>Crypto: encrypt private-items JSON with keypair
Crypto-->>List: encrypted content (base64)
end
List->>Event: build event with tags + content
Event-->>Client: nostr_event*
sequenceDiagram
participant Client as Client
participant Event as nostr_event
participant Crypto as NIP44_Decryption
participant List as nostr_list
Client->>List: nostr_list_from_event(event, keypair)
List->>Event: extract tags and content
alt Encrypted content present and NIP44 available
List->>Crypto: decrypt content with keypair
Crypto-->>List: decrypted private-items JSON
end
List->>List: parse tags + (decrypted) JSON → items
List-->>Client: reconstructed nostr_list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/nip51.c`:
- Around line 649-664: The current block silently skips decrypting private
content when NOSTR_FEATURE_NIP44 is not defined (even if event->content is
present and keypair is non-NULL); update the logic in the branch that checks
event->content and keypair so that when NOSTR_FEATURE_NIP44 is not available you
either return NOSTR_ERR_NOT_SUPPORTED or set an output flag/error to indicate
private content was skipped (instead of doing nothing). Locate the check around
event->content and keypair in src/nip51.c and modify the behavior used when
NOSTR_FEATURE_NIP44 is undefined (the branch that would call nostr_nip44_decrypt
and parse_private_content) to propagate a clear non-supported status (or toggle
a provided out-parameter) so callers can detect that encrypted content was not
processed.
In `@tests/test_nip51.c`:
- Around line 74-92: The test calls nostr_list_create(&list,
NOSTR_LIST_KIND_MUTE) but does not verify the creation succeeded before using
list; update test_list_add_pubkey to check the creation result and/or that list
is non-NULL (e.g., assert nostr_list_create returned NOSTR_OK and/or
TEST_ASSERT_NOT_NULL(list)) right after the nostr_list_create call, and if the
check fails ensure the test cleans up (skip further calls to
nostr_list_add_pubkey, nostr_list_count, nostr_list_get, nostr_list_free) so the
test fails cleanly instead of dereferencing a NULL list.
♻️ Duplicate comments (16)
tests/test_nip51.c (16)
94-115: Same create-check concern applies here.
117-128: Same create-check concern applies here.
130-148: Same create-check concern applies here.
150-165: Same create-check concern applies here.
167-182: Same create-check concern applies here.
184-199: Same create-check concern applies here.
201-218: Same create-check concern applies here.
220-246: Same create-check concern applies here.
248-280: Same create-check concern applies here.
282-326: Same create-check concern applies here.
328-360: Same create-check concern applies here.
362-394: Same create-check concern applies here.
396-413: Same create-check concern applies here.
415-430: Same create-check concern applies here.
432-446: Same create-check concern applies here.
448-468: Same create-check concern applies here.
🧹 Nitpick comments (5)
include/nostr.h (1)
1394-1455: Document nostr_list thread-safety expectations.These list types are mutable; a short thread-safety note (similar to NIP-65’s) would prevent misuse in multi-threaded clients.
src/nip51.c (4)
29-51: Control character escaping is lossy and non-standard.The
json_escape_tofunction maps all control characters (< 0x20) except\n,\r,\tto\n. This loses information - for example,\b(backspace, 0x08) or\f(form feed, 0x0C) would be incorrectly converted to\n. Per JSON spec, these should use\uXXXXencoding or their specific escape sequences.♻️ Suggested improvement for proper JSON escaping
default: if (c < 0x20) { - *dest++ = '\\'; - *dest++ = 'n'; + static const char hex[] = "0123456789abcdef"; + *dest++ = '\\'; + *dest++ = 'u'; + *dest++ = '0'; + *dest++ = '0'; + *dest++ = hex[c >> 4]; + *dest++ = hex[c & 0x0f]; } else { *dest++ = c; }Note: This requires updating
json_escaped_lento return 6 instead of 2 for control characters.
204-220: Consider adding hex character validation for pubkey and event_id.The validation only checks string length (64 characters) but doesn't verify that the characters are valid hexadecimal. This could allow malformed identifiers to be added to lists.
♻️ Suggested hex validation helper
static bool is_valid_hex64(const char* s) { if (!s || strlen(s) != 64) return false; for (size_t i = 0; i < 64; i++) { char c = s[i]; if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'))) { return false; } } return true; }Then use
is_valid_hex64(pubkey)instead ofstrlen(pubkey) != 64.
513-516: Fixed-size buffers may silently truncate long values.The parsing uses fixed-size buffers (
value[1024],relay_hint[512], etc.) which will silently truncate longer content without any error indication. For robustness, consider either dynamically allocating based on actual content length or returning an error when content exceeds buffer limits.
556-565: Unicode escape sequences are replaced with?, losing data.The
\uXXXXJSON escape handling replaces all Unicode escape sequences with a literal?character. This loses potentially important data (e.g., emoji, non-ASCII characters). Consider implementing proper UTF-8 conversion for Unicode escape sequences.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/nip51.c`:
- Around line 40-46: The default branch currently encodes all control chars
(<0x20) as the two-character sequence '\n' which is wrong; change the serializer
(the code that writes to *dest in this switch) to emit a 6-character JSON
Unicode escape "\\u00XX" for control bytes except for the already-handled '\n',
'\r', '\t' cases (ensure the hex is zero-padded to four digits), and update
json_escaped_len to add 6 characters (not 2) for those control bytes so length
accounting matches the new "\\u00XX" output.
🧹 Nitpick comments (2)
src/nip51.c (1)
513-516: Fixed-size buffers may silently truncate long values.The parser uses fixed-size stack buffers (e.g.,
value[1024]) that silently truncate longer strings due to thelen < max_lencheck at line 547. Consider either:
- Returning an error if content exceeds buffer size
- Using dynamic allocation for values
This is a minor concern since NIP-51 list items are typically short identifiers.
tests/test_nip51.c (1)
95-116: Inconsistent error checking for list creation.While
test_list_add_pubkey(lines 77-78) now properly checks the creation result,test_list_add_pubkey_with_hintsand several other tests still callnostr_list_createwithout verifying success. If creation fails, subsequent operations will dereference NULL.Apply the same pattern used in
test_list_add_pubkey:♻️ Suggested fix for test_list_add_pubkey_with_hints
static int test_list_add_pubkey_with_hints(void) { nostr_list* list = NULL; - nostr_list_create(&list, NOSTR_LIST_KIND_FOLLOW_SET); + TEST_ASSERT_EQUAL(NOSTR_OK, nostr_list_create(&list, NOSTR_LIST_KIND_FOLLOW_SET)); + TEST_ASSERT_NOT_NULL(list); const char* pubkey = "abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890";The same pattern should be applied to these tests:
test_list_add_pubkey_invalid,test_list_add_event,test_list_add_hashtag,test_list_add_word,test_list_add_relay,test_list_add_reference,test_list_remove,test_list_to_event_standard,test_list_to_event_parameterized,test_list_from_event,test_list_from_event_parameterized,test_list_set_metadata,test_list_capacity_growth,test_list_get_out_of_bounds.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/nip51.c`:
- Around line 519-522: The fixed-size local buffers tag_type[64], value[1024],
relay_hint[512], and petname[256] can silently truncate long inputs; change the
parser to detect truncation and fail rather than silently corrupt data (or
switch to dynamic allocation). Specifically, when copying/parsing into tag_type,
value, relay_hint, and petname, check the source length against sizeof(...) (or
the snprintf return value) and return an error code if the input is larger than
the buffer, or replace the stack buffers with heap buffers sized to the actual
input length (malloc/free) and propagate ownership to callers; ensure all call
sites handle the new error or freed memory paths.
- Around line 307-389: The buffer size calculation in build_private_content
undercounts when item->petname is present but item->relay_hint is false because
the code emits an extra empty-string placeholder (the ,"") but item_size only
adds json_escaped_len(item->petname) + 4; update the size calculation logic for
that loop (the item_size computation using json_escaped_len and checks for
item->relay_hint/item->petname) to add the extra 4 bytes for the placeholder
when petname exists and relay_hint is absent, and keep the existing overflow
check (buf_size > SIZE_MAX - item_size) intact so malloc gets a correctly sized
buffer.
- Around line 562-571: In the case 'u' escape handling block, instead of
skipping the four hex chars and appending '?', parse the four hex digits from *p
to compute the Unicode code point (use p and the loop that currently advances
over 4 hex digits), then if the value is <= 0xFF write that byte into
dest[len++] (respecting max_len) so \u00XX round-trips correctly; for values >
0xFF either encode the code point as UTF-8 into dest (respecting max_len) or
fall back to '?' if you don't want to implement UTF-8 here, but do not silently
drop valid \u00XX sequences.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/nip51.c`:
- Around line 701-714: The current block calls nostr_nip44_decrypt and only
handles the success case, silently ignoring errors; change it so that when
nostr_nip44_decrypt returns a non-NOSTR_OK error you propagate that error and
jump to cleanup instead of continuing as if OK. Specifically, after calling
nostr_nip44_decrypt (with nostr_keypair_private_key(keypair),
nostr_keypair_public_key(keypair), event->content, &decrypted, &decrypted_len)
check if err != NOSTR_OK and if so goto cleanup (ensuring any allocated
decrypted is freed if set); keep the existing success path that calls
parse_private_content(decrypted, *list) and free(decrypted) and propagate parse
errors to cleanup as before.
- Around line 595-607: The UTF-8 encoding branches in nip51.c silently drop
multi-byte output when there isn’t room (branches handling codepoint <= 0x7FF
and else for 3-byte sequences write fewer bytes only if space is available), so
update those branches to detect insufficient buffer space and trigger the
existing truncation/error path instead of skipping bytes: check remaining space
(using len and max_len) before starting a multi-byte write, and if there isn’t
enough room set the same truncation indicator or return value used elsewhere in
this function (or call the same error/truncation handler) so the caller observes
failure rather than getting silently truncated output; ensure you reference and
update the logic around dest, len, max_len and the codepoint encoding blocks so
behavior is consistent for 1-, 2- and 3-byte sequences.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/nip51.c`:
- Around line 465-468: Check that the keypair is initialized (i.e.,
nostr_keypair_private_key(keypair) and nostr_keypair_public_key(keypair) are
non-NULL) before calling nostr_nip44_encrypt or nostr_nip44_decrypt; if either
returns NULL, fail fast by returning an error (or setting err) and avoid passing
NULL into nostr_nip44_* functions. Add the guard around the encryption call
where encrypted is produced (the nostr_nip44_encrypt call) and the analogous
guard around the decrypt path (the nostr_nip44_decrypt usage referenced later),
using the same keypair variable and ensure any allocated resources are cleaned
up before returning.
🧹 Nitpick comments (1)
src/nip51.c (1)
548-553: Make unknown-field skipping escape‑aware.
If an extra field contains escaped quotes, the current skip loop can desync parsing. Consider skipping with the same escape-aware logic used in the main parse loop.♻️ Suggested refinement
- default: - while (*p && *p != '"') p++; - if (*p == '"') p++; - field++; - continue; + default: + while (*p) { + if (*p == '\\' && *(p + 1)) { p += 2; continue; } + if (*p == '"') { p++; break; } + p++; + } + field++; + continue;
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Add all 15 missing list kinds to enum (10009-39092) - Add group tag helper for NIP-29 simple groups - Add emoji tag helper for NIP-30 custom emoji - Fix type mismatch in NIP-44 encrypt/decrypt calls - Add hex validation for pubkey/event_id inputs - Fix UTF-16 surrogate pair handling - Simplify code with extracted helper functions - Add NIP-51 to README features list
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/nip51.c`:
- Around line 98-106: In parse_hex4(const char** pp) ensure you don't read past
the NUL by adding a short-length guard that verifies there are at least four
chars available at p before the for-loop (e.g. check p[0..3] are not '\0' or use
strlen(p) < 4) and return -1 if fewer than four remain; keep the existing
hex_digit_value checks and behavior otherwise so malformed \u escapes produce -1
instead of causing OOB reads.
🧹 Nitpick comments (1)
include/nostr.h (1)
1496-1503: Fix the typo in the public enum name before it becomes an API wart.
NOSTR_LIST_KIND_KIND_MUTE_SETat Line 1502 looks accidental. Consider renaming toNOSTR_LIST_KIND_MUTE_SETand keeping a deprecated alias to avoid breaking early adopters.♻️ Proposed fix (retain compatibility)
- NOSTR_LIST_KIND_KIND_MUTE_SET = 30007, + NOSTR_LIST_KIND_MUTE_SET = 30007, + NOSTR_LIST_KIND_KIND_MUTE_SET = NOSTR_LIST_KIND_MUTE_SET, /* deprecated alias */
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/nip51.c`:
- Around line 178-196: Each setter (nostr_list_set_d_tag, nostr_list_set_title,
nostr_list_set_description, nostr_list_set_image) currently computes
&list->member before calling set_string_field which is undefined if list is
NULL; add an explicit NULL check at the top of each function (if (list == NULL)
return an appropriate nostr_error_t invalid-argument error, e.g.,
NOSTR_ERROR_INVALID_ARGUMENT) and only then call set_string_field(&list->...,
...); this ensures you never use the -> operator on a NULL pointer.
2 participants
Implements NIP-51 list management with support for:
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.