Repository navigation
feat(standard-tests): lint airbyte_secret marking and secret leaks - #1219
Conversation
Add the first slice of the credential-free spec lint from airbytehq/airbyte-internal-issues#17136. - `test_docker_image_build_and_spec` and `SourceTestSuiteBase.test_spec` now require exactly one SPEC message and fail when a property whose name denotes a secret (`password`, `api_key`, `*_token`, ...) is not marked `airbyte_secret: true`, when a boolean/object property is marked secret, or when any `airbyte_secret` value is not a boolean (the CDK's secret filter ignores a string `"true"`). `airbyte-cdk image test` builds with `no_verify=True`, so the Docker spec test is the only place a Python connector's spec gets checked. - `test_docker_image_build_and_check` and both in-process `test_check` tests fail when a non-CONTROL message contains an `airbyte_secret` value from the scenario's config. The excerpt in the failure message is masked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@tolik0/cdk/spec-lint#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch tolik0/cdk/spec-lintPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Full)5 273 tests 5 261 ✅ 12m 51s ⏱️ Results for commit 4d000bf. ♻️ This comment has been updated with latest results. |
…xcerpt The leak report printed a masked excerpt of the message that held a secret. Masking replaced only the literal value of each secret, so a second secret in the same message passed through whenever it appeared in another form: a PEM key or service-account JSON inside a json.dumps(config), a repr, or a URL. The failure text reaches the JUnit XML that CI publishes, outside the runner's log masking. The report now holds no text from the output at all. Each line names the config paths of the secrets found and where they were found: the message number, its type and its stream. The stream name is left out when it overlaps any secret. Detection also searches the JSON-escaped, repr-escaped and URL-encoded forms of each secret and each line of a multi-line secret, so a key that only appears escaped is still caught. The secrets now come from a local walker that also descends into array items and every anyOf/allOf/oneOf variant. get_secrets, which the runtime log filter uses, skips those, so a per-account API key such as source-onesignal's applications[].app_api_key was never searched. The walker builds paths from spec property names and array indices only. get_secrets itself is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tions The leak check ran after the outcome and message-count assertions, and those build their failure text from raw messages: the last CONNECTION_STATUS, the error traces, or every message. A success scenario whose check failed with a token echoed in its status message therefore printed the token, and the leak check never ran. The Docker check test now runs `check` without raising, runs the leak check, and only then raises on errors and asserts the outcome. In-process, run_test_job takes an `inspect_output` callback that it calls before any of its own assertions; both test_check methods pass the leak check through it, so it runs whatever the outcome. The method that does this is renamed to `_assert_no_secrets_in_output`: it was a new public method on a base class that connectors subclass, and it shared its name with the module function it calls. EntrypointOutput gains an optional `stderr`, which run_docker_airbyte_command fills, and the leak check searches it line by line. In-process, a raw print() to stdout is still not captured; the docstring says which output each path can see. The new tests cover the in-process failing path for both suites. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cess keys Property names were only lowercased, so `clientSecret`, `accessToken` and `privateKey` never matched. They are now split into snake case first. No connector in the fleet has an unmarked camelCase secret today, so this adds no failures. The `access_key` suffix also matched the public half of a key pair, such as source-100ms `app_access_key`, which sits next to the marked `app_secret` and plays the role of an AWS access key ID. An `*access_key` with a sibling named `secret`, `*_secret` or `*secret_key` is now exempt. A lone `access_key` is still required to be secret, since many APIs take a single `access_key` parameter as their only credential. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test_docker_image_build_and_check ran an extra `docker run <image> spec` for every check scenario, only to read the secret paths for the leak check. _run_docker_spec is now cached per image and connector name, so the spec test and every check scenario share one run. The image under a tag does not change within a session, since each test builds it before it runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Thirteen credential properties across twelve connector specs were not marked `airbyte_secret: true`, so the platform stored and displayed them in plain text. destination-convex had the flag as the string "true", which the CDK does not treat as a secret. These are the fleet violations of the spec secret-field lint added in airbytehq/airbyte-python-cdk#1219, and must land before that CDK release. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Daryna Ishchenko (darynaishchenko)
left a comment
There was a problem hiding this comment.
Verdict: fix before merge — 0 P0 · 4 P1 · 6 P2 · 3 P3 (all inline).
Breaking change: BREAKING under criterion A.7 only — two new mandatory standard tests (spec marking lint, check secret-leak check) that every Python and manifest-only connector picks up automatically on the next release, because monorepo CI installs airbyte-cdk[dev] unpinned (poe-tasks/*-connector-tasks.toml). No runtime, API, manifest or state break; every signature change is additive and keyword-only. The call-out is in the body only (title is plain feat:, while sibling #1218 uses feat! for the same kind of gate). The rollout depends on three open monorepo PRs that this PR does not link: airbytehq/airbyte#88404, airbytehq/airbyte#88413, airbytehq/airbyte#88414 — together they cover all 12 connectors / 13 properties. Please link them under Risks and do not release this version before they merge (inline P1).
Done well
- Fleet homework before review: the marking lint was dry-run against every connector; I re-ran the head
_spec_lint.pyover all committed specs plus the OSS registry and got exactly the same 12 connectors / 13 properties, 0 certified. - The failure text cannot leak: pointers from spec property names and indices only, stream name dropped on any overlap, and the leak check runs before the outcome assertions that print raw messages — with tests proving the ordering on both paths.
- Additive, keyword-only API (
EntrypointOutput(stderr=),run_test_job(inspect_output=)); 90 hermetic unit tests with no CI-hiding markers; the Docker spec cache really does collapse the spec test and every check scenario into onedocker runwhenspecsucceeds.
Rollout / risk notes
Check: destination-motherduckis red here but identically red on #1217, #1218 and #1220 (MotherDuck token rejected at_assert_check_outcome, after the new leak check passed) — baseline, not this diff.Skipped Check: source-google-drivedid not run.- The leak check has only been dry-run with credential-free configs (27 searchable scenarios). With GSM credentials every secret becomes searchable; a
/prerelease+airbyte-cdk image testwith creds on a few certified connectors that build their own CONNECTION_STATUS would tell us before the fleet does. - Conflicts: #1220 conflicts in
docker_base.py(both add a spec-running helper after_assert_check_outcomeand both edit the import block); #1217 touchessource_base.pybut merges cleanly. Neither is in the body's conflict note (#1142/#1199 are).
`check_config_against_spec_or_exit` builds "Config validation error: ..." from jsonschema's message, which quotes the rejected value, and `AirbyteTracedException.as_connection_status_message` put it into the CONNECTION_STATUS without the secret filter. A secret that failed a `pattern`, `enum` or length check was printed in clear. - `as_connection_status_message` now passes the message through `filter_secrets`. - `AirbyteEntrypoint.check` emits a connector-raised config error as a sanitized TRACE and builds its CONNECTION_STATUS through the same method. - `Destination.run_cmd` registers the config's secrets with the filter before validating the config, as the source entrypoint already does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Marking lint: - Accept the pydantic-v2 optional secret (`airbyte_secret` next to `anyOf`): variants are no longer linted as the property, and a property typed only by its variants is checked against their types. - Report `airbyte_secret: true` under `anyOf`/`allOf` or a top-level combinator, where `get_secret_paths` cannot resolve it, and count a flag on a `oneOf` variant, which the filter does resolve. - Treat an explicit `airbyte_secret: false` as a reviewed non-secret, and say so in the failure text. Leak check: - Walk only the `oneOf` variant the config selects by its discriminators. - Stop yielding the field names of a secret object. - Search numbers emitted as JSON numbers. - Hide a stream name only when it overlaps a searched secret form. - Label a hit in the CDK's own "Config validation error" CONNECTION_STATUS. Harness: - Cache a failing Docker `spec` per image, and skip the leak check (Docker and in-process) when `spec` fails, since the spec tests report it. Tests cover each harness change the review found untested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ons.md Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Combines #1217, #1219 and #1220 with this branch: - test_discover keeps the section gate of this branch and runs the catalog checks from main; an empty catalog now fails the enforced 'catalog has streams' check, so its test expects that assertion. - test_check runs the secret leak check before asserting the check outcome. - cdk-migrations.md lists every section under one 7.36.0 heading. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
👉 TL;DR: The standard connector tests now fail when a connector's configuration form leaves a credential field unmasked, and when a connection check prints a secret from its config. Neither check needs real credentials, and both run on the path Python connectors use in CI today.
Specifically,
test_docker_image_build_and_specandSourceTestSuiteBase.test_speclintairbyte_secretmarking in the emitted spec, andtest_docker_image_build_and_checkplus both in-processtest_checktests assert that noairbyte_secretvalue from the scenario config appears in the output.Important
Release after the connector fixes. Do not release a CDK version with this change until these monorepo PRs are merged; together they fix all 12 connectors (13 properties) that the marking lint fails:
airbyte_secretairbyte#88404 (8 connectors)destination-kvdb)source-convertkit,source-mailjet-mail,source-n8n)Part of https://github.com/airbytehq/airbyte-internal-issues/issues/17136 (epic airbytehq/airbyte-internal-issues#17129). This is slice 1 of the issue's landing order: secret marking and leakage.
test_config_match_specand the structural lints follow in separate PRs.How
The checks hang off the Docker spec and check tests because
airbyte-cdk image test, the only standard-test path Python connectors run in monorepo CI, builds withno_verify=Trueand so never checks the spec at build time. The same assertions run in-process for manifest-only connectors. The marking lint walks the spec'sconnectionSpecificationand matches property names against a secret name list and suffixes. It follows the CDK's runtime secret filter (get_secret_paths/get_secrets), so a flag the lint accepts is one the runtime masks. The leak check runsspecto learn which config paths are secret, then searches every non-CONTROLmessage fromcheckfor those values.The leak check found a real CDK leak, fixed here: the CDK's own
Config validation errorCONNECTION_STATUS quoted the rejected value without the secret filter.Changes
AirbyteTracedException.as_connection_status_messagepasses its message throughfilter_secrets. jsonschema quotes the rejected value, so a secret that failed apattern,enumor length check was printed in clear in thecheckoutput.AirbyteEntrypoint.checknow also emits a connector-raised config error as a sanitized TRACE, andDestination.run_cmdregisters the config's secrets with the filter before validating, as the source entrypoint already did.airbyte_cdk/test/standard_tests/_spec_lint.py:find_secret_marking_errorswalksproperties,oneOf/anyOf/allOfanditems, and reports four cases:airbyte_secret: true, on itself or on aoneOfvariant;const) cannot hold a secret;airbyte_secretvalue, such as the string"true", which the CDK's secret filter ignores;airbyte_secret: trueunderanyOf,allOfor a top-level combinator, where the runtime filter cannot resolve it.Optional[str]secret (flag next toanyOf: [{type: string}, {type: null}]) passes: variants are not linted as the property, and a property typed only by its variants is checked against their types.airbyte_secret: falserecords a reviewed non-secret, such as a sortkey. The failure text says so. This lives in the spec rather than on the suite class because monorepo CI generates the suite class per connector.get_single_specasserts exactly one SPEC message and returns its spec.find_config_secretsreturns everyairbyte_secretvalue in a config with its config path. Unlikeget_secrets, it also walks arrayitemsandanyOf/allOfvariants. Of aoneOf, it walks only the variant the config selects by itsconst(or single-valueenum) discriminators, and all variants when none matches. It yields the values of a secret object, not its field names.find_leaked_secrets/assert_no_secrets_in_outputsearch every non-CONTROLmessage, numbers included, and stderr on the Docker path. The failure names the config path of each leaked secret and where it was found (message number, type, stream, or stderr line). It contains no text from the output. A hit in a CONNECTION_STATUS that starts withConfig validation error:is labeled as built by the CDK, with a hint to upgradeairbyte-cdkor fix the scenario config, since no connector code ran.DockerConnectorTestSuite.test_docker_image_build_and_specnow parses the image'sspecoutput, requires exactly one SPEC message, and lints it. Before this PR it only checked that the command did not error.DockerConnectorTestSuite.test_docker_image_build_and_checkruns the leak check before it raises on errors or asserts the outcome, because those assertions print raw messages. Thespecrun it needs is cached per image for the session, failures included, so there is no extradocker runper scenario. Ifspecfails, each check scenario is skipped with a reason; the spec test reports the failure once.SourceTestSuiteBase.test_specuses the same single-spec assertion and lint in-process.ConnectorTestSuiteBase.test_checkandSourceTestSuiteBase.test_checkrun the leak check through a newinspect_outputcallback onrun_test_job, which runs before any of the job's own assertions. The helper is private (_assert_no_secrets_in_output). It runs an extra in-processspecper check scenario, uncached becausecreate_connectortakes the scenario; if thatspecfails or emits other than one SPEC message, the check test is skipped with a reason.EntrypointOutputgains an optionalstderr, filled byrun_docker_airbyte_command.unit_tests/test/test_spec_lint.py, including Docker-path tests against a fakeddocker runand in-process tests against a fake source, plus masking tests intest_entrypoint.py,test_destination.pyandtest_traced_exception.py.cdk-migrations.md: a section on the two new checks and how to fix each failure.Recommended Review Order
airbyte_cdk/test/standard_tests/_spec_lint.pyairbyte_cdk/test/standard_tests/docker_base.pyairbyte_cdk/test/standard_tests/connector_base.pyairbyte_cdk/test/standard_tests/source_base.pyairbyte_cdk/test/standard_tests/_job_runner.pyairbyte_cdk/test/entrypoint_wrapper.pyandairbyte_cdk/utils/docker.pyairbyte_cdk/utils/traced_exception.py,airbyte_cdk/entrypoint.py,airbyte_cdk/destinations/destination.py(the masking fix)The decisions most likely to need discussion are the secret name list and suffixes (
SECRET_PROPERTY_NAMES,SECRET_PROPERTY_NAME_SUFFIXES), which decide which connectors go red, and the leak heuristic (_secret_variants,find_leaked_secrets), all in_spec_lint.py.Fleet impact: 12 community connectors go red on their next PR
Monorepo CI installs
airbyte-cdk[dev]unpinned, so these tests reach every connector on the next CDK release without opt-in. The marking lint was dry-run against every connector on airbytemaster(3ab60a0e19f, re-checked ona65948f9c42, and on239eafd614eafter the review changes, with the same result). It used the committed spec (manifest.yamlspec block, or the packagespec.json/spec.yaml), and the published OSS registry spec for Python connectors whose spec is built in code. Only Python and manifest-only connectors run these tests.No certified connector fails. The 12 failures cover 13 properties:
destination-convex(access_keyis marked with the string"true"),destination-kvdb(secret_key),destination-surrealdb(surrealdb_password),destination-typesense(api_key).source-convertkit(api_secret),source-keka(api_key),source-kisi(api_key),source-mailjet-mail(api_key),source-n8n(api_key),source-nexus-datasets(api_key,secret_key),source-shippo(shippo_token),source-track-pms(api_key).destination-kvdbis disabled in both registries, so it has no published spec, and the first dry run missed it. Its image cannot runspectoday either (the console script points at a missingdestination_kvdb.run), so it is already red; whoever fixes that will hit this lint next.source-100msapp_access_keyis no longer flagged: it sits next to the markedapp_secretand is the public half of that key pair.Each remaining finding is a credential that the platform shows in plain text in the UI and that the CDK does not filter from logs.
source-convertkit'sapi_secretis a hidden legacy field, but it still carries an API key. No connector fails the reverse check (marked secret on a type that cannot hold one), and the camelCase matching adds no failures.Risks
airbyte_secretairbyte#88404, fix(destination-kvdb): mark secret_key as airbyte_secret and fix the image entrypoint airbyte#88413 and fix(source-convertkit, source-mailjet-mail, source-n8n): mark credential fields as airbyte_secret airbyte#88414 are merged. They mark the 13 properties above in the 12 connectors listed; without them each of those connectors fails CI on its next unrelated change. A CDK major version would not shield anyone, since monorepo CI installs the CDK unpinned. #88404 and #88413 are approved; #88414 waits on refreshed GSM test secrets for its three connectors.:latestimages, through the realtest_docker_image_build_and_checkand, for manifest-only connectors, the in-processtest_check:source-gcs,source-s3,source-klaviyo,source-sentry,source-stripe(certified) andsource-recharge. Static API keys, AWS keys and a service-account key only; no rotating refresh tokens. 16 scenarios, 10 of them with a searchable secret: 0 leaks, and every scenario passed exceptsource-s3config_iam_role.json, whose role assumption cannot run outside CI (its secrets sit outside the spec, so the leak check has nothing to search there). Without credentials, only 27 of the 240 committedcheckscenarios carry a searchable secret.docker_base.py: both PRs add a spec-running helper after_assert_check_outcomeand both edit the import block. Whichever lands second should keep one helper that is both cached per image and deployment-mode aware.source_base.py(test_discoveronly) and merges cleanly there, but both PRs add a## Upgrading to the next minor release (unreleased)section to the top ofcdk-migrations.md; resolve by keeping one heading with both subsections.docker_base.pyimport block, a module-level helper next to feat(test): persist configs rotated during standard tests to secrets/updated_configurations #1199's_run_docker_airbyte_command_with_config_updates, and one string literal inconnector_base.py(feat(standard-tests): run each acceptance-test-config.yml config with its section's command, and assert the check status and basic_read records #1142). Each resolves by keeping both sides.Test plan
pytest unit_tests/test/test_spec_lint.py: 127 passed.pytest unit_tests/test unit_tests/test_entrypoint.py unit_tests/destinations unit_tests/utils: 607 passed. The fullunit_testsrun passes apart from two unstructured-file scenarios that need a local NLTK download and a timing-flaky JWT test that passes on rerun.raise_if_errors, raising it unconditionally, removing the in-processassert_spec_is_valid, droppingstderr=inrun_docker_airbyte_command) and removing each encoded search form. Thesource_pokeapi_w_components_pystandard suite: 7 passed, 1 skipped (the read test, skipped by--read-from-streams=false), including the real Docker build+spec and build+check tests.ruff format --check,ruff checkandmypyon the changed files: clean.airbyte-cdk image test --no-credsfrom this branch on real Python connectors from airbytemaster:source-rechargeandsource-okta(images built locally): spec andcheck[invalid_config]pass. Both invalid configs carry a searchable fake secret, so the leak check really ran.destination-typesensewith--image airbyte/destination-typesense:latest:test_docker_image_build_and_specfails with/properties/api_key` looks like a secret but is not marked `airbyte_secret: true`.. The image built from master cannot runspecat all (No module named 'destination_typesense.run', a known pre-existing break), so the published image was used.checkagainst the published:latestimage for all 27 searchable credential-free scenarios (24 manifest-only, 7 of them certified; 3 Python). All 27 returnedFAILEDas their configs expect, with 0 leaks and 0 errors.Follow-ups
Remaining slices of airbytehq/airbyte-internal-issues#17136, in the issue's landing order:
test_config_match_spec: validate every committed config against the connector's own spec.test_oneof_usage,test_enum_usage,test_forbidden_complex_types,test_array_type,test_object_not_empty,test_property_type_is_not_array,test_defined_refs_exist_in_json_spec_file,test_additional_properties_is_true.test_date_pattern,test_date_format,test_display_type,test_duplicate_order,test_nested_group): harness, QA checks, or drop.test_oauth_flow_parameters,test_oauth_is_default_method) and image metadata (test_image_labels,test_image_environment_variables).discoverandreadoutput.Design notes: secret names and the leak heuristic
Show/Hide Content
Opt-out. When a secret-looking name holds no secret, set
airbyte_secret: falseon the property. The UI treats it as unmarked, as before, and the explicit value makes the decision visible in review.Where the flag may sit.
get_secret_pathsdrops onlypropertiesandoneOffrom the schema path of a flag and looks the rest up in the config, starting at the top-levelproperties. So the flag works on the property and on aoneOfvariant of it, and does nothing underanyOf,allOfor a top-level combinator; the lint follows exactly that. A flag inside arrayitemsis not masked by the runtime either, but the lint does not report it, since an array of credentials has no other place to mark them. No connector in the fleet has a flag underanyOforallOftoday.Secret names. The exact-name list comes from CAT's
test_secret_is_properly_marked, with these changes.tenant_id,app_idandappidare removed. They identify an account or an app rather than authenticate it, and flagging them would push connectors to hide values that are not secret._token,_secret,password,api_key,apikey,secret_key,private_key,access_key). CAT matched exact names only, so it never flaggedapi_key, the most common secret property name in the fleet. Unmarkedapi_keyaccounts for 7 of the 13 findings.*access_keynext to a property namedsecret,*_secretor*secret_keyis exempt. It is the public half of a key pair, like an AWS access key ID. A loneaccess_keyis still flagged, because many APIs take it as their only credential.clientSecret,client-secretandClient_Secretall matchclient_secret.Only properties with a
typeare checked. An object such as acredentialsgroup cannot hold a secret itself, so only the fields inside it are checked.Leak check scope. CAT's
test_secret_never_in_the_outputwas an empty stub, so this is new coverage, not a port. The CDK log formatter and traced exceptions already mask registered secrets. ACONNECTION_STATUSmessage that the connector builds itself, raw stdout and stderr go through unfiltered, and that is the realistic leak class. On the Docker path all three are searched. In-process, only yielded messages and root-logger records are captured, so a rawprint()is visible only on the Docker path.CONTROLmessages are excluded because a connector config update carries the full config, secrets included, by design. Secrets under arrayitemsandanyOf/allOfare searched too, although the runtime log filter (get_secrets) still skips them; fixing that filter is a separate change. For aoneOf, only the variant the config selects is searched, so a value that is secret only in another variant is not reported.Heuristic. Only secret values of at least 8 characters that contain a digit are searched for. Shorter or word-like values (
123,client_secret,invalid_api_key) show up in ordinary output by coincidence: they would make the test flaky without catching anything. Most real credentials pass this filter; a purely alphabetic key is skipped. Each secret is searched in its literal, JSON-escaped,repr-escaped and URL-encoded forms, and line by line for a multi-line secret such as a PEM key.Failure text. The failure never quotes the output. It names the config path of each leaked secret (built from spec property names and array indices only) and where it was found, for example:
/credentials/client_secretin message #4 (CONNECTION_STATUS), or/api_keyin stderr line 12. A stream name is shown only when it overlaps no secret. Masking an excerpt cannot be made safe, because a second secret can appear in any encoding; with no excerpt, the test output cannot print a secret in any form. The leak check also runs before the outcome assertions, which print raw messages, so it sees every outcome.🤖 Generated with Claude Code