Skip to content

Fixes #34386: Recognize complete card, URL and IP spans - #34674

Queued
stout-yeoman wants to merge 14 commits into
mainfrom
stout-yeoman/34386-card-url-ip-spans
Queued

stout-yeoman wants to merge 14 commits into
mainfrom
stout-yeoman/34386-card-url-ip-spans

Conversation

@stout-yeoman

@stout-yeoman stout-yeoman commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes #34386

Detect complete credit-card, URL and IP candidates with spans anchored to the original input. Embedded cards previously disappeared or returned offsets into sanitized text; URL and IPv6 matches could return incomplete spans.

The card follow-up recovers complete compact, spaced or dashed cards after a calendar-valid YYYY-MM-DD date separated by whitespace, or a case-insensitive Qty/Quantity integer field. For example, 2024-01-15 4111111111111111 and Qty 2 4111 1111 1111 1111 recover the card span. Complete 19-digit candidates take priority. Both 4111111111111111 123 and 4111 1111 1111 1111 123 remain rejected. Existing Diners 4-6-4, repeated-space and separate-year behavior remains supported.

Type of change:

  • Bug fix

High-level design:

Configured Presidio adapters in presidio_utils.py validate complete bounded candidates without changing thresholds or classification policy.

  • Cards require supported groupings, consistent separators, issuer checks and a valid checksum, with successful spans mapped back to original text. An independent phone match no longer vetoes a supported card. Invalid complete candidates cannot salvage a valid shorter prefix. The entire remainder after an explicit date/quantity field must validate; arbitrary suffixes are not stripped. Existing recovery of a separate four-digit year in the 1900s/2000s is retained.
  • URL extraction includes path/query/fragment, validates authority/port, and handles surrounding punctuation. The 5,000-character bound and existing host eligibility remain; internal domains are separate (Existing email, URL and crypto recognizers omit internal domains and Ethereum addresses #34569).
  • IP extraction validates complete addresses with ipaddress, retains IPv6 boundaries, and validates port/CIDR suffixes while returning the address span.
  • Preserve configured recognizer identity/context. Persisted-label tests now assert configured CreditCardRecognizer identity, its actual stored ID, score and pattern evidence, matching the upstream attribution contract.

No schema or migration changes. Supported Luhn-valid card-shaped operational values still receive protective Sensitive tags. The numeric-prefix decision and rejected alternatives are recorded in docs/decisions/2026-10-15-card-prefix-recovery-requires-explicit-fields.md. Expiry-date recovery and additional URL/IP edge cases are outside this follow-up.

Tests:

Use cases covered

Exact original card/URL/IP spans, canonical and grouped cards (including Diners and 19 digits), consistent/repeated separators, invalid checksums and extra digits, phone collisions, explicit numeric prefixes and their negative controls, separate years, complete URL punctuation/authority/port handling, IPv4/IPv6/CIDR, and configured TagProcessor attribution.

Unit tests

At 512b065669e5889415ea46b631b9ed91624d9b97:

cd ingestion
../env/bin/python -m pytest tests/unit/pii tests/unit/metadata/pii --cov=metadata.pii.algorithms.presidio_utils --cov-branch --cov-report=term-missing -q

792 passed; 96% utility coverage including branches. Updated regression files: tests/unit/pii/algorithms/test_presidio_utils.py, test_presidio_recognizer_factory.py, and tests/unit/metadata/pii/test_tag_processor_integration.py. Focused red/green tests preceded the full run. Scoped Ruff/format and configured basedpyright passed (0 errors, 0 warnings); applicable pre-commit checks and git diff --check passed. Independent review found no actionable issues.

Frozen replay: 3,104 existing cases, zero changes to spans, scores, names or pattern evidence after the numeric-prefix follow-up. Earlier corrective replay covered English plus Spanish/Italian/Polish card cases and a held-back English supplement; those historical results are superseded for current unit coverage by the run above.

Backend integration tests

Not applicable: no Java, REST endpoint, schema, migration, seed or pom changes. python3 .github/scripts/plan_local_java_tests.py --check-branch passed; no affected Java tests.

Ingestion integration tests

Updated ingestion/tests/integration/auto_classification/databases/{init.sql,test_tag_processor.py}. Ran the complete affected database test file against real PostgreSQL and OpenMetadata: 2 passed at 512b065669. Used the normal repository fixtures/credentials with only the local server endpoint adapted to port 18585. Persisted labels verify configured ID/name, score 1.0 and matched patterns. The baseline reproduced the old subclass-name assertion failure before the correction.

The gated tests/integration/auto_classification/test_model_provisioning.py check reported 1 skipped, because no candidate wheel was supplied; packaging/model provisioning code is unchanged. No clean-wheel certification is claimed.

Playwright (UI) tests

Shared validation uses local combined commit d84bd7b9204de848334bce46f7e033ccefb6f804: the production union of exact published card head 512b065669 and identifier head 0cf68d9f51, plus32 local preservation/control tests. UI sources and lockfiles are unchanged by either PR. A matching freshly built backend, production UI bundle, real managed Airflow runner and isolated MySQL/Metabase/Kafka/schema-registry fixtures resolve the earlier missing-team-type/runner prerequisites.

The current combined impact planner selected 37 specs /616 tests. Full original execution against the Vite development UI completed 603 passed,13 failed,0 skipped. Original failures and traces are retained. Ten failures were traced to shared global settings interference/precondition conflicts (domain search, generic entity domain/glossary helpers and the logical test suite); three bulk-edit variants stalled before export in development mode.

Unchanged failed cases were rerun sequentially against the production UI with their normal settings preconditions. All13 original failing test IDs/projects now pass: Entity9/9 (including one extra control), Logical1/1, Domain3/3, Bulk3/3, plus setup2/2. Production bulk success supports the development lifecycle explanation. This is a full original run plus controlled reruns, not a claim that one uninterrupted616-case run was green. No assertion or product source was weakened. All six AutoPilot connectors and the real Incident workflow passed in the original run. A separate local tooling repair isolates global-setting writers and restores settings; it is outside both PII PRs.

Additional shared regression validation

  • 957 combined unit tests pass:925 from the production union plus32 local preservation/control cases. This is local combined coverage, separate from this PR's individual unit count above. PII coverage96%; scoped typecheck/format/lint checks pass.
  • 5 real database integration tests and 62 normal CLI persisted-column checks pass across the two exact individual heads and combined validation. Stored configured recognizer names/IDs/scores/patterns are checked.
  • 48,536 paired executions (12,134 each for baseline/card/identifier/combined) found no serious new combination regression. Reviewed removals are intended malformed/ambiguous candidate rejections or the requested Date-to-Card recovery. Rare pre-existing URL limitations remain deferred.
  • Live reclassification preserves original manual/generated/unrelated labels, adds Sensitive beside unrelated labels and avoids duplicates. Existing mutually exclusive PII labels retain the existing preservation behavior; this change does not automatically delete old applied tags or upgrade a pre-existing NonSensitive label.
  • Card/identifier and multilingual labels were checked in the actual UI, including reload/navigation persistence;18 live URL/IP/independent-email controls pass.
  • Scoped downstream Collate CLI/UI and processor contracts found no serious new regression; retained expectation differences were checked against the recorded baseline. Full current Collate integration remains incomplete because its generated schema overlay lacks TestCaseEvaluationScope; no full current-schema downstream certification is claimed.

Manual testing performed

  1. Started the designated local OpenMetadata/PostgreSQL/search test stack. Confirmed server version/revision; imported Python source and the normal metadata CLI point to the isolated checkout at 512b065669.
  2. Created a disposable PostgreSQL source with synthetic card values and controls; ran normal metadata ingest -c ... and metadata classify -c ... using the shipped TagProcessor.
  3. Queried persisted catalog columns: 16 checks passed, covering date/quantity prefixes, compact/spaced/dashed/Diners/repeated-space cards, complete 19-digit priority, separate years and ambiguous suffix rejection, with configured recognizer evidence.
  4. Removed the source and catalog service and verified deletion. Scoped Collate runtime/UI compatibility was subsequently checked as described above; full current-schema integration remains incomplete.

UI screen recording:

Not applicable: backend ingestion bug fixes, with no UI feature or UI source changes. The normal ingestion/classification CLI was exercised as described above.

Checklist:

  • Read the contributing guide.
  • PR title follows Fixes <issue-number>: <short explanation> and the issue is linked above.
  • Comments explain non-obvious validation and scoring behavior.
  • Added unit and ingestion integration tests for the exact regressions; results and limitations are listed above.
  • JSON schema/migrations: not applicable; no schema changes.
  • UI recording: not applicable for these backend ingestion bug fixes.
  • Decision check: explicit-prefix recovery contract recorded in the ADR above.

Merge coordination

This PR shares presidio_utils.py, database fixtures and processor tests with #34672. Merge sequentially, reconcile/rebase the remaining PR and rerun the combined fixture and configured TagProcessor checks. Complete internal-domain acceptance remains owned by #34569.

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

Comment thread ingestion/src/metadata/pii/algorithms/presidio_utils.py Outdated
Comment thread ingestion/src/metadata/pii/algorithms/presidio_utils.py Outdated
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

Comment thread ingestion/src/metadata/pii/algorithms/presidio_utils.py Outdated
@stout-yeoman stout-yeoman self-assigned this Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@stout-yeoman stout-yeoman added the safe to test Add this label to run secure Github workflows on PRs label Oct 8, 2026
@stout-yeoman
stout-yeoman marked this pull request as ready for review October 8, 2026 13:42
@stout-yeoman
stout-yeoman requested a review from a team as a code owner October 8, 2026 13:42
@stout-yeoman
stout-yeoman added this pull request to the merge queue Oct 13, 2026
Any commits made after this event will not be merged.
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — failed_checks (2026-10-14T01:51:31Z)

Blocked the queue: py-tests-status

  • Prune dead merge-queue refs — cancel-stale-runs
  • py-tests — py-tests-status, python / Verify Expected Jobs, python / Integration Tests (shard-2), python / Integration Tests (shard-2), python / Integration Tests (shard-2)
  • py-tests-postgres — python / Integration Tests (shard-2), python / Integration Tests (shard-2), python / Integration Tests (shard-2)

@akashverma0786
akashverma0786 added this pull request to the merge queue Oct 14, 2026
Any commits made after this event will not be merged.
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — failed_checks (2026-10-14T20:55:12Z)

Blocked the queue: py-tests-status, py-tests-status

  • Prune dead merge-queue refs — cancel-stale-runs
  • Postgresql PR Playwright E2E Tests — playwright / playwright-ci (chromium-06)
  • py-tests — py-tests-status, python / Verify Expected Jobs, python / Integration Tests (shard-2), python / Integration Tests (shard-2), python / Integration Tests (shard-2)
  • py-tests-postgres — py-tests-status, python / Verify Expected Jobs, python / Integration Tests (shard-2), python / Integration Tests (shard-2), python / Integration Tests (shard-2)

@TeddyCr
TeddyCr added this pull request to the merge queue Oct 14, 2026
Any commits made after this event will not be merged.
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 15, 2026
@github-actions github-actions Bot added the queue conflict Conflicted only with an entry ahead in the merge queue label Oct 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — merge_conflict (2026-10-15T06:32:31Z)

The entry left the queue before it was built, so no checks ran against it.

Applies cleanly to the latest main; it conflicted only with an entry ahead in the queue. Re-queue once that entry has merged or left the queue.

@github-actions github-actions Bot removed the queue conflict Conflicted only with an entry ahead in the merge queue label Oct 15, 2026
Comment thread ingestion/src/metadata/pii/algorithms/presidio_utils.py
@akashverma0786
akashverma0786 added this pull request to the merge queue Oct 15, 2026
Any commits made after this event will not be merged.
@akashverma0786
akashverma0786 removed this pull request from the merge queue due to a manual request Oct 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — manual (2026-10-15T07:35:20Z)

The entry left the queue before it was built, so no checks ran against it.

Comment thread ingestion/src/metadata/pii/algorithms/presidio_utils.py
@gitar-bot

gitar-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Code Review ✅ Approved 5 closed / 5 findings

🔴 High risk · Changing credit-card, URL, and IP recognition could misclassify catalog data, leaving sensitive values untagged or applying sensitive tags to operational identifiers.

Adds complete card, URL, and IP span detection to the Presidio PII recognizers, with exact original-text offsets and handling for grouped card formats and IPv6 boundaries. Resolved: IPs following non-CIDR paths outside http(s) URLs, IPv4 after key: prefixes, and quoted or bracketed bare-host URLs ending in . or ,.

Open: a valid card is still dropped when a non-card number sits beside it separated by a space or dash (e.g. 2024-01-15 4111111111111111 or 4111111111111111 12 25), so the embedded-card bug is not fully fixed. Also, the quantity-label lookup in _card_start_after_explicit_prefix rescans up to 5,000 characters per numeric candidate, which can be costly on large inputs and should check only the preceding token.

✅ 5 closed
✅ Bug: IP dropped when followed by a non-CIDR path outside http(s) URLs

📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:382-394
In IpRecognizer.analyze, if / follows the token and the 10 characters before the address don't match https?://, the code only accepts a /NN CIDR suffix. It also skips any token whose port was stripped (token_end != end). So common values like 10.0.0.5:8080/health, 192.168.1.1/index.html, ftp://10.0.0.1/file or smb://10.0.0.1/share produce no IP_ADDRESS result, even though the address is valid. The previous Presidio recognizer did detect these. This PR is meant to improve IP recall, and these are realistic values in log and config columns. Only reject the address when the suffix looks like a malformed CIDR (/ followed by digits that fail the prefix-length check, or a second /digits segment). Otherwise accept the address and leave the path alone.

✅ Edge Case: IPv4 after key: prefix (e.g. src_ip:10.0.0.1) is never matched

📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:358
The candidate regex uses the lookbehind (?<![\w:.%-]), and : is also in the start class. In client_ip:10.0.0.1, a match starting at : fails because p precedes it. A match starting at 1 fails because : precedes it, and every later position follows a digit or a dot. So key:value IPv4 values, which are common in logs, are no longer detected. Fix: if a token begins with a single : and is preceded by a word character, try again with the leading colon stripped. Alternatively, let a candidate start after : when the remaining token parses as an IPv4Address.

✅ Edge Case: Quoted/bracketed bare-host URLs ending in . or , are now dropped

📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:292-296 📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:305-314
Trailing ./, is now kept whenever the next character is >, " or '. If the URL has no path, e.g. He said "visit https://example.com." or 'example.com,', that punctuation ends up in the authority. urlsplit then gives the hostname example.com., which fails fullmatch(BASE_URL_REGEX, host) because the regex has to end on a TLD, so the URL is skipped completely. Before this change the punctuation was stripped and the URL was detected, so this loses detection rather than just widening a span. One fix is to keep trailing punctuation only when the parsed candidate has a path, query or fragment, and otherwise fall back to the stripped span.

Closed: Performance: Quantity-label lookup re-scans up to 5,000 chars per numeric candidate

📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:83 📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:294-296
_card_start_after_explicit_prefix runs _QUANTITY_LABEL.search(text, start - 5000, start) for every candidate that starts with digits + space and whose whole-candidate check failed. Because of the leading lookbehind, the regex engine tries a match at every position in the 5,000-char window. The $ anchor means only a label sitting right before start can ever match, so all those other attempts are wasted. Input with many short candidates such as 1 2,1 2,1 2,... (JSON arrays, CSV-like blobs) does about 5,000 match attempts per candidate. The cost grows with candidate count times 5,000, where a direct check would be constant per candidate. A fix is to walk back over whitespace from start and check only the token right before it.

Closed: Edge Case: Card is missed when another number sits next to it, separated by a space or dash

📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:77 📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:261-269 📄 ingestion/src/metadata/pii/algorithms/presidio_utils.py:295-309
_CARD_CANDIDATES (\d[\d -]*\d) puts every digit run joined by spaces or dashes into one candidate. In SanitizedCreditCardRecognizer.analyze that candidate has three ways to match: the whole candidate has a card shape, every whitespace-separated part has a card shape, or the only trailing part is a 19xx/20xx year. Common inputs fail all three, for example 2024-01-15 4111111111111111 (log timestamp before the card), Qty 2 4111 1111 1111 1111, 4111111111111111 12 25 (expiry after the card) or 4111111111111111 - 100. Each of these is one candidate that has no card shape as a whole and contains a part that is not card-shaped, so the valid card is dropped completely. This is the 'embedded card disappears' bug the PR sets out to fix. Fix: when the whole candidate fails, test each whitespace-separated part (or each maximal card-shaped run) by itself, and skip non-card-shaped neighbours instead of requiring all parts to be card-shaped.

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Not enabled

Set up

For more verification details, comment gitar display:verbose.

Options

Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@sonarqubecloud

Copy link
Copy Markdown

@stout-yeoman
stout-yeoman added this pull request to the merge queue Oct 15, 2026
Any commits made after this event will not be merged.

This branch has not been deployed

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

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Card, URL, and IP recognizers miss candidates or report incomplete spans

3 participants