fix(pii): never echo original PII and generate guaranteed-invalid dummies by rhiannalitchfield · Pull Request #594 · dataiku/kiji-proxy

rhiannalitchfield · 2026-07-14T14:45:50Z

Purpose

Part of #465. The lookup-first mapping reuse (step 1 of the issue) already landed in #468, so this PR covers the remaining generator-realism and test work.

Two gaps in replacement generation:

Nothing guaranteed a generated dummy differed from the original it was masking. Most list-based generators used pickExcluding, but StreetGenerator did not, and the format-based generators (email, phone, SSN, etc.) had no guard at all. A dummy equal to the original silently leaks the PII to the upstream provider.
The SSN, phone, and credit card generators could emit values that are plausibly real: SSNs in issued area ranges, dialable phone numbers, and card numbers that pass the Luhn checksum. A generated dummy could collide with a real person's data.

What Changed

GeneratorService.GenerateReplacement now guarantees the replacement differs from the original: the label's generator gets a few attempts, then it falls back to the deterministic [REDACTED_{label}_{hash}] placeholder, which embeds a hash of the original and can never equal it.
StreetGenerator uses pickExcluding like the other list generators.
Realistic-looking but guaranteed-invalid formats, as suggested in the issue:
- SSN: area number drawn from 900-999, which the SSA has never issued.
- Phone: exchange fixed to 555 with line numbers 0100-0199, the NANP block reserved for fictional use.
- Credit card: the final digit is chosen to always fail the Luhn checksum, so the number is never issuable.

Testing

New service-level test feeds each label's own output back in as the original (the most collision-prone input) and asserts GenerateReplacement never returns it, across all 26 labels plus the generic fallback.
New generator tests assert the 555-01XX phone range, the 900-999 SSN area range, Luhn failure for card numbers, and that StreetGenerator excludes the original.
All existing generator tests pass unchanged; golangci-lint reports 0 issues. Also includes a gofmt pass over pre-existing formatting drift in the test file this PR touches.

Changeset included (patch).

🤖 Generated with Claude Code

…mies Part of dataiku#465 (the lookup-first mapping half already landed in dataiku#468). - GenerateReplacement now guarantees the replacement differs from the original, falling back to the [REDACTED_...] placeholder after repeated generator collisions, so a dummy can never leak the PII it is meant to mask. - StreetGenerator uses pickExcluding like the other list generators. - SSNs use the never-issued 900-999 area range, phone numbers the reserved 555-0100..0199 fictional block, and credit card numbers deliberately fail the Luhn checksum, so dummies look realistic but can never collide with a real person's data. - Tests for the new invariants; gofmt pass over the touched test file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

rhiannalitchfield · 2026-07-14T14:50:59Z

A note on the failing "Check PR scope" job: I think this is a false positive worth a closer look, on two levels.

The changeset is misclassified as docs. In classify-pr-files.sh, the chore patterns include .changeset/* (line 37), but the case statement evaluates in order and the docs pattern *.md (line 34) comes first. Since changesets are all .md files, they always land in docs and the intended .changeset/* match is unreachable. That is what pushed this PR to 3 categories (code, test, docs).

Even with that fixed, compliant PRs would still trip the check. CONTRIBUTING requires code changes to ship with tests, and user-visible changes to ship with a changeset. So a PR that follows the checklist always touches code + test + changeset, which counts as 3 areas (code, test, chore) and fails. Splitting along those lines would produce a code PR with no tests, which the same guidelines forbid.

I would suggest the classifier treat *_test.go (and the other test patterns) plus .changeset/* as coupled to code rather than counting them as separate areas, so the check keeps catching genuinely mixed PRs (e.g. CI changes bundled with feature work) without flagging the compliant ones. Happy to open a small follow-up PR for that if you agree.

hanneshapke

Thank you for the great PR.

How would you extend the list of names, city and locations?

hanneshapke · 2026-07-16T02:07:26Z

@@ -0,0 +1,5 @@
+---
+"kiji-privacy-proxy": patch


@rhiannalitchfield Do you mind removing the changeset file from your PR. It will be set by the maintainers when we cut the next version.

Thank you!

Removed in 571d443. Good to know changesets are handled by the maintainers at release time, thank you!

hanneshapke · 2026-07-16T02:10:48Z


 	// Randomly choose format
-	formats := []string{"%d-%d-%d", "%d.%d.%d", "(%d) %d-%d"}
+	formats := []string{"%d-555-%04d", "%d.555.%04d", "(%d) 555-%04d"}


hanneshapke · 2026-07-16T02:13:02Z

+// drawn from 900-999, which the SSA has never issued and has reserved against
+// future issuance, so the output looks real but can never collide with an
+// actual person's SSN.
 func SSNGenerator(rng *rand.Rand, original string) string {


The solution works. If you want to make it even more realistic, avoid all prefixes shown in this list

https://www.ssa.gov/employer/ssnvs/highgroup.txt

These are all active US SSNs

Good pointer, and it sent me down a useful rabbit hole. I'd argue the 900-999 range already avoids every prefix in that list, with a stronger guarantee than consuming the list would give:

The high group list was frozen on June 25, 2011, the day the SSA switched to randomized assignment. Since then the SSA assigns from essentially all of 001-899 (excluding 000 and 666), so a prefix being absent from the list no longer means it is inactive. Generating from the list's complement within 001-899 would still produce SSNs that can belong to real people.

Every prefix that has ever been issued (and everything in the list) lies in 001-899. 900-999 is reserved against SSN issuance, so the current generator avoids 100% of the listed prefixes by construction, and stays correct as randomization fills in the rest of 001-899.

Your comment did surface one real gap though: IRS ITINs also start with 9 and use group digits 50-65, 70-88, 90-92, and 94-99, so a generated 9XX dummy could have matched a real ITIN. Fixed in 6d677d8 by constraining the group digits to 10-49, with a test asserting both ranges.

Wow, your comment is incredible insightful! Thank you!

I didn't know that the SSA changed to random numbers. In that light, going with the 9XX prefix makes a lot of sense. Thank you also for considering ITINs!

Requested in review: changesets for this repo are set by the maintainers when cutting the next version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review follow-up: 9XX numbers with group digits 50-65, 70-88, 90-92, or 94-99 are the IRS ITIN space, so a generated dummy could match a real person's ITIN. Constrain the group to 10-49 so the output avoids both issued SSN areas (900-999 was never assigned) and all ITIN ranges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

rhiannalitchfield · 2026-07-16T18:46:55Z

How would you extend the list of names, city and locations?

My preference would be to move the pools out of the inline Go arrays into data files embedded with embed.FS, roughly:

One file per label (firstnames.txt, surnames.txt, cities.txt, ...), one value per line, loaded once at startup into the same slices the generators use today. The generator code and pickExcluding behavior stay unchanged; only the data source moves.
That makes pool size a data problem instead of a code problem: expanding from ~80 names to thousands is a file edit that never touches Go, keeps pii_generators.go reviewable, and the binary stays self-contained since the files compile in.
It also opens the door to locale-aware pools later (cities.en.txt, cities.ja.txt, ...) with selection keyed off the detected language, which Improve PII generator: more realistic fake data and lookup existing mappings first #465 and the XLM-RoBERTa work (Switch to XLM-RoBERTa for Asian language support #457) would pair well with.

Issue #465 already floats the embed.FS idea, so I'd scope that as a follow-up PR rather than growing this one, if that works for you.

hanneshapke · 2026-07-16T18:59:31Z

+// drawn from 900-999, which the SSA has never issued and has reserved against
+// future issuance, so the output looks real but can never collide with an
+// actual person's SSN.
 func SSNGenerator(rng *rand.Rand, original string) string {


Wow, your comment is incredible insightful! Thank you!

I didn't know that the SSA changed to random numbers. In that light, going with the 9XX prefix makes a lot of sense. Thank you also for considering ITINs!

hanneshapke · 2026-07-16T19:00:44Z

Thank you @rhiannalitchfield for all your updates - this PR is looking good!

hanneshapke · 2026-07-16T19:01:56Z

@rhiannalitchfield I really like the thought towards "It also opens the door to locale-aware pools later"

rhiannalitchfield mentioned this pull request Jul 14, 2026

Improve PII generator: more realistic fake data and lookup existing mappings first #465

Open

rhiannalitchfield mentioned this pull request Jul 14, 2026

check-scope flags PRs that follow the CONTRIBUTING checklist (code + tests + changeset) #595

Open

hanneshapke requested changes Jul 16, 2026

View reviewed changes

rhiannalitchfield and others added 2 commits July 16, 2026 14:39

chore: drop changeset, maintainers add it at release time

571d443

Requested in review: changesets for this repo are set by the maintainers when cutting the next version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

hanneshapke approved these changes Jul 16, 2026

View reviewed changes

hanneshapke merged commit aad146d into dataiku:main Jul 16, 2026
8 checks passed

github-actions Bot locked and limited conversation to collaborators Jul 16, 2026

Uh oh!

Conversation

rhiannalitchfield commented Jul 14, 2026

Purpose

What Changed

Testing

Uh oh!

rhiannalitchfield commented Jul 14, 2026

Uh oh!

hanneshapke left a comment

Choose a reason for hiding this comment

Uh oh!

hanneshapke Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

rhiannalitchfield Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

hanneshapke Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

hanneshapke Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

rhiannalitchfield Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

hanneshapke Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

rhiannalitchfield commented Jul 16, 2026

Uh oh!

hanneshapke Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

hanneshapke commented Jul 16, 2026

Uh oh!

Uh oh!

hanneshapke commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants