Skip to content

docs(review): add same-tenant object-level authorization bar - #136

Merged
kyle-sexton merged 2 commits into
mainfrom
docs/review-security-bola
Jul 16, 2026
Merged

docs(review): add same-tenant object-level authorization bar#136
kyle-sexton merged 2 commits into
mainfrom
docs/review-security-bola

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

  • Adds an Object-level authorization section to security.md covering the same-tenant BOLA/IDOR case: a request that carries an object id checked for well-formedness but not for the caller's authorization over that specific record.
  • Fills a gap surfaced during dogfood on docs(review): add multi-tenancy review criteria #130: multi-tenancy.md's own Boundaries section explicitly scopes out "one user reaching another's record inside the same tenant" as outside its file — security.md owned no object-level/ownership authorization check to pick it up.
  • Sourced from OWASP API1:2023 (Broken Object Level Authorization), the OWASP Authorization Cheat Sheet, and CWE-639; adds security.md's first ## Sources section.
  • Cross-references multi-tenancy.md's "Fetch-by-id without ownership" bullet for the cross-tenant analog, using a code-span sibling ref (that PR, docs(review): add multi-tenancy review criteria #130, has not yet merged to main as of this branch).

Test plan

  • lefthook pre-commit (typos, gitleaks, editorconfig, markdownlint) — passed locally.
  • CI green (markdown lint, lychee, review bot).

🤖 Generated with Claude Code

security.md owned no object-level/ownership authorization check;
multi-tenancy.md's own Boundaries section explicitly scopes out the
within-tenant case (one user reaching another's record inside the
same tenant), leaving it with no home. Closes that gap with a bar
sourced from OWASP API1:2023 and CWE-639.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V1x1KNkkjSe2CjbuTb4Gis
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 0s —— View job


I'll analyze this and get back to you.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16a485734d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread conventions/review/security.md Outdated
The object-level authorization bar in security.md pointed to
multi-tenancy.md, which does not exist in this tree. Drop the
unresolvable cross-reference rather than ship a broken pointer.
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 14s —— View job


PR Review

  • Gather context
  • Read CLAUDE.md and repo conventions
  • Review the PR diff and changed file
  • Analyze content quality and accuracy
  • Post findings

The addition is technically sound, well-sourced, and fills the documented gap from multi-tenancy.md's explicit scope-out of the same-tenant case. Two commits in this branch — the main addition (16a4857) and a follow-up that removed the forward reference to multi-tenancy.md (d4f18b7). Notes below cover both.

Content quality

The criterion itself is accurate:

  • "checks that the requesting principal is authorized for that specific record, not just that a well-formed id was supplied or that the caller holds some role over the object's type" — this correctly distinguishes record-level ownership from type-level RBAC, which is the core of the BOLA gap.
  • "an interface may render only the caller's own records while the endpoint itself still accepts and serves any id" — this is the canonical real-world manifestation and is well worth stating explicitly.
  • "An unguessable id (a GUID) is defense in depth, never a substitute for the check" — critical point, correctly placed after the examples so the contrast lands.

The ## Sources section follows the pattern already established in code-quality.md, error-handling.md, architecture.md, and others — no consistency issue.

Findings

Suggestion — README table description is now incomplete

conventions/review/README.md L27 describes security.md as covering "secrets, injection, trust boundaries, supply chain" — Object-level authorization is not listed. Since the README table is the entry point readers use to choose which review file applies to a change, this omission means reviewers on object-scoped endpoints may not reach security.md at all.

Suggested description update: secrets, injection, trust boundaries, object-level authorization, supply chain

Fix this →

Observation — PR description references the removed cross-reference

The PR summary says the new bullet "cross-references multi-tenancy.md's 'Fetch-by-id without ownership' bullet" and cites the code-span convention as the reason. The follow-up commit (d4f18b7) removed that reference entirely, so the description no longer matches the shipped content. Not a code issue, but the description can be trimmed before merge to avoid confusing reviewers who read it against the diff.

Verdict

The core criterion and sources addition are ready. The README table gap is worth a follow-up on this branch or in a fast PR; the description note is cosmetic. No blocking issues.

@kyle-sexton
kyle-sexton merged commit 1f68997 into main Jul 16, 2026
39 checks passed
@kyle-sexton
kyle-sexton deleted the docs/review-security-bola branch July 16, 2026 12:17
kyle-sexton added a commit that referenced this pull request Jul 17, 2026
No linked issue.

Registers `ec91c3433a8c3c0a7ebbdd239286e5a6a25eeec5` (ci-workflows
v0.6.0, containing ci-workflows#135/#136/#137) so the consumer pin wave
can proceed. Selector-lockstep step 2; the consumer repin PRs (step 3)
follow after this syncs.

- **Selector** — appended to the `melodic-software` owner scope. The
revision admits `merge_group` and `pull_request_target` for
metadata-only required gates (`merge_group` has no fork variant and only
write-access users can enqueue one; `pull_request_target` executes the
trusted base-ref definition) and extends the fork guard to both
pull-request event names, so fork-origin PR contexts still route off the
managed fleet. README review note added; approved-revision counts
updated (nine total, six strict-scheduling).
- **claude-review contract @ec91c343** — mirrors the registered
`df54d0e` runner-input shape exactly (`runner` + `skip-actors`,
`CLAUDE_CODE_OAUTH_TOKEN`, same `allowedCallerPermissions` waiver). The
interface is unchanged since `df54d0e`; the revision's delta is
behavioral only (per-(PR, head-SHA) concurrency keying + superseded-head
guard). The `standards-ref` mount input and `STANDARDS_REVIEW_APP_*`
secrets stay unregistered — no consumer passes them, and
grants/allowances are never provisioned ahead of need.
- **link-check contract @ec91c343** — carries the `3dfb1845`
runner-input shape forward (`runner`, `args`, `contents: read` +
`issues: write` waiver) and admits the revision's new rolling-issue
interface (`issue-title`, `issue-labels`, `issue-type`, `auto-close`),
which the medley docs-link-check adoption consumes next.
- **semantic-pr, do-not-merge-gate, pr-issue-linkage contracts
@ec91c343** — selector-result contracts decline the structural
auto-approval unconditionally (fail-closed required-check behavior is
unobservable to the surface diff), so uniform pins need reviewed
entries. pr-issue-linkage is byte-identical to `f7e94a80`; the other two
changed only `prerequisite-result` description text. Shapes carried
forward verbatim.
- **pulumi-version-drift-check contract @ec91c343** — carries
`allowedCallerPermissions`, which declines auto-approval
unconditionally; byte-identical to the reviewed `3dfb1845` revision.
Shape carried forward verbatim.
- **pester contract @ec91c343** — input-declaration surface changed
(`pester-version` default 5.7.1 → 6.0.0 plus an import version
assertion), so auto-approval correctly refuses; hosted-only
`windows-2025` shape carried forward verbatim.
- **zizmor / osv-scanner** — intentionally NOT registered at `ec91c343`:
both are byte-identical to reviewed revisions with no unobservable-trust
terms, so the deterministic auto-approval extension covers their pin
bumps.

## Verification

- `GITHUB_REPOSITORY=melodic-software/standards npm run
lint:runner-policy` — passed
- `npm run test:runner-policy` — 208/208 (selector-allowlist snapshot
extended)
- `npm run lint:md` — 0 errors

## Related

- melodic-software/github-iac#78 (epic — Campaign A hosted-floor
elimination)
- melodic-software/ci-workflows#135, melodic-software/ci-workflows#136,
melodic-software/ci-workflows#137 (v0.6.0 content)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant