Skip to content

Apply complete data masking rules to RDP - #1717

Open
racerxdl wants to merge 2 commits into
mainfrom
fix/rdp-custom-data-masking
Open

Apply complete data masking rules to RDP#1717
racerxdl wants to merge 2 commits into
mainfrom
fix/rdp-custom-data-masking

Conversation

@racerxdl

Copy link
Copy Markdown
Contributor

Description

Apply each RDP connection's complete Data Masking policy in the agent, including supported entities, custom regex/deny-list matchers, and per-rule score thresholds.

  • Forward the raw connection-scoped policy through SessionStarted, negotiate complete-policy support explicitly, and fail closed for incompatible agents.
  • Build request-scoped Presidio ad_hoc_recognizers for custom entities while preserving deterministic supported-entity and threshold semantics.
  • Bind capability advertisements to the originating WebSocket instance so a stale reconnect cannot authorize guard delegation to a replacement agent.

User-facing impact

RDP live masking now honors the supported and custom entities configured on the selected connection instead of using gateway-wide entity settings.

How to test

Automated

cd gateway
go test ./broker ./transport ./rdp

cd ../agentrs
cargo test piigate

Expected: all three Go packages pass; the Rust PII-gate suites pass, including complete-policy parsing, custom recognizer serialization, invalid-policy fail-closed behavior, and stale capability lifecycle coverage.

Local RDP flow

  1. Build and start the local stack:

    docker build --platform linux/arm64 -f Dockerfile.localbuild -t hoophq/hoop:local .
    docker build --platform linux/arm64 -f Dockerfile.agent-ocr --build-arg HOOP_IMAGE=hoophq/hoop:local -t hoophq/hoop-agent-ocr:local .
    scripts/dev/run-pii-demo.sh
  2. Assign an active Live Data Masking rule to an RDP connection. Select PERSON and DATE_TIME, then add this regex-only custom entity:

    {"name":"FIXTURE_REGEX","regex":"rdp-gold-fixture-[0-9]{4}","deny_list":[],"score":0.8}
  3. Display rdp-gold-fixture-2026, a person name, and a date on the target desktop, then open the connection's RDP web client.

Expected: the gateway logs piigate: agent-side guard active; a detection includes FIXTURE_REGEX; the matching fixture and date/name regions are blacked out while the session remains usable under the default redact policy.

Negative path

Connect through an older agent that does not advertise supports_pii_data_masking_rules, or pass malformed complete-policy metadata in the Rust resolver tests.

Expected: the gateway refuses delegation to the older agent, and malformed complete policy returns an error instead of falling back to partial entity metadata.

Verification performed

Go and Rust focused suites passed locally. A rebuilt local gateway/agent stack detected PERSON, DATE_TIME, and regex-only FIXTURE_REGEX; the matching RDP regions were visibly redacted. Agent violation persistence still exposes the pre-existing broker-session/database-session ID mismatch and is not changed by this PR.

Automated by MisterMal

🤖 Generated with Mister Maluco

Co-Authored-By: MisterMal <teskeslab@lucasteske.dev>
@racerxdl racerxdl added the patch Bumps the patch version on release (bug fixes) label Aug 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Migration Safety Analysis

No database migrations were changed in this PR. Safe to deploy to sandbox.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Apply complete connection Data Masking rules to agent-side RDP live masking

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Forward full connection Data Masking policy to the agent for RDP sessions.
• Negotiate complete-policy support and fail closed on incompatible agents.
• Prevent stale WebSocket capability frames from overwriting active agent capabilities.
Diagram

graph TD
  C["RDP Web Client"] --> G["Gateway RDP"] --> B["Broker"] --> A["Agent (piigate)"] --> P{{"Presidio Analyzer"}}
  A -. "Capabilities: supports_pii_data_masking_rules" .-> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Send derived params instead of raw rule payload
  • ➕ Smaller on-wire metadata; avoids sending unused rule fields
  • ➕ Gateway remains the single place validating rule JSON schema
  • ➖ Gateway/agent can drift on rule-combination semantics (threshold min, entity union)
  • ➖ Harder to support custom entities without persisting recognizers or expanding params schema
2. Introduce versioned policy envelope (schema version + capability)
  • ➕ Explicit evolution path for future rule fields
  • ➕ Clearer debugging than multiple ad-hoc capability keys
  • ➖ More protocol and rollout complexity than a single new capability key
  • ➖ Still needs strict fail-closed behavior and instance binding logic
3. Keep gateway-side realtime gate as fallback when agent lacks support
  • ➕ Maintains masking for older agents without refusing connections
  • ➖ Policy divergence risk (agent vs gateway implementations)
  • ➖ Duplicated OCR/Presidio cost and higher latency; contradicts single enforcement point

Recommendation: Current approach (forward raw connection-scoped rules + explicit supports_pii_data_masking_rules negotiation + fail-closed) is the best fit: it keeps policy semantics consistent across protocols, enables custom entities via request-scoped recognizers without persistent registration, and avoids duplicated gateway-side gating. If policy format is expected to evolve rapidly, consider adding a versioned policy envelope later.

Files changed (12) +463 / -248

Enhancement (5) +335 / -34
config.rsResolve complete Data Masking rules into Presidio analysis params +243/-8

Resolve complete Data Masking rules into Presidio analysis params

• Adds parsing/validation for data_masking_entity_data and converts supported/custom entity definitions into an entity allowlist plus request-scoped Presidio ad_hoc_recognizers. Implements fail-closed behavior on malformed complete-policy metadata while retaining legacy allowlist/denylist compatibility when complete rules are absent.

agentrs/src/piigate/config.rs

presidio.rsAdd request-scoped Presidio ad_hoc_recognizers support +65/-1

Add request-scoped Presidio ad_hoc_recognizers support

• Introduces AdHocRecognizer and AdHocRecognizerPattern structures and includes them in the Presidio /analyze request payload. Threads ad_hoc_recognizers through AnalysisParams and analyze_text so custom regex/deny-list entities can be enforced per request without persistent recognizer registration.

agentrs/src/piigate/presidio.rs

client.rsAdvertise complete Data Masking rule support in capabilities +13/-7

Advertise complete Data Masking rule support in capabilities

• Extends the agent capability frame to include supports_pii_data_masking_rules while keeping legacy supports_pii_entity_allowlist for older gateways. Also logs advertised capabilities using a single computed supports_guard value.

agentrs/src/ws/client.rs

headers.goDefine capability for complete Data Masking rule payload support +4/-3

Define capability for complete Data Masking rule payload support

• Replaces the older allowlist-focused capability constant with CapabilitySupportsPIIDataMaskingRules, documenting that it covers full rule payload support including custom regex and deny-list entities.

gateway/broker/headers.go

protocol_rdp.goSend raw Data Masking rule payload in RDP SessionStarted metadata +10/-15

Send raw Data Masking rule payload in RDP SessionStarted metadata

• Refactors RDPGuardConfig to carry DataMaskingEntityData (raw JSON) rather than derived threshold/allowlist fields. Updates session metadata emission to forward data_masking_entity_data when guarding is enabled.

gateway/broker/protocol_rdp.go

Bug fix (3) +23 / -50
session.goIgnore stale capability updates using agent instance ID +6/-7

Ignore stale capability updates using agent instance ID

• Changes SetAgentCapabilities to accept an instanceID and ignore updates when the ID does not match the currently registered agent connection. Preserves defensive copying and waiter unblocking semantics for the live connection.

gateway/broker/session.go

irongw.goFail closed unless agent supports complete rules; remove gateway gate path +13/-40

Fail closed unless agent supports complete rules; remove gateway gate path

• Switches RDP gating delegation checks to require supports_pii_data_masking_rules and rejects sessions when the agent cannot enforce the connection policy. Removes the gateway-side realtime PIIGate (hold-and-release) path when agent-side guard is active, ensuring a single enforcement point and avoiding duplicated OCR/Presidio work.

gateway/rdp/irongw.go

websocket.goPass agent instance ID through WebSocket frame handling for capabilities +4/-3

Pass agent instance ID through WebSocket frame handling for capabilities

• Threads the per-connection agentInstanceID into handleWebSocketMessage so capability control frames are attributed to the correct live WebSocket connection. Calls broker.SetAgentCapabilities with (agentName, agentInstanceID) to prevent late/stale frames from overwriting current state.

gateway/transport/websocket.go

Refactor (1) +24 / -135
piigate.goPreserve complete Data Masking policy for agent delegation and validate custom entities +24/-135

Preserve complete Data Masking policy for agent delegation and validate custom entities

• Updates rule parsing to validate both supported and custom entity definitions for RDP while no longer deriving an allowlist/threshold locally. Returns params containing the original raw rule JSON (DataMaskingEntityData) plus band padding, and removes the gateway-side realtime PIIGate implementation and persistence helpers from this file.

gateway/rdp/piigate.go

Tests (3) +81 / -29
analyze.rsUpdate analysis test params for ad-hoc recognizers +1/-0

Update analysis test params for ad-hoc recognizers

• Extends AnalysisParams initialization in tests to include the new ad_hoc_recognizers field so existing test scaffolding continues to compile and reflect the expanded request model.

agentrs/src/piigate/analyze.rs

agent_capabilities_test.goBind stored capabilities to agent instance ID and test stale frames +40/-11

Bind stored capabilities to agent instance ID and test stale frames

• Updates tests to pass the agent connection's instanceID into SetAgentCapabilities and adds coverage ensuring stale capability frames from replaced connections are ignored. Keeps existing behaviors around unknown/unregistered agents and defensive copying.

gateway/broker/agent_capabilities_test.go

piigate_test.goUpdate RDP masking tests to assert raw-policy preservation and custom entities +40/-18

Update RDP masking tests to assert raw-policy preservation and custom entities

• Reworks tests to ensure parseRDPDataMaskingRules preserves the full rule payload for the agent and accepts custom entity definitions, while adding negative cases for invalid custom entity names/scores and missing patterns. Adjusts threshold-related assertions to reflect that threshold is no longer computed in the gateway.

gateway/rdp/piigate_test.go

@github-actions

Copy link
Copy Markdown
Contributor

📋 API Changelog

API Changelog unknown vs. unknown

No changes detected

@qodo-code-review

qodo-code-review Bot commented Aug 14, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Missing custom score accepted ✓ Resolved 🐞 Bug ≡ Correctness
Description
In apply_data_masking_rules, a missing/null custom entity score is treated as 0.0, which can
silently prevent custom entities from meeting score_threshold and being detected/redacted. This
contradicts the gateway API/model contract where custom entity scores are required, and it
undermines the PR’s intended “invalid policy fails closed” behavior.
Code

agentrs/src/piigate/config.rs[R131-134]

+            let score = custom.score.unwrap_or(0.0);
+            if !(0.0..=1.0).contains(&score) || !score.is_finite() {
+                anyhow::bail!("custom entity {:?} has invalid score {score}", custom.name);
+            }
Relevance

●●● Strong

Team often prefers fail-fast over defaulting missing required fields; aligns with “invalid policy
fails closed”.

PR-#1362
PR-#1621

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The agent defaults missing custom-entity scores to 0.0, while the gateway’s data model and OpenAPI
types define custom entity scores as required; accepting missing scores is therefore
malformed-policy acceptance and can suppress detections under common thresholds.

agentrs/src/piigate/config.rs[120-136]
gateway/models/datamasking.go[37-42]
gateway/api/openapi/types.go[2299-2310]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`apply_data_masking_rules` currently accepts custom entities with a missing or explicit `null` `score` by defaulting to `0.0` (`unwrap_or(0.0)`). This can silently disable custom entity detections (and therefore masking) whenever the session/rule score threshold is above 0.0.

### Issue Context
Gateway models/API treat `custom_entity_types[].score` as required. This PR’s tests validate invalid scores and missing regex/deny_list, but do not cover missing score.

### Fix Focus Areas
- agentrs/src/piigate/config.rs[120-136]

### Proposed fix
- Change the custom entity parsing to *fail closed* when `custom.score` is missing or null:
 - Prefer making `score` non-optional in the deserialized struct, or
 - Keep it optional but replace `unwrap_or(0.0)` with an explicit error when `None`.
- Add a regression test in the existing `invalid_complete_data_masking_rules_fail_closed` table to cover a custom entity with missing `score` (and optionally `"score": null`).
- (Optional but recommended) Consider aligning the gateway-side JSON decoding/validation so malformed rules are rejected before a guarded RDP session is attempted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. IronRDPGateway.handle lacks Swagger block 📘 Rule violation ⚙ Maintainability
Description
The modified RDP proxy HTTP handler does not have Swagger/Swag annotations (e.g., @Summary,
@Router) attached to the handler declaration, so it will not be documented/validated by the
OpenAPI tooling. This violates the requirement that new or modified gateway handlers include Swagger
annotations.
Code

gateway/rdp/irongw.go[R259-262]

+	// the plaintext already flows. The gateway sends the complete resource
+	// policy while the agent supplies its local Presidio/OCR endpoints. Masked
+	// sessions fail closed unless the selected agent can enforce that policy.
	agentGuard := broker.RDPGuardConfig{Enabled: maskingParams != nil}
Relevance

● Weak

Very close precedent: request to add Swagger block above IronRDPGateway.handle was explicitly
rejected.

PR-#1711

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1800918 requires Swagger annotations on new or modified gateway API handlers. The
IronRDPGateway.handle handler is modified in this PR but has no // @... Swagger comment block
directly above its declaration.

Rule 1800918: Require Swagger annotations on new or modified gateway API handlers
gateway/rdp/irongw.go[147-175]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A modified gateway HTTP handler (`IronRDPGateway.handle`) is missing Swagger/Swag annotations (e.g., `// @Summary`, `// @Router ...`), which violates the requirement for annotated handlers.

## Issue Context
This handler is registered on the API server under the `/rdpproxy` route group, but there is no adjacent Swagger comment block above the handler function.

## Fix Focus Areas
- gateway/rdp/irongw.go[147-175]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
✅ Compliance rules (platform): 39 rules
✅ Cross-repo context

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread agentrs/src/piigate/config.rs Outdated
@sandromello

Copy link
Copy Markdown
Contributor

✅ Build Completed with Success, Version=1717.0.0-g0f3dc76

🤖 Generated with Mister Maluco

Co-Authored-By: MisterMal <teskeslab@lucasteske.dev>
@sandromello

Copy link
Copy Markdown
Contributor

✅ Build Completed with Success, Version=1717.0.0-g1fc5d3d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Bumps the patch version on release (bug fixes)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants