Feature/auth package - #24
Merged
Merged
Conversation
renderMarkdown() only HTML-escaped the contents of fenced and inline code spans. Everything else in a .md/.mdx body passed through untouched, and the resulting HTML was injected with dangerouslySetInnerHTML in both createApp.ts and build.ts — so a body containing a literal <script> tag executed in the visitor's browser. escapeHtml() only touches &, <, > and ", so escaping the entire source up front leaves every markdown construct (headings, fences, backticks, brackets) intact while guaranteeing prose, code, link text and headings can never emit raw HTML. The tags the renderer introduces are emitted after escaping, so they remain real HTML; the code-fence pass no longer re-escapes (that would double-encode the & entities just produced). While here, add support for '- item' / '1. item' lists: the block-wrapping logic already checked for <ul>/<ol> prefixes but nothing produced them, so list lines fell through as plain paragraphs. A new renderListBlock() converts contiguous list blocks into <ul>/<ol> before the paragraph wrapper runs, with inline formatting applied first. Regression tests cover raw-HTML escaping in prose, fenced and inline code, list rendering, inline formatting inside items, and no double-escaping.
Escapes the entire markdown source before rendering so raw HTML in .md/.mdx bodies can't execute when injected via dangerouslySetInnerHTML, and adds <ul>/<ol> list rendering plus regression tests.
…ns atomically
Two related defects in the migration runner:
1. SQL injection in runPostgresMigrations(). The bookkeeping INSERT was
built with raw string interpolation (VALUES ('${file}')) instead of a
parameterized call, so a migration filename containing a quote could
inject SQL into the statement. Fix it to use $1 via a parameterized
unsafe() call, matching the SQLite path which already used ?1.
2. No transactional guarantees around a migration's SQL + its bookkeeping
insert. Neither runner wrapped the two together, so a migration that
failed partway left the schema half-applied with no record it was
attempted — a retry then replayed the broken statements against the
already-mutated schema (e.g. 'table already exists').
- SQLite: db.transaction() now wraps db.run(sql) and the INSERT, so the
pair commits or rolls back together.
- Postgres: PostgresClient gains a begin() that runs a callback inside a
transaction (committing on resolve, rolling back on throw), backed by
Bun.SQL's transaction API and routed through the retry/priming proxy.
runPostgresMigrations() uses it for migration SQL + bookkeeping insert.
Adds migrate.test.ts, the migration runner's first test coverage
(ROADMAP.md listed it as untested), using an in-memory SQLite db:
apply-in-order, skip-already-applied, rollback-on-failure, and
retry-after-fix. Note: bun:sqlite 1.3.14 does not roll back DDL statements
(CREATE TABLE/INDEX) inside transactions even when explicitly begun —
verified against the sqlite3 CLI which does — so the rollback/retry tests
exercise a DML failure that the driver rolls back correctly; the transaction
wrapping still protects the bookkeeping record on every path.
Parameterizes the Postgres migration bookkeeping INSERT to close the SQL injection, and wraps each migration's SQL + bookkeeping record in a transaction for both SQLite (db.transaction) and Postgres (new client.begin) so a failing migration rolls back instead of leaving a half-applied schema. Adds migrate.test.ts covering apply-in-order, skip, rollback, retry.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds the ChangesAuthentication package
Markdown rendering
Transactional migrations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Auth
participant GitHub
participant SessionStore
Client->>Auth: request sign-in or callback
Auth->>GitHub: exchange authorization code
GitHub-->>Auth: return tokens and user profile
Auth->>SessionStore: create or find session
SessionStore-->>Auth: return session
Auth-->>Client: set cookie and return response
sequenceDiagram
participant MigrationRunner
participant PostgresClient
participant PostgreSQL
MigrationRunner->>PostgresClient: begin migration transaction
PostgresClient->>PostgreSQL: execute migration SQL
PostgresClient->>PostgreSQL: record migration filename
PostgreSQL-->>PostgresClient: commit or rollback
PostgresClient-->>MigrationRunner: return migration result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
abdelkabirouadoukou
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/data/migrate.test.ts (1)
32-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCreate the migration fixtures in
beforeAll.
beforeAllcreates only an empty directory. Each test creates migration files in its test body. Define the required fixture files inbeforeAll, then keep cleanup inafterAll.As per coding guidelines, “Write tests with
bun:test; create fixtures inbeforeAlland clean them up inafterAllunder__fixtures__/.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/data/migrate.test.ts` around lines 32 - 128, Move migration fixture creation from the individual tests into the existing beforeAll setup, defining the required SQL files there while retaining resetFixtures for per-test isolation. Keep afterAll responsible for removing FIXTURE_DIR, and update tests to reuse the shared fixtures rather than writing them in each test body.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/content.ts`:
- Around line 124-142: Update renderMarkdown’s link-generation logic to validate
each URL before emitting an href attribute, allowing only approved schemes and
safe relative URLs while rejecting javascript: and other unsafe protocols.
Preserve link text rendering, ensure rejected URLs cannot become active links,
and add a regression test covering an encoded javascript: URL such as the
example.
- Around line 134-142: Update the Markdown conversion flow around the html
transformation in content.ts to extract fenced code blocks into unique
placeholders before heading and inline-formatting replacements, render the
remaining Markdown, then restore each original fenced block afterward. Ensure
fenced contents remain literal code, including headings and inline formatting,
and add regression coverage for both cases.
In `@packages/core/src/data/migrate.ts`:
- Around line 89-97: Add a regression test in migrate.test.ts covering
runPostgresMigrations: execute a multi-statement migration that fails, assert
both its data changes and _x_migrations bookkeeping row are rolled back, then
retry with corrected SQL and verify the migration succeeds and is recorded. Use
the existing PostgreSQL test setup and migration helpers rather than changing
runPostgresMigrations.
---
Nitpick comments:
In `@packages/core/src/data/migrate.test.ts`:
- Around line 32-128: Move migration fixture creation from the individual tests
into the existing beforeAll setup, defining the required SQL files there while
retaining resetFixtures for per-test isolation. Keep afterAll responsible for
removing FIXTURE_DIR, and update tests to reuse the shared fixtures rather than
writing them in each test body.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52fc3704-a686-4673-9833-58c2aef6d1ba
📒 Files selected for processing (5)
packages/core/src/content.test.tspackages/core/src/content.tspackages/core/src/data/migrate.test.tspackages/core/src/data/migrate.tspackages/core/src/data/postgres.ts
New workspace package providing plug-and-play authentication for x apps,
following the same layout as packages/env (tsup build, dist/, README,
MIT LICENSE, changeset for the first release).
What it ships:
- defineAuth(): resolves a secret, normalizes providers (GitHub configs
become full OAuth2 configs), and returns handleRequest + getSession +
setSessionCookie/clearSessionCookie.
- Providers: credentials (username/password, wired to your user table via
authorize) and OAuth2 authorization-code, with a preconfigured GitHub
preset (defaults: github.com URLs, read:user user:email scope).
- Password hashing with Argon2id through Bun.password (hashPassword /
verifyPassword).
- Session stores on the framework data layer: createSQLiteSessionStore
(bun:sqlite via connectSQLite) and createPostgresSessionStore
(structural subset of the connectPostgres client). Single x_sessions
table; tokens are opaque 128-bit strings stored as HMAC-SHA256 digests
keyed by the configured secret, so a DB leak doesn't expose usable
cookies. Sessions expire after sessionMaxAge and are revocable.
- A single catch-all handler (api/auth/[...auth].ts) routing:
GET/POST /api/auth/signin/:id
GET /api/auth/callback/:id (OAuth state-challenge verified)
POST /api/auth/signout (CSRF-protected)
GET /api/auth/session (JSON user or 401)
Credentials and signout POSTs run the core checkCsrf (origin/referer
verification; honors security.csrf.requireToken) and return 403 on
failure. Cookies are HttpOnly; SameSite=Lax, Secure in production.
Why this design:
- Reuses the framework's data layer + CSRF module instead of duplicating
them, so auth inherits connection pooling, migration behavior, and the
existing CSRF posture.
- Tokens-at-rest hashing means sessions are revocable server-side without
ever persisting the raw cookie value.
- OAuth state is an HMAC'd cookie challenge (5-minute expiry) rather than
a plain state value, closing login-CSRF / session-fixation via crafted
callbacks.
Verification:
- 14 new unit tests in packages/auth/src/auth.test.ts covering credentials
sign-in (success/wrong password/unknown account), CSRF rejection (missing
and cross-origin), /session auth + 401, sign-out revocation, session
expiry/revocation, GitHub sign-in redirect + state challenge, mocked
callback flow, and mismatched-state rejection. Full suite: 188 pass.
- bun run typecheck (every package), bun run lint, and build:packages all
green. core dist rebuilt so the auth store typechecks against the
updated PostgresClient (begin/params).
Docs: packages/auth/README.md (quick start, endpoint map, security notes,
session stores), plus root README (What's inside + Authentication section)
and SECURITY.md (Authentication & sessions rewritten around the package).
CodeRabbit review finding: the link pass `.replace(/\[([^\]]+)\]\(([^)]+)\)/g, '<a href="$2">$1</a>')` poured whatever URL was inside the parens straight into the href attribute. escapeHtml() only neutralizes & < > " and does nothing about URL schemes, so markdown like `[run](javascript:alert(1))` — or the percent-encoded `[run](javascript:alert%281%29)` — rendered a live javascript: link that executes on click. hrefs are introduced after escaping, so they were never subject to any sanitization at all. The fix validates every link URL against an allowlist before emitting an anchor: - Only http:, https:, and mailto: schemes are accepted. Anything else with a scheme prefix (javascript:, data:, vbscript:, ftp:, ...) is rejected. - The URL is percent-decoded (tolerating malformed escapes) before the scheme is checked, so obfuscated forms like `java%73cript:alert(1)` or a scheme-encoded `javascript%3A...` can't slip past the allowlist. - Relative / scheme-relative URLs (`/foo`, `./bar`, `../up`, `#anchor`, `//cdn.example.com`) carry no scheme and still render as anchors. - A rejected URL falls back to rendering the link text as plain text (no <a> tag), matching the pre-existing behavior for unsafe input rather than throwing or silently dropping content. Regression tests added for: plain javascript:, uppercase JavaScript:, percent-encoded javascript:, scheme-encoded java%73cript:, data:, and unknown-scheme (vbscript:/ftp:) URLs — all must render no <a>/href — plus confirmation that http/https/mailto/relative links still render. Verified with bun test packages/core/src/content.test.ts (21 pass) and biome check on the touched files.
CodeRabbit review finding: the heading (##/###/##), inline-formatting (**bold**, *italic*), and link regexes ran over the whole escaped source BEFORE the fenced-code-block regex did. A `# heading` or `**bold**` line inside a ```fenced block``` was therefore converted into a real <h1>/<strong> tag instead of staying literal text inside <pre><code> — violating the block's verbatim promise and, for links, letting a javascript: href through the same path. The fix extracts fenced code blocks into unique placeholders immediately after the initial escapeHtml() pass, before any other regex runs (the same placeholder pattern inline code already uses). Heading, inline-formatting, list, and link passes then operate only on non-fence content, and the blocks are restored verbatim — already escaped, never re-processed — as the last step before the paragraph-wrapping pass. Regression tests confirm that a # heading, ##/### subheadings, **bold**, *italic*, and a markdown link inside a fenced block all render as literal escaped text inside <pre><code> and never as HTML elements, and that a fence followed by prose still leaves the prose formatted. Verified with bun test packages/core/src/content.test.ts (26 pass), the full core suite (157 pass), and biome check on the touched files.
CodeRabbit review finding: migrate.test.ts only exercised runSQLiteMigrations() through an in-memory bun:sqlite Database — the Postgres migration runner, including its transaction rollback path, had no coverage at all. This adds a describe.if(process.env.DATABASE_URL) block that tests runPostgresMigrations() against a real Postgres server (no mock of Bun.SQL — a mock would only test itself) and skips cleanly when DATABASE_URL is unset, so the rest of the suite is untouched locally. Isolation: each test runs in its own throwaway database, created and dropped per test via a superuser admin client, so the four cases can run against a shared server without stepping on each other. The covered cases mirror the SQLite tests: - applies migrations in filename order (and records both) - skips migrations that were already applied - rolls back a migration that fails partway — this is the key one the old suite couldn't reach: a failing file that first creates a table (audit_log) and inserts a valid row before a PK violation must leave NO trace. Postgres DDL is transactional, so both the schema change and the _x_migrations bookkeeping insert (and the DML) are rolled back, verified by tableExists(audit_log)=false, COUNT(items)=0, and appliedNames() containing only the earlier file. - retries a previously failed migration after a fix and records it CI: adds a dedicated test-postgres job (ubuntu-latest) that runs a postgres:16 service container with health checks and runs migrate.test.ts with DATABASE_URL set, so this coverage actually runs on every PR/push. The existing matrix test job keeps skipping the gated block as before. Verified locally against a real PostgreSQL 16.14 instance (via EDB binaries, brew being unavailable): 8 pass in migrate.test.ts with DATABASE_URL set, 4 pass + clean skip without it; full core suite 161 pass; bun run typecheck and bun run lint green.
…-followups Fix/coderabbit review followups
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
packages/core/src/data/migrate.test.ts (1)
218-276: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest failure at the bookkeeping insert boundary.
Lines 224-231 and Lines 254-257 make
tx.unsafe(raw)fail beforerunPostgresMigrationsattempts its_x_migrationsinsert. A regression that commits migration SQL before separately inserting bookkeeping would still pass these tests.Make
002_broken.sqlcreate schema and insert its own filename into_x_migrations. The runner bookkeeping insert will then fail. Assert that the schema, inserted rows, and migration record all roll back. Use the same setup in the retry test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/data/migrate.test.ts` around lines 218 - 276, Update both “rolls back a migration that fails partway” and “retries a previously failed migration after a fix” fixtures so 002_broken.sql creates its schema/data and also inserts its own filename into _x_migrations, causing failure at the runner’s bookkeeping insert. Keep assertions verifying the schema, rows, and migration record are rolled back, and use the same self-bookkeeping setup in the retry scenario so the corrected migration can succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 44-80: Add a job-level permissions block to test-postgres granting
only contents: read, ensuring its GITHUB_TOKEN cannot inherit broader repository
permissions.
In `@packages/auth/README.md`:
- Around line 23-27: Update the quick-start example before defineAuth to
initialize the application database through the project’s data-layer helper, or
explicitly declare that db is an existing application database. Ensure the
authorize callback’s db reference resolves in a copied example without changing
its authentication logic.
In `@packages/auth/src/auth.ts`:
- Line 220: Update the authorization flow around provider.authorize to catch
credential-provider rejection errors and return the documented 401 response
instead of allowing them to escape. Preserve successful authorization behavior,
and add coverage for a credentials provider that throws during invalid
authentication.
- Line 252: Update the base URL resolution in the OAuth handling path around the
baseUrl option so it never falls back to localhost. Require a trusted configured
public base URL or derive it from a validated request origin, and add coverage
using a non-localhost request URL to verify the generated OAuth redirect URI
matches that origin.
- Around line 214-215: Expose a typed CSRF option in defineAuth and pass it to
both sign-in and sign-out checkCsrf calls, enabling requireToken for the
double-submit defense. Update packages/auth/src/auth.ts lines 214-215 and
236-237 accordingly; revise packages/auth/README.md lines 91-95 to describe the
auth option, and update SECURITY.md lines 171-174 to remove or document the
framework-config claim.
- Around line 81-87: Update the secret initialization around config.secret to
reject a missing secret when running in production, rather than generating one.
For non-production environments, replace the Math.random()-based fallback with a
cryptographically secure generator while preserving the existing warning and
secret string contract.
In `@packages/auth/src/providers.ts`:
- Around line 109-114: Update the authorization-parameter assembly around
provider.authorizationParams so optional entries are applied before the required
client_id, redirect_uri, response_type, and state parameters, ensuring required
values cannot be overwritten. Apply the same ordering change to the tokenParams
handling around the corresponding token exchange parameters, including code and
client_secret.
In `@packages/auth/src/session.ts`:
- Line 76: Replace the direct x_sessions DDL in the session store initialization
with a tracked migration, adding the schema change to the applicable
SQLite/Postgres migration definitions. Invoke runSQLiteMigrations or
runPostgresMigrations through the existing connection helpers before the store
accepts traffic, and remove the direct db.run/create-table calls so the schema
version is recorded in _x_migrations.
- Around line 108-111: Update the ensure function’s initialization promise
handling so ready is cleared when client.unsafe(CREATE_POSTGRES_TABLE) rejects,
allowing subsequent session operations to retry table creation while retaining
the existing promise reuse on success.
In `@packages/core/src/content.ts`:
- Around line 170-174: Update the fenced-code regex in the content
transformation around the fence replacement callback so the closing ``` must
occupy its own line, rather than matching backticks embedded in code. In the
callback, remove only the final newline adjacent to the closing delimiter and
preserve all other leading, trailing, and intentional blank-line whitespace; add
coverage for embedded backticks and preserved whitespace.
- Around line 146-148: Update isSafeLinkUrl to remove or normalize ASCII control
characters from the trimmed, decoded URL before applying LINK_SCHEME_RE and
validating ALLOWED_LINK_SCHEMES, so newline- and tab-obfuscated javascript
schemes are rejected. Add regression coverage for both newline- and
tab-obfuscated unsafe schemes.
In `@packages/core/src/data/migrate.test.ts`:
- Around line 148-163: The PostgresClient shutdown is not awaited during test
cleanup. Update PostgresClient so close(): Promise<void>, make afterAll
asynchronous and await admin.close() directly, and await client.close() in
withDatabase’s finally block before dropping the database, removing the casts.
---
Nitpick comments:
In `@packages/core/src/data/migrate.test.ts`:
- Around line 218-276: Update both “rolls back a migration that fails partway”
and “retries a previously failed migration after a fix” fixtures so
002_broken.sql creates its schema/data and also inserts its own filename into
_x_migrations, causing failure at the runner’s bookkeeping insert. Keep
assertions verifying the schema, rows, and migration record are rolled back, and
use the same self-bookkeeping setup in the retry scenario so the corrected
migration can succeed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a5458e3-42bb-444e-82ee-7f86675d99b6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.changeset/tidy-wolves-wave.md.github/workflows/ci.ymlAGENTS.mdREADME.mdROADMAP.mdSECURITY.mdpackage.jsonpackages/auth/LICENSEpackages/auth/README.mdpackages/auth/package.jsonpackages/auth/src/auth.test.tspackages/auth/src/auth.tspackages/auth/src/cookies.tspackages/auth/src/index.tspackages/auth/src/password.tspackages/auth/src/providers.tspackages/auth/src/session.tspackages/auth/src/types.tspackages/auth/tsconfig.jsonpackages/core/src/content.test.tspackages/core/src/content.tspackages/core/src/data/migrate.test.ts
Comment on lines
+44
to
+80
| test-postgres: | ||
| name: Test (Postgres migrations) | ||
| runs-on: ubuntu-latest | ||
| services: | ||
| postgres: | ||
| image: postgres:16 | ||
| env: | ||
| POSTGRES_USER: postgres | ||
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_DB: postgres | ||
| ports: | ||
| - 5432:5432 | ||
| options: >- | ||
| --health-cmd "pg_isready -U postgres" | ||
| --health-interval 5s | ||
| --health-timeout 5s | ||
| --health-retries 10 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| # Pinned for reproducible builds — bump deliberately. | ||
| bun-version: 1.3.14 | ||
|
|
||
| - name: Install dependencies | ||
| run: bun install --frozen-lockfile | ||
|
|
||
| # The runPostgresMigrations tests are gated on DATABASE_URL and skipped | ||
| # otherwise (see migrate.test.ts), so only this job exercises the real | ||
| # Postgres rollback/retry behavior via the service container above. | ||
| - name: Test (Postgres migrations) | ||
| env: | ||
| DATABASE_URL: postgres://postgres:postgres@localhost:5432/postgres | ||
| run: bun test packages/core/src/data/migrate.test.ts |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set explicit read-only token permissions.
This job has no permissions block. It inherits the repository default GITHUB_TOKEN permissions. A compromised action or executed dependency can use write access when the repository default permits it.
Add permissions: { contents: read } to test-postgres.
🧰 Tools
🪛 Checkov (3.3.9)
[medium] 79-80: Basic Auth Credentials
(CKV_SECRET_4)
🪛 zizmor (1.29.0)
[warning] 44-80: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 44 - 80, Add a job-level permissions
block to test-postgres granting only contents: read, ensuring its GITHUB_TOKEN
cannot inherit broader repository permissions.
Source: Linters/SAST tools
Comment on lines
+23
to
+27
| async authorize({ email, password }) { | ||
| const user = await db.query("SELECT * FROM users WHERE email = ?").get(email); | ||
| if (!user) return null; | ||
| if (!(await verifyPassword(password, user.password_hash))) return null; | ||
| return { id: String(user.id), name: user.name, email: user.email }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Initialize the application user database in the quick start.
Line 24 uses db, but the example does not import or create it. A copied example cannot run.
Show a server-side database initialization through the data-layer helper before defineAuth, or state that db is an existing application database.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/auth/README.md` around lines 23 - 27, Update the quick-start example
before defineAuth to initialize the application database through the project’s
data-layer helper, or explicitly declare that db is an existing application
database. Ensure the authorize callback’s db reference resolves in a copied
example without changing its authentication logic.
Comment on lines
+81
to
+87
| let secret = config.secret; | ||
| if (!secret) { | ||
| secret = Math.random().toString(36).slice(2) + Date.now().toString(36); | ||
| console.warn( | ||
| "[@thexjs/auth] No `secret` configured — generated an ephemeral one. " + | ||
| "Set a stable `secret` in production so sessions survive restarts.", | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Generate a cryptographic fallback secret.
Line 83 uses Math.random() for the HMAC key. This key protects stored session-token digests and OAuth state values. A production deployment without secret can use a predictable key.
Reject a missing secret in production. Use a cryptographic generator for development.
Proposed fix
if (!secret) {
- secret = Math.random().toString(36).slice(2) + Date.now().toString(36);
+ if (process.env.NODE_ENV === "production") {
+ throw new Error("[`@thexjs/auth`] A stable `secret` is required in production");
+ }
+ secret = crypto.randomUUID();
console.warn(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let secret = config.secret; | |
| if (!secret) { | |
| secret = Math.random().toString(36).slice(2) + Date.now().toString(36); | |
| console.warn( | |
| "[@thexjs/auth] No `secret` configured — generated an ephemeral one. " + | |
| "Set a stable `secret` in production so sessions survive restarts.", | |
| ); | |
| let secret = config.secret; | |
| if (!secret) { | |
| if (process.env.NODE_ENV === "production") { | |
| throw new Error("[`@thexjs/auth`] A stable `secret` is required in production"); | |
| } | |
| secret = crypto.randomUUID(); | |
| console.warn( | |
| "[`@thexjs/auth`] No `secret` configured — generated an ephemeral one. " + | |
| "Set a stable `secret` in production so sessions survive restarts.", | |
| ); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 82-82: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/auth/src/auth.ts` around lines 81 - 87, Update the secret
initialization around config.secret to reject a missing secret when running in
production, rather than generating one. For non-production environments, replace
the Math.random()-based fallback with a cryptographically secure generator while
preserving the existing warning and secret string contract.
Source: Linters/SAST tools
Comment on lines
+214
to
+215
| const csrf = checkCsrf(req); | ||
| if (!csrf.ok) return new Response(`CSRF check failed: ${csrf.reason}`, { status: 403 }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/core/src/security/csrf.ts --items all
sed -n '1,180p' packages/core/src/security/csrf.ts
rg -n -C 3 'checkCsrf\(|interface AuthConfig|defineAuth\(' packages/auth/src packages/core/srcRepository: abdelkabirouadoukou/x
Length of output: 12604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '22,90p' packages/auth/src/auth.ts
sed -n '198,245p' packages/auth/src/auth.ts
rg -n -C 3 'security:\s*|csrf:|defineAuth|AuthConfig|sessionMaxAge|sessionSecret|secret' packages -g '!LICENSE' -g '!node_modules' | head -n 200Repository: abdelkabirouadoukou/x
Length of output: 17917
Expose and forward double-submit CSRF configuration.
checkCsrf(req, { requireToken: true }) enables the double-submit check, but defineAuth only uses the defaults for both signin and signout. Add an auth-provided CSRF option and pass it through to both checkCsrf calls so the documented “double-submit defense in depth” path is actually usable.
packages/auth/src/auth.ts#L214-L215: pass typed CSRF options to the sign-in check.packages/auth/src/auth.ts#L236-L237: pass typed CSRF options to the sign-out check.packages/auth/README.md#L91-L95: update therequireTokenwording to match the implemented auth option.SECURITY.md#L171-L174: remove the unsupported framework-config claim, or document the new auth option.
📍 Affects 3 files
packages/auth/src/auth.ts#L214-L215(this comment)packages/auth/src/auth.ts#L236-L237packages/auth/README.md#L91-L95SECURITY.md#L171-L174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/auth/src/auth.ts` around lines 214 - 215, Expose a typed CSRF option
in defineAuth and pass it to both sign-in and sign-out checkCsrf calls, enabling
requireToken for the double-submit defense. Update packages/auth/src/auth.ts
lines 214-215 and 236-237 accordingly; revise packages/auth/README.md lines
91-95 to describe the auth option, and update SECURITY.md lines 171-174 to
remove or document the framework-config claim.
| const params: Record<string, string> = {}; | ||
| for (const [key, value] of form.entries()) params[key] = String(value); | ||
|
|
||
| const user = await provider.authorize(params, { request: req }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor the credential-provider rejection contract.
CredentialsProvider.authorize documents that it can throw to reject a sign-in. Line 220 lets that rejection escape instead of returning the documented 401 response.
Catch provider rejection errors here, or remove the throw-to-reject contract. Add a test for a provider that throws during invalid authentication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/auth/src/auth.ts` at line 220, Update the authorization flow around
provider.authorize to catch credential-provider rejection errors and return the
documented 401 response instead of allowing them to escape. Preserve successful
authorization behavior, and add coverage for a credentials provider that throws
during invalid authentication.
| /** A session store backed by `bun:sqlite` (via `@thexjs/core/data`'s `connectSQLite`). */ | ||
| export function createSQLiteSessionStore(options: SQLiteSessionStoreOptions = {}): SessionStore { | ||
| const db = options.db ?? connectSQLite({ path: options.path ?? "data/auth.db" }); | ||
| db.run(CREATE_SQLITE_TABLE); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Run the session schema changes through tracked migrations.
These direct DDL calls bypass runSQLiteMigrations and runPostgresMigrations. They also do not record the schema version in _x_migrations.
Move x_sessions creation into tracked migrations. Run the applicable migration helper before the store accepts traffic.
As per coding guidelines, “Use the data-layer helpers for database access and migrations: connectSQLite, connectPostgres, runSQLiteMigrations, and runPostgresMigrations; migrations are tracked in _x_migrations.”
Also applies to: 108-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/auth/src/session.ts` at line 76, Replace the direct x_sessions DDL
in the session store initialization with a tracked migration, adding the schema
change to the applicable SQLite/Postgres migration definitions. Invoke
runSQLiteMigrations or runPostgresMigrations through the existing connection
helpers before the store accepts traffic, and remove the direct
db.run/create-table calls so the schema version is recorded in _x_migrations.
Source: Coding guidelines
Comment on lines
+108
to
+111
| let ready: Promise<unknown> | null = null; | ||
| const ensure = () => { | ||
| ready ??= client.unsafe(CREATE_POSTGRES_TABLE); | ||
| return ready; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Allow PostgreSQL table initialization to retry after a failure.
If client.unsafe(CREATE_POSTGRES_TABLE) rejects once, ready retains the rejected promise. Every later session operation fails without another initialization attempt.
Clear ready when initialization rejects.
Proposed fix
let ready: Promise<unknown> | null = null;
const ensure = () => {
- ready ??= client.unsafe(CREATE_POSTGRES_TABLE);
+ if (!ready) {
+ ready = client.unsafe(CREATE_POSTGRES_TABLE).catch((error) => {
+ ready = null;
+ throw error;
+ });
+ }
return ready;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let ready: Promise<unknown> | null = null; | |
| const ensure = () => { | |
| ready ??= client.unsafe(CREATE_POSTGRES_TABLE); | |
| return ready; | |
| let ready: Promise<unknown> | null = null; | |
| const ensure = () => { | |
| if (!ready) { | |
| ready = client.unsafe(CREATE_POSTGRES_TABLE).catch((error) => { | |
| ready = null; | |
| throw error; | |
| }); | |
| } | |
| return ready; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/auth/src/session.ts` around lines 108 - 111, Update the ensure
function’s initialization promise handling so ready is cleared when
client.unsafe(CREATE_POSTGRES_TABLE) rejects, allowing subsequent session
operations to retry table creation while retaining the existing promise reuse on
success.
Comment on lines
+146
to
+148
| function isSafeLinkUrl(url: string): boolean { | ||
| const scheme = LINK_SCHEME_RE.exec(decodeLinkUrl(url.trim()))?.[1]?.toLowerCase(); | ||
| return scheme === undefined || ALLOWED_LINK_SCHEMES.has(scheme); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Reject control-character-obfuscated unsafe schemes.
Line 147 allows java\nscript:alert(1) because the scheme regex does not match across the newline. Browser URL parsing removes ASCII tabs and newlines, so the emitted href becomes a javascript: URL when clicked.
Normalize ASCII control characters after decoding and before scheme validation. Add a regression test for newline- and tab-obfuscated schemes.
Proposed fix
function isSafeLinkUrl(url: string): boolean {
- const scheme = LINK_SCHEME_RE.exec(decodeLinkUrl(url.trim()))?.[1]?.toLowerCase();
+ const normalizedUrl = decodeLinkUrl(url.trim()).replace(/[\u0000-\u0020\u007f-\u009f]/g, "");
+ const scheme = LINK_SCHEME_RE.exec(normalizedUrl)?.[1]?.toLowerCase();
return scheme === undefined || ALLOWED_LINK_SCHEMES.has(scheme);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function isSafeLinkUrl(url: string): boolean { | |
| const scheme = LINK_SCHEME_RE.exec(decodeLinkUrl(url.trim()))?.[1]?.toLowerCase(); | |
| return scheme === undefined || ALLOWED_LINK_SCHEMES.has(scheme); | |
| function isSafeLinkUrl(url: string): boolean { | |
| const normalizedUrl = decodeLinkUrl(url.trim()).replace(/[\u0000-\u0020\u007f-\u009f]/g, ""); | |
| const scheme = LINK_SCHEME_RE.exec(normalizedUrl)?.[1]?.toLowerCase(); | |
| return scheme === undefined || ALLOWED_LINK_SCHEMES.has(scheme); | |
| } |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 147-147: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/content.ts` around lines 146 - 148, Update isSafeLinkUrl to
remove or normalize ASCII control characters from the trimmed, decoded URL
before applying LINK_SCHEME_RE and validating ALLOWED_LINK_SCHEMES, so newline-
and tab-obfuscated javascript schemes are rejected. Add regression coverage for
both newline- and tab-obfuscated unsafe schemes.
Comment on lines
+170
to
+174
| let html = escapeHtml(md).replace(/`{3}(\w*)\n([\s\S]*?)`{3}/gm, (_m, _lang, code) => { | ||
| // The fence body was already escaped up front — re-escaping here would | ||
| // double-encode the `&` → `&` entities just produced. | ||
| fences.push(`<pre><code>${code.trim()}</code></pre>`); | ||
| return `__X_FENCE_${fences.length - 1}__`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve fenced code content and fence boundaries.
Line 170 treats any three backticks as a closing delimiter, including backticks inside a code line. Line 173 then removes leading indentation, trailing spaces, and intentional blank lines with trim().
Require the closing delimiter to occupy its own line. Remove only the delimiter-adjacent final newline. Add tests for embedded backticks and preserved whitespace.
Proposed fix
- let html = escapeHtml(md).replace(/`{3}(\w*)\n([\s\S]*?)`{3}/gm, (_m, _lang, code) => {
+ let html = escapeHtml(md).replace(/^`{3}(\w*)\n([\s\S]*?)^`{3}[ \t]*$/gm, (_m, _lang, code) => {
// The fence body was already escaped up front — re-escaping here would
// double-encode the `&` → `&` entities just produced.
- fences.push(`<pre><code>${code.trim()}</code></pre>`);
+ fences.push(`<pre><code>${code.replace(/\r?\n$/, "")}</code></pre>`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let html = escapeHtml(md).replace(/`{3}(\w*)\n([\s\S]*?)`{3}/gm, (_m, _lang, code) => { | |
| // The fence body was already escaped up front — re-escaping here would | |
| // double-encode the `&` → `&` entities just produced. | |
| fences.push(`<pre><code>${code.trim()}</code></pre>`); | |
| return `__X_FENCE_${fences.length - 1}__`; | |
| let html = escapeHtml(md).replace(/^`{3}(\w*)\n([\s\S]*?)^`{3}[ \t]*$/gm, (_m, _lang, code) => { | |
| // The fence body was already escaped up front — re-escaping here would | |
| // double-encode the `&` → `&` entities just produced. | |
| fences.push(`<pre><code>${code.replace(/\r?\n$/, "")}</code></pre>`); | |
| return `__X_FENCE_${fences.length - 1}__`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/content.ts` around lines 170 - 174, Update the fenced-code
regex in the content transformation around the fence replacement callback so the
closing ``` must occupy its own line, rather than matching backticks embedded in
code. In the callback, remove only the final newline adjacent to the closing
delimiter and preserve all other leading, trailing, and intentional blank-line
whitespace; add coverage for embedded backticks and preserved whitespace.
Comment on lines
+148
to
+163
| afterAll(() => { | ||
| (admin as unknown as { close(): void }).close(); | ||
| }); | ||
|
|
||
| /** Creates a throwaway database, runs `fn` against it, and drops it. */ | ||
| async function withDatabase<T>(fn: (client: PostgresClient) => Promise<T>): Promise<T> { | ||
| const dbName = `x_mig_${crypto.randomUUID().replace(/-/g, "")}`; | ||
| await admin.unsafe(`CREATE DATABASE "${dbName}"`); | ||
| const url = new URL(PG_TEST_URL as string); | ||
| url.pathname = `/${dbName}`; | ||
| const client = connectPostgres({ url: url.toString(), max: 1 }); | ||
| try { | ||
| return await fn(client); | ||
| } finally { | ||
| (client as unknown as { close(): void }).close(); | ||
| await admin.unsafe(`DROP DATABASE IF EXISTS "${dbName}" WITH (FORCE)`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For Bun 1.3.14, does Bun.SQL.close() return Promise<void>, and must callers await it before dropping a PostgreSQL database?
💡 Result:
Yes, in Bun 1.3.14, the Bun.SQL.close method returns Promise [1][2]. Callers should await this method [1][3][4]. Awaiting ensures that the SQL client finishes pending queries (or waits for the specified timeout) and fully releases the connection pool before proceeding [1][5][4]. Failing to await the close operation may result in active queries being interrupted or incomplete cleanup of resources, which is particularly relevant if you intend to perform subsequent actions like dropping a database [1][5][6]. As documented, the method behaves as follows: - If called without arguments, it waits for all pending queries to finish before closing all connections in the pool [1][2]. - If a timeout (in seconds) is provided, it will wait up to that duration for queries to finish [1][2]. - If a timeout of 0 is provided, it closes the connection pool immediately [1][2][4].
Citations:
- 1: https://bun.com/reference/bun/SQL/close
- 2: https://bun.sh/reference/bun/ReservedSQL
- 3: https://bun.com/docs/runtime/sql
- 4: https://oven-sh-bun.mintlify.app/runtime/sql
- 5: https://github.com/oven-sh/bun/blob/1498d7b7/docs/runtime/sql.mdx
- 6: sql: drain queries awaited in the same tick as close()/end() oven-sh/bun#33740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'migrate\.test\.ts|postgres|sql|PostgresClient' . | sed 's#^\./##' | head -80
echo
echo "== migrate test excerpt =="
if [ -f packages/core/src/data/migrate.test.ts ]; then
nl -ba packages/core/src/data/migrate.test.ts | sed -n '1,220p'
fi
echo
echo "== references to close and connectPostgres =="
rg -n "connectPostgres|PostgresClient|\.close\(\)|afterAll|withDatabase|CREATE DATABASE|DROP DATABASE" packages/core/src test -S || true
echo
echo "== package bun version =="
rg -n '"bun"|"`@bun`|bun\.' package.json pnpm-lock.yaml bun.lockb yarn.lock package-lock.json . --glob '!node_modules' --glob '!dist' --glob '!build' | head -120 || trueRepository: abdelkabirouadoukou/x
Length of output: 776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migrate test excerpt =="
if command -v nl >/dev/null 2>&1; then
nl -ba packages/core/src/data/migrate.test.ts | sed -n '1,220p'
else
cat -n packages/core/src/data/migrate.test.ts | sed -n '1,220p'
fi
echo
echo "== postgres implementation excerpt =="
cat -n packages/core/src/data/postgres.ts | sed -n '1,240p'
echo
echo "== references to close and connectPostgres in relevant files =="
rg -n "connectPostgres|PostgresClient|\.close\(\)|afterAll|withDatabase|CREATE DATABASE|DROP DATABASE|Bun" packages/core/src packages packages --glob '!node_modules' || true
echo
echo "== package bun version =="
for f in package.json pnpm-lock.yaml bun.lock bun.lockb yarn.lock package-lock.json; do
[ -f "$f" ] && { echo "--- $f"; rg -n '"bun"|`@bun`|bun@|bun:' "$f" | head -80 || true; }
doneRepository: abdelkabirouadoukou/x
Length of output: 50377
Await PostgreSQL pool shutdown.
Bun.SQL.close() returns a promise, but afterAll ignores the admin client result and withDatabase does not await the test client closure before dropping the database. Add close(): Promise<void> to PostgresClient and change the cleanup hooks to async; use await admin.close() without casting and await client.close() in the finally block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/data/migrate.test.ts` around lines 148 - 163, The
PostgresClient shutdown is not awaited during test cleanup. Update
PostgresClient so close(): Promise<void>, make afterAll asynchronous and await
admin.close() directly, and await client.close() in withDatabase’s finally block
before dropping the database, removing the casts.
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
1 participant
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.
What does this PR do?
Adds the
@thexjs/authpackage plus the follow-up fixes from the CodeRabbitreview of this PR (all verified green locally):
@thexjs/auth— new workspace package built on@thexjs/core'sexisting data + CSRF modules:
defineAuth(config)returninghandleRequest,getSession,setSessionCookie,clearSessionCookie.Bun.password(
hashPassword/verifyPassword); session tokens stored only asHMAC-SHA256 digests, revocable and expiring after
sessionMaxAge.SessionStoreinterface:createSQLiteSessionStore(bun:sqlite) and
createPostgresSessionStore.src/api/auth/[...auth].tswiringsign-in, OAuth callback, sign-out, and
/session.checkCsrf()automatically.SECURITY.md"Authentication & sessions" updated to point at@thexjs/auth(demoadmin/adminkept, clearly marked demo-only);new root README "Authentication" section +
packages/auth/README.md.CodeRabbit review follow-ups (also in this PR):
fix(core):renderMarkdownvalidates every link URL against anhttp:/https:/mailto:allowlist (percent-decoded first) and rendersunsafe URLs (
javascript:,data:, ...) as plain text — closes thejavascript:XSS.fix(core): fenced code blocks are extracted into placeholders before anyheading/inline/link pass, so
#,**bold**,[link](url)inside ``` blocksstay literal.
test(core):runPostgresMigrations()coverage against a real Postgres —rollback of schema changes + bookkeeping insert on partial failure, retry
after fix, apply/skip ordering — gated on
DATABASE_URL(skips cleanly whenunset) plus a new
test-postgresCI job with apostgres:16service.Related issue
CodeRabbit review findings on this PR (3, all addressed). No GitHub issue.
Type of change
Checklist
bun run typecheckpassesbun run lintpassesbun testpasses (201 pass, 0 fail; Postgres tests verified against areal PostgreSQL 16.14 with
DATABASE_URLset)regression tests + Postgres migration tests)
bunx changeset) if this affects a published package(
.changeset/tidy-wolves-wave.mdfor@thexjs/auth)AGENTS.md,
packages/auth/README.md)