Skip to content

feat: Sign in with OpenClaw ID - #166

Open
steipete wants to merge 3 commits into
mainfrom
feat/openclaw-id-login
Open

feat: Sign in with OpenClaw ID#166
steipete wants to merge 3 commits into
mainfrom
feat/openclaw-id-login

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Adds OIDC authorization-code login against id.openclaw.ai alongside GitHub and magic links: hardened flow mirroring the GitHub OAuth path (DB-backed single-use state, browser-binding cookie, S256 PKCE, client_secret_basic), user linking by verified email via the shared GetOrCreateUserByEmail helper, same session issuance. Web login gains an OpenClaw ID button (browser only; desktop handoff is a follow-up). Full Go suite, web tests, lint/typecheck/fmt green. Codex autoreview clean (0.96). Client registered (PKCE required); Worker secrets set — redeploy picks them up.

Adds an authorization-code login against the first-party OpenClaw ID
provider (issuer https://id.openclaw.ai/api/auth) alongside GitHub OAuth:
GET /api/auth/openclaw/start redirects with signed state, browser binding,
and a S256 PKCE challenge reusing the existing OAuth transaction store;
GET /api/auth/openclaw/callback exchanges the code via client_secret_basic,
validates the id_token claims (issuer, audience, expiry, verified email),
links or creates the user by email exactly like the magic-link path, and
issues the same session cookie. Credentials arrive via OPENCLAW_ID_CLIENT_ID
and OPENCLAW_ID_CLIENT_SECRET (optional OPENCLAW_ID_ISSUER override) and are
passed through the Cloudflare Worker container env when present. The web
login shows a browser-only OpenClaw ID button next to GitHub.
@clawsweeper

clawsweeper Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 14, 2026
@clawsweeper

clawsweeper Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 14, 2026, 10:12 PM ET / August 15, 2026, 02:12 UTC.

ClawSweeper review

What this changes

This PR adds browser-only OpenClaw ID OIDC sign-in, its API routes and deployment configuration, documentation, metrics, and a web login button.

Merge readiness

Blocked until real behavior proof is added - 13 items remain

Keep open: the prior P1 security findings remain unfixed, and the optional login button also exposes an unavailable route on unconfigured deployments. Real browser/provider proof is still missing.

Priority: P1
Reviewed head: 5439a69eaf8ac22d5f4f2bd673dbea57e2f1caa2
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The implementation has focused tests but remains blocked by two P1 authentication flaws and missing real provider proof.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The supplied context contains no redacted after-fix browser login-and-return trace against the registered provider; in-process tests and stated checks are supplemental only. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The supplied context contains no redacted after-fix browser login-and-return trace against the registered provider; in-process tests and stated checks are supplemental only. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 8 items Feature is not on current main: Current main exposes GitHub OAuth routes but has no OpenClaw ID start or callback route; the requested capability remains unique to this PR.
Signature verification is skipped: The callback parses the returned ID token with ParseUnverified and then uses its claims to provision a user and issue a session.
Tests accept arbitrary HMAC-signed ID tokens: The new flow test creates HS256 tokens with a local arbitrary key, which passes because the production handler does not verify a signature or permitted signing method.
Findings 3 actionable findings [P1] Preserve restricted-deployment admission controls
[P1] Verify the ID-token signature before using claims
[P2] Hide the optional sign-in control when unavailable
Security Needs attention ID-token signature is never verified: The callback parses claims with ParseUnverified, so no trusted signing key or allowed signing algorithm authenticates the token before a session is issued.
Configured admission controls are bypassed: The new provider grants normal workspace membership without applying the existing organization or moderator admission policy.

How this fits together

ClickClack’s web login starts an OAuth transaction and sends the browser to an identity provider. The callback validates identity data, provisions or links a user, and creates the normal session cookie for the web app.

flowchart LR
  Browser[Browser login] --> Start[OAuth start route]
  Start --> Provider[OpenClaw ID provider]
  Provider --> Callback[OAuth callback]
  Callback --> Validation[Identity validation]
  Validation --> Account[User and workspace]
  Account --> Session[Session cookie]
  Session --> App[ClickClack web app]
Loading

Decision needed

Question Recommendation
Should OpenClaw ID be unavailable on deployments using GitHub organization or moderator restrictions, or should ClickClack define an equivalent provider-backed admission and role contract first? Preserve existing restrictions: Reject or hide OpenClaw ID where existing organization or guest-role restrictions are configured until equivalent provider-backed checks exist.

Why: The current provider has no implemented organization or moderator mapping, so accepting all verified OpenClaw ID accounts changes established deployment access boundaries.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The supplied context contains no redacted after-fix browser login-and-return trace against the registered provider; in-process tests and stated checks are supplemental only. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Preserve restricted-deployment admission controls (P1) - When GitHub organization or moderator restrictions are configured, the GitHub callback verifies membership and selects the appropriate workspace role. This callback instead creates a normal default-workspace member for every verified OpenClaw ID email, bypassing those deployment controls.
  • Verify the ID-token signature before using claims (P1) - ParseUnverified does not authenticate the token, yet its email claim is used to create a user and session. Fetch the provider’s trusted signing keys and use ParseWithClaims with an allowed algorithm, issuer, audience, and expiry checks; the current HS256 fixture masks this gap.
  • Hide the optional sign-in control when unavailable (P2) - The new button is rendered on every browser login page, while the start route returns 501 when OpenClaw ID credentials are unset. Self-hosted deployments without this optional provider receive a dead login action. This was also present on the earlier reviewed head; expose configured providers to the UI or suppress the control.
  • Resolve security concern: ID-token signature is never verified - The callback parses claims with ParseUnverified, so no trusted signing key or allowed signing algorithm authenticates the token before a session is issued.
  • Resolve security concern: Configured admission controls are bypassed - The new provider grants normal workspace membership without applying the existing organization or moderator admission policy.
  • Resolve merge risk (P1) - Restricted or guest deployments can create ordinary member sessions through OpenClaw ID without the configured GitHub organization and moderator controls.
  • Resolve merge risk (P1) - An ID token’s email claim can create a session without cryptographic signature verification.
  • Resolve merge risk (P1) - No redacted real browser login-and-return proof against the registered provider is present.
  • Complete next step (P2) - Maintainers need to choose the restricted-deployment policy before a repair can safely complete the authentication boundary.
  • Improve patch quality - Resolve the token-verification and admission-policy findings.
  • Improve patch quality - Add redacted browser login-and-return evidence against the registered provider, then update the PR body for re-review.

Findings

  • [P1] Preserve restricted-deployment admission controls — apps/api/internal/httpapi/openclawid.go:169-174
  • [P1] Verify the ID-token signature before using claims — apps/api/internal/httpapi/openclawid.go:232-234
  • [P2] Hide the optional sign-in control when unavailable — apps/web/src/ChatApp.svelte:4093-4094
  • [high] ID-token signature is never verified — apps/api/internal/httpapi/openclawid.go:234
  • [high] Configured admission controls are bypassed — apps/api/internal/httpapi/openclawid.go:169
Agent review details

Security

Needs attention: The added identity-provider trust boundary has two concrete session-creation flaws that must be resolved before merge.

Review metrics

Metric Value Why it matters
Patch size 25 files, +863/-49 lines The feature spans API authentication, deployment configuration, protocol documentation, web UI, and generated web assets.
Authentication surface 2 routes added, 3 environment inputs added The new provider adds an authorization callback boundary and deployment-managed credentials.

Merge-risk options

Maintainer options:

  1. Repair the trust and admission boundary (recommended)
    Add cryptographic ID-token verification and preserve restricted-deployment admission behavior before merge.
  2. Explicitly approve the access-policy change
    Merge only after maintainers choose and document that verified OpenClaw ID accounts may bypass existing GitHub-specific restrictions.

Technical review

Best possible solution:

Use standard OIDC key discovery or a provider-authoritative JWKS path to verify ID-token signatures, and preserve existing restricted-deployment admission and role policy before enabling this provider.

Do we have a high-confidence way to reproduce the issue?

Yes: source inspection shows that a valid-claim token is parsed without signature verification and that the new callback provisions a normal member without the existing GitHub organization gate.

Is this the best way to solve the issue?

No: the feature needs cryptographic token verification and an explicit restricted-deployment policy before the new provider can safely create sessions.

Full review comments:

  • [P1] Preserve restricted-deployment admission controls — apps/api/internal/httpapi/openclawid.go:169-174
    When GitHub organization or moderator restrictions are configured, the GitHub callback verifies membership and selects the appropriate workspace role. This callback instead creates a normal default-workspace member for every verified OpenClaw ID email, bypassing those deployment controls.
    Confidence: 0.98
  • [P1] Verify the ID-token signature before using claims — apps/api/internal/httpapi/openclawid.go:232-234
    ParseUnverified does not authenticate the token, yet its email claim is used to create a user and session. Fetch the provider’s trusted signing keys and use ParseWithClaims with an allowed algorithm, issuer, audience, and expiry checks; the current HS256 fixture masks this gap.
    Confidence: 0.99
  • [P2] Hide the optional sign-in control when unavailable — apps/web/src/ChatApp.svelte:4093-4094
    The new button is rendered on every browser login page, while the start route returns 501 when OpenClaw ID credentials are unset. Self-hosted deployments without this optional provider receive a dead login action. This was also present on the earlier reviewed head; expose configured providers to the UI or suppress the control.
    Confidence: 0.96
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against 18acea79465c.

Labels

Label justifications:

  • P1: The patch can bypass existing deployment admission controls and create sessions from unverified identity-token signatures.
  • merge-risk: 🚨 compatibility: Existing organization and guest-role deployment settings do not apply to the new sign-in path.
  • merge-risk: 🚨 security-boundary: The callback accepts OAuth identity tokens and creates browser sessions without signature verification.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The supplied context contains no redacted after-fix browser login-and-return trace against the registered provider; in-process tests and stated checks are supplemental only. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

  • [high] ID-token signature is never verified — apps/api/internal/httpapi/openclawid.go:234
    The callback parses claims with ParseUnverified, so no trusted signing key or allowed signing algorithm authenticates the token before a session is issued.
    Confidence: 0.99
  • [high] Configured admission controls are bypassed — apps/api/internal/httpapi/openclawid.go:169
    The new provider grants normal workspace membership without applying the existing organization or moderator admission policy.
    Confidence: 0.98

What I checked:

Likely related people:

  • Shakker: Introduced GitHub OAuth PKCE hardening and subsequent OAuth error-redaction work in the affected authentication area. (role: OAuth hardening contributor; confidence: high; commits: 75768abdb205, 92d4b8243ed8; files: apps/api/internal/httpapi/github.go)
  • Peter Steinberger: Introduced the current signed-JWT verification path for Cloudflare Access and recently owns current-main authentication surfaces. (role: identity-verification pattern contributor; confidence: high; commits: 992f404d4204, 18acea79465c; files: apps/api/internal/httpapi/access.go, apps/api/internal/httpapi/server.go)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-14T19:59:14.430Z sha c563f5f :: needs real behavior proof before merge. :: [P1] Preserve the configured organization admission boundary
  • reviewed 2026-08-14T21:28:19.102Z sha 336d52d :: needs real behavior proof before merge. :: [P1] Preserve the configured organization admission boundary | [P1] Verify the ID-token signature before creating a session

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant