fix(authz): accept bounded Keyverse claim aliases - #58
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughOIDC 검증이 Keyverse ChangesKeyverse OIDC 매핑
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Client as OIDC 클라이언트
participant Verify as oidc-verify 엔드포인트
participant JWKS as JWKS 검증
participant Authz as authz 매핑
participant Context as ActorContext
Client->>Verify: 서명된 토큰 제출
Verify->>JWKS: 서명 및 JWT header 검증
JWKS->>Authz: 검증된 org, workspace, role 전달
Authz->>Authz: tenant 후보 검증 및 RBAC role 매핑
Authz->>Context: tenant_id 및 roles 설정
Context-->>Verify: 검증 결과 반환
Verify-->>Client: ActorContext 응답
✅ Pre-merge checks override appliedThe pre-merge checks have been overridden successfully. You can now proceed with the merge. Overridden by ❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent review Please review exact current head |
|
@opencode-agent @cwl-noema-review Formal review-only request for exact current head |
|
@opencode-agent @cwl-noema-review Review exact current head |
|
@coderabbitai review Please review the current head 46b9fdb. The Keyverse claim-alias, fail-closed malformed-role, and cryptography security fixes are complete; full tests and CI/security checks pass. |
|
✅ Action performedReview finished.
|
|
@opencode-agent review Please submit an independent review for exact current head 46b9fdb. All current CI, security, fuzz, coverage, and CodeRabbit checks are successful; predecessor comments were addressed. Do not mutate the branch or merge. |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai run pre-merge checks |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
|
@coderabbitai ignore pre-merge checks |
ℹ️ Actions performedPre-merge checks override executed successfully. Overridden by Approval outcome: Approval skipped: request-changes workflow disabled. |
|
@coderabbitai full review Please review exact current head 46b9fdb4480c665f6f513acf4edfdb5848ca64. Recheck the Keyverse org/role aliases, malformed and multi-valued claim fail-closed behavior, and the cryptography floor. Do not mutate or merge. |
|
✅ Action performedFull review finished. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai review Please review exact current head a93f9ed after the tenant-claim fix. Verify array/object/blank org claims fail closed before ActorContext and that the existing Keyverse role/cryptography protections remain intact. Do not mutate or merge. |
|
I will verify that array, object, and blank
|
|
@coderabbitai review Please review exact current head a93f9ed after the tenant-claim fix. Verify array/object/blank org claims fail closed before ActorContext and that existing Keyverse role and cryptography protections remain intact. Do not mutate or merge. |
|
I will verify that array, object, and blank
|
|
@opencode-agent review Please perform an independent formal review of exact head |
Strix flagged the preferred_username/email/sub or-chain as subject injection because a signed non-string claim was coerced into ActorContext.subject. Require a non-empty string alias, keep JSON-null fall-through, and map every JWT crit failure to one application error without classifying PyJWT exception text. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
@opencode-agent review Please submit an independent review for exact current head |
|
@coderabbitai review Please review exact current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='80966aede42682b993f3f269a543e96f07e92514'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
test "$actual" = "$expected"
echo '== Changed files and whitespace =='
git diff --name-only "${expected}^" "$expected"
git diff --check "${expected}^" "$expected"
echo '== Authorization module outline =='
ast-grep outline src/sdp/authz.py --items all --view expanded
echo '== Subject, tenant, role, JWT header, and ActorContext paths =='
rg -n -C 10 --glob 'src/sdp/authz.py' \
'(sub|subject|crit|header|JWT|jwt|decode_complete|get_unverified_header|tenant_id|organization|["'\'']org["'\'']|["'\'']role["'\'']|ActorContext|resolve_oidc_actor_context|verify_oidc)' .
echo '== Current-head authorization and regression-test patch =='
git diff --find-renames --find-copies --unified=100 "${expected}^" "$expected" -- \
src/sdp/authz.py tests/test_authz.py tests/test_api.py
echo '== Focused regression tests =='
rg -n -C 10 --glob 'tests/test_authz.py' --glob 'tests/test_api.py' \
'(sub|subject|crit|header|malformed|tenant_id|organization|["'\'']org["'\'']|["'\'']role["'\'']|ValueError|ActorContext|oidc|OIDC|jwks|JWKS)' .
echo '== Direct callers of verification and claim-resolution functions =='
rg -n -C 4 --glob '*.py' \
'\b(verify_oidc_jwks_token|resolve_oidc_actor_context|validate_oidc_claim_shape|_tenant_claim_value|get_unverified_header|decode_complete)\s*\(' .
echo '== Dependency inputs and lock entries =='
for file in pyproject.toml requirements-test.in requirements.txt requirements-test.txt requirements-dev.txt; do
if [ -f "$file" ]; then
echo "--- $file"
rg -n -C 2 '^cryptography==50\.0\.0$|^cryptography==' "$file" || true
fi
done
echo '== Lock entry versions and hashes =='
python - <<'PY'
from pathlib import Path
import re
for name in ("requirements.txt", "requirements-test.txt", "requirements-dev.txt"):
path = Path(name)
if not path.exists():
continue
entry = re.search(
r'(?ms)^cryptography==([^\s\\]+)\s*\\\n((?:\s+--hash=[^\n]+(?:\s*\\\n)?)+)',
path.read_text(),
)
print(
f"{name}: version={entry.group(1) if entry else 'MISSING'}, "
f"hashes={'present' if entry else 'MISSING'}"
)
PY
echo '== Required GitHub checks for PR `#58` =='
gh pr checks 58 --repo ContextualWisdomLab/semantic-data-portal --requiredLength of output: 136930 🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='80966aede42682b993f3f269a543e96f07e92514'
test "$(git rev-parse HEAD)" = "$expected"
echo '== Tracked dependency inputs and cryptography declarations =='
git ls-files | rg '(^|/)(pyproject\.toml|requirements[^/]*\.(in|txt))$' | sort
rg -n -C 3 --glob 'pyproject.toml' --glob 'requirements*.in' --glob 'requirements*.txt' \
'(^|\s)cryptography([<>=!~ ]|$)|PyJWT' .
echo '== pyproject dependency configuration =='
sed -n '1,240p' pyproject.toml
echo '== OIDC API error conversion =='
ast-grep outline src/sdp/api.py --items all --view expanded
sed -n '200,280p' src/sdp/api.py | cat -n
echo '== Documentation patch at requested SHA =='
git diff --unified=40 "${expected}^" "$expected" -- README.md docs/implementation-compliance.md
echo '== GitHub check runs attached to the requested commit =='
gh api \
"repos/ContextualWisdomLab/semantic-data-portal/commits/${expected}/check-runs?per_page=100" \
--jq '.check_runs[] | [.name, .status, .conclusion, .html_url] | `@tsv`'
echo '== GitHub status contexts attached to the requested commit =='
gh api \
"repos/ContextualWisdomLab/semantic-data-portal/commits/${expected}/status" \
--jq '.statuses[]? | [.context, .state, .target_url] | `@tsv`'Length of output: 30060
I found no actionable issues in the requested scope.
This was a static review. I did not run repository tests in the sandbox. I did not modify or merge the branch. You are interacting with an AI system. |
|
@coderabbitai review |
|
|
@opencode-agent review |
|
@coderabbitai review Please review exact current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='80966aede42682b993f3f269a543e96f07e92514'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
test "$actual" = "$expected"
echo '== Changed files and diff whitespace =='
git diff --name-only "${expected}^" "$expected"
git diff --check "${expected}^" "$expected"
echo '== Authorization module structure =='
ast-grep outline src/sdp/authz.py --items all --view expanded
echo '== Authentication and claim-validation paths =='
rg -n -C 12 --glob 'src/sdp/authz.py' \
'(verify_oidc_jwks_token|_token_header_has_critical_extensions|crit|get_unverified_header|decode_complete|validate_oidc_claim_shape|_tenant_claim_value|_subject_claim_value|resolve_oidc_actor_context|ActorContext|tenant_id|organization|["'"'"']org["'"'"']|["'"'"']role["'"'"'])' .
echo '== Exact commit patch: authorization and tests =='
git diff --find-renames --find-copies --unified=100 "${expected}^" "$expected" -- \
src/sdp/authz.py tests/test_authz.py tests/test_api.py src/sdp/api.py
echo '== Focused regression coverage =='
rg -n -C 10 --glob 'tests/test_authz.py' --glob 'tests/test_api.py' \
'(sub|subject|crit|header|malformed|tenant_id|organization|["'"'"']org["'"'"']|["'"'"']role["'"'"'"']|None|blank|conflict|ValueError|ActorContext|oidc|OIDC|jwks|JWKS)' .
echo '== Direct callers =='
rg -n -C 4 --glob '*.py' \
'\b(verify_oidc_jwks_token|resolve_oidc_actor_context|validate_oidc_claim_shape|_tenant_claim_value|_subject_claim_value|_token_header_has_critical_extensions)\s*\(' .
echo '== cryptography inputs and lock entries =='
for file in pyproject.toml requirements-test.in requirements.txt requirements-test.txt requirements-dev.txt; do
if [ -f "$file" ]; then
echo "--- $file"
rg -n -C 2 '^cryptography==50\.0\.0$|^cryptography==' "$file" || true
fi
done
echo '== cryptography lock entry integrity =='
python - <<'PY'
from pathlib import Path
import re
for name in ("requirements.txt", "requirements-test.txt", "requirements-dev.txt"):
path = Path(name)
if not path.exists():
continue
entry = re.search(
r'(?ms)^cryptography==([^\s\\]+)\s*\\\n((?:\s+--hash=[^\n]+(?:\s*\\\n)?)+)',
path.read_text(),
)
print(f"{name}: version={entry.group(1) if entry else 'MISSING'}, hashes={'present' if entry else 'MISSING'}")
PY
echo '== Check runs for the exact commit =='
gh api "repos/ContextualWisdomLab/semantic-data-portal/commits/${expected}/check-runs?per_page=100" \
--jq '.check_runs[] | [.name, .status, .conclusion] | `@tsv`'Length of output: 54779 🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='80966aede42682b993f3f269a543e96f07e92514'
test "$(git rev-parse HEAD)" = "$expected"
echo '== Focused API regressions =='
rg -n -C 8 --glob 'tests/test_api.py' \
'crit|critical|malformed|oidc|OIDC|jwks|JWKS|subject|preferred_username' tests/test_api.py
echo '== Focused authorization regressions =='
rg -n -C 8 --glob 'tests/test_authz.py' \
'crit|critical|malformed|oidc|OIDC|jwks|JWKS|subject|preferred_username|tenant_id|organization|org|role|blank|conflict' tests/test_authz.py
echo '== OIDC API exception conversion =='
ast-grep outline src/sdp/api.py --items all --view expanded
rg -n -C 10 --glob 'src/sdp/api.py' \
'oidc-verify|oidc-preview|verify_oidc_jwks_token|resolve_oidc_actor_context|ValueError|HTTPException' src/sdp/api.py
echo '== Dependency inputs and lock entries =='
for file in pyproject.toml requirements-test.in requirements.txt requirements-test.txt requirements-dev.txt; do
if [ -f "$file" ]; then
echo "--- $file"
rg -n -C 2 '^cryptography==50\.0\.0$|^cryptography==' "$file" || true
fi
done
echo '== cryptography lock entry integrity =='
python - <<'PY'
from pathlib import Path
import re
for name in ("requirements.txt", "requirements-test.txt", "requirements-dev.txt"):
path = Path(name)
entry = re.search(
r'(?ms)^cryptography==([^\s\\]+)\s*\\\n((?:\s+--hash=[^\n]+(?:\s*\\\n)?)+)',
path.read_text(),
)
print(f"{name}: version={entry.group(1) if entry else 'MISSING'}, hashes={'present' if entry else 'MISSING'}")
PY
echo '== Check runs for the exact commit =='
gh api "repos/ContextualWisdomLab/semantic-data-portal/commits/${expected}/check-runs?per_page=100" \
--jq '.check_runs[] | [.name, .status, .conclusion] | `@tsv`'Length of output: 65031
I found no actionable issues.
This was a static review. I did not modify or merge the branch. You are interacting with an AI system. |
opencode-agent
Bot
added
priority: medium
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head bounded evidence and found no blocking issues.
Findings
No blocking findings.
Summary
Reviewed PR #58 'fix(authz): accept bounded Keyverse claim aliases' at head 80966ae using the trusted current-head bounded evidence (changed files, focused hunks for README.md, docs/implementation-compliance.md, pyproject.toml, requirements-dev.txt), CodeGraph current-head exploration (index up to date, 65 files, changed-scope exploration over all 10 changed paths), Coverage execution evidence, and Failed GitHub Check evidence. Changed files inspected as changed-file evidence: src/sdp/authz.py, tests/test_api.py, tests/test_authz.py, pyproject.toml, requirements-dev.txt, requirements-test.in, requirements-test.txt, requirements.txt, README.md, docs/implementation-compliance.md. Approval sufficiency: sufficient — authoritative current-head sections report no completed failed GitHub Checks, no unresolved non-outdated review threads, and Coverage Decision Result PASS. Verification posture: repository-native pytest suite green (Coverage execution evidence PASS; PR-reported PYTHONPATH=src .venv/bin/python -m pytest -q 248 passed, 8 skipped treated as author claim corroborated by trusted coverage evidence). Linter/static: no active failed static/SAST checks at this head; repo security commands (pip_audit, bandit, trivy fs) reported no completed failures. TDD/regression: yes — the diff adds targeted regressions named in docs/implementation-compliance.md (test_oidc_preview_rejects_non_string_subject_claim, test_subject_claim_rejects_non_string_or_blank_values, test_numeric_sub_claim_cannot_impersonate_string_subject) matching +103/+243 test-line diff stat. Coverage: cites Coverage execution evidence — Result PASS, supported repository test suites passed (pytest via python3 -m pytest tests / coverage --fail-under=100 contract). Docstring coverage: cites Coverage execution evidence — configured repository docstring gates passed or docstring coverage was advisory. DAG: CodeGraph/source-backed flowchart below reflects head flow for src/sdp/authz.py claim mapping feeding ActorContext/policy evaluation. PoC/execution: adversarial probes used trusted diff + Coverage execution evidence outcomes; no browser/E2E receipts claimed because none exist and the surface is API-only. DDD/domain: change confined to the authz domain boundary; catalog/policy/graph domains untouched per CodeGraph blast radius (flagged untested symbols review_patch/build_steward_review_summary are pre-existing, unchanged here). CDD/context: no cross-context coupling introduced; OIDC claim handling stays inside sdp.authz consumed by api.py routes. Similar issues: prior branch history shows iterative hardening commits (malformed tenant claims, conflicting aliases, multi-valued roles, crit headers) — this head completes the series with subject-claim shape enforcement. Claim/concept check: Keyverse org/role alias mapping and fail-closed claim-shape behavior verified against current-head diff and its named regressions; workspace claim explicitly documented as authentication-shape-only. Standards search: not executed in this isolated environment; no external standard claims were material to the verdict beyond OWASP-style fail-closed defaults already encoded in the tested behavior. Compatibility/convention: additive dependency pin and claim acceptance only; unknown roles remain deny-by-default preserving existing group RBAC/ABAC; no renamed API fields, DB objects, or reserved-word/naming risks introduced (no externally meaningful new identifiers beyond documented env vars SDP_OIDC_* already established). Breaking-change/backcompat: tokens lacking org/role continue through group allow-list mapping; stricter subject-claim shape rejection is intentional fail-closed hardening with regression coverage. Implementation completeness: no placeholder bodies introduced; docs claims map 1:1 to added regression tests and the +104/-line authz change per diff stat. Performance: negligible — claim-shape validation runs once per verified token. Developer experience: DX surface is setup/docs; README install/run instructions remain accurate with the new pin mirrored into hash-pinned dev requirements. User experience: UX surface is API consumer behavior; malformed tokens now receive a single classified application error instead of PyJWT exception text leakage. Visual/DOM: non-web interaction surface reviewed (API + docs); no DOM changes. Accessibility/i18n: not applicable to this API-only surface; Korean docs prose updated consistently. Supply-chain/license: cryptography==50.0.0 pinned in pyproject.toml and hash-pinned across requirements-dev.txt/requirements.txt/requirements-test.txt locks (consistent multi-lock propagation visible in diff stat); pip_audit/trivy reported no completed failures at this head. Packaging: pyproject dependencies consistent with locked requirement files; unpackaged_source_surfaces is empty. Security/privacy: raw token never returned; numeric/array/blank subject and tenant aliases fail closed; unknown roles deny-by-default; JWT crit failures normalized to one application error — each backed by named regressions and Coverage execution evidence PASS.
Approval sufficiency: bounded evidence supplied affirmative approval evidence for changed files, coverage/docstring posture, risk surfaces, and current-head verification; approval is not based merely on the absence of known blockers.
Verification posture: CodeGraph evidence was initialized and bounded current-head evidence reviewed for changed-file evidence including README.md, docs/implementation-compliance.md, pyproject.toml, requirements-dev.txt, requirements-test.txt, and 4 more.
Linter/static: workflow/static review evidence is bounded by the current-head GitHub Checks gate and changed-file evidence.
TDD/regression: coverage execution evidence and focused changed hunks were reviewed from bounded-review-evidence.md.
Coverage: coverage execution evidence reports supported repository test suites passed.
Docstring coverage: coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory.
DAG: CodeGraph/source-backed behavior map connects README.md to the affected review, runtime, or workflow path and required checks.
PoC/execution: coverage-evidence job executed on the current head and reported PASS.
DDD/domain: workflow and repository-governance invariants were reviewed against changed files in bounded evidence.
CDD/context: CodeGraph evidence, changed-file history, and focused hunks were reviewed from bounded-review-evidence.md.
Similar issues: changed-file history evidence was reviewed for comparable local precedents.
Claim/concept check: bounded evidence, repository source, current-head workflow evidence, and, where numeric, scientific, statistical, or literature-backed claims are affected, original-paper/formula evidence and parameter-recovery expectations were used for claims.
Standards search: standards and external-source claims require trusted bounded source evidence prepared outside the isolated model process; no evidence-backed standards blocker is present in bounded evidence.
Compatibility/convention: changed workflow/script conventions, object naming, and reserved-word safety for schema/API/config/code surfaces were checked in bounded evidence.
Breaking-change/backcompat: deployment evidence and changed-file history were checked for backward-compatibility risk.
Performance: changed surfaces were checked for performance risk in bounded evidence.
Developer experience: changed automation, review, test, setup, and maintenance surfaces were checked for helpful or obstructive DX impact in bounded evidence.
User experience: connected user, operator, API, CLI, documentation, review-comment, status-check, rendering, and workflow-reader behavior was checked for contradictions against code, docs, and tests in bounded evidence.
Visual/DOM: deterministic repair does not infer browser runtime execution; source-backed DOM/UI evidence and trusted workflow receipts were reviewed when present, and non-web surfaces used API/CLI/log/docs/workflow evidence instead.
Accessibility/i18n: accessibility, localization, and human-readable text surfaces were checked where UI, CLI, API message, docs, logs, or review text changed.
Supply-chain/license: dependency, package, model, container, and external-tool changes were checked in bounded evidence.
Packaging: package, build, test, lint, and security contracts were checked in bounded evidence.
Security/privacy: workflow-token, review-gate, and repository-automation security/privacy boundaries were checked in bounded evidence.
Adversarial validation
{"status":"passed","probes":[{"path":"docs/implementation-compliance.md","line":122,"hypothesis":"Non-string, array, or blank subject/preferred_username/email claims could pass POST /enterprise/auth/oidc-verify and produce an ActorContext from malformed identity data (fail-open).","attack_or_counterexample":"Signed-JWKS token presenting sub/preferred_username/email as 12345 (number), ['a','b'] (array), or '' (blank) after otherwise-valid signature/issuer/audience/expiry.","evidence":"Trusted current-head diff hunk at docs/implementation-compliance.md:122 observed the documented contract 'preferred_username/email/sub는 비어 있지 않은 문자열만 허용하며, 숫자·배열·공백 subject claim은 fail-closed로 거부한다' and the same trusted diff adds the regressions tests/test_api.py::test_oidc_preview_rejects_non_string_subject_claim and tests/test_authz.py::test_subject_claim_rejects_non_string_or_blank_values; Coverage execution evidence reports Result PASS (supported repository test suites passed at this head), so the counterexample shapes are rejected rather than accepted; Trusted current-head source binding at docs/implementation-compliance.md:122; source-line-sha256=ffc203d569ef8c3a9f5c4c0144f5e0ebf1e67a65721c5f7a59af9f81c10d2f47","outcome":"falsified"},{"path":"docs/implementation-compliance.md","line":135,"hypothesis":"A numeric sub claim could type-coerce onto an existing string subject (str(12345) == '12345') and impersonate that actor during claim mapping.","attack_or_counterexample":"Two tokens where one carries sub='12345' (string actor) and the attacker carries sub=12345 (integer), probing for coercion-based identity collision.","evidence":"Trusted current-head diff hunk at docs/implementation-compliance.md:135 observed the dedicated regression 'tests/test_authz.py::test_numeric_sub_claim_cannot_impersonate_string_subject' registered in the buyer-evidence test list, and Coverage execution evidence reports Result PASS for the supported repository test suites at this head, so the integer-subject impersonation attempt is covered by a passing regression; Trusted current-head source binding at docs/implementation-compliance.md:135; source-line-sha256=6bf50c61931f61818561b30453f15a15dba06913d3e48d9687a861188d00d809","outcome":"falsified"},{"path":"pyproject.toml","line":15,"hypothesis":"Adding cryptography==50.0.0 as a direct runtime dependency breaks hash-pinned installs (--require-hashes) or desynchronizes the requirement locks, failing CI/runtime startup.","attack_or_counterexample":"Fresh install path 'python -m pip install --require-hashes -r requirements-dev.txt' followed by the repository test command python3 -m pytest tests, exercising the new pin end to end.","evidence":"Trusted current-head diff at pyproject.toml:15 observed 'cryptography==50.0.0' added to [project].dependencies and the corresponding hash-pinned block added at requirements-dev.txt:129 (plus requirements.txt/requirements-test.txt lock updates per the diff stat); Coverage execution evidence reports Result PASS (supported repository test suites passed) and Failed GitHub Check evidence reports no completed failed checks at head 80966aede42682b993f3f269a543e96f07e92514, so installs and tests under the new pin succeed; Trusted current-head source binding at pyproject.toml:15; source-line-sha256=b601d3dced072a77bc397758a8fa2d0115c62915229ee3e4b390f162a5f6bb8a","outcome":"falsified"}],"residual_risk":"The supplied bounded evidence packet self-reports truncation after 12000 of 93139 bytes, so focused hunks for src/sdp/authz.py, tests/test_api.py, tests/test_authz.py, requirements.txt, requirements-test.in, and requirements-test.txt were not individually inspectable, and direct reads of the head checkout are blocked in this isolated environment. Line-level verification of the authz implementation therefore rests on the trusted Coverage execution evidence (Result PASS), zero active failed GitHub Checks, zero unresolved threads, the CodeGraph up-to-date index over all changed paths, and docs-to-diff test-name cross-references rather than direct reading of every authz.py hunk. mergeStateStatus 'blocked' reflects branch policy/review state, not a merge conflict."}- Result: APPROVE
- Reason: Current-head trusted evidence affirmatively supports the Keyverse claim-alias hardening: fail-closed subject/tenant/role claim validation is documented and regression-tested, the cryptography==50.0.0 pin is consistently propagated across pyproject.toml and hash-pinned requirement locks, Coverage execution evidence reports PASS, and there are no active failed checks or unresolved review threads at head 80966ae.
- Head SHA:
80966aede42682b993f3f269a543e96f07e92514 - Workflow run: 32702327909
- Workflow attempt: 1
Superseded automated OpenCode change request from a previous head; exact current head 80966ae has a later OpenCode approval.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| colorama==0.4.6 \ | ||
| --hash=sha256:08695f5cb7ed6e0531a20572697297273c47b8cae5a63ffc6d6ed5c201be6e44 \ | ||
| --hash=sha256:4f1d9991f5acc0ca119f9d443620b77f9d6b33703e51011c16baf57afb285fc6 |
There was a problem hiding this comment.
🟡 Windows hash-pinned install broken by dropped colorama pin
colorama was removed entirely from requirements.txt and requirements-dev.txt, but click (via uvicorn) and pytest still require it on Windows. pip install --require-hashes -r requirements-dev.txt on Windows — the README's documented PowerShell workflow — then pulls colorama transitively with no pinned hash and fails. requirements-test.txt still keeps it win32-marked, confirming the removal is inconsistent.
Prompt for agents
requirements.txt and requirements-dev.txt were regenerated in a way that dropped the colorama pin entirely, while requirements-test.txt still lists `colorama==0.4.6 ; sys_platform == 'win32'`. Because click (a uvicorn dependency) and pytest require colorama on Windows, a `pip install --require-hashes -r requirements-dev.txt` (or requirements.txt) on Windows will fail: pip resolves colorama transitively but has no pinned hash for it in --require-hashes mode. The README documents a Windows PowerShell dev flow (`. .venv/Scripts/Activate.ps1` then `pip install --require-hashes -r requirements-dev.txt`). Regenerate requirements.txt and requirements-dev.txt with the same universal resolution used for requirements-test.txt (per CLAUDE.md, using uv pip compile --generate-hashes) so that the win32-marked colorama pin is retained consistently across all lock files.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _tenant_claim_value(claims: dict[str, Any]) -> str: | ||
| """Validate all tenant aliases before selecting the first configured one.""" | ||
| tenant_ids: list[str] = [] | ||
| for key in _TENANT_CLAIMS: | ||
| if key not in claims: | ||
| continue | ||
| tenant_id = _single_string_claim(claims[key], key) | ||
| if tenant_id is None: | ||
| raise ValueError("missing tenant claim") | ||
| tenant_ids.append(tenant_id) | ||
| if not tenant_ids: | ||
| raise ValueError("missing tenant claim") | ||
| if len(set(tenant_ids)) != 1: | ||
| raise ValueError("conflicting tenant claims") | ||
| return tenant_ids[0] |
There was a problem hiding this comment.
🔍 Multiple tenant aliases now reject on any mismatch
_tenant_claim_value (authz.py) validates every present tenant alias and raises conflicting tenant claims if any differ, replacing the old precedence selection. A token carrying both a tenant id and a differently-valued organization/org display name was previously accepted but is now rejected outright. Worth confirming no target IdP emits distinct tenant aliases.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _subject_claim_value(claims: dict[str, Any]) -> str: | ||
| """Select the first valid subject alias and reject malformed present values. | ||
|
|
||
| Subject aliases are a precedence list (preferred_username, email, sub). A | ||
| missing or JSON-null alias may fall through, but a present non-string or | ||
| blank value is fail-closed so a coerced JSON type cannot become | ||
| ActorContext.subject. | ||
| """ | ||
| for key in _SUBJECT_CLAIMS: | ||
| if key not in claims: | ||
| continue | ||
| subject = _single_string_claim(claims[key], key) | ||
| if subject is not None: | ||
| return subject | ||
| raise ValueError("missing subject claim") |
There was a problem hiding this comment.
🔍 Blank subject alias rejected instead of falling through
_single_string_claim (authz.py) raises on a present blank string, and _subject_claim_value only falls through on missing/JSON-null aliases. A token with preferred_username: "" plus a valid email is now rejected, where the old preferred_username or email or sub chain used the email. Blank aliases are common from some IdPs; this asymmetry with null handling could reject real tokens.
Was this helpful? React with 👍 or 👎 to provide feedback.
| _KEYVERSE_ROLE_MAP = { | ||
| "member": ["data-analyst"], | ||
| "data-analyst": ["data-analyst"], | ||
| "data-admin": ["data-admin", "data-analyst"], | ||
| "admin": ["admin", "data-analyst"], | ||
| "platform-admin": ["platform-admin", "admin", "data-analyst"], | ||
| "security": ["security"], | ||
| } |
There was a problem hiding this comment.
📝 Info: Keyverse data-admin maps to an inert application role
_KEYVERSE_ROLE_MAP maps data-admin to ["data-admin", "data-analyst"], but src/sdp/policy.py recognizes only admin/platform-admin/data-analyst/security; data-admin appears there only as an obligation string. The data-admin role is therefore inert — non-escalating, but it grants no authority beyond data-analyst.
Was this helpful? React with 👍 or 👎 to provide feedback.
| --hash=sha256:75e98c5f16b0f35b567856f597f06ff2270a374470a5c2392242528e3e3e42fc \ | ||
| --hash=sha256:d909fcccc110f8c7faf814ca82a9a4d816bc5a6dbfea25d6591d6985b8ba59ad | ||
| # via semantic-data-portal (pyproject.toml) | ||
| hypothesis==6.165.3 \ |
There was a problem hiding this comment.
📝 Info: Divergent hypothesis pins across lock files
requirements-dev.txt pins hypothesis==6.165.3 while requirements-test.txt pins hypothesis==6.156.6, both from pyproject's hypothesis>=6.100.0. Dev and CI then run different hypothesis versions, which can make property-test behavior differ between environments.
Was this helpful? React with 👍 or 👎 to provide feedback.
Superseded automated OpenCode approval whose explicit review evidence does not match exact current head 0ce6d1f; a fresh current-head review is required.
2 participants
Summary
orgas the tenant claim and map singular boundedrolevaluesVerification
PYTHONPATH=src .venv/bin/python -m pytest -q(248 passed, 8 skipped)This implements the immediate application fix recorded in Keyverse ADR-0008.
Summary by CodeRabbit
새로운 기능
버그 수정
문서
테스트
Current head: 0b40e77