Repository navigation
feat(standard-tests): run the secret leak check on discover and read output - #1267
Draft
Anatolii Yatsuk (tolik0) wants to merge 8 commits into
Draft
Anatolii Yatsuk (tolik0) wants to merge 8 commits into
Anatolii Yatsuk (tolik0) wants to merge 8 commits into
Conversation
…output The `airbyte_secret` leak check ran only for `check`. `test_discover`, `test_basic_read` and `test_fail_read_with_bad_catalog` (in-process), and the Docker discover and read tests, raised the connector's raw error traces, which can quote a request URL holding a config secret. - In-process, those tests pass the leak check to `run_test_job` as `inspect_output`, so it runs before any assertion prints raw messages. The spec it needs is run once per test, not once per command. - On the Docker path, `discover` and `read` run with `raise_if_errors=False`; the leak check runs first, then the errors are raised. - RECORD messages are no longer searched: records are data the platform never logs and the assertions never print, an API may return a value equal to a secret, and a read can emit many records. - `entrypoint_wrapper._run_command` masks config secrets in the traceback it prints for an unexpected error, as the CDK entrypoint does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…read output Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a test pytest's log capture formats records without the CDK's secret filter, so a failed standard test printed an HTTP log line with the request URL's secret in it. The leak check now records the secret forms it searched for, and the standard-tests plugin masks them in the captured log, stdout and stderr sections and the failure text. 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/leak-check-discover-read#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/leak-check-discover-readPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+400
to
+402
| filter_secrets( | ||
| "".join(traceback.format_exception(None, exception, exception.__traceback__)) | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
False positive: this print is the masking this PR adds. The traceback goes through filter_secrets before it is printed, as the CDK entrypoint does for an uncaught exception; CodeQL does not model filter_secrets as a sanitizer.
| @staticmethod | ||
| def read_config(config_path: str) -> MutableMapping[str, Any]: | ||
| config = Source.read_config(config_path) | ||
| logging.getLogger("airbyte").info("Loaded config for %s", _url_with(config["api_key"])) |
Contributor
Author
There was a problem hiding this comment.
False positive: this test fixture logs a fake key on purpose, so the tests can assert that the leak check reports it and that the pytest report masks it.
PyTest Results (Fast)6 511 tests +22 6 475 ✅ +22 7m 19s ⏱️ + 1m 35s Results for commit 5ffceb1. ± Comparison against base commit 43ce5ea. This pull request removes 2 and adds 24 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)6 514 tests +22 6 478 ✅ +22 11m 6s ⏱️ +13s Results for commit 5ffceb1. ± Comparison against base commit 43ce5ea. This pull request removes 2 and adds 24 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
A state message holds the position the platform stores, and a partition key can legitimately equal a secret-marked config value (source-twilio partitions by its `account_sid`). Skip STATE alongside CONTROL and RECORD in every test and drop the `skip_message_types` parameter and `FUTURE_STATE_READ_UNSCANNED_MESSAGES`, which only the future-state read used.
A skipped test's `longrepr` is a `(path, lineno, reason)` tuple that the terminal summary unpacks. Replacing it with a string made pytest exit with INTERNALERROR whenever a skip reason held a searched secret. Mask the reason inside the tuple instead, and say in the hook docstring that output printed live under `--capture=no` is not masked.
…-error masking The leak check entry now lists STATE among the unsearched messages with the partition-key reason, notes that the future-state tests have run the check since 7.46.0, says that `--capture=no` in the monorepo's pytest.ini leaves stdout and stderr unmasked, and reflects that the concurrent stream-error traces are filtered after 7.47.0.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Until now, the standard tests ran the
airbyte_secretleak check only forcheck.discoverandreadraised the connector's raw error traces, which can carry config secrets into public CI logs. For example,source-tiktok-marketing's test app gets a 403 fromoauth2/advertiser/get/, and the stream-error trace quotes the request URL withapp_idandsecretin the query.This PR runs the leak check on every
discoverandreadjob of the standard tests, before any assertion prints raw messages. It also masks the two other places that printed secrets during a test.In process (
source_base.py,connector_base.py): the check is passed torun_test_jobasinspect_output, so it runs before the outcome assertions. It covers:test_discovertest_basic_readtest_fail_read_with_bad_catalogThe future-state tests from #1265 (
test_read_from_future_stateand its Docker twin) already ran the check, with their ownskip_message_types=FUTURE_STATE_READ_UNSCANNED_MESSAGES. That parameter and constant are removed: every test now skips the same message types (see below).test_fail_read_with_invalid_configis left out, because its config holds no real values.Docker (
docker_base.py):test_docker_image_build_and_discover, and the discover and the read oftest_docker_image_build_and_read, now:raise_if_errors=False;raise_if_errors().The Docker
checkuses the same new helper.Printing paths masked:
entrypoint_wrapper._run_commandprinted the raw traceback of an unexpected exception to stdout. It now appliesfilter_secrets, as the CDK entrypoint does for an uncaught exception.-rA) printed lines like'GET' request to '…/oauth2/advertiser/get/?secret=<raw>&app_id=<raw>'in "Captured log call". The dry-run found 28 raw occurrences in one tiktoktest_basic_readlog.assert_no_secrets_in_outputnow records the secret forms it searched for: literal, JSON-escaped, repr-escaped and URL-encoded.pytest_runtest_makereporthookwrapper inpytest_hooks.pymasks those forms in the report sections and the failure text.(path, lineno, reason)report tuple, with the reason masked. Replacing it with a string made pytest exit with INTERNALERROR when a skip reason held a searched secret.log_cli) prints during the test, before the check; the monorepopytest.inidoes not turn it on. Thatpytest.inidoes set--capture=no, so stdout and stderr print live and are not masked, beyond the CDK's exact-match filter on the log lines the CDK formats.Records and state are not searched
find_leaked_secretsnow skipsRECORDandSTATEmessages, as it skipsCONTROLmessages:airbyte_secretthat a stream lists, and a partitioned stream keeps its partition key in its state:source-twiliopartitions by itsaccount_sid, which its spec marksairbyte_secret. Searching state turned twilio'stest_basic_readred with/account_sidfound only in STATE messages, while the raw value appeared nowhere in the printed output.Trace, log, connection-status and the other message types are searched, plus stderr on the Docker path. The same skip applies to
check, which emits neither records nor state. The lint tests that used a STATE message to carry a leak now use TRACE messages, and two tests pin the skips.Cost
specruns at most once per test in process. pytest builds a new suite instance per test, so the spec is cached on the instance._docker_spec_cache.specfails, the test is skipped, ascheckalready was, and the spec tests report the failure.Mutation pinning
Each guard was reverted or flipped in place, and
unit_tests/testwas run each time. All 21 mutations were killed. M1 to M18 ran beforemainwas merged in; M1 and M19 to M21 ran on the current head.test_discovertest_basic_readtest_basic_readtest_fail_read_with_bad_catalograise_if_errors()raise_if_errors()on the Docker read test's discoverraise_if_errors()on the Docker read test's readfilter_secretson the traceback that_run_commandprintsunit_tests/test: 1159 passed.ruff format,ruff checkandmypy airbyte_cdk/testare clean.Fleet dry-run
The monorepo CI installs the CDK unpinned, so this reaches every connector on release. The dry-run used
airbyte-cdk connector testfrom this branch with real secrets (airbyte-ops secrets fetch) on five certified manifest-only connectors. Every config used holds at least one searchable secret. Python connectors were not run.checkfails forconfig_oauth(in process and Docker), andtest_basic_readgets no records from a stream.test_basic_read2 passedtest_basic_read, idle on a network wait; the rerun took 3 min 45 s.test_basic_readfailed/credentials/app_idand/credentials/secretin stream-error TRACE messagestest_basic_read['config']only: passed, as it does in monorepo CI on the released CDK/account_sidfound in STATE messages only.tiktok is not newly red. Its
test_basic_readalready fails on main with the 403AirbyteEntrypointException, and it now fails with the leak report instead.The leaked traces come from the CDK itself.
concurrent_read_processor.py(lines 253 and 259) emits a stream error's trace withas_airbyte_message(), while the synchronousabstract_source.pyusesas_sanitized_airbyte_message(). That trace reaches production sync logs unfiltered. A local patch switching those two calls to the sanitized form made tiktok's leak report go away, and the test went back to failing on the 403.#1266 has since fixed the concurrent read: it masks config secrets in those traces. It is merged into this branch and is not released yet. The
cdk-migrations.mdentry names tiktok and the cause.Sibling PRs
main(with #1264, #1265 and #1266) is merged into this branch.git merge-treewas run against each open PR that touches these files:test_basic_read): merges cleanly with main, but conflicts with this branch insource_base.py(thetest_basic_readdocstring, and therun_test_jobcall, where this branch addsinspect_output=and feat(standard-tests): check the cursor state of declarative sources in test_basic_read #1270 installs itsCursorStateRecorder) and incdk-migrations.md. Resolve by keeping both sides; the read must keep both the recorder andinspect_output._job_runner.pyanddocker_base.py. This branch adds twodocker_base.pyhunks to that conflict:raise_if_errors=Falseversus feat(test): persist configs rotated during standard tests to secrets/updated_configurations #1199'sscenario=,connector_root=andtemp_config_file=arguments. Resolve by keeping both sides.__init__.pyandcdk-migrations.md, as it already does against main. For the migration-guide hunks, keep both entries.cdk-migrations.mdonly. Keep both entries.entrypoint.py,http_client.py,cdk-migrations.md) already exist against main.No merge order is required.
Public logs
I checked the "Test source-tiktok-marketing Connector" job logs of monorepo PRs 90787, 85316, 81416, 76231 and 84446. Each has one
secret=occurrence, and each showssecret=****, masked by the CDK runtime filter and by the CI masks thatairbyte-ops secrets fetch --print-ci-secrets-masksregisters. Both of those mask exact matches only, so encoded forms, keys missing from the mask list, local runs and agent session logs were still exposed. This PR closes that gap in the test harness.🤖 Generated with Claude Code