feat: add /tls-check endpoint for Caddy on-demand TLS#8
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe pull request adds a new TLS verification endpoint ( Changes
Sequence DiagramsequenceDiagram
participant Caddy as Caddy<br/>(On-Demand TLS)
participant App as PDS Application<br/>(/tls-check endpoint)
participant Validation as Domain & Handle<br/>Validation Logic
Caddy->>App: POST /tls-check<br/>(domain request)
activate App
App->>Validation: Validate domain
activate Validation
alt Domain is PDS hostname or auth subdomain
Validation-->>App: Valid (allow unconditionally)
else Check hosted handles
Validation->>Validation: Search hosted handles<br/>on configured domains
alt Handle found and account resolved
Validation-->>App: Valid
else Handle not found
Validation-->>App: Not Found (404)
else Account resolution failed
Validation-->>App: Internal Server Error (500)
end
else Invalid domain format
Validation-->>App: Bad Request (400)
end
deactivate Validation
App-->>Caddy: Success / Error Response
deactivate App
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Caddyfile`:
- Line 4: Replace the dynamic upstream reference in the Caddy 'ask' directive
with the Docker Compose service name: change the current ask URL that uses
{$PDS_UPSTREAM:core:3000} to use http://core:3000/tls-check so the on-demand TLS
check targets the Compose service name "core" directly.
In `@packages/pds-core/src/index.ts`:
- Around line 478-491: The domain matching in the /tls-check route uses
domain.endsWith(avail) (inside the isHostedHandle computation) which allows
sibling domains like evil-example.com to match example.com; update the check to
only accept exact matches or proper subdomain matches by testing (domain ===
avail) || domain.endsWith('.' + avail) against each entry in
pds.ctx.cfg.identity.serviceHandleDomains when computing isHostedHandle (use the
existing domain variable and isHostedHandle logic) so only identical domains or
true subdomains are authorized.
packages/pds-core/src/index.ts
Outdated
Comment on lines
+478
to
+491
| const isHostedHandle = pds.ctx.cfg.identity.serviceHandleDomains.find( | ||
| (avail: string) => domain.endsWith(avail), | ||
| ) | ||
| if (!isHostedHandle) { | ||
| return res.status(400).json({ | ||
| error: 'InvalidRequest', | ||
| message: 'handles are not provided on this domain', | ||
| }) | ||
| } | ||
| const account = await pds.ctx.accountManager.getAccount(domain) | ||
| if (!account) { | ||
| return res | ||
| .status(404) | ||
| .json({ error: 'NotFound', message: 'handle not found for this domain' }) |
There was a problem hiding this comment.
Tighten domain boundary matching in /tls-check.
Line 479 uses domain.endsWith(avail), which can match sibling domains (for example, evil-example.com against example.com). This can incorrectly authorize domains for cert checks.
Proposed fix
- const isHostedHandle = pds.ctx.cfg.identity.serviceHandleDomains.find(
- (avail: string) => domain.endsWith(avail),
- )
+ const normalizedDomain = domain.trim().toLowerCase().replace(/\.$/, '')
+ const isHostedHandle = pds.ctx.cfg.identity.serviceHandleDomains.some(
+ (avail: string) => {
+ const normalizedAvail = avail.toLowerCase()
+ return (
+ normalizedDomain === normalizedAvail ||
+ normalizedDomain.endsWith(`.${normalizedAvail}`)
+ )
+ },
+ )
@@
- const account = await pds.ctx.accountManager.getAccount(domain)
+ const account = await pds.ctx.accountManager.getAccount(normalizedDomain)📝 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
| const isHostedHandle = pds.ctx.cfg.identity.serviceHandleDomains.find( | |
| (avail: string) => domain.endsWith(avail), | |
| ) | |
| if (!isHostedHandle) { | |
| return res.status(400).json({ | |
| error: 'InvalidRequest', | |
| message: 'handles are not provided on this domain', | |
| }) | |
| } | |
| const account = await pds.ctx.accountManager.getAccount(domain) | |
| if (!account) { | |
| return res | |
| .status(404) | |
| .json({ error: 'NotFound', message: 'handle not found for this domain' }) | |
| const normalizedDomain = domain.trim().toLowerCase().replace(/\.$/, '') | |
| const isHostedHandle = pds.ctx.cfg.identity.serviceHandleDomains.some( | |
| (avail: string) => { | |
| const normalizedAvail = avail.toLowerCase() | |
| return ( | |
| normalizedDomain === normalizedAvail || | |
| normalizedDomain.endsWith(`.${normalizedAvail}`) | |
| ) | |
| }, | |
| ) | |
| if (!isHostedHandle) { | |
| return res.status(400).json({ | |
| error: 'InvalidRequest', | |
| message: 'handles are not provided on this domain', | |
| }) | |
| } | |
| const account = await pds.ctx.accountManager.getAccount(normalizedDomain) | |
| if (!account) { | |
| return res | |
| .status(404) | |
| .json({ error: 'NotFound', message: 'handle not found for this domain' }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pds-core/src/index.ts` around lines 478 - 491, The domain matching
in the /tls-check route uses domain.endsWith(avail) (inside the isHostedHandle
computation) which allows sibling domains like evil-example.com to match
example.com; update the check to only accept exact matches or proper subdomain
matches by testing (domain === avail) || domain.endsWith('.' + avail) against
each entry in pds.ctx.cfg.identity.serviceHandleDomains when computing
isHostedHandle (use the existing domain variable and isHostedHandle logic) so
only identical domains or true subdomains are authorized.
Adds GET /tls-check to pds-core so Caddy's on-demand TLS ask URL has a proper guard instead of the unconditional /xrpc/_health. Returns 200 for the PDS hostname, the auth subdomain, and any hosted handle with an existing account; non-200 otherwise so Caddy won't provision certs for unknown domains. Updates Caddyfile to point on_demand_tls ask at /tls-check.
aspiers
approved these changes
Mar 2, 2026
Contributor
aspiers
left a comment
aspiers
left a comment
There was a problem hiding this comment.
Very nice! Good find that the pds repo already had a solution for this.
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
/tls-checkendpoint to pds-core that Caddy's on-demand TLSaskdirective calls to verify whether a domain should get a certificate provisionedCaddyfileto pointaskat/tls-checkinstead of/xrpc/_health(which always returned 200 regardless of domain, allowing certificate provisioning for arbitrary domains)Why
Previously Caddy's
askpointed at/xrpc/_health, which unconditionally returns 200. This meant Caddy would provision TLS certificates for any domain pointed at the server, which is both a security risk and wastes Let's Encrypt rate limits. The new/tls-checkendpoint validates that the requested domain is actually one we host.Changes
packages/pds-core/src/index.tscheckHandleRoute()function +/tls-checkrouteCaddyfileaskat/tls-check, add malicious IP blocks, minor whitespace cleanupCode
directly copied from the bleusky pds image:
Summary by CodeRabbit
Security
New Features