✨ server: track user source attribution#674
Conversation
🦋 Changeset detectedLatest commit: 3430400 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Summary of ChangesHello @nfmelendez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new capability to track and store the origin or source of user accounts during their initial registration and subsequent authentication. By capturing an optional Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds optional source attribution to credentials: new Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as Auth/Registration API
participant DB as Credentials DB
participant Sardine as Sardine Service
Client->>API: POST /auth or /registration (credential data + optional Client-Fid)
API->>API: Validate headers (Client-Fid maxLength 36)
API->>API: Call createCredential(c, credentialId, { webauthn?, source: Client-Fid })
API->>DB: INSERT credential (includes source)
DB-->>API: Insert result (credential id)
API->>Sardine: customer(...) with tags including source
Sardine-->>API: Sardine response
API-->>Client: 200 OK (credential + customer result)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![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 successfully implements user source attribution by introducing a "source" field in the credentials database and integrating it into the authentication and registration flows. The changes are well-tested with new unit tests covering both SIWE and WebAuthn scenarios, including the default source value. The refactoring of the createCredential function to accept an options object is a clean approach, and the update to the sardine customer tracking with the new source tag is correctly handled. The suggested additions of maxLength constraints for client_fid headers are good practices for data integrity.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #674 +/- ##
==========================================
- Coverage 63.66% 61.88% -1.79%
==========================================
Files 169 169
Lines 5882 5289 -593
Branches 1753 1496 -257
==========================================
- Hits 3745 3273 -472
+ Misses 1964 1844 -120
+ Partials 173 172 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@cursor review |
PR SummaryUser source attribution
Written by Cursor Bugbot for commit 81847de. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/createCredential.ts (1)
29-63: Keep source defaults consistent between DB and external tagging.Line 45 stores raw
options?.sourcewhile Line 61 defaults to"EXA"for Sardine tags. This can leave DB rows withnullwhile external tagging shows"EXA". Consider deriving a singlesourcevalue and reusing it.🛠️ Suggested fix
const { x, y } = decodePublicKey(publicKey); const account = deriveAddress(exaAccountFactoryAddress, { x, y }); + const source = options?.source ?? "EXA"; setUser({ id: account }); const expires = new Date(Date.now() + AUTH_EXPIRY); await database.insert(credentials).values([ { account, id: credentialId, publicKey, factory: exaAccountFactoryAddress, transports: options?.webauthn?.transports, counter: options?.webauthn?.counter, - source: options?.source, + source, }, ]); await Promise.all([ setSignedCookie(c, "credential_id", credentialId, authSecret, { expires, httpOnly: true, ...(domain === "localhost" ? { sameSite: "lax", secure: false } : { domain, sameSite: "none", secure: true, partitioned: true }), }), updateWebhookAddresses(webhookId, [account]).catch((error: unknown) => captureException(error)), customer({ flow: { name: "signup", type: "signup" }, customer: { id: credentialId, - tags: [{ name: "source", value: options?.source ?? "EXA", type: "string" }], + tags: [{ name: "source", value: source, type: "string" }], }, }).catch((error: unknown) => captureException(error, { level: "error" })), ]);
🤖 Fix all issues with AI agents
In @.changeset/better-sites-lead.md:
- Around line 1-5: There are duplicate patch changesets for the same package
"@exactly/server"; consolidate them by merging the two patch entries into a
single changeset (combine the human-readable descriptions like "add source to
credentials database table" and any other notes) and remove the redundant
changeset file so only one .changeset declares a patch for "@exactly/server";
ensure the resulting changeset retains all relevant information and uses the
same YAML header format as the existing changesets.
In `@server/api/auth/authentication.ts`:
- Line 249: The handler is reading client_fid directly via
c.req.header("client_fid") instead of using the validated header produced by the
vValidator(...) middleware; replace that raw access with the validated value
provided by the middleware (use the middleware-provided validated header object
– e.g. the request/context validated store produced by vValidator, such as
c.req.valid("header") or c.validated.header – to read client_fid) so the route
uses the validated header data rather than c.req.header("client_fid").
- Line 327: Update the valibot header schema in this file to require a numeric
string for the Client-Fid header (use pipe(string(), regex(/^\d+$/))) so the
header is validated as a Farcaster FID; locate the headers/schema used to
validate request headers in this file (the schema that governs
c.req.header("client_fid")), replace the optional/string validation with
pipe(string(), regex(/^\d+$/)), and keep the existing usage when passing
c.req.header("client_fid") into createCredential(assertion.id, { source: ... })
so only numeric FIDs are accepted.
In `@server/api/auth/registration.ts`:
- Line 254: The code validates the "Client-Fid" header via vValidator("header",
optional(object({ client_fid: optional(string()) }))) but later directly reads
c.req.header("Client-Fid"); change the latter to use the validated header value
produced by the vValidator (the validated client_fid field) instead of
re-reading the raw header so normalization/validation are preserved—replace
usages of c.req.header("Client-Fid") with the validated client_fid from the
validator result (the same property name client_fid returned by the header
validation).
In `@server/test/api/auth.test.ts`:
- Around line 99-122: The test currently asserts only the credential id after
creating via siwe; update the test (in the "creates a credential using siwe"
case) to also fetch the created credential via
database.query.credentials.findFirst (already used) and assert that the
credential's source field is undefined when no client_fid is provided; locate
the call that checks credentials.id (using credentials.id symbol) and add an
expectation that credential?.source is undefined to mirror the registration
tests and ensure default source is not set.
In `@server/test/api/registration.test.ts`:
- Around line 58-82: The test "creates a credential using webauthn (defaults to
EXA)" asserts the customer tag but doesn't verify the stored credential source;
update the test to assert the credential's source is set to "EXA" by extending
the existing mock verification on customer (or adding a separate expectation) to
include credential: expect.objectContaining({ source: "EXA" }) (or use
expect.any/more specific shape if the code stores it under a different key),
referencing the existing expect(customer).toHaveBeenCalledWith call so the
database/save payload includes credential.source === "EXA".
|
Unable to authenticate your request. Please make sure to connect your GitHub account to Cursor. Go to Cursor |
|
@cursor review |
|
Skipping Bugbot: Unable to authenticate your request. Please make sure Bugbot is properly installed and configured for this repository. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/database/schema.ts (1)
23-36: Add migration forcredentials.sourceand configure drizzle output directory.Schema change to
credentials.sourcelacks a corresponding migration file. Additionally,drizzle.config.tsis missing theoutparameter specifying where migrations should be generated. Update the config to include:out: "server/database/migrations"Then run
drizzle-kit generateto generate the required migration file for thesourcecolumn addition.server/api/auth/registration.ts (1)
303-361: Use validated header value instead of re-reading the raw header.Line 255 validates
"Client-Fid", but Line 361 reads the header directly viac.req.header("Client-Fid"). This bypasses the validation and is inconsistent with the established pattern (e.g., howsession_idis accessed viac.req.valid("cookie")). Use the validated header value for consistency.♻️ Suggested refactor
async (c) => { const attestation = c.req.valid("json"); setContext("auth", attestation); const { session_id: sessionId } = c.req.valid("cookie"); + const header = c.req.valid("header"); const challenge = await redis.get(sessionId); if (!challenge) return c.json({ code: "no registration", legacy: "no registration" }, 400); // ... (rest of validation logic) - const result = await createCredential(c, attestation.id, { webauthn, source: c.req.header("Client-Fid") }); + const result = await createCredential(c, attestation.id, { webauthn, source: header?.["Client-Fid"] });server/utils/createCredential.ts (1)
37-46: Add test assertion to verify counter defaults to 0 in SIWE registration.The schema correctly defines
counter: integer("counter").notNull().default(0). Whenoptions?.webauthnis undefined in the SIWE flow,counterwill beundefinedand Drizzle should apply the default. However, the SIWE tests inserver/test/api/auth.test.tsonly validate thesourcefield; they should also assert thatcounteris set to 0 to confirm the default works as expected.
🤖 Fix all issues with AI agents
In `@server/api/auth/authentication.ts`:
- Line 250: The header validation uses the key "Client-Fid" which Hono
normalizes to lowercase so the rule never matches; update the
vValidator('header', ...) schema entries (the object containing "Client-Fid") to
use "client-fid" (lowercase) so the optional(pipe(string(), maxLength(36)))
constraint on client ID is actually enforced — apply this change for both
occurrences referenced in the diff (the vValidator header schemas around the
authentication logic).
♻️ Duplicate comments (1)
server/test/api/auth.test.ts (1)
117-122: Consider verifying source is undefined for the default case.This test verifies the customer call includes
"EXA"as the default source but only assertscredential?.id. For consistency with the "creates a credential with source" test and to ensure the database correctly storesundefinedwhen noClient-Fidis provided, add an assertion forsource.♻️ Suggested addition
const credential = await database.query.credentials.findFirst({ where: eq(credentials.id, id), - columns: { id: true }, + columns: { id: true, source: true }, }); expect(credential?.id).toBe(id); + expect(credential?.source).toBeUndefined(); });
2 participants
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.