Admin audit log does not capture IP addresses #9

New Issue

marcel · 2026-04-02T11:21:07+02:00

marcel commented

2026-04-02 11:21:07 +02:00

Problem

The admin_audit_log table has an ip_address inet column, but the AdminController never passes HttpServletRequest to the service layer. All audit log entries are saved with null for IP address, reducing forensic value.

Affected files

AdminController.java — does not pass HttpServletRequest to service methods
AdminService.java:66-68, 111-112, 130-131 — saves audit logs with null IP

Recommended fix

Add HttpServletRequest parameter to AdminController methods
Pass request.getRemoteAddr() (or the X-Forwarded-For header if behind a reverse proxy) to AdminService
Store the IP in the AdminAuditLog entity

Severity

Medium — reduces forensic and incident response capabilities.

## Problem The `admin_audit_log` table has an `ip_address inet` column, but the `AdminController` never passes `HttpServletRequest` to the service layer. All audit log entries are saved with `null` for IP address, reducing forensic value. ## Affected files - `AdminController.java` — does not pass `HttpServletRequest` to service methods - `AdminService.java:66-68, 111-112, 130-131` — saves audit logs with `null` IP ## Recommended fix 1. Add `HttpServletRequest` parameter to `AdminController` methods 2. Pass `request.getRemoteAddr()` (or the `X-Forwarded-For` header if behind a reverse proxy) to `AdminService` 3. Store the IP in the `AdminAuditLog` entity ## Severity Medium — reduces forensic and incident response capabilities.

marcel added the kind/security priority/medium labels 2026-04-02 11:21:44 +02:00

marcel commented

2026-04-03 09:25:17 +02:00

👨‍💻 Kai — Frontend Engineer

Not much frontend surface to this one — the IP capture happens entirely in the backend. But a few things worth flagging from my side:

Admin UI feedback: If the audit log eventually surfaces in an admin screen (say, an E1/E2 type admin view), will the IP address column always be populated after this fix, or should the UI handle a null gracefully for older entries? I'd want to design the display component to tolerate both states from day one rather than assuming it's always present.
X-Forwarded-For rendering: If the fix passes through a raw X-Forwarded-For header (which can contain multiple IPs like client, proxy1, proxy2), the UI display component needs to handle that string form, not assume it's always a single clean address.
No frontend changes needed today — but when the admin audit log view is built, we should know which fields are guaranteed non-null vs. nullable so Atlas can spec the table correctly without ambiguity.

Questions:

Is there already a planned screen for viewing the admin_audit_log? If so, should this issue link to it or wait for it?
Will the API response DTO for audit log entries expose ipAddress as a nullable string or always guaranteed non-null post-fix?

## 👨‍💻 Kai — Frontend Engineer Not much frontend surface to this one — the IP capture happens entirely in the backend. But a few things worth flagging from my side: - **Admin UI feedback**: If the audit log eventually surfaces in an admin screen (say, an E1/E2 type admin view), will the IP address column always be populated after this fix, or should the UI handle a `null` gracefully for older entries? I'd want to design the display component to tolerate both states from day one rather than assuming it's always present. - **X-Forwarded-For rendering**: If the fix passes through a raw `X-Forwarded-For` header (which can contain multiple IPs like `client, proxy1, proxy2`), the UI display component needs to handle that string form, not assume it's always a single clean address. - **No frontend changes needed today** — but when the admin audit log view is built, we should know which fields are guaranteed non-null vs. nullable so Atlas can spec the table correctly without ambiguity. Questions: - Is there already a planned screen for viewing the `admin_audit_log`? If so, should this issue link to it or wait for it? - Will the API response DTO for audit log entries expose `ipAddress` as a nullable string or always guaranteed non-null post-fix?

marcel commented

2026-04-03 09:25:32 +02:00

🔧 Backend Engineer — Spring Boot / PostgreSQL Specialist

Solid forensic improvement. The fix is straightforward but there are a few design decisions worth making deliberately:

On request.getRemoteAddr() vs. X-Forwarded-For:

getRemoteAddr() will return the reverse proxy's IP (e.g., nginx, Traefik) when deployed — not the actual client. We need to decide upfront: are we behind a proxy in production?
If yes, use X-Forwarded-For header extraction, but do it safely — Spring has ForwardedHeaderFilter which handles this properly and avoids header spoofing. Don't roll a manual header-parse.
Recommend: configure ForwardedHeaderFilter as a bean and rely on request.getRemoteAddr() after the filter does its job, rather than reading X-Forwarded-For manually in the controller.

On passing HttpServletRequest through the service layer:

I'd avoid passing HttpServletRequest all the way down to AdminService. That couples the service layer to the HTTP context, which makes it harder to test and violates layer separation.
Better: extract the IP string in the controller, pass it as a plain String ipAddress parameter. The service stays portable and testable.

On the inet column type:

PostgreSQL's inet type validates IP format. If the value coming in is malformed (e.g., a spoofed multi-value X-Forwarded-For), the insert will throw. Make sure we sanitize/truncate to just the first valid IP before storing, or catch the constraint violation gracefully.

Questions:

Is the app deployed behind a reverse proxy in production? This determines the entire strategy.
Is AdminAuditLog a JPA entity mapped to inet as a String? If so, does Hibernate handle the inet → varchar cast cleanly, or do we need a custom type converter?

## 🔧 Backend Engineer — Spring Boot / PostgreSQL Specialist Solid forensic improvement. The fix is straightforward but there are a few design decisions worth making deliberately: **On `request.getRemoteAddr()` vs. `X-Forwarded-For`:** - `getRemoteAddr()` will return the reverse proxy's IP (e.g., nginx, Traefik) when deployed — not the actual client. We need to decide upfront: are we behind a proxy in production? - If yes, use `X-Forwarded-For` header extraction, but do it safely — Spring has `ForwardedHeaderFilter` which handles this properly and avoids header spoofing. Don't roll a manual header-parse. - Recommend: configure `ForwardedHeaderFilter` as a bean and rely on `request.getRemoteAddr()` after the filter does its job, rather than reading `X-Forwarded-For` manually in the controller. **On passing `HttpServletRequest` through the service layer:** - I'd avoid passing `HttpServletRequest` all the way down to `AdminService`. That couples the service layer to the HTTP context, which makes it harder to test and violates layer separation. - Better: extract the IP string in the controller, pass it as a plain `String ipAddress` parameter. The service stays portable and testable. **On the `inet` column type:** - PostgreSQL's `inet` type validates IP format. If the value coming in is malformed (e.g., a spoofed multi-value `X-Forwarded-For`), the insert will throw. Make sure we sanitize/truncate to just the first valid IP before storing, or catch the constraint violation gracefully. Questions: - Is the app deployed behind a reverse proxy in production? This determines the entire strategy. - Is `AdminAuditLog` a JPA entity mapped to `inet` as a `String`? If so, does Hibernate handle the `inet` → `varchar` cast cleanly, or do we need a custom type converter?

marcel commented

2026-04-03 09:25:49 +02:00

🧪 QA Engineer

Good find — null IP addresses in the audit log are a silent data quality problem that's easy to miss until you need the data in an incident. Here's the test coverage I'd want to see:

Unit tests (AdminService):

shouldSaveAuditLogWithIpAddressWhenDirectlyConnected() — IP extracted from request, stored correctly
shouldSaveAuditLogWithClientIpWhenBehindReverseProxy() — X-Forwarded-For value stored (first IP only)
shouldSaveAuditLogWithNullIpWhenRequestIsNull() — defensive: service doesn't blow up if IP is unavailable

Integration tests (AdminController + DB):

After each admin action (promote user, revoke, etc.), verify the admin_audit_log row has a non-null ip_address
Test with a mock request that has a valid X-Forwarded-For header and verify the stored value matches the client IP, not the proxy IP
Test with a malformed X-Forwarded-For (e.g., "not-an-ip, 1.2.3.4") — assert it doesn't throw a 500; either sanitize gracefully or store null

Regression check:

All existing AdminService unit tests still pass after the signature change — if HttpServletRequest is extracted in the controller and passed as a String, the service tests should become simpler, not more complex

Edge cases I'm thinking about:

IPv6 addresses — does the inet column and the Hibernate mapping handle them correctly?
What happens when the same admin performs multiple actions in quick succession — do all rows get distinct, correct IPs?
Existing null rows in production: will there be a migration strategy, or is "null for old entries" acceptable and handled in the UI?

Questions:

Which AdminService methods at lines 66–68, 111–112, 130–131 are affected? Knowing the action types helps me enumerate test scenarios.
Is there a coverage threshold configured in CI that this change needs to maintain or improve?

## 🧪 QA Engineer Good find — null IP addresses in the audit log are a silent data quality problem that's easy to miss until you need the data in an incident. Here's the test coverage I'd want to see: **Unit tests (AdminService):** - `shouldSaveAuditLogWithIpAddressWhenDirectlyConnected()` — IP extracted from request, stored correctly - `shouldSaveAuditLogWithClientIpWhenBehindReverseProxy()` — `X-Forwarded-For` value stored (first IP only) - `shouldSaveAuditLogWithNullIpWhenRequestIsNull()` — defensive: service doesn't blow up if IP is unavailable **Integration tests (AdminController + DB):** - After each admin action (promote user, revoke, etc.), verify the `admin_audit_log` row has a non-null `ip_address` - Test with a mock request that has a valid `X-Forwarded-For` header and verify the stored value matches the client IP, not the proxy IP - Test with a malformed `X-Forwarded-For` (e.g., `"not-an-ip, 1.2.3.4"`) — assert it doesn't throw a 500; either sanitize gracefully or store null **Regression check:** - All existing `AdminService` unit tests still pass after the signature change — if `HttpServletRequest` is extracted in the controller and passed as a `String`, the service tests should become *simpler*, not more complex **Edge cases I'm thinking about:** - IPv6 addresses — does the `inet` column and the Hibernate mapping handle them correctly? - What happens when the same admin performs multiple actions in quick succession — do all rows get distinct, correct IPs? - Existing null rows in production: will there be a migration strategy, or is "null for old entries" acceptable and handled in the UI? Questions: - Which `AdminService` methods at lines 66–68, 111–112, 130–131 are affected? Knowing the action types helps me enumerate test scenarios. - Is there a coverage threshold configured in CI that this change needs to maintain or improve?

marcel commented

2026-04-03 09:26:07 +02:00

🔐 Sable — Security Engineer

This is a legitimate forensic gap — admin actions without a source IP are nearly useless in an incident response scenario. But the fix itself introduces a security surface I want to flag explicitly:

X-Forwarded-For header spoofing:

This header is client-controlled. If the app reads it directly without a trusted proxy configuration, an attacker can set X-Forwarded-For: 1.2.3.4 to any value and log a fake IP.
The fix must only trust X-Forwarded-For when the actual network source (getRemoteAddr()) is a known, trusted proxy.
Spring's ForwardedHeaderFilter with RelativeRedirectFilter configuration handles this correctly — do not hand-roll the header extraction.

IP logging and data minimization:

Logging IP addresses is personal data under GDPR in many jurisdictions. If this app is EU-facing, we need to confirm: is IP logging appropriate, and what is the retention policy for admin_audit_log rows?
Consider whether we want to log full IPs or only a masked version (e.g., last octet zeroed) depending on privacy requirements.

Audit log integrity:

This issue only addresses the IP field. As a reminder: the admin_audit_log must remain append-only. The fix should not introduce any UPDATE path to existing rows. Confirm the repository layer has no UPDATE/DELETE methods for this entity.

Sensitive data in logs:

When logging the "Unexpected error" to the server log (e.g., log.error), confirm we're not inadvertently logging the full request including session cookies or request bodies.

Questions:

Do we have a deployment topology diagram showing whether a proxy sits in front of the app? This is the critical input for how to safely extract the real client IP.
Is there an existing ForwardedHeaderFilter bean registered, or will this be new infrastructure?
Has anyone assessed GDPR implications of storing admin action IPs? Even for admin users, IP is personal data.

## 🔐 Sable — Security Engineer This is a legitimate forensic gap — admin actions without a source IP are nearly useless in an incident response scenario. But the fix itself introduces a security surface I want to flag explicitly: **`X-Forwarded-For` header spoofing:** - This header is client-controlled. If the app reads it directly without a trusted proxy configuration, an attacker can set `X-Forwarded-For: 1.2.3.4` to any value and log a fake IP. - The fix must only trust `X-Forwarded-For` when the actual network source (`getRemoteAddr()`) is a known, trusted proxy. - Spring's `ForwardedHeaderFilter` with `RelativeRedirectFilter` configuration handles this correctly — do not hand-roll the header extraction. **IP logging and data minimization:** - Logging IP addresses is personal data under GDPR in many jurisdictions. If this app is EU-facing, we need to confirm: is IP logging appropriate, and what is the retention policy for `admin_audit_log` rows? - Consider whether we want to log full IPs or only a masked version (e.g., last octet zeroed) depending on privacy requirements. **Audit log integrity:** - This issue only addresses the IP field. As a reminder: the `admin_audit_log` must remain append-only. The fix should not introduce any UPDATE path to existing rows. Confirm the repository layer has no UPDATE/DELETE methods for this entity. **Sensitive data in logs:** - When logging the "Unexpected error" to the server log (e.g., `log.error`), confirm we're not inadvertently logging the full request including session cookies or request bodies. Questions: - Do we have a deployment topology diagram showing whether a proxy sits in front of the app? This is the critical input for how to safely extract the real client IP. - Is there an existing `ForwardedHeaderFilter` bean registered, or will this be new infrastructure? - Has anyone assessed GDPR implications of storing admin action IPs? Even for admin users, IP is personal data.

marcel commented

2026-04-03 09:26:24 +02:00

🎨 Atlas — UI/UX Designer

This is a backend data capture fix with no immediate UI impact, but it's worth thinking about how the audit log data will eventually be presented — because that shapes what we need from the API.

Future admin audit log view (whenever it's built):

IP addresses are technical data. When we design the admin audit log table screen, IPs should appear in DM Mono (our monospace token font) to make them scannable — consistent with how we'd treat timestamps, UUIDs, and other structured technical strings.
Older entries with null IP will need a graceful fallback display: a dash (—) or an "Unknown" label in --color-subtle rather than an empty cell, which looks broken.

Data display considerations:

IPv4 (192.168.1.1) and IPv6 (2001:db8::1) have very different display widths. The column in the admin table will need enough space for IPv6 without wrapping — or we use a truncate + tooltip pattern for long values.
If the audit log is filterable by IP in the future, that's a search/filter UI component we haven't designed yet. Worth flagging as a future scope item rather than designing it now.

No design action needed for this issue — but please ensure the API response DTO includes ipAddress as a nullable string field so the frontend component can handle it without a breaking schema change later.

Questions:

Is the admin audit log view in scope for v1, or is this data only accessible directly in the database for now?
If it is v1, which screen ID would it belong to — is there an E-series admin screen planned for this?

## 🎨 Atlas — UI/UX Designer This is a backend data capture fix with no immediate UI impact, but it's worth thinking about how the audit log data will eventually be presented — because that shapes what we need from the API. **Future admin audit log view (whenever it's built):** - IP addresses are technical data. When we design the admin audit log table screen, IPs should appear in `DM Mono` (our monospace token font) to make them scannable — consistent with how we'd treat timestamps, UUIDs, and other structured technical strings. - Older entries with `null` IP will need a graceful fallback display: a dash (`—`) or an "Unknown" label in `--color-subtle` rather than an empty cell, which looks broken. **Data display considerations:** - IPv4 (`192.168.1.1`) and IPv6 (`2001:db8::1`) have very different display widths. The column in the admin table will need enough space for IPv6 without wrapping — or we use a truncate + tooltip pattern for long values. - If the audit log is filterable by IP in the future, that's a search/filter UI component we haven't designed yet. Worth flagging as a future scope item rather than designing it now. **No design action needed for this issue** — but please ensure the API response DTO includes `ipAddress` as a nullable string field so the frontend component can handle it without a breaking schema change later. Questions: - Is the admin audit log view in scope for v1, or is this data only accessible directly in the database for now? - If it is v1, which screen ID would it belong to — is there an E-series admin screen planned for this?

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/mealprep#9