importsdk: redact sensitive source params in outward-facing errors#67719
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/L
GMHDBJD
added
type/bugfix
|
Hi @GMHDBJD. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces outward-facing uses of raw source paths with a redacted value on Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/importsdk/file_scanner_test.go (1)
156-158: Prefer deriving the expected redacted source in test setup.At Line 157, hardcoding the masked URL duplicates redaction logic and can drift when redaction rules evolve.
♻️ Suggested refactor
- fs.sourcePath = "s3://bucket/path?access-key=ak&secret-access-key=sk&session-token=token" - fs.redactedSourcePath = "s3://bucket/path?access-key=xxxxxx&secret-access-key=xxxxxx&session-token=xxxxxx" + fs.sourcePath = "s3://bucket/path?access-key=ak&secret-access-key=sk&session-token=token" + fs.redactedSourcePath = redactSourcePath(fs.sourcePath)As per coding guidelines, "Code SHOULD remain maintainable for future readers with basic TiDB familiarity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/importsdk/file_scanner_test.go` around lines 156 - 158, Replace the hardcoded masked URL by deriving fs.redactedSourcePath from fs.sourcePath using the package's redaction utility instead of duplicating masking logic; specifically, set fs.redactedSourcePath = RedactSourcePath(fs.sourcePath) (or call the existing RedactCredentials/RedactSource helper present in the package) so the test uses the same redaction codepath as production and won't drift as rules change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/importsdk/file_scanner_test.go`:
- Around line 156-158: Replace the hardcoded masked URL by deriving
fs.redactedSourcePath from fs.sourcePath using the package's redaction utility
instead of duplicating masking logic; specifically, set fs.redactedSourcePath =
RedactSourcePath(fs.sourcePath) (or call the existing
RedactCredentials/RedactSource helper present in the package) so the test uses
the same redaction codepath as production and won't drift as rules change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32d2c6a2-eb3b-4000-9ce8-610353a04189
📒 Files selected for processing (2)
pkg/importsdk/file_scanner.gopkg/importsdk/file_scanner_test.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/importsdk/file_scanner_test.go (1)
131-136: Extract duplicated redaction assertions into a helper.Both new subtests repeat the same secret-masking checks; a small helper would keep this easier to maintain.
♻️ Optional refactor
+ assertSecretsRedacted := func(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + require.ErrorContains(t, err, "access-key=xxxxxx") + require.ErrorContains(t, err, "secret-access-key=xxxxxx") + require.ErrorContains(t, err, "session-token=xxxxxx") + require.NotContains(t, err.Error(), "access-key=ak") + require.NotContains(t, err.Error(), "secret-access-key=sk") + require.NotContains(t, err.Error(), "session-token=token") + } + t.Run("NewFileScannerRedactsSensitiveSourcePathInInitErrors", func(t *testing.T) { _, err := NewFileScanner( ctx, "s3://?access-key=ak&secret-access-key=sk&session-token=token", db, cfg, ) - require.Error(t, err) - require.ErrorContains(t, err, "access-key=xxxxxx") - require.ErrorContains(t, err, "secret-access-key=xxxxxx") - require.ErrorContains(t, err, "session-token=xxxxxx") - require.NotContains(t, err.Error(), "access-key=ak") - require.NotContains(t, err.Error(), "secret-access-key=sk") - require.NotContains(t, err.Error(), "session-token=token") + assertSecretsRedacted(t, err) }) ... err = invalidScanner.CreateSchemasAndTables(ctx) - require.Error(t, err) - require.ErrorContains(t, err, "access-key=xxxxxx") - require.ErrorContains(t, err, "secret-access-key=xxxxxx") - require.ErrorContains(t, err, "session-token=xxxxxx") - require.NotContains(t, err.Error(), "access-key=ak") - require.NotContains(t, err.Error(), "secret-access-key=sk") - require.NotContains(t, err.Error(), "session-token=token") + assertSecretsRedacted(t, err) require.ErrorContains(t, err, "invalid schema statement") require.NoError(t, invalidMock.ExpectationsWereMet()) })Also applies to: 167-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/importsdk/file_scanner_test.go` around lines 131 - 136, Extract the repeated secret-masking assertions in pkg/importsdk/file_scanner_test.go into a small test helper (e.g., func assertSecretsRedacted(t *testing.T, err error)) and call it from the two subtests instead of duplicating the five require.* checks; update both the blocks that currently assert access-key/secret-access-key/session-token redaction (the block around the shown diff and the similar block at the later occurrence) to call this helper, preserving the same require.ErrorContains and require.NotContains checks inside the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/importsdk/file_scanner_test.go`:
- Around line 131-136: Extract the repeated secret-masking assertions in
pkg/importsdk/file_scanner_test.go into a small test helper (e.g., func
assertSecretsRedacted(t *testing.T, err error)) and call it from the two
subtests instead of duplicating the five require.* checks; update both the
blocks that currently assert access-key/secret-access-key/session-token
redaction (the block around the shown diff and the similar block at the later
occurrence) to call this helper, preserving the same require.ErrorContains and
require.NotContains checks inside the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 08bfbb8f-e187-445a-b5fb-54d227baf816
📒 Files selected for processing (3)
pkg/importsdk/BUILD.bazelpkg/importsdk/file_scanner.gopkg/importsdk/file_scanner_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/importsdk/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/importsdk/file_scanner.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67719 +/- ##
================================================
- Coverage 77.6233% 77.4439% -0.1794%
================================================
Files 1981 1965 -16
Lines 548326 548346 +20
================================================
- Hits 425629 424661 -968
- Misses 121887 123683 +1796
+ Partials 810 2 -808
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test mysql-test |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 4
- Inline comments: 4
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (1)
- Malformed storage URLs bypass source-path redaction in parse-error path (pkg/importsdk/file_scanner.go:62, pkg/parser/ast/misc.go:3818)
🟡 [Minor] (2)
fileScannerkeeps parallel source-path state with one dead representation (pkg/importsdk/file_scanner.go:51, pkg/importsdk/file_scanner_test.go:162)- Parse-backend error branch changes error detail contract without intent documentation (pkg/importsdk/file_scanner.go:65, pkg/importsdk/file_scanner.go:69)
🧹 [Nit] (1)
- Subtest name overstates redaction coverage across initialization errors (pkg/importsdk/file_scanner_test.go:133)
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, joechenrh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
4 participants
What problem does this PR solve?
Issue Number: close #67718
Problem Summary:
pkg/importsdkwrapped several outward-facing errors with the raw import source path. When the source path contained secret query parameters such asaccess-key,secret-access-key,session-token, orsas-token, those values were leaked in the returned error string.What changed and how does it work?
redactedSourcePathtofileScannerand initialize it once inNewFileScanner.source=%serror annotations through the redacted path for initialization, schema creation, and size-estimation failures.ast.RedactURL, so malformed URLs and unsupported schemes do not bypass masking.Check List
Tests
Manual test details:
upstream/masterworktree, apply the new regression tests and run:go test -run 'TestFileScanner/(NewFileScannerRedactsSensitiveSourcePathInInitErrors|CreateSchemasAndTablesRedactsSensitiveSourcePathOnError)$' -tags=intest,deadlock ./pkg/importsdkgo test -run 'TestFileScanner/(NewFileScannerRedactsSensitiveSourcePathInInitErrors|CreateSchemasAndTablesRedactsSensitiveSourcePathOnError)$' -tags=intest,deadlock ./pkg/importsdkgo test -tags=intest,deadlock ./pkg/importsdkmake lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit