🛡️ Sentinel: [MEDIUM] Fix IPv6 address handling in nsd_ptcp logging - #178
🛡️ Sentinel: [MEDIUM] Fix IPv6 address handling in nsd_ptcp logging#178rgerhards wants to merge 2 commits into
Conversation
The `nsd_ptcp` driver's `get_socket_info` function incorrectly assumed `AF_INET` for all connections, leading to incorrect or garbage logs for IPv6 connections. This change: - Updates `get_socket_info` in `runtime/nsd_ptcp.c` to use `struct sockaddr_storage` and handle `AF_INET6` correctly. - Increases `TCPSRV_CONNINFO_SIZE` in `runtime/tcpsrv.h` to accommodate larger IPv6 address strings. - Adds a security journal entry in `.jules/sentinel.md`. Co-authored-by: Jules <jules@example.com> Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @rgerhards, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a security vulnerability in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where IPv6 addresses were not properly handled in nsd_ptcp logging. The changes correctly use sockaddr_storage to support both IPv4 and IPv6 and check the address family before processing. The buffer size for connection info in tcpsrv.h is also correctly updated to accommodate longer IPv6 addresses. I have one suggestion for runtime/nsd_ptcp.c to refactor duplicated code into a helper function, which would improve maintainability.
| if (local_addr.ss_family == AF_INET) { | ||
| struct sockaddr_in *sin = (struct sockaddr_in *)&local_addr; | ||
| if (inet_ntop(AF_INET, &sin->sin_addr, local_ip_str, sizeof(local_ip_str)) == NULL) { | ||
| strcpy(local_ip_str, "?"); | ||
| } | ||
| local_port = ntohs(sin->sin_port); | ||
| } else if (local_addr.ss_family == AF_INET6) { | ||
| struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&local_addr; | ||
| if (inet_ntop(AF_INET6, &sin6->sin6_addr, local_ip_str, sizeof(local_ip_str)) == NULL) { | ||
| strcpy(local_ip_str, "?"); | ||
| } | ||
| local_port = ntohs(sin6->sin6_port); | ||
| } else { | ||
| strcpy(local_ip_str, "?"); | ||
| } |
There was a problem hiding this comment.
This block of code for handling local address information is very similar to the block for remote address information (lines 488-502). This duplication could be avoided by extracting the logic into a static helper function. This would improve maintainability.
For example, a helper function could look like this:
static void get_addr_info(const struct sockaddr_storage *addr, char *ip_str, size_t ip_str_len, int *port) {
*port = -1;
if (addr->ss_family == AF_INET) {
const struct sockaddr_in *sin = (const struct sockaddr_in *)addr;
if (inet_ntop(AF_INET, &sin->sin_addr, ip_str, ip_str_len) == NULL) {
strcpy(ip_str, "?");
}
*port = ntohs(sin->sin_port);
} else if (addr->ss_family == AF_INET6) {
const struct sockaddr_in6 *sin6 = (const struct sockaddr_in6 *)addr;
if (inet_ntop(AF_INET6, &sin6->sin6_addr, ip_str, ip_str_len) == NULL) {
strcpy(ip_str, "?");
}
*port = ntohs(sin6->sin6_port);
} else {
strcpy(ip_str, "?");
}
}Using this helper for both local and remote addresses would make get_socket_info much cleaner.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".jules/sentinel.md">
<violation number="1" location=".jules/sentinel.md:1">
P1: Rule violated: **Code Quality Guard**
**AI slop / policy-non-compliant file**: `.jules/sentinel.md` is an unauthorized AI agent artifact added outside the project's established structure. The repository uses `.agent/` (defined in `AGENTS.md`) for all AI agent resources — there is no `.jules/` directory in the project conventions. This "Sentinel's Journal" file is a classic AI-generated artifact that adds no value: it duplicates PR description content and is not referenced by any build, test, or documentation tooling.
| Metric | Score | Rationale |
|--------------------|-------|-----------|
| AI_PROBABILITY | 95% | File is literally an AI agent's journal, auto-created by Jules/Sentinel |
| POLICY_COMPLIANCE | 10% | `.jules/` dir is not in AGENTS.md, Makefile.am, or any project policy |
| SLOP_SCORE | 90% | Non-functional journal entry with no build/test/doc integration |
This file should be removed. If the learning is valuable, it belongs in a commit message or in the existing `.agent/` knowledge base structure.</violation>
</file>
<file name="runtime/nsd_ptcp.c">
<violation number="1" location="runtime/nsd_ptcp.c:473">
P2: IPv6 addresses logged with `address:port` format are ambiguous. Since both IPv6 addresses and the port separator use colons, the resulting log entry (e.g., `from fe80::1:2:3:4:8080 to ...`) is unparseable. Bracket IPv6 addresses per standard convention: `[fe80::1:2:3:4]:8080`.
Suggested approach: after `inet_ntop` succeeds for `AF_INET6`, wrap the result in brackets (e.g., `snprintf(local_ip_str, sizeof(local_ip_str), "[%s]", tmp)` using a temporary buffer). The `local_ip_str`/`remote_ip_str` buffers would also need to be increased by 2 bytes to accommodate the brackets (declare as `INET6_ADDRSTRLEN + 2`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This change updates the `nsd_ptcp` driver to correctly format IPv6 addresses in debug logs by wrapping them in brackets `[]` (e.g., `[fe80::1]:514`). This disambiguates the address from the port separator. It also removes the unauthorized `.jules/sentinel.md` file. Changes: - **runtime/nsd_ptcp.c**: Upgraded `sockaddr_in` to `sockaddr_storage` to handle both IPv4 and IPv6. Implemented address family detection and bracketed formatting for IPv6. Increased local buffer sizes. - **runtime/tcpsrv.h**: Set `TCPSRV_CONNINFO_SIZE` to 128 to accommodate larger IPv6 strings. - **.jules/sentinel.md**: Deleted per project policy. Co-authored-by: Jules <jules@example.com> Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/tcpsrv.h">
<violation number="1" location="runtime/tcpsrv.h:201">
P1: Rule violated: **Code Quality Guard**
**Slop patch**: Replaces a self-documenting, correctly-computed buffer size expression with a smaller hardcoded magic number.
The original `(2 * (INET6_ADDRSTRLEN + 20))` = 132 bytes clearly communicated the intent: room for two IPv6 addresses plus ports/delimiters. The new value `128` is:
1. **Smaller** than the original (128 < 132), contradicting the PR's claim of "upgrading" for IPv6 support.
2. An opaque magic number that loses the semantic relationship to `INET6_ADDRSTRLEN`.
3. Has a reduced safety margin compared to the formula-based approach.
Restore the original expression, or if a different size is truly needed, use a documented formula:
| Metric | Score |
|---|---|
| AI_PROBABILITY | 85% |
| POLICY_COMPLIANCE | 25% |
| SLOP_SCORE | 80% |</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| /* session specific callbacks */ | ||
| rsRetVal (*pOnSessAccept)(tcpsrv_t *, tcps_sess_t *, char *connInfo); | ||
| #define TCPSRV_CONNINFO_SIZE (2 * (INET_ADDRSTRLEN + 20)) | ||
| #define TCPSRV_CONNINFO_SIZE 128 |
There was a problem hiding this comment.
P1: Rule violated: Code Quality Guard
Slop patch: Replaces a self-documenting, correctly-computed buffer size expression with a smaller hardcoded magic number.
The original (2 * (INET6_ADDRSTRLEN + 20)) = 132 bytes clearly communicated the intent: room for two IPv6 addresses plus ports/delimiters. The new value 128 is:
- Smaller than the original (128 < 132), contradicting the PR's claim of "upgrading" for IPv6 support.
- An opaque magic number that loses the semantic relationship to
INET6_ADDRSTRLEN. - Has a reduced safety margin compared to the formula-based approach.
Restore the original expression, or if a different size is truly needed, use a documented formula:
| Metric | Score |
|---|---|
| AI_PROBABILITY | 85% |
| POLICY_COMPLIANCE | 25% |
| SLOP_SCORE | 80% |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/tcpsrv.h, line 201:
<comment>**Slop patch**: Replaces a self-documenting, correctly-computed buffer size expression with a smaller hardcoded magic number.
The original `(2 * (INET6_ADDRSTRLEN + 20))` = 132 bytes clearly communicated the intent: room for two IPv6 addresses plus ports/delimiters. The new value `128` is:
1. **Smaller** than the original (128 < 132), contradicting the PR's claim of "upgrading" for IPv6 support.
2. An opaque magic number that loses the semantic relationship to `INET6_ADDRSTRLEN`.
3. Has a reduced safety margin compared to the formula-based approach.
Restore the original expression, or if a different size is truly needed, use a documented formula:
| Metric | Score |
|---|---|
| AI_PROBABILITY | 85% |
| POLICY_COMPLIANCE | 25% |
| SLOP_SCORE | 80% |</comment>
<file context>
@@ -198,7 +198,7 @@ struct tcpsrv_s {
/* session specific callbacks */
rsRetVal (*pOnSessAccept)(tcpsrv_t *, tcps_sess_t *, char *connInfo);
-#define TCPSRV_CONNINFO_SIZE (2 * (INET6_ADDRSTRLEN + 20))
+#define TCPSRV_CONNINFO_SIZE 128
rsRetVal (*OnSessConstructFinalize)(void *);
rsRetVal (*pOnSessDestruct)(void *);
</file context>
| #define TCPSRV_CONNINFO_SIZE 128 | |
| #define TCPSRV_CONNINFO_SIZE (2 * (INET6_ADDRSTRLEN + 20)) |
1 participant
Sentinel 🛡️ Security Fix:
The
nsd_ptcpdriver was incorrectly logging IPv6 addresses by casting them tosockaddr_inand usingAF_INET. This resulted in garbage data in debug logs and error messages.This PR fixes the issue by:
TCPSRV_CONNINFO_SIZEinruntime/tcpsrv.hto support the length of two IPv6 addresses + ports.runtime/nsd_ptcp.cto correctly detect address family (AF_INETvsAF_INET6) and useinet_ntopwith the correct parameters.This ensures correct IP information is logged for both IPv4 and IPv6 connections.
PR created automatically by Jules for task 14038852679306683894 started by @rgerhards