fix(discovery): validate plugin callback resolves to client address - #1725
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughDiscovery validates callback host resolution against the request socket address. JWT validation no longer uses ChangesDiscovery address validation
Schema field type resolution
Integration container networking
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/build_test |
|
Workflow started at 8/7/2026, 2:42:14 PM. View Actions Run. |
|
CI build: |
![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
🤖 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 `@src/main/java/io/cryostat/discovery/Discovery.java`:
- Around line 878-893: Update getRemoteAddress so X-Forwarded-For is honored
only when the immediate socket peer matches a configured trusted proxy;
otherwise always return the socket peer address. Preserve the existing callback
validation flow in Discovery and ensure registerAgent cannot use an untrusted
client-supplied forwarded address.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c774a13-4d7f-4d27-8941-26622a36fadc
📒 Files selected for processing (3)
src/main/java/io/cryostat/discovery/Discovery.javasrc/main/webuisrc/test/java/io/cryostat/discovery/DiscoveryPluginTest.java
|
/build_test |
|
Workflow started at 8/7/2026, 5:10:19 PM. View Actions Run. |
|
No OpenAPI schema changes detected. No GraphQL schema changes detected. WebSocket notifications schema change detected: diff --git a/schema/notifications.yaml b/schema/notifications.yaml
index b263685..bce6681 100644
--- a/schema/notifications.yaml
+++ b/schema/notifications.yaml
@@ -1081,43 +1081,39 @@ components:
properties:
event:
type: object
properties:
kind:
type: object
description: Payload of type EventKind
serviceRef:
type: object
properties:
- id:
- type: integer
connectUrl:
type: object
description: Payload of type URI
alias:
type: string
jvmId:
type: string
labels:
type: object
additionalProperties: true
annotations:
type: object
properties:
platform:
type: object
additionalProperties: true
cryostat:
type: object
additionalProperties: true
- agent:
- type: boolean
jvmId:
type: string
required:
- meta
- message
TemplateDeleted:
name: TemplateDeleted
title: Template Deleted
summary: 'Notification: TemplateDeleted'
description: WebSocket notification for Template Deleted events
|
|
CI build: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
compose/auth_proxy.yml (1)
10-10: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrefer an IP or CIDR for proxy trust when available.
Line 10 uses the Docker hostname
auth. Quarkus supports hostnames, but performs DNS lookups for configured hostnames during requests and recommends IP or CIDR values instead. Use a stable proxy address or network CIDR if available. (quarkus.io)🤖 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 `@compose/auth_proxy.yml` at line 10, Update the QUARKUS_HTTP_PROXY_TRUSTED_PROXIES setting to use the auth proxy’s stable IP address or network CIDR instead of the Docker hostname “auth”, while preserving trust for the intended proxy.
🤖 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.
Nitpick comments:
In `@compose/auth_proxy.yml`:
- Line 10: Update the QUARKUS_HTTP_PROXY_TRUSTED_PROXIES setting to use the auth
proxy’s stable IP address or network CIDR instead of the Docker hostname “auth”,
while preserving trust for the intended proxy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4b57c77-27f8-4da0-bb4f-d0b31a87721a
📒 Files selected for processing (4)
compose/auth_proxy.ymlsrc/main/java/io/cryostat/discovery/Discovery.javasrc/main/java/io/cryostat/discovery/DiscoveryJwtValidator.javasrc/test/java/io/cryostat/discovery/DiscoveryPluginTest.java
💤 Files with no reviewable changes (2)
- src/main/java/io/cryostat/discovery/DiscoveryJwtValidator.java
- src/main/java/io/cryostat/discovery/Discovery.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/io/cryostat/discovery/DiscoveryPluginTest.java
|
@jtolentino1 ignore coderabbit's feedback on the compose auth proxy setup and using |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@schema-generator/src/main/java/io/cryostat/schema/PayloadTypeAnalyzer.java`:
- Around line 430-444: Update the contextualRecord lookup in PayloadTypeAnalyzer
to resolve the matching RecordDeclaration within the field’s lexical scope,
rather than selecting the first simple-name match across the compilation unit.
Use available symbol resolution or scope-aware candidate filtering so nested
records with identical names resolve to the declaration visible from the field,
and add a regression test covering same-named nested records.
In `@src/test/java/io/cryostat/resources/AgentApplicationResource.java`:
- Around line 89-90: Update the container configuration in
AgentApplicationResource to set CRYOSTAT_AGENT_WEBSERVER_HOST to 127.0.0.1 and
configure CRYOSTAT_AGENT_CALLBACK with http://127.0.0.1:%d/. Keep API writes
enabled and preserve the existing host network and readiness-wait behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 46f62fe3-c8cc-437e-b3bd-36036d47da7f
📒 Files selected for processing (3)
schema-generator/src/main/java/io/cryostat/schema/PayloadTypeAnalyzer.javasrc/main/java/io/cryostat/discovery/DiscoveryJwtValidator.javasrc/test/java/io/cryostat/resources/AgentApplicationResource.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/io/cryostat/discovery/DiscoveryJwtValidator.java
|
/build_test |
|
Workflow started at 8/10/2026, 4:33:32 PM. View Actions Run. |
|
No OpenAPI schema changes detected. No GraphQL schema changes detected. No WebSocket notifications schema changes detected. |
|
CI build: |
|
CI build: |
2 participants
Welcome to Cryostat! 👋
Before contributing, make sure you have:
mainbranch[chore, ci, docs, feat, fix, test]To recreate commits with GPG signature
git fetch upstream && git rebase --force --gpg-sign upstream/mainFixes: #1603
Description of the change:
Validates that a discovery plugin callback resolves to the registering client's address.
Motivation for the change:
Prevents plugins from registering callbacks that target Cryostat or another host.
How to manually test:
./smoketest.bash -O.X-Forwarded-For: 192.0.2.1.