Skip to content

fix(connectors): validate connector key before it becomes a path - #4083

Merged
spetz merged 3 commits into
apache:masterfrom
ryankert01:fix/connectors-validate-key
Sep 7, 2026
Merged

fix(connectors): validate connector key before it becomes a path#4083
spetz merged 3 commits into
apache:masterfrom
ryankert01:fix/connectors-validate-key

Conversation

@ryankert01

Copy link
Copy Markdown
Member

Which issue does this PR address?

Closes #4058

Rationale

The {key} path parameter of the connectors control API reached a filesystem path unvalidated; the issue explains why the missing exploit is an accident of the filename prefix.

What changed?

POST /{sinks,sources}/{key}/configs spliced the {key} segment straight into config_dir/{sink,source}_{key}_{version}.toml; only the literal sink_/source_ prefix kept .. from escaping.

Every {key} route now parses the segment into a ConnectorKey (at most 128 bytes of [A-Za-z0-9._-], leading alphanumeric so ., .. and hidden names are impossible) through a small extractor that answers with the API's usual 400 {"code": "invalid_connector_key"} body, and the provider create_* methods take the parsed type so the write site cannot see a raw string. Keys loaded from TOML or the HTTP provider are not rejected, since that would stop running deployments; the runtime warns at startup about any key it cannot address.

Local Execution

  • Passed: cargo clippy --all-targets -- -D warnings on iggy-connectors and integration, cargo test -p iggy-connectors, cargo test -p integration -- connectors::api:: connectors::runtime::error_isolation::source_with_invalid_state, markdownlint, typos
  • Pre-commit hooks not ran (prek not installed on this machine); the checks above were run by hand

AI Usage

  • Tools: Claude Code (Claude Fable 5.1).
  • Scope: drafted the implementation, tests and README changes, then ran a four-reviewer adversarial review whose confirmed findings were folded in.
  • Verification: the local runs listed above, plus a verbose run confirming the startup warning fires for the in-tree slash-key fixture.
  • Yes, I can explain every line.

The control API took the `{key}` path segment as-is and the local
config provider spliced it into the config filename under
`config_dir`. Nothing rejected `..`, separators, or control
characters; only the literal `sink_`/`source_` prefix and the absence
of any subdirectory under `config_dir` kept a traversal from landing,
and a refactor of either would have removed that by accident.

Parse the segment into a `ConnectorKey` at the API boundary: at most
128 bytes of ASCII letters, digits, `-`, `_` and `.`, starting with a
letter or digit so `.`, `..` and hidden-file names are impossible
outright. The rejection goes through `ApiError` so it carries the same
`{code, reason}` envelope as every other error, and the two `create_*`
provider methods take the parsed type so the write site cannot be
reached with a raw string.

Keys loaded from configuration files or the HTTP provider are not
rejected, since that would stop existing deployments from starting;
the runtime warns at startup about any key the API cannot address
instead.

Closes apache#4058
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

  • /ready - back to S-waiting-on-review after addressing feedback
  • /author - flip to S-waiting-on-author while you finish changes
  • /request-review @user-or-team - request a reviewer
  • /pin - exempt the PR from the stale bot, /unpin to undo

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Sep 7, 2026
@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.88172% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.43%. Comparing base (b87b6a6) to head (3c63e80).

Files with missing lines Patch % Lines
core/connectors/runtime/src/api/sink.rs 35.00% 10 Missing and 3 partials ⚠️
core/connectors/runtime/src/api/error.rs 10.00% 9 Missing ⚠️
core/connectors/runtime/src/api/source.rs 55.00% 6 Missing and 3 partials ⚠️
core/connectors/runtime/src/configs/connectors.rs 91.48% 7 Missing and 1 partial ⚠️
core/connectors/runtime/src/main.rs 87.50% 1 Missing and 1 partial ⚠️
core/connectors/runtime/src/api/key.rs 75.00% 0 Missing and 1 partial ⚠️
...s/runtime/src/configs/connectors/local_provider.rs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             master    #4083       +/-   ##
=============================================
- Coverage     85.90%   57.43%   -28.47%     
+ Complexity     1431     1402       -29     
=============================================
  Files          1244     1241        -3     
  Lines        192435   157081    -35354     
  Branches     158133   123387    -34746     
=============================================
- Hits         165307    90225    -75082     
- Misses        23037    62914    +39877     
+ Partials       4091     3942      -149     
Components Coverage Δ
Rust Core 50.26% <76.88%> (-36.57%) ⬇️
Java SDK 67.29% <ø> (-0.23%) ⬇️
C# SDK 76.42% <ø> (-0.56%) ⬇️
Python SDK 90.06% <ø> (-0.48%) ⬇️
PHP SDK 85.65% <ø> (ø)
Node SDK 96.22% <ø> (-0.03%) ⬇️
Go SDK 69.31% <ø> (-0.09%) ⬇️
Files with missing lines Coverage Δ
core/connectors/runtime/src/api/mod.rs 92.89% <ø> (ø)
...rs/runtime/src/configs/connectors/http_provider.rs 41.73% <ø> (ø)
core/connectors/runtime/src/error.rs 43.75% <100.00%> (+17.08%) ⬆️
core/connectors/runtime/src/api/key.rs 75.00% <75.00%> (ø)
...s/runtime/src/configs/connectors/local_provider.rs 65.82% <95.23%> (+3.03%) ⬆️
core/connectors/runtime/src/main.rs 85.86% <87.50%> (+0.09%) ⬆️
core/connectors/runtime/src/configs/connectors.rs 65.10% <91.48%> (+24.03%) ⬆️
core/connectors/runtime/src/api/error.rs 46.15% <10.00%> (-5.58%) ⬇️
core/connectors/runtime/src/api/source.rs 74.86% <55.00%> (+0.26%) ⬆️
core/connectors/runtime/src/api/sink.rs 56.61% <35.00%> (+0.46%) ⬆️

... and 468 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ryankert01

Copy link
Copy Markdown
Member Author

/request-review @hubcio

@github-actions
github-actions Bot requested a review from hubcio September 7, 2026 09:22

@spetz spetz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@spetz
spetz merged commit ab734f1 into apache:master Sep 7, 2026
65 checks passed
@github-actions github-actions Bot removed the S-waiting-on-review PR is waiting on a reviewer label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

connectors: Connectors control API does not validate key before using it in a filesystem path

3 participants