feat(auth): auto-generate loopback client_id for local development#131
Open
Kzoeps wants to merge 5 commits intohypercerts-org:developfrom
Open
feat(auth): auto-generate loopback client_id for local development#131Kzoeps wants to merge 5 commits intohypercerts-org:developfrom
Kzoeps wants to merge 5 commits intohypercerts-org:developfrom
Conversation
🦋 Changeset detectedLatest commit: 3321c10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAuto-generates loopback OAuth client_id query params for bare Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer App
participant SDK as SDK OAuthClient
participant Auth as AT Protocol Authorization Server
participant Browser as User Agent
Dev->>SDK: buildClientMetadata(oauth.config)
SDK->>SDK: buildLoopbackClientId(clientId, scope, redirectUri)
SDK-->>Dev: metadata (client_id includes scope & redirect_uri)
Dev->>Browser: redirect user to Auth with metadata.client_id
Browser->>Auth: Authorization request (client_id includes loopback params)
Auth-->>Browser: Authorization response to redirect_uri
Browser->>Dev: deliver auth response to redirect_uri
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sdk-core/README.md`:
- Around line 130-132: The blockquote in README.md contains an empty line inside
the quoted paragraph causing markdownlint MD028; edit the blockquote so there
are no bare blank lines—either remove the blank line or prefix it with ">" to
keep it part of the blockquote (ensure the quoted lines like "**Embed scope and
redirect in client_id**: For loopback clients, embed scope and redirect in
client_id. Otherwise the oauth complains about missing scope and redirect."
remain contiguous and properly prefixed).
aspiers
reviewed
Feb 17, 2026
packages/sdk-core/README.md
Outdated
| // Serve JWKS from your app | ||
| jwksUri: "http://127.0.0.1:3000/.well-known/jwks.json", | ||
| // Client ID embeds all metadata as query parameters | ||
| clientId: `http://localhost?scope=${encodeURIComponent(scope)}&redirect_uri=${encodeURIComponent(redirectUri)}`, |
Contributor
There was a problem hiding this comment.
@Kzoeps Couldn't we simply make it auto-generate these query params in the localhost case? Otherwise it's just creating extra work for the developer, and the SDK is supposed to make dev work as easy as possible.
aspiers
requested changes
Feb 18, 2026
Contributor
aspiers
left a comment
aspiers
left a comment
There was a problem hiding this comment.
Think we can make the DX nicer here, but I could easily be missing something so totally open to hearing arguments why the dev should construct the callback themselves.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.beads/issues.jsonl:
- Line 9: The comment points out mismatches between the description and
acceptance criteria for buildLoopbackClientId (param order and space encoding)
and a missing edge-case for explicit port 80; update the implementation and
tests accordingly: in OAuthClient.add the private method
buildLoopbackClientId(clientId, scope, redirectUri) and its usage in
buildClientMetadata should use the Web URL API/URLSearchParams so spaces are
encoded as "+" (not "%20"), pick the concrete param ordering (make it
redirect_uri first then scope to match the acceptance criterion), and log the
info message "Auto-generating loopback client_id with scope and redirect_uri
query parameters" when you transform; ensure the localhost detection rejects any
clientId that contains explicit port notation (detect by checking the original
clientId string for :<digits> after the hostname so that "http://localhost:80"
is treated as having a port even though URL.port can be empty), do not mutate
this.config, and update tests/acceptance criteria to expect URLSearchParams
encoding (plus for spaces) and the chosen param order and to include the new
case that "http://localhost:80" must remain unchanged.
- Line 12: Update the epic object with id "hypercerts-sdk-bub" so its "status"
is "closed", add a "closed_at" timestamp in the same ISO format as other
entries, and set "close_reason" to a short explanatory string (e.g., "All child
tasks complete: use isValidUri change applied in hypercerts-sdk-bub.1"). Locate
the JSON object whose "id" equals "hypercerts-sdk-bub" in .beads/issues.jsonl
and modify those three fields, leaving all other fields unchanged.
- Line 10: The tests only assert logging and should also verify the actual
client_id produced; update the new describe("loopback client_id
auto-generation") block in OAuthClient.test.ts to, after creating the
OAuthClient via createTestConfigAsync (using the same oauth overrides and
MockLogger), call the client metadata builder (e.g., buildClientMetadata or
inspect OAuthClient.metadata/client_id returned by the constructor) and add
assertions: for tests 1 and 2 assert metadata.client_id contains both encoded
scope and redirect_uri (or exactly equals the expected appended URL), and for
tests 3–5 assert metadata.client_id remains unchanged (no appended params); keep
the MockLogger assertions but add these client_id checks so the tests validate
actual URL transformation in addition to logging.
In `@packages/sdk-core/README.md`:
- Line 69: The clientId example uses `http://localhost?…` which lacks the
explicit root path; update the `clientId` construction to use
`http://localhost/?scope=...&redirect_uri=...` (i.e., add the trailing slash)
and make the same change for the other occurrence mentioned in the README so the
example aligns with the "IP vs Hostname" guidance; locate the `clientId` example
string in the README and replace the URL form accordingly.
- Line 131: Replace the lowercase occurrence of "oauth" in the sentence "oauth
complains about missing scope and redirect." with the proper capitalisation
"OAuth" so the README uses consistent acronym casing; locate that exact string
in the README.md and update it to "OAuth complains about missing scope and
redirect." ensuring no other wording is changed.
---
Duplicate comments:
In `@packages/sdk-core/README.md`:
- Around line 130-132: The MD028 lint error is caused by a blank line breaking
the blockquote after the line starting with "Embed scope and redirect in
client_id"; fix it by either removing the empty line or prefixing it with '>' so
it remains part of the blockquote in packages/sdk-core/README.md, ensuring the
quoted paragraph stays contiguous and the blockquote is not terminated
prematurely.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.beads/issues.jsonl:
- Line 12: Update the epic issue record for "hypercerts-sdk-bub" so its "status"
is "closed" and add matching "closed_at" and "close_reason" values to mirror its
closed child "hypercerts-sdk-bub.1" (which references commit 9eb7015);
specifically set the parent issue's "status":"closed", populate "closed_at" with
the same timestamp used by the child, and set "close_reason" to the same commit
reference/text so the parent and child closure metadata match.
- Line 10: Tests only assert logging, so a broken buildLoopbackClientId could
pass; update the new describe("loopback client_id auto-generation") tests to
also assert the generated client_id value. For cases that should be
auto-modified, call buildLoopbackClientId directly (or construct OAuthClient via
new OAuthClient(createTestConfigAsync(...)) and read the effective client_id
from the OAuthClient instance/config) and assert it contains the expected
appended query params (scope=... and redirect_uri=...) or the specific "atproto
transition:generic" scope; for cases that should not change, assert the
client_id remains exactly unchanged. Keep using MockLogger for log assertions
and createTestConfigAsync + MockLogger for setup in each test.
- Line 9: The comment flags an edge case where explicit ":80" must not be
treated as “no port” and asks for a deterministic encoding form (use + for
spaces); update the new private method buildLoopbackClientId and its usage in
buildClientMetadata so that you: parse the original clientId into a URL, treat
it as a localhost loopback only if url.protocol === 'http:', url.hostname ===
'localhost', url.search === '' (no query params), and there is no explicit port
— i.e., url.port === '' AND the original clientId string does NOT contain an
explicit ':80' token (detect via a simple substring or regex like /:80(\/|$)/)
so that "http://localhost:80" is NOT auto-generated; when auto-generating build
the query using URLSearchParams (so spaces become '+' as required) with scope
hardcoded to "atproto transition:generic" and redirect_uri set from
this.config.oauth.redirectUri, log the info message and a warning if
this.config.oauth.scope differs, set metadata.client_id to the new URL string
and metadata.scope to "atproto transition:generic", and do not mutate
this.config — call buildLoopbackClientId at the top of buildClientMetadata and
use its returned value for client_id.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/sdk-core/src/auth/OAuthClient.ts (1)
199-241: Redundantnew URL(clientId)on line 221.
url(line 201) already holds the parsed URL and can be reused. CreatingloopbackUrlfrom the sameclientIdis unnecessary.Proposed fix
- // Auto-generate query parameters - const loopbackUrl = new URL(clientId); - loopbackUrl.searchParams.set("scope", "atproto transition:generic"); - loopbackUrl.searchParams.set("redirect_uri", redirectUri); + // Auto-generate query parameters + url.searchParams.set("scope", "atproto transition:generic"); + url.searchParams.set("redirect_uri", redirectUri);and update line 236:
- return loopbackUrl.toString(); + return url.toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/src/auth/OAuthClient.ts` around lines 199 - 241, In buildLoopbackClientId, remove the redundant new URL(clientId) assigned to loopbackUrl and reuse the already-parsed url variable: update the URL's searchParams (set "scope" to "atproto transition:generic" and "redirect_uri" to redirectUri) on url and return url.toString(); ensure you no longer create loopbackUrl and that the info/warn logging remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk-core/src/auth/OAuthClient.ts`:
- Around line 199-241: The buildLoopbackClientId function incorrectly treats
URLs like "http://localhost:80" as portless because URL normalization hides
default ports; fix it by detecting an explicit port in the original clientId
string before deciding it's a bare localhost. In buildLoopbackClientId, add a
check that inspects the raw clientId (e.g., regex to detect :<digits>
immediately after the hostname) and only consider it "no port" when the original
string contains no explicit port; keep the existing URL-based checks (protocol,
hostname, pathname, search) but require the new explicit-port check to be false
before auto-generating loopback query params and logging/warning.
---
Duplicate comments:
In @.beads/issues.jsonl:
- Line 12: Update the epic record with id "hypercerts-sdk-bub" to mark it
closed: set "status" to "closed", add a "closed_at" timestamp (use the same or
an appropriate ISO timestamp) and set "close_reason" to reference the child task
closure (e.g., "closed due to child task hypercerts-sdk-bub.1 closed via commit
9eb7015"). Ensure the epic's metadata mirrors the child (hypercerts-sdk-bub.1)
close information so the parent is no longer open.
---
Nitpick comments:
In `@packages/sdk-core/src/auth/OAuthClient.ts`:
- Around line 199-241: In buildLoopbackClientId, remove the redundant new
URL(clientId) assigned to loopbackUrl and reuse the already-parsed url variable:
update the URL's searchParams (set "scope" to "atproto transition:generic" and
"redirect_uri" to redirectUri) on url and return url.toString(); ensure you no
longer create loopbackUrl and that the info/warn logging remains unchanged.
Kzoeps
force-pushed
the
local-dev-docs
branch
2 times, most recently
from
b0a24dd to
6345e19
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/sdk-core/tests/auth/OAuthClient.test.ts (1)
295-526: Good coverage of the main loopback scenarios.The 7 test cases cover positive, negative, and scope-override paths effectively. One gap: consider adding a test for
http://localhost:80— the default HTTP port is normalized away bynew URL(), making it indistinguishable from barehttp://localhostin the current implementation (related to thebuildLoopbackClientIdedge case).Suggested additional test case
it("should not modify localhost client_id with default port 80", async () => { const { MockLogger } = await import("../utils/mocks.js"); const logger = new MockLogger(); const baseConfig = await createTestConfigAsync({ logger }); const configWithPort80 = await createTestConfigAsync({ logger, oauth: { ...baseConfig.oauth, clientId: "http://localhost:80", scope: "atproto", redirectUri: "http://127.0.0.1:3000/callback", developmentMode: true, }, }); const client = new OAuthClient(configWithPort80); try { await client.authorize("test.bsky.social"); } catch (_error) { // Expected } const infoLogs = logger.logs.filter((log) => log.level === "info"); const autoGenLog = infoLogs.find((log) => log.message.includes("Loopback client detected")); expect(autoGenLog).toBeUndefined(); });Note: this test would currently fail due to the port-normalization issue in
buildLoopbackClientId— fixing it there would make this test pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/auth/OAuthClient.test.ts` around lines 295 - 526, buildLoopbackClientId treats "http://localhost:80" the same as "http://localhost" because new URL() normalizes away the default port, causing incorrect auto-generation; modify buildLoopbackClientId to detect an explicitly-specified default port by inspecting the raw clientId string (e.g., check if the original clientId includes ":80" for http) or use URL.port and fallback to checking the original string, and if an explicit port is present do not perform the loopback auto-append; update the logic in the OAuthClient initialization/path that calls buildLoopbackClientId to use this enhanced detection so a clientId with explicit ":80" is treated as having a port and will not trigger auto-generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.beads/issues.jsonl:
- Line 12: The epic entry with "id":"hypercerts-sdk-bub" is still marked
"status":"open" while its child `hypercerts-sdk-bub.1` is closed; update that
JSON object so "status" is "closed" and add a "closed_at" timestamp (ISO 8601)
and a "close_reason" string (e.g., "all child issues closed" or "resolved");
ensure you preserve all other fields and JSON formatting for the object in
.beads/issues.jsonl and do not modify other entries.
In `@packages/sdk-core/README.md`:
- Around line 130-136: The MD028 lint error is caused by the blank line
separating the two blockquote paragraphs beginning with "**Auto-generated
loopback params**" and "**Authorization Server Support**"; fix it by either
removing that empty line to join them into a single continuous blockquote or by
prefixing the blank line with ">" so both sections remain separate but still
inside the blockquote (edit the README.md content around those two headings).
In `@packages/sdk-core/src/auth/OAuthClient.ts`:
- Around line 199-241: In buildLoopbackClientId, the parsed URL's port is empty
for default ports (e.g., "http://localhost:80"), so the current !url.port check
wrongly treats explicit :80/:443 as bare localhost; add a pre-parse string-level
guard that returns clientId unchanged if the original clientId contains an
explicit port on localhost (e.g., match /^https?:\/\/localhost:\d+(\/|$)/ or
specifically check for :80 when protocol is http and :443 when https) before
creating new URL, so explicit-port URLs bypass the auto-generation; update the
method buildLoopbackClientId to perform this check and then continue with the
existing URL-based logic.
---
Nitpick comments:
In `@packages/sdk-core/tests/auth/OAuthClient.test.ts`:
- Around line 295-526: buildLoopbackClientId treats "http://localhost:80" the
same as "http://localhost" because new URL() normalizes away the default port,
causing incorrect auto-generation; modify buildLoopbackClientId to detect an
explicitly-specified default port by inspecting the raw clientId string (e.g.,
check if the original clientId includes ":80" for http) or use URL.port and
fallback to checking the original string, and if an explicit port is present do
not perform the loopback auto-append; update the logic in the OAuthClient
initialization/path that calls buildLoopbackClientId to use this enhanced
detection so a clientId with explicit ":80" is treated as having a port and will
not trigger auto-generation.
e06d6ee to
dee032e
Compare
When clientId is any loopback URL (detected via isLoopbackUrl), the SDK takes over completely: builds http://localhost?scope=atproto+transition:generic &redirect_uri=<config.redirectUri> regardless of what the user passed. - If isLoopbackUrl(clientId), always rewrite to http://localhost with params - Scope hardcoded to 'atproto transition:generic' (AT Protocol spec requirement) - Warn only when rewriting from non-standard loopback (e.g. 127.0.0.1:3000) - No developmentMode flag needed — loopback URL is the signal
7 tests covering: bare localhost, trailing slash, 127.0.0.1 rewrite, localhost:3000 rewrite, existing query params rewrite, non-loopback passthrough, and scope always being atproto transition:generic.
Kzoeps
force-pushed
the
local-dev-docs
branch
5 times, most recently
from
0a8f51c to
2a98b05
Compare
Kzoeps
added a commit
to Kzoeps/hypercerts-sdk
that referenced
this pull request
Feb 20, 2026
Kzoeps
force-pushed
the
local-dev-docs
branch
2 times, most recently
from
ca07a52 to
0c15a58
Compare
Kzoeps
changed the title
Contributor
Author
the redirect uri still has to be in the developer or sdk consumers hand since their project structures might be different but. I agree with you for the loopback client_id the sdk can just generate it and make it easy. So now the SDK if the client id is a loopback url automatically configures the client_id with embedded scope and redirect_uri. Let me know if this makes sense or it needs any changes? |
Kzoeps
force-pushed
the
local-dev-docs
branch
2 times, most recently
from
913d94c to
9e68179
Compare
Kzoeps
force-pushed
the
local-dev-docs
branch
3 times, most recently
from
42749eb to
828fc9f
Compare
Loopback clients previously hardcoded 'atproto transition:generic' as the scope in both the client_id URL and metadata body, ignoring the user's configured scope. Now buildClientId() and buildClientMetadata() both use config.oauth.scope, keeping the client_id URL, metadata, and authorize() call consistent. - Use scope param in buildClientId() instead of hardcoded value - Remove loopback ternary override in buildClientMetadata() - Update tests to verify configured scope is embedded - Fix misleading README comment about scope being auto-set - Update changeset to reflect scope comes from config
Kzoeps
force-pushed
the
local-dev-docs
branch
from
828fc9f to
3321c10
Compare
Contributor
Author
|
@coderabbitai review it once pless |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
client_idwhen a loopback URL is detected (localhost,127.0.0.1,[::1], with or without port)atproto transition:genericfor loopback clients; a warning is logged if the configured scope would have been overriddenMigration
No breaking changes. If you were previously constructing the loopback
client_idmanually, you can simplify:Before
After
Summary by CodeRabbit