docs(security): recover OIDC review fixes dropped from #637's merge - #654
docs(security): recover OIDC review fixes dropped from #637's merge#654dawsontoth wants to merge 2 commits into
Conversation
…empty by design
The retracted finding on this PR surfaced the actual reason my badge check and the
reviewer's disagreed, and it is worth recording because it will recur.
Harper cuts patch releases from the v5.2 release branch, not from main. v5.2.5's
commit (6a1699f1) diverges from main at 374408e3 and is not an ancestor of it. So
git tag --contains on a main merge commit returns nothing for any v5.2.x tag no
matter how recent that tag is — the feature can only appear there via a
back-port.
That means an empty --contains says "the next minor", not "nobody has released
since", and the existence of a newer patch tag is not evidence against a v5.3.0
badge. Mis-reading that is what produced four Critical findings here.
Also notes to compare <tag>^{commit} rather than tag objects: an annotated tag's
own SHA differs from the commit it points at, which is why the reviewer and I
cited v5.2.5 as two different hashes and were both right.
…e gaps Four findings from @kriszyp, all verified in source first. **The `operations`-scope example could not have been accepted.** It pinned the repository and gated the ref via `environment` but pinned no workflow, so `assertPolicyIsSpecific` would refuse it — in a block presented as copyable. Added `workflow_ref`. Worth noting the failure mode: the page states the three structural requirements correctly a few paragraphs above, and I still wrote an example violating one of them, which is an argument for checking examples against the rules the same page states rather than against intent. **Canonical audience form applies to every issuer, not just GitHub Actions.** `assertAudienceIsCanonical` requires an http(s) audience to carry an explicit port and a trailing slash — `https://my-instance.example.com:9925/`, not `https://my-instance.example.com` — because the audience is matched byte-for-byte and the CLI requests its token for the normalized target. A plausible-looking generic-issuer value is rejected at write time. Documented ahead of the specificity discussion, and the "generic audiences are not checked" line now says what it does *not* cover. **`list_oidc_trust` can return `invalid_reason`**, set when a policy names a user that was later deleted or deactivated. This matters more than a missing field usually would, because the docs send readers to `list_oidc_trust` to diagnose an exchange rejection that deliberately says nothing — and a policy carrying `invalid_reason` is the fastest answer available. Documented with that framing. **The one-style table still sent CI to a refresh token.** It now leads with workload identity where the runtime supports it, refresh token as fallback. I had made this edit earlier and lost it: when the OIDC content was extracted to this branch I re-applied the precedence entry and the section but not the table row, so the page recommended a rotatable secret three paragraphs above telling readers they need no stored credential.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the release and tag documentation in AGENTS.md, recommends workload identity (OIDC) for CI/CD pipelines in the CLI authentication reference, and documents OIDC policy audience matching and the invalid_reason field in the Operations API reference. The review feedback suggests replacing an em dash with a hyphen in the newly added invalid_reason description to comply with Markdown style guidelines.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-654 This preview will update automatically when you push new commits. |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Heh, I hate it when that happens :). Anyway, the main updates look good, but might clarify the agents guidance.
🤖 Reviewed with Codex
| - Derive the version from the core release the change ships in, stripping prerelease suffixes (`5.1.0-beta.1` → `v5.1.0`). | ||
| - **Determine that release from the core repo's git tags, not from the feature branch's `package.json`.** A branch reading `5.2.0-beta.3` says which release was open when the branch started, not which one the change lands in — if a release is cut before the feature merges, the badge is silently wrong. Check `git tag --sort=-creatordate` for the newest release, and `git tag --contains <merge-commit>` for whether the change is in one; a merged-but-untagged feature ships in the _next_ version, which may be a minor bump. Re-check on every refresh pass of a long-lived docs PR, because a release cut between passes invalidates a badge that was correct when written. | ||
| - **Fetch tags and confirm the tag object before trusting the answer.** `git tag --contains` is only as good as the local tag it compares against: a stale or divergent tag of the same name gives a confidently wrong answer in either direction. Run `git fetch --tags`, then check `git rev-parse <tag>` against `git ls-remote --tags origin <tag>`. When a badge is disputed, the decisive check is not `--contains` at all but whether the feature's files exist at the tag — `git ls-tree -r --name-only <tag> | grep <path>`, or `git show <tag>:<file>` — since that cannot be confounded by ancestry or by which tag your clone happens to hold. | ||
| - **Patch tags are cut from a release branch, not from `main`, so `--contains` returns nothing for them by design.** `v5.2.5`'s commit diverges from `main` and is not an ancestor of it — the `v5.2` line is its own branch with selective cherry-picks. A feature merged to `main` therefore appears in **no** `v5.2.x` tag however recent that tag is, and shows up only when a release is cut from `main` (the next minor) or someone back-ports the commit. So an empty `--contains` on a main merge means "the next minor", not "nobody has released since" — and a newer patch tag existing is not evidence against it. Check the release branch directly (`git branch -r --contains <sha>`) if you need to know whether a back-port happened. |
There was a problem hiding this comment.
This backport check cannot detect a selective cherry-pick: cherry-picking creates a new commit SHA, so git branch -r --contains <original-main-sha> remains empty even when the feature is present on the release branch. That can incorrectly assign the next-minor badge to a backported feature. Please recommend inspecting the feature's files on the release branch/tag (as the preceding bullet does) or using patch-equivalence tooling such as git log --cherry instead of an ancestry check.
There was a problem hiding this comment.
To make things funner, 1/3 of the time patch releases come from main, 2/3 of the time they come from a release branch.
There was a problem hiding this comment.
You are right, and it breaks the bullet in the direction that matters: a cherry-pick creates a new SHA, so git branch -r --contains <original-main-sha> stays empty even when the feature is sitting on the release branch — which would badge a back-ported feature as next-minor, the exact error that bullet exists to prevent.
Rewritten in f55789e9 to ask whether the code is there rather than whether the commit is an ancestor, which is what the preceding bullet already does: git show <tag>:<path/to/feature/file> or git ls-tree -r --name-only <tag> | grep <path>. Added git log --cherry-pick --left-right <tag>...main for when you need the commit rather than the file.
The through-line I had missed: every ancestry-based shortcut in this area has a case that inverts it. File presence at the tag is the only test that survives all of them — which is also what settled the disputed badge on #637.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
That is worse than the version I had, and useful — thank you. My bullet asserted patch tags are cut from a release branch, full stop, which is wrong a third of the time and implies an empty --contains reliably means "next minor".
Fixed in f55789e9: roughly a third come off main and the rest off a release branch, so --contains tells you nothing either way until you know which kind of cut that tag was. Combined with the cherry-pick point on the other thread, ancestry is out entirely — the guidance now sends you to check for the feature's files at the tag.
Which is the more useful rule anyway: it does not require knowing how the release was cut, so it holds for all three cases.
🤖 Addressed by Claude Code
2 participants
Recovers three commits that were pushed to #637 but not included when it merged. The squash captured through
e9b474ca;92aa1858,e26277c1anda7c885aclanded after that point and were dropped.92aa1858's content turned out to be inmainalready (its cherry-pick was empty), so this carries the other two.What was lost
@kriszyp's four review findings (
a7c885ac), all verified in source before fixing:operations-scope example pinned the repository and gated the ref but pinned no workflow, soassertPolicyIsSpecificwould refuse it — in a block presented as copyable. Addedworkflow_ref.assertAudienceIsCanonicalrequires an http(s) audience to carry an explicit port and trailing slash, so a plausiblehttps://my-instance.example.comis rejected at write time. Documented, and the "generic audiences are not checked for specificity" line now says what it does not cover.list_oidc_trustreturnsinvalid_reasonwhen a policy's user was deleted or deactivated. It matters more than a missing field usually would: the docs send readers there precisely because the exchange refuses to say what failed.Release-branch badge guidance (
e26277c1) — patch tags are cut from thev5.2release branch, notmain, sogit tag --containsreturning nothing for them is by design rather than evidence. Mis-reading that produced four Critical findings on #637 that were later retracted; the note exists so the next docs PR does not repeat it.Verification
Cherry-picked onto current
mainwith no conflicts.npm run format:checkclean;npm run buildsucceeds with the two anchors pre-existing onmainand no others. Content confirmed present after the pick rather than assumed from a clean apply.Note
Worth knowing why this happened, since the same shape will recur: review comments kept arriving on #637 after it was approved, and each fix pushed a new commit. A squash merge takes the branch state its merge was raised against, so commits landing between approval and merge are silently absent — the PR shows as merged and the branch as fully pushed, which is exactly what made it hard to spot. Re-checking the merge against the branch head is the cheap guard.
Description drafted by Claude Code (Opus 5).