Skip to content

Phase 1: Security Hardening - #7

Merged
padak merged 1 commit into
mainfrom
phase-1-security-hardening
Feb 27, 2026
Merged

Phase 1: Security Hardening#7
padak merged 1 commit into
mainfrom
phase-1-security-hardening

Conversation

@padak

@padak padak commented Feb 27, 2026

Copy link
Copy Markdown
Member

Implementation Summary

Fix critical and medium security vulnerabilities: token exposure, URL validation, directory permissions, error message sanitization, path traversal prevention, and assert removal.

Acceptance Criteria

  • --token is NOT accepted as CLI argument for project add and project edit
  • Token is accepted via KBC_TOKEN env var or interactive hidden prompt
  • http:// and file:// URLs are rejected at project add time with clear error
  • https:// URLs are accepted
  • Config directory is created with 0o700 permissions
  • API error messages are truncated to 500 characters
  • Rich markup brackets in error messages are escaped or stripped
  • component_id, config_id, job_id are URL-encoded in API paths
  • No assert statements remain in production code (only in tests)

Tests

  • All 449 existing + new tests pass (0 failures)
  • New security tests added:
    • test_project_add_token_from_env -- token from KBC_TOKEN env var
    • test_project_add_token_interactive -- interactive hidden prompt
    • test_project_add_rejects_http_url -- http:// rejected
    • test_project_add_rejects_file_url -- file:// rejected
    • test_project_add_accepts_https_url -- https:// accepted
    • test_config_dir_permissions -- directory 0o700
    • test_api_error_message_truncation -- long response truncated to 500 chars
    • test_url_path_encoding_* -- special chars in IDs encoded

Files Changed

  • src/keboola_agent_cli/commands/project.py -- Remove --token CLI option, add _resolve_token() with env var + interactive prompt
  • src/keboola_agent_cli/models.py -- Add @field_validator('stack_url') enforcing https:// scheme
  • src/keboola_agent_cli/config_store.py -- mkdir mode=0o700
  • src/keboola_agent_cli/client.py -- Truncate api_message to 500 chars, URL-encode path params
  • src/keboola_agent_cli/manage_client.py -- Truncate api_message to 500 chars
  • src/keboola_agent_cli/services/project_service.py -- Replace assert with ConfigError
  • tests/test_cli.py -- Updated all project add/edit tests, added security tests
  • tests/test_client.py -- Added truncation and URL encoding tests
  • tests/test_config_store.py -- Added directory permission tests
  • tests/test_models.py -- Added URL validation tests

@padak
Phase 1: Security hardening - fix critical vulnerabilities
3adc582 
- S1: Remove --token CLI argument (visible in ps aux), add interactive
  hidden prompt and KBC_TOKEN env var support for project add/edit
- S2: Add HTTPS-only URL validation on ProjectConfig via field_validator
- S3: Set config directory permissions to 0o700 (was default umask)
- S4: Truncate API error messages to 500 chars to prevent Rich markup
  injection and excessive output
- S5: URL-encode component_id, config_id, job_id in API paths to prevent
  path traversal attacks
- S6: Replace assert in production code with proper ConfigError handling
@padak
padak merged commit f679572 into main Feb 27, 2026
@padak
padak deleted the phase-1-security-hardening branch February 27, 2026 23:57
@ottomansky ottomansky mentioned this pull request May 3, 2026
Closed
8 tasks
@padak padak mentioned this pull request May 22, 2026
Open
4 tasks
@ottomansky ottomansky mentioned this pull request Jun 3, 2026
Merged
Matovidlo added a commit that referenced this pull request Jun 15, 2026
@Matovidlo @claude
refactor(release): address review — extract scripts, composite action…
d13d226 
…, dedup

Thermo-nuclear review follow-ups (behavior-preserving):
- #1/#6: move inline nfpm loop -> build/package/linux/build_packages.sh and the
  zip+sha256 -> build/package/zip_binary.sh (explicit per-OS archiver); workflow
  is now pure orchestration.
- #2: .github/actions/setup-build composite (uv + optional Node) replaces the
  repeated setup preamble in pypi/freeze.
- #3: index.sh deb/rpm/apk collapsed into a publish_repo() helper + per-format indexers.
- #5: sign.sh parses the Azure token with jq instead of a sed regex.
- #7: nfpm.yaml notes it's rendered via envsubst.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Matovidlo added a commit that referenced this pull request Jun 17, 2026
@Matovidlo @claude
refactor(release): address review — extract scripts, composite action…
ebac348 
…, dedup

Thermo-nuclear review follow-ups (behavior-preserving):
- #1/#6: move inline nfpm loop -> build/package/linux/build_packages.sh and the
  zip+sha256 -> build/package/zip_binary.sh (explicit per-OS archiver); workflow
  is now pure orchestration.
- #2: .github/actions/setup-build composite (uv + optional Node) replaces the
  repeated setup preamble in pypi/freeze.
- #3: index.sh deb/rpm/apk collapsed into a publish_repo() helper + per-format indexers.
- #5: sign.sh parses the Azure token with jq instead of a sed regex.
- #7: nfpm.yaml notes it's rendered via envsubst.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
martinsifra added a commit that referenced this pull request Aug 26, 2026
@martinsifra @claude
fix(service): apply the PR #703 self-review findings [DMD-1899]
51eaa2c 
The big one (#2, upgraded): the diff sides' wire shape was WRONG in the
implementation and in the test fixtures that defined it. Verified against
connection (ConfigurationVersionResponse + ConfigurationDiffData OA
schemas, now recorded in the notes wire-truth table): each side is
{version, isDeleted, diff: {name, description, changeDescription,
isDisabled, configuration, rows}} -- content NESTED under diff,
version/deletion as side metadata. The classification and both take modes
now read the envelope; the flat-side code would have been dead on arrival
against the live API.

#1: resolve_conflict no longer takes branch_id -- it derives the branch
from the MR itself (branches.branchFromId), so the conflict-set guard and
the branch being written to can never disagree; a caller-supplied id could
point the REPLACing rebase at an unrelated dev branch the guard never
checked. A published/canceled MR (null branchFromId) is refused readably.

#3: take=theirs of a deleted side collapses to the delete resolution,
symmetric with ours ('production deleted it, dev changed it' is a live
conflict shape).
#4: deletion surfaces as top-level ours_deleted/theirs_deleted booleans on
get_config_diff (None = side never existed) -- it is side metadata, not a
content path.
#5: a 'both' row where the sides agree on the identical value carries
agreed: true -- agreement, not a conflict hotspot.
#2 (message half): a take side missing required envelope keys is reported
as a backend contract violation pointing at the resolved-body workaround,
not as caller error.
#6: wire ids compared via _same_id / int-coerced (find_merge_request_for_
branch, merge()'s was_active) -- a string-serialized branchFromId can no
longer silently defeat the post-merge cleanup.
#7: the feature pre-flight raises FeatureNotEnabledError carrying the new
ErrorCode.FEATURE_NOT_ENABLED (value matches the string SearchService
already emits; categorized 'configuration' like PAYG_NOT_AVAILABLE).
#8: ConfigError imported from ..errors like every other service; the
isDefault scan hoisted to services.base.find_default_branch_id and the
copies in config/sync/workspace services migrated (lib.py keeps its own
loop -- the SDK facade does not import the services layer); verify_token
is skipped when the server already serialized viewer (the polyfill's cost
dies with the polyfill); list --state validates against the closed
vocabulary instead of returning a silent count: 0 on a typo.
#9: test imports hoisted (no mid-file noqa), mocks spec'd at the L3 seam
(KeboolaClient + MergeRequests -- a renamed L3 method now fails the tests),
and regression tests added for every finding (82 tests total).

Doc drift: layer2/layer3 references updated to BRANCHES_MERGE_REQUESTS_
FEATURE, the layer3 open nit closed, tokens.py line ref fixed.

Review: tasks/pr-703-review.md (2026-08-27).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
martinsifra added a commit that referenced this pull request Aug 27, 2026
@martinsifra @claude
fix(service): apply the Opus wire-truth review -- 409 codes, allowed_…
a0160a4 
…actions, derive_state honesty [DMD-1899]

Adversarial review of every wire assumption against Connection source
(tasks/pr-703-opus-wire-review.md): 7 CONFIRMED, 3 MISMATCH. The fixes:

409 mapping (finding #4): the conflict 409 is NOT code-less -- an earlier
reading missed ExceptionConverter, which serializes
MergeValidationException's own string code storage.mergeRequests.validation
top-level as `code`, plus the conflicting configurations in
`params.errors`. _remap_merge_conflict now matches BOTH codes explicitly
(code-less 409 falls back to conflict for older stacks; a 409 with any
OTHER code passes through unmapped instead of being confidently mislabeled
a conflict), and http_base surfaces `params` as
details.api_error_params so the conflict list travels with the error
instead of demanding a second round trip. Stale 'carries no code' claims
corrected in errors.py, the L3 docstring, error-codes.md, the notes
wire-truth table and the RFC.

allowed_actions (finding #7): `approve` removed from the `approved`
tuple -- the transition's sole `from` place is in_review; from approved
the backend answers 422 (the UI button offering it there is wrong).
`update` added to in_merge (the server blocks update only in terminal
states). Docstring records the AddApprovalGuard gating and that with the
non-SOX default of 0 required approvals, approve is 422 in every state and
in_review itself is unreachable.

derive_state honesty (finding #10, the significant one): reviewers[].status
is populated only within a review round anchored by a review_requested
activity event -- which skip_review never writes -- and explicit reviewers
shadow every non-reviewer's decision (the creator can never BE a reviewer).
So in a default non-SOX project the rejected / self-closed derivations
never fire; the UI badge has the identical blind spot, since our table is
its port. Documented in the docstring and the RFC rather than re-derived
from the activity log client-side: the reliable fix is server-side
(commented on DMD-1988 -- derive from the activity log, not reviewers[]).

Sharp edges from confirmed items: a take side with a null/empty `name`
(nullable in the diff envelope, required non-empty by the rebase validator)
is refused as a contract violation instead of sailing into a server 400;
_branch_from_id_of documents that its null check is racy (branchFromId is
nulled by the FK when the async branch delete lands, not by the state
change); the viewer docstring notes detail/conflicts require an admin token
anyway (MergeRequestVoter).

Regression tests for each fix; 87 service tests total.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@padak