Skip to content

fix(auth): tolerate unreadable dashboard fingerprints at startup - #1086

Open
FindMalek wants to merge 2 commits into
databuddy-analytics:mainfrom
FindMalek:fix/auth-secret-guard-review
Open

FindMalek wants to merge 2 commits into
databuddy-analytics:mainfrom
FindMalek:fix/auth-secret-guard-review

Conversation

@FindMalek

@FindMalek FindMalek commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

An unreadable dashboard fingerprint response can abort API startup before the existing warning path runs. Treat invalid JSON as an unavailable fingerprint, preserving the runtime object/string checks and the fatal rejection of a confirmed secret mismatch. Keep the existing synchronous API startup error output.

Validation: 11 isolated tests exercise the real auth guard and fingerprint endpoint, covering malformed JSON, invalid payloads, unreachable/non-OK responses, matching and mismatched secrets, and self-hosted/development skips. The main-source baseline reproduces the malformed-JSON failure; a mutation that bypasses mismatch rejection fails the relevant test. bun run lint and bun run check-types passed.

AI disclosure: Codex assisted the maintainer cleanup, review and regression tests.

Summary by CodeRabbit

  • Bug Fixes
    • Authentication checks now handle unreadable dashboard responses without failing unexpectedly.
  • Tests
    • Added coverage for matching and mismatched secrets, invalid or missing responses, request failures, and configurations that skip the dashboard check.

@vercel

vercel Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
documentation Skipped Skipped Oct 6, 2026 10:11am UTC

@vercel
vercel Bot temporarily deployed to Preview – documentation October 5, 2026 10:00 Inactive
@vercel

vercel Bot commented Oct 5, 2026

Copy link
Copy Markdown

@FindMalek is attempting to deploy a commit to the Databuddy OSS Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 52a6d6fc-2d5e-4154-983a-89654f5d09bd
📥 Commits

Reviewing files that changed from the base of the PR and between 5a81411 and 2099347.

📒 Files selected for processing (2)
  • packages/auth/src/auth-secret-fingerprint.test.ts
  • packages/auth/src/auth.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: Greptile Review
  • GitHub Check: Test
⚠️ CI failures not shown inline (2)

Commit Status: Vercel – dashboard: Vercel – dashboard

Conclusion: failure

Authorization required to deploy.

Commit Status: Vercel – databuddy-status: Vercel – databuddy-status

Conclusion: failure

Authorization required to deploy.
🧰 Additional context used
📚 Code guidelines (3)
.cursor/rules/performance.mdc — auto-discovered
.cursor/rules/ui-guidelines.mdc — auto-discovered
.cursor/rules/01-MUST-DO.mdc — auto-discovered
📓 Path-based instructions (3)
Source excerpt: When you discover a new performance improvement, optimization pattern, or fix a performance regression, add a concise bullet to the relevant section below in the same session.

📄 CodeRabbit inference engine (.cursor/rules/performance.mdc)

Files:

  • packages/auth/src/auth.ts
  • packages/auth/src/auth-secret-fingerprint.test.ts
Source excerpt: MUST use Tailwind CSS defaults unless custom values already exist or are explicitly requested Source excerpt: MUST use motion/react (formerly framer-motion) when JavaScript animation is required Source excerpt: SHOULD use tw...

📄 CodeRabbit inference engine (.cursor/rules/ui-guidelines.mdc)

Files:

  • packages/auth/src/auth.ts
  • packages/auth/src/auth-secret-fingerprint.test.ts
Source excerpt: description: Basic guidelines for the project so vibe coders don't fuck it up globs: alwaysApply: true when using 'text-right', always add 'text-balance' so its not ugly Source excerpt: description: Basic guidelines for the...

📄 CodeRabbit inference engine (.cursor/rules/01-MUST-DO.mdc)

Files:

  • packages/auth/src/auth.ts
  • packages/auth/src/auth-secret-fingerprint.test.ts
🪛 Betterleaks (1.8.1)
packages/auth/src/auth-secret-fingerprint.test.ts

[high] 40-40: Detected a password embedded in a service connection URI, which may expose direct access to the referenced service.

(generic-credential-uri)

🔇 Additional comments (3)
packages/auth/src/auth-secret-fingerprint.test.ts (2)

40-40: Credential-like URI is a synthetic test value. No change is needed.

The Betterleaks hint flags postgres://test:test@127.0.0.1:1/test. The host is a loopback address on an unusable port. The credentials are dummy values and grant no access. Do not change correct code solely to silence this diagnostic.


1-95: LGTM!

packages/auth/src/auth.ts (1)

1391-1393: LGTM!


Walkthrough

The dashboard auth-secret check now treats a rejected response-body JSON parse as a missing body. New tests cover malformed responses, fetch failures, secret matches and mismatches, and configurations that skip the request.

Changes

Dashboard auth-secret check

Layer / File(s) Summary
Fingerprint response handling
packages/auth/src/auth.ts, packages/auth/src/auth-secret-fingerprint.test.ts
A rejected JSON parse now produces a missing body. Tests check invalid responses, fetch failures, matching and mismatched secrets, and skipped requests in isolated processes.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: izadoesdev

Merge Risk: ⚪ Minimal · up to 20993

Startup no longer fails when the dashboard fingerprint response contains invalid JSON, and a confirmed secret mismatch is still rejected. The new tests cover the main scenarios. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: auth startup now tolerates unreadable dashboard fingerprints.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Critical risk] Changes auth secret validation at startup.

The PR appears safe to merge; no outstanding finding remains in the final two-file change.

What we checked:

  • Confirmed secret mismatch still stops startup: No. The catch applies to failed JSON reads; a readable string still reaches the comparison and throws when it differs.

Summary

The API auth check now treats unreadable dashboard fingerprint JSON like an unavailable fingerprint, so startup can continue through its existing warning path. A readable secret mismatch still stops startup.

  • Unreadable dashboard fingerprints now let API startup continue.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[API startup] --> B{Production and hosted?}
  B -->|No| C[Skip request]
  B -->|Yes| D[Fetch dashboard fingerprint]
  D --> E{Readable string?}
  E -->|No| F[Warn and continue]
  E -->|Yes| G{Secrets match?}
  G -->|Yes| H[Continue startup]
  G -->|No| I[Print error and exit]
Loading

Reviews (2) · Last reviewed commit: "fix(auth): preserve startup validation f..."

Comment thread apps/api/src/index.ts Outdated
Comment thread packages/auth/src/auth.ts Outdated
FindMalek and others added 2 commits October 6, 2026 13:04
A 200 with a non-JSON body from the dashboard (or a proxy in front of it)
rejected outside the fetch catch, so the API exited without confirming the
secrets differ. Treat it like an unreachable dashboard and only warn.

Also log the fatal mismatch through evlog instead of console, and drop the
`unknown` annotations.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@izadoesdev
izadoesdev marked this pull request as draft October 6, 2026 10:05
@izadoesdev izadoesdev changed the title fix(api): warn instead of exiting when the fingerprint body isn't JSON fix(auth): tolerate unreadable dashboard fingerprints at startup Oct 6, 2026
@izadoesdev
izadoesdev changed the base branch from staging to main October 6, 2026 10:07
@izadoesdev
izadoesdev force-pushed the fix/auth-secret-guard-review branch from acf46d6 to 2099347 Compare October 6, 2026 10:10
@vercel
vercel Bot temporarily deployed to Preview – documentation October 6, 2026 10:11 Inactive
@izadoesdev
izadoesdev marked this pull request as ready for review October 6, 2026 10:13
@izadoesdev

Copy link
Copy Markdown
Member

@coderabbitai full review

Please review the complete final two-file auth slice on 2099347 against main, including the real-function regression tests. The existing runtime shape validation, confirmed mismatch rejection and synchronous fatal API logging are preserved.

@izadoesdev

Copy link
Copy Markdown
Member

@greptile-apps review

Please review the final two-file slice on 2099347 against main. Both earlier findings are fixed: the API structured-log change is removed and 11 real-function regressions cover malformed responses, secret mismatch rejection and skipped environments.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@izadoesdev

Copy link
Copy Markdown
Member

Final-head review dispositions for 2099347:

  • Both original inline findings are fixed and resolved, including their outdated threads. API startup logging is identical to main, and all 11 real-function auth regressions pass.
  • The synthetic credential scanner hint is already correctly dismissed by CodeRabbit: fixtures refer only to inert loopback services and do not grant access. No private account credentials were read or included.
  • Declining the generic docstring percentage warning: this slice changes one JSON parsing branch in the existing guard and adds behavioral regression coverage; its runtime checks and warning/error paths express the contract. Adding a docstring solely to satisfy the percentage would add commentary without clarifying this small change.
  • Greptile’s summary has an inconsistent “No” label beside its mismatch check, while its explanation and diagram correctly describe rejection. The final source still throws for a readable mismatched string, and the mismatch-bypass mutation fails the test. Its final review reports no outstanding finding.
  • Native CI is green, including hosted 54 and self-hosted 14 Playwright tests. Vercel dashboard/status previews separately require Databuddy OSS team authorization for this contributor-fork commit; those failed statuses have not been overridden.

This branch was previously deployed

1 inactive deployment
Preview – documentation — 2099347f Deployed Oct 6, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants