Skip to content

feat(noema): review any owner that installed the Noema App - #2505

Draft
seonghobae wants to merge 3 commits into
codex/security-baseline-final-20260930from
feat/noema-review-any-installed-owner
Draft

seonghobae wants to merge 3 commits into
codex/security-baseline-final-20260930from
feat/noema-review-any-installed-owner

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Why

noema-review.yml accepted only ^ContextualWisdomLab/... targets and minted App tokens with a fixed owner: ContextualWisdomLab, so another organization (e.g. HYOSUNG-ITX-AI-Business-Department) could not be reviewed. The owner asked to remove the fixed repository restriction. Companion to ContextualWisdomLab/noema#733 (ADR-0019: the App installation is the owner's consent).

Change

  • Five target checks accept any valid owner/name (^[A-Za-z0-9][A-Za-z0-9-]{0,38}/[A-Za-z0-9_.-]+$).
  • The three credential-selection steps also emit owner=${TARGET_REPOSITORY%%/*}; all four actions/create-github-app-token steps use that output instead of ContextualWisdomLab.
  • Unchanged: the workflow's own identity (ContextualWisdomLab/.github, trusted noema-review.yml@ ref, source tarball, re-dispatch target), PR/head/base validation, and the rule that a non-central origin may only target itself.

Tests

  • New tests/test_noema_review_installed_owner.py; test_required_workflow_queue_contract.py, test_noema_reviewer_token_lifetime.py and the foreign case of test_noema_native_metadata_credentials.py updated to the new rule (foreign owner admitted with owner=OtherOwner; malformed metadata still rejected).
  • Exact stacked tree 05339c05620172ae2229d0de2d1f6a6358b505b1: direct product + prerequisite contracts 115 passed; expanded Noema/required-workflow/security dependency impact suite 496 passed, 2 skipped under GITHUB_ACTIONS=true and -W error (Python 3.12.14).

Stack and causal owner

This PR is intentionally stacked on #2531, the canonical owner of the shared PyJWT/PyO3 security baseline. Ordinary two-parent merge 1898bcd664c26ac690b20bf7fbd3700cf87aeaf8 preserves both prior head 0aeca39457d89e2c3ecdddde07f869480514207d and prerequisite head d1aa3659fca527a6c7330151f3ab4df3d7578391; no force push, suppression, or copied dependency repair was used. The previous Bandit/Semgrep/PyJWT/PyO3 failures were exact logs from the stale pre-prerequisite head and are not reused as current evidence.

Keep this PR Draft/Proposed until #2531 reaches protected main. Then retarget to current main, regenerate exact-head hosted Checks/review, and only use ordinary protected merge after terminal GREEN and qualifying approval.

Still needed to use it from a new organization

The Noema GitHub App must be installable there and installed by an org admin; the consumer repository then needs a way to trigger this workflow (it is pull_request_target/repository_dispatch, not workflow_call).

🤖 Generated with Claude Code

https://claude.ai/code/session_019hNrSzZrSNyJDUjXpaJMWs

The central Noema review accepted only ContextualWisdomLab targets and
minted App tokens with a fixed `owner: ContextualWisdomLab`, so another
organization could not be reviewed without editing this workflow. The
App installation on the target owner is the consent (noema ADR-0019).

Accept any valid owner/name target, derive the App-token owner from the
validated target, and keep the workflow's own identity (this repository,
its trusted ref and source tarball) pinned.

Signed-off-by: Seongho Bae <me@seonghobae.me>
Commit-Message-Assisted-by: Claude Code (Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019hNrSzZrSNyJDUjXpaJMWs
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: e432c3fd-fe83-4f63-b3f8-13bf8d1904ff

📥 Commits

Reviewing files that changed from the base of the PR and between 00a6e39 and 0aeca39.

📒 Files selected for processing (2)
  • .github/workflows/noema-review.yml
  • tests/test_noema_review_installed_owner.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

워크플로에 workflow_call 트리거를 추가하고, 대상 저장소 검증을 일반적인 owner/name 형식으로 확장합니다. 검증된 소유자를 메타데이터, 리뷰, 게시용 GitHub App 토큰 발급에 사용합니다. 테스트는 외부 소유자 저장소와 잘못된 형식을 확인합니다.

Changes

대상 저장소 소유자 처리

Layer / File(s) Summary
호출 및 대상 저장소 검증
.github/workflows/noema-review.yml, tests/test_noema_native_metadata_credentials.py, tests/test_noema_review_installed_owner.py
workflow_call 트리거를 추가합니다. 대상 저장소 검사를 일반적인 owner/name 형식으로 변경하고, 메타데이터 단계에서 소유자와 저장소 이름을 출력합니다. 테스트는 외부 소유자 저장소 허용과 잘못된 형식 거부를 확인합니다.
검증된 소유자로 토큰 발급
.github/workflows/noema-review.yml, tests/test_noema_review_installed_owner.py, tests/test_noema_reviewer_token_lifetime.py, tests/test_required_workflow_queue_contract.py
메타데이터, 리뷰, 게시용 GitHub App 토큰의 owner 입력을 검증된 소유자 출력에 연결합니다. 관련 테스트의 기대값도 갱신합니다.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant TARGET_REPOSITORY
  participant noema_credential
  participant GitHub_App_token_action
  TARGET_REPOSITORY->>noema_credential: owner/name 형식 검증
  noema_credential->>GitHub_App_token_action: 검증된 owner 전달
Loading

Merge Risk: ⚪ Minimal · up to 0aeca

The documented external-repository review path appears ready to merge after normal checks; no actionable defect was established.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 0aeca

Reviews can now operate across organizations. Tokens issued through the App are limited to the selected repository, but the newly widened target policy also reaches an existing dispatch path whose authority to select another organization's target is not established here.

Retained concerns

  • Medium · security · inferred: Newly eligible installed owners can be selected by a repository_dispatch payload without an in-workflow binding between the dispatch caller and target. This makes the central dispatch permission, and the authority of any selected fallback credential, material to cross-organization review authorization.
Security review details

Security Blast Radius

  • inferred — The App-token branch can now act on one selected repository under any owner with a usable installation, rather than only under the former fixed owner. The independent scope of PAT and OIDC alternatives is not established here.

Security Findings and Attack Paths

  • inferred — An actor able to submit a central repository_dispatch event can supply another eligible owner's repository and an open PR head. The workflow checks that the PR is current, but does not itself establish that actor's authority over the selected owner. This is a widened authority path, not evidence that an unprivileged actor can submit such an event.

Trust Boundaries and Controls

  • observed — The App branch requests repository-specific tokens, while publication refuses an absent selected credential and invokes the live-head verdict publisher. The reviewer selector chooses NOEMA_REVIEW_TOKEN before the App branch when that secret is present.

Hardening Proposals

  • proposed — Make dispatch target authority explicit, including how a retry proves its origin, and establish whether PAT and OIDC fallback scopes satisfy the intended installed-owner consent rule before relying on that rule across organizations.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 Noema App을 설치한 모든 소유자의 저장소를 검토하도록 워크플로를 변경한 핵심 내용을 정확하고 간결하게 요약합니다.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Add a workflow_call trigger so a repository outside ContextualWisdomLab
can run this exact workflow from its own pull_request_target caller.
The OIDC job_workflow_ref still names this file, which is the Noema
exchange trust anchor, and the caller's own repository becomes the
token target.

Signed-off-by: Seongho Bae <me@seonghobae.me>
Commit-Message-Assisted-by: Claude Code (Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019hNrSzZrSNyJDUjXpaJMWs

Copy link
Copy Markdown
Contributor Author

Exact-head admission correction — 0aeca39457d89e2c3ecdddde07f869480514207d

Ready is review admission only. Fresh audit against base f458d315210a2dca00e5e4b7a855d68ed5c6b0e6 found:

  • latest terminal workflow blockers: SAST Semgrep 36514309511=failure, Python Security 36514309484=failure, Security Scan 36514309545=failure, CodeQL PR 36514309452=failure

This PR is moved to Draft/Proposed until the causal owner repair is present on a successor exact head and re-audited. Queued/pending work is neither an additional blocker nor passing evidence. No Close, force push, destructive rebase, manual rerun, synthetic status/approval, merge, auto-merge, or bypass was performed.

@seonghobae
seonghobae marked this pull request as draft September 30, 2026 05:25
Preserve PR #2505's Noema installed-owner delta while taking the canonical PyJWT/PyO3 security prerequisite from #2531. This is an ordinary two-parent merge with no force push; #2531 remains the single writer for the shared dependency repair.
@seonghobae
seonghobae changed the base branch from main to codex/security-baseline-final-20260930 September 30, 2026 06:17

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head stacked review for 1898bcd664c26ac690b20bf7fbd3700cf87aeaf8 (tree 05339c05620172ae2229d0de2d1f6a6358b505b1) against prerequisite #2531@d1aa3659fca527a6c7330151f3ab4df3d7578391.

Topology: ordinary two-parent merge preserves prior #2505 head 0aeca39457d89e2c3ecdddde07f869480514207d and the canonical PyJWT/PyO3 repair head; compare is ahead 3 / behind 0 with exactly five intended Noema paths (+62/-13). No force push, copied dependency repair, or security suppression.

Reviewed the complete five-path product delta. External owners are admitted only after the owner/name shape check; non-central origins remain restricted to their own repository; every App token receives the validated target owner/repository; the central workflow identity and orchestrator/free pin remain unchanged. Fresh stacked-tree verification: direct product + prerequisite contracts 115 passed; expanded Noema/required-workflow/security dependency impact suite 496 passed, 2 skipped under GITHUB_ACTIONS=true, -W error, Python 3.12.14. I found no source-backed Critical, Important, or Minor defect in this exact delta.

This COMMENT is not approval. #2505 remains Draft/Proposed until #2531 lands on protected main; after retargeting to fresh main it still needs exact-head hosted GREEN and qualifying independent approval.

This branch has not been deployed

No deployments
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.

1 participant