Skip to content

[codex] Harden draft OAuth client auth#27

Open
glassBead-tc wants to merge 2 commits into
mainfrom
codex/issue-20-auth-hardening
Open

[codex] Harden draft OAuth client auth#27
glassBead-tc wants to merge 2 commits into
mainfrom
codex/issue-20-auth-hardening

Conversation

@glassBead-tc

Copy link
Copy Markdown
Member

Summary

Addresses #20.

  • Validate authorization-response iss for authorization-code exchanges and pass the captured issuer through the HTTP auth retry path.
  • Bind saved dynamic client credentials to the issuing authorization server, re-register on issuer changes, set DCR application_type, prefer Client ID Metadata Documents when advertised, and union scopes during re-authorization.
  • Add check:auth-hardening to the package verification gate and update conformance/readiness docs to separate passing client-auth evidence from the missing authorization-server target.

Validation

  • corepack pnpm run verify passed.
  • corepack pnpm run e2e:draft passed.
  • corepack pnpm run conformance:client-auth passed: 14 scenarios, 569 checks, 0 failures, 0 warnings. Artifact: .local/conformance/client-auth-2026-06-27T22-09-51-088Z.
  • corepack pnpm run conformance:authorization recorded the expected missing-target blocker because no MCP_AUTHORIZATION_CONFORMANCE_FILE or MCP_AUTHORIZATION_CONFORMANCE_URL was configured. Artifact: .local/conformance/authorization-2026-06-27T22-08-03-278Z; readiness summary: .local/readiness-evidence/conformance-authorization.json.

@glassBead-tc glassBead-tc marked this pull request as ready for review June 28, 2026 04:39

@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: 7f19e5e32e

ℹ️ 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 src/examples/everything-client.ts Outdated
Comment on lines +113 to +115
if (!this.authorizationIssuerState) {
throw new Error("No authorization response issuer")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Allow redirects without an iss value

When an authorization server does not advertise authorization_response_iss_parameter_supported, auth() accepts an undefined issuer, but this provider now throws as soon as the redirect lacks iss. In those environments the normal authorization-code retry in handle401 fails before token exchange even though missing iss is allowed; store/pass undefined instead of making the getter mandatory.

Useful? React with 👍 / 👎.

@greptile-apps

greptile-apps Bot commented Jun 28, 2026

Copy link
Copy Markdown

Greptile Summary

This PR hardens draft OAuth client authentication and updates the related verification evidence. The main changes are:

  • Validates authorization-response iss for authorization-code exchanges.
  • Binds dynamic client credentials to the issuing authorization server.
  • Re-registers clients when the issuer changes.
  • Adds DCR application_type, CIMD preference, and scope union behavior.
  • Adds check:auth-hardening to verification and readiness gates.
  • Updates conformance docs for passing client-auth evidence and remaining authorization-server setup.

Confidence Score: 4/5

The OAuth hardening is mostly well scoped, but the example HTTP auth retry path can reject valid authorization-code redirects before token exchange.

The reviewed change set is narrow and the remaining issue is localized to the example client retry helper, with a focused runtime check confirming the failure mode.

src/examples/everything-client.ts

T-Rex T-Rex Logs

What T-Rex did

  • Reproduced a focused local OAuth authorization-code flow with metadata that omitted the issuer parameters, and confirmed the core validator allows a missing issuer.
  • Initially, the auth-hardening step failed due to an undefined registration application_type, and later it passed with AUTH_HARDENING_EXIT_CODE: 0.
  • Checked the docs verification gates before and after; after shows all static contract checks pass, but the core auth-hardening check still fails.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 New auth-hardening verification gate fails on head

    • Bug
      • The PR adds check:auth-hardening to the verification/readiness contract, but running the new gate on head fails immediately in the test DCR marks localhost redirect clients as native applications. The captured output shows Auth hardening check failed and an assertion where recorder.registrations[0].application_type is undefined instead of the expected 'native'. Because this new script is also included in verify.mjs, the verification gate added by the PR is red.
    • Cause
      • scripts/check-auth-hardening.mjs expects dynamic client registration for localhost redirects to include application_type: 'native', but the actual registration payload produced by the current auth flow does not set that field.
    • Fix
      • Update the auth/DCR implementation so localhost redirect registrations include application_type: 'native', or adjust the hardening check if the contract is intentionally different. Re-run corepack pnpm run check:auth-hardening and the verify gate after the fix.

    T-Rex Ran code and verified through T-Rex

Fix All in Cursor Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/examples/everything-client.ts:112-117
**Allow missing issuer**
`handle401` always calls `getAuthorizationResponseIssuer()` before passing the value into `auth`, but `auth.validateAuthorizationResponseIssuer` accepts `undefined` when the authorization server does not advertise `authorization_response_iss_parameter_supported`. A valid authorization-code redirect from that server has no `iss`, so this helper throws before `auth` can complete the token exchange.

Reviews (1): Last reviewed commit: "Harden draft OAuth client auth" | Re-trigger Greptile

Comment thread src/examples/everything-client.ts Outdated
Comment on lines +112 to +117
getAuthorizationResponseIssuer(): string {
if (!this.authorizationIssuerState) {
throw new Error("No authorization response issuer")
}
return this.authorizationIssuerState
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Allow missing issuer
handle401 always calls getAuthorizationResponseIssuer() before passing the value into auth, but auth.validateAuthorizationResponseIssuer accepts undefined when the authorization server does not advertise authorization_response_iss_parameter_supported. A valid authorization-code redirect from that server has no iss, so this helper throws before auth can complete the token exchange.

Artifacts

Repro: focused local OAuth missing issuer harness

  • Contains supporting evidence from the run (text/javascript; charset=utf-8).

Repro: execution log showing helper throw before token exchange

  • Keeps the command output available without making the summary code-heavy.

View artifacts

T-Rex Ran code and verified through T-Rex

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/examples/everything-client.ts
Line: 112-117

Comment:
**Allow missing issuer**
`handle401` always calls `getAuthorizationResponseIssuer()` before passing the value into `auth`, but `auth.validateAuthorizationResponseIssuer` accepts `undefined` when the authorization server does not advertise `authorization_response_iss_parameter_supported`. A valid authorization-code redirect from that server has no `iss`, so this helper throws before `auth` can complete the token exchange.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Cursor Fix in Claude Code Fix in Codex

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