Repository navigation
security(secrets): wire credential redaction into the remaining 11 KB connectors (#13708 follow-up) #16985
Description
Activity
- added a commit that references this issue
on Sep 18, 2026 - added a commit that references this issue
on Sep 18, 2026 CodeRabbit's review of PR #16895 raised two related points, worth folding into this issue's scope rather than filing a duplicate (linked this issue as a native sub-issue of #13708 in the process):
Make
_ENTRY_POINTSbidirectional, not hand-appended. The current 2nd AC asks to extend the list to 12 entries by hand. That still lets a future 13th connector (or a new non-connector ingestion route) go unlisted and unredacted with the guard staying green — it only catches a regression on an entry already in the list. Prefer deriving the expected set programmatically (e.g. fromConnectorRegistry's registered connector classes that definefetch_content, plusupload_file_to_knowledge) and asserting exact equality against_ENTRY_POINTS, so both an omitted entry and a stale one fail the test.scan_content_for_credentials's_claim()overlap check is O(n²).autobot_shared/secret_redaction.py's generic high-entropy pass can accept thousands of spans on a large document (connectors permit up to 100MB), and each new candidate is checked against every previously-claimed span — real but low severity (perf only, not correctness). Worth a look once the connectors above are wired in and start feeding it real-sized documents; low priority relative to the redaction-wiring work itself.Verified against current code: confirmed 11 connector files (
audio_connector.py,confluence.py,database.py,external_adapter.py,file_server.py,gitlab.py[GitLabConnector+GiteaConnector],jira.py,nextcloud.py,notion.py,slack.py,web_crawler.py) definefetch_contentwith zeroredact_content/content_extractioncalls, matching this issue's list exactly — no change needed to the connector list itself.Owner ruling (2026-09-19): one choke point, tested per connector.
All 11 connectors store their content through
AbstractConnector._ingest_content→kb.store_fact(), which callssanitize_fact_content(knowledge/facts.py:777, the #16770 KB-write choke point). #16895, riding vehicle #17077, adds credential redaction there. Addingredact_content()to each connector as well would redact twice and create two competing rules for content entering the KB.Criterion 1 is replaced by: every listed connector's content reaches the
store_factredaction choke point, and no connector writes to the KB by any other path. This is proven per connector.Criteria 2–4 stand, adapted:
- the guard asserts that each entry point reaches the choke point;
- one functional test per connector drives the real
fetch_contentpath with a credential-shaped payload and asserts it is redacted in the stored fact; - a negative control shows ordinary content is not mangled.
The PR is #17016 (bb). It closes this issue only once every criterion is met with code evidence.
AC verification against merged main (post #17133 vehicle merge) — reopening, not closing
The literal ACs as written are not what got built. None of the 11 connectors call
redact_content()directly (confirmed: zero hits across nextcloud/file_server/database/gitlab/confluence/notion/jira/slack/web_crawler/audio_connector/external_adapter), andrepo_tests/ingestion_redaction_guard_test.py's_ENTRY_POINTSlist was not extended to 12.Instead, the fix was re-scoped — documented directly in the new
repo_tests/connector_redaction_guard_test.py's docstring: redaction moved to a single chokepoint,knowledge/ingest_sanitize.py:sanitize_fact_content, called by bothstore_factandupdate_fact. The docstring traces all 11 connectors and shows every one already reacheskb.store_factthrough some path (the standard_process_change→_ingest_contentroute, or a documented bypass that still lands in_ingest_content/_store_fact_in_kb), so redaction at that one point covers everything without a 12th per-connector call.What I can verify is real and solid:
connector_redaction_guard_test.py: discoversAbstractConnectorsubclasses by class hierarchy (not a hand list), checks each for the chokepoint, and has a negative control proving the override-detection can actually fail.connector_redaction_functional_test.py: one real end-to-end test per connector (fetch_content→_ingest_content→kb.store_fact→sanitize_fact_content), mocking only the narrowest external dependency — stronger than the AC asked (redaction verified at persistence, not just atfetch_content's return).test_negative_control_ordinary_content_is_unchangedcovers the "don't mangle ordinary content" requirement.
So the underlying security gap does appear closed, verifiably, and arguably more robustly than a 12-site patch would have been (a future 13th connector can't add its own redaction bypass without this guard catching it). But this is a real architectural re-scope from what the issue asked for, decided and executed without it being reflected back here before merge. Reopening so the re-scope gets an explicit yes/no rather than passing silently via auto-close — not because the security posture looks wrong, but because "the ACs as written" and "what shipped" are genuinely different documents.
Criteria rewritten and verified against merged
main, per the owner's decision to reconcile the re-scope rather than either rubber-stamp it or re-implement the 11-connector version.5 of 6 met with code evidence (quoted in the body above). The chokepoint design is not merely equivalent to what the original criteria described — on connector coverage it is stronger, because
connector_redaction_guard_test.pydiscovers connectors by import and subclass walk instead of relying on a hand-maintained list. A connector added next month is covered without anyone remembering to update anything.Staying open for one item, and deliberately not closed:
ingestion_redaction_guard_test.py's_ENTRY_POINTSstill lists three entries from the #16895 era. Nothing about it is incorrect, but under the chokepoint design it reads as a coverage claim of three entry points when actual coverage is every write path. Either extend it or retire it in favour of the discovery guards, with the reason recorded in its docstring.Worth stating plainly, since it is the reason this issue needed a decision at all: the shipped code was right and the paperwork was wrong. A closure that ticked the original four boxes would have recorded eleven per-connector edits that do not exist, and the next person reading
gdrive.pyfor the pattern would not have found it. The re-scope should have been reflected back into the issue before merge, not after.Chunking triage — a proposal, not an assignment
- Proposed priority:
priority: medium— not applied. - triage(v0.9.0): release criticality of the 123 open issues — 26 blocking, 36 small, 18 umbrellas, 41 misfiled, 2 undetermined #17639 criterion met: none of the five. Placed as scoped and small: one PR, no open question, not blocking.
- triage(v0.9.0): release criticality of the 123 open issues — 26 blocking, 36 small, 18 umbrellas, 41 misfiled, 2 undetermined #17639 bucket: 2 — scoped and small (one PR, no open question, not blocking)
- Umbrella / container: no.
- Pre-filter: ⚠ a merged commit references this issue —
c2bfb489b5 style(format): auto-format for current black pin. A reference is not a delivery: verify AC coverage before putting it in a chunk, and consider a closure pass first. - Basis: triage(v0.9.0): release criticality of the 123 open issues — 26 blocking, 36 small, 18 umbrellas, 41 misfiled, 2 undetermined #17639's per-issue row, reused rather than re-derived — "5 of 6 ACs met on main (chokepoint redaction and discovery guards). Only left: reconcile or retire the hand-enumerated _ENTRY_POINTS (3 entries) in repo_tests/ingestion_redaction_guard_test.py, with the reason in its doc"
Nothing was relabelled, moved or closed by this pass.
- Proposed priority:
Closure pass — the one open criterion is still open on current
main. Not closed; 5 of 6 stand ticked.Verified at
origin/main7adaca8c29, because the merged commitc2bfb489b5that names this issue is a formatting pass and does not touch the remaining criterion.The open AC is accurate as written.
repo_tests/ingestion_redaction_guard_test.py:35still holds exactly the three #16895-era entries:_ENTRY_POINTS = [ ("knowledge.connectors.gdrive", "GoogleDriveConnector.fetch_content"), ("knowledge.connectors.onedrive", "OneDriveConnector.fetch_content"), ("api.knowledge", "upload_file_to_knowledge"), ]
parametrised at
:64. So the criterion is unmet and the issue stays open on that alone.Two things worth recording while this is open, because they change how the fix should be written:
1. This guard is already right about the trap next door. Its
_calls_redact_contentdocstring says it parses withastrather than substring-matching, "so a comment or a docstring mentioningredact_content(in passing must not satisfy this" — and it checks the function's own source rather than the module's imports, so an entry point that imports the redactor and never calls it still fails. That is the same mention-is-not-implementation family as #17693, handled correctly here. Whatever replaces the hand-enumerated list must keep both properties.2. The hand-enumerated list is the declaration-pattern risk, not just a staleness nuisance. A three-entry allow-list cannot report on an entry point nobody added to it, so its passing says nothing about a fourth ingestion path. That is the shape recorded in #17649 (a detector whose population is its own spelling) and in #17246's lesson about a detector naming its inputs by allow-list. The fix is to derive the entry points from something the product changes — the ingestion registry or the chokepoint's own callers — so a new path arrives inside the guard's reach rather than outside it.
No code change proposed here and nothing relabelled. Recorded so the remaining work is about the right thing when someone takes it.
Closure evidence for the last open criterion, recorded after the fact. This issue was auto-closed by #17732's merge (
7da76edd68) with AC6 unticked; the tick belongs on the record.AC6 is met by rename plus assertion rather than by derivation:
_ENTRY_POINTS→_HISTORICAL_ENTRY_POINTSatrepo_tests/ingestion_redaction_guard_test.py:68, with:32-33recording why — "the list keeps its job and loses its implied scope…_ENTRY_POINTSread like the answer to 'which are the entry points'" — which is exactly this issue's finding (a three-entry list read as a coverage claim it never made).test_the_authoritative_guards_still_existkeeps the cross-reference from rotting silently.Also landed in the same PR, from review: the guard's own scope was wider than its claim —
ast.walkdescended into nested scopes, so a required test could be deleted, its name survive in a closure, and the guard stay green while pytest ran nothing. It now takes pytest's real collection surface (module-level functions plus class methods). Verification by the read-only audit session. Closure stands.
Problem
#13708 built a content-scanning credential redactor (
autobot_shared/secret_redaction.py'sredact_content()) and PR #16895 wired it into the two connectors a review flagged as live gaps:gdrive.py'sGoogleDriveConnector.fetch_contentandonedrive.py'sOneDriveConnector.fetch_content, each at the single point where every extraction branch already converges into onetext/contentvariable beforereturn ContentResult(...).The independent review of that PR spot-checked the rest of
knowledge/connectors/and found the same unguarded shape in every other connector'sfetch_content(or equivalent) method — content is decoded/extracted and returned with zero credential scanning:nextcloud.py—NextcloudConnector(WebDAV GET decodes bytes tocontent, returnsContentResultdirectly — spot-checked atnextcloud.py:156-206)file_server.pydatabase.pygitlab.py—GitLabConnectorandGiteaConnectorconfluence.pynotion.pyjira.pyslack.pyweb_crawler.pyaudio_connector.pyexternal_adapter.py—ExternalConnectorAdapterA credential synced or ingested through any of these reaches the KB / embedding pipeline unmasked, the same vulnerability class #13708 and #16895 closed for Drive/OneDrive.
Acceptance criteria
Rewritten 2026-09-20 to describe the design that shipped, on the owner's decision. The
criteria below replace four that specified adding
redact_content()to each of 11 connectorsindividually. What shipped instead is a single chokepoint, which closes the same gap by
construction rather than by 11 correct repetitions. The original wording is preserved in the
edit history; the re-scope was never agreed before merge, which is why it is being reconciled
here rather than quietly ticked.
per connector —
sanitize_fact_content(autobot-backend/knowledge/ingest_sanitize.py:96),which calls
redact_contentat line 122. Production callers areknowledge/facts.pyandknowledge/versioning.py.which is the whole safety argument for a single point:
repo_tests/store_fact_chokepoint_guard_test.py::test_no_writer_reaches_the_store_around_the_chokepoint,plus
::test_the_chokepoint_still_sanitizes_what_it_storesso the chokepoint cannot behollowed out while the bypass guard still passes.
repo_tests/kb_content_redaction_chokepoint_guard_test.pycovers the same property at thesink level (
hset/lpush/upsertcalls writing acontentfield).the hand-maintained list the original criterion asked for, because a connector added
tomorrow is covered without anyone remembering to add it.
repo_tests/connector_redaction_guard_test.pyimports every*.pydirectly underknowledge/connectors/, walks every transitive subclass, and checks which define their own_ingest_content.autobot-backend/knowledge/connectors/connector_redaction_functional_test.py, 12 tests(confluence, jira, notion, slack, gitlab, nextcloud, database, external_adapter,
web_crawler, audio and others).
connector_redaction_functional_test.py:363 test_negative_control_ordinary_content_is_unchanged,with the same property asserted independently at other layers
(
store_fact_sanitize_16770_test.py:85,knowledge_upload_redaction_test.py:52,bulk_restore_redaction_test.py:81).repo_tests/ingestion_redaction_guard_test.py'shand-enumerated
_ENTRY_POINTSstill lists exactly three entries (gdrive.fetch_content,onedrive.fetch_content,api.knowledge.upload_file_to_knowledge) from the security(secrets): content-scanning credential redactor -- catches a credential in free text (#13708) #16895 era. Itis not wrong, but under the chokepoint design it is misleading: a reader who finds it will
read coverage as three entry points when the real coverage is every write path. Either
extend it to match the discovery guards, or retire it in favour of them and say so in its
docstring. This is a legibility defect, not a security hole — the redaction itself is
enforced by the guards ticked above.
Related
Follow-up from #13708 / PR #16895's independent review. Not blocking that PR — the review found this out-of-scope for #13708's own literal file list, but flagged it as the same open security hole.