Skip to content

fix(pii): Prevent cycles in rule collection - #6344

Open
jjbayer wants to merge 8 commits into
masterfrom
fix/pii-recursion
Open

fix(pii): Prevent cycles in rule collection#6344
jjbayer wants to merge 8 commits into
masterfrom
fix/pii-recursion

Conversation

@jjbayer

@jjbayer jjbayer commented Sep 2, 2026

Copy link
Copy Markdown
Member

When collecting rules from a PII config, temporarily insert non-leaf nodes into the set of seen nodes to prevent cycles.

Fixes INGEST-1127

@jjbayer
jjbayer marked this pull request as ready for review September 2, 2026 10:15
@jjbayer
jjbayer requested a review from a team as a code owner September 2, 2026 10:15
@linear-code

linear-code Bot commented Sep 2, 2026

Copy link
Copy Markdown

INGEST-1127

Comment thread relay-pii/src/compiledconfig.rs
@jjbayer
jjbayer enabled auto-merge September 2, 2026 13:32
Comment thread relay-pii/src/compiledconfig.rs Outdated
@jjbayer
jjbayer disabled auto-merge September 2, 2026 13:49
Comment thread relay-pii/src/compiledconfig.rs Outdated
} else {
None
};
rules.insert(rule.clone()); // insert to break cycles

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to insert before the match and remove with a drop impl?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See mstange/pdb-addr2line#71 for some prior art for this, though it might be overkill.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up simplifying the entire implementation by adding a second set which keeps track of all seen nodes.

@jjbayer
jjbayer enabled auto-merge September 7, 2026 08:21
@jjbayer
jjbayer disabled auto-merge September 7, 2026 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants