Domain check using fly.io certificates #410

yaslama · 2025-11-13T12:50:20Z

No description provided.

Clarify instructions for configuring DNS records for LNURL server.

The check is done only on register

Copilot

Pull Request Overview

This PR introduces a flexible domain validation system using Fly.io certificates, refactoring the hardcoded domain list approach to a trait-based validator pattern. The implementation enables dynamic domain validation through Fly.io's certificate API as an alternative to static configuration.

Key Changes

Created domain-validator crate with a DomainValidator trait and ListDomainValidator implementation for backwards compatibility with static domain lists
Created fly-api crate with FlyDomainValidator that queries Fly.io's GraphQL API to validate domains against app certificates
Refactored lnurl crate to use the DomainValidator trait, replacing direct domain set checks and removing the sanitize_domain helper function

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
crates/domain-validator/Cargo.toml	Defines the new domain-validator crate with trait definition and list-based implementation
crates/domain-validator/src/lib.rs	Implements the DomainValidator trait and ListDomainValidator for static domain lists
crates/fly-api/Cargo.toml	Defines the new fly-api crate for Fly.io certificate-based validation
crates/fly-api/src/lib.rs	Implements FlyDomainValidator that queries Fly.io GraphQL API for certificate domains
crates/breez-sdk/lnurl/Cargo.toml	Adds dependencies on domain-validator and fly-api crates
crates/breez-sdk/lnurl/src/state.rs	Replaces domains HashSet with Arc trait object
crates/breez-sdk/lnurl/src/main.rs	Adds CLI args for Fly.io configuration and instantiates appropriate validator
crates/breez-sdk/lnurl/src/routes.rs	Updates all route handlers to use domain validator trait, removes sanitize_domain function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/fly-api/Cargo.toml

crates/breez-sdk/lnurl/src/routes.rs

Copilot · 2025-11-13T12:57:15Z

crates/fly-api/src/lib.rs

+pub struct FlyDomainValidator {
+    app_name: String,
+    api_token: String,
+    client: Client,
+}


The FlyDomainValidator struct lacks documentation. Add doc comments explaining its purpose (validates domains against Fly.io app certificates), the required parameters (app_name and api_token), and usage example. This is a public API that will be used by consumers of this crate.

Copilot · 2025-11-13T12:57:16Z

crates/fly-api/src/lib.rs

+        let allowed_domains = self
+            .get_certificate_domains()
+            .await
+            .map_err(|e| DomainValidatorError::DomainNotAllowed(e.to_string()))?;


The get_certificate_domains() method is called on every domain validation, which makes a network request to the Fly.io API each time. This creates an N+1 query pattern for multiple domain validations and can significantly impact performance. Consider implementing caching (e.g., with a TTL) to avoid repeated API calls for the same certificate data.

Copilot · 2025-11-13T12:57:16Z

crates/fly-api/src/lib.rs

+pub struct FlyDomainValidator {
+    app_name: String,
+    api_token: String,
+    client: Client,


The api_token field is stored as a plain String. Consider using a secure type like secrecy::Secret<String> or similar to prevent accidental logging or exposure of the API token through debug output. The Debug trait on the struct will currently expose the token.

Copilot · 2025-11-13T12:57:18Z

crates/fly-api/src/lib.rs

+
+        if allowed_domains.is_empty() {
+            return Err(FlyApiError::FetchError(
+                "No domains found in Fly.io certificates".to_string(),


The error message "No domains found in Fly.io certificates" is unclear about the cause. This could happen for multiple reasons: no certificates exist, certificates have no hostnames, or the GraphQL response structure is unexpected. Consider a more specific message like "No valid certificates with hostnames found for app '{app_name}'" or include troubleshooting hints.

Suggested change

"No domains found in Fly.io certificates".to_string(),

format!(

"No valid certificates with hostnames found for app '{}'. \

Possible causes: no certificates exist, certificates have no hostnames, or the app name is incorrect. \

Please check your Fly.io app configuration and certificate status.",

self.app_name

),

Copilot · 2025-11-13T12:57:19Z

crates/domain-validator/src/lib.rs

+#[async_trait::async_trait]
+pub trait DomainValidator: Send + Sync {


The DomainValidator trait lacks documentation. Add doc comments explaining its purpose, the expected behavior of validate_domain(), and when DomainNotAllowed error should be returned. This is a public trait that will be implemented by external consumers.

Suggested change

#[async_trait::async_trait]

pub trait DomainValidator: Send + Sync {

#[async_trait::async_trait]

/// A trait for validating whether a domain is allowed.

///

/// This trait should be implemented by external consumers to provide custom domain validation logic.

/// The `validate_domain` method checks if the given domain is permitted according to the implementation's rules.

/// If the domain is not allowed, it should return `Err(DomainValidatorError::DomainNotAllowed(domain.to_string()))`.

/// Otherwise, it should return `Ok(())`.

pub trait DomainValidator: Send + Sync {

/// Asynchronously validates whether the provided domain is allowed.

///

/// # Arguments

///

/// * `domain` - The domain name to validate.

///

/// # Returns

///

/// * `Ok(())` if the domain is allowed.

/// * `Err(DomainValidatorError::DomainNotAllowed)` if the domain is not permitted.

///

/// Implementors should return `DomainNotAllowed` only when the domain fails validation according to their rules.

crates/breez-sdk/lnurl/src/routes.rs

Copilot · 2025-11-13T12:57:20Z

crates/breez-sdk/lnurl/src/routes.rs

        }

        let username = sanitize_username(&identifier);
+        let domain = host.trim().to_lowercase();


The request_invoice function does not validate the domain before creating invoices. Unlike the register function (lines 134-137), this allows invoice generation for unauthorized domains. Consider adding domain validation here to ensure only allowed domains can request invoices.

Suggested change

let domain = host.trim().to_lowercase();

let domain = host.trim().to_lowercase();

// Domain validation: ensure only allowed domains can request invoices

if let Some(allowed_domains) = &state.allowed_domains {

if !allowed_domains.contains(&domain) {

return Err((

StatusCode::FORBIDDEN,

Json(Value::String("domain not allowed".into())),

));

}

}

crates/breez-sdk/lnurl/src/routes.rs

Copilot

Pull Request Overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-13T13:36:26Z

crates/breez-sdk/lnurl/src/main.rs

        max_sendable: args.max_sendable,
        include_spark_address: args.include_spark_address,
-        domains,
+        domain_validator: domain_validator,


Redundant use of variable name in field initialization. This can be simplified from domain_validator: domain_validator, to just domain_validator, using Rust's field init shorthand.

Suggested change

domain_validator: domain_validator,

domain_validator,

Copilot · 2025-11-13T13:36:27Z

crates/fly-api/src/lib.rs

+            return Err(FlyApiError::FetchError(
+                "No domains found in Fly.io certificates".to_string(),
+            ));


The error message "No domains found in Fly.io certificates" doesn't distinguish between an empty response and a malformed response. This could be misleading if the app exists but has no certificates configured. Consider making the error message more specific, such as "No certificates configured for the Fly.io app" or checking for the app's existence first.

crates/breez-sdk/lnurl/src/routes.rs

Copilot

Pull Request Overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-13T13:46:37Z

crates/breez-sdk/lnurl/src/routes.rs

+        let domain = host.trim().to_lowercase();
        let user = state
            .db
-            .get_user_by_pubkey(&sanitize_domain(&state, &host)?, &pubkey.to_string())
+            .get_user_by_pubkey(&domain, &pubkey.to_string())


Domain validation is missing in the recover_lnurl_pay endpoint. The old sanitize_domain function validated the domain before allowing recovery operations, but the new implementation directly uses the domain without validation. This could allow unauthorized operations on invalid domains. Consider adding domain validation similar to the register_lnurl_pay endpoint.

Copilot · 2025-11-13T13:46:38Z

crates/breez-sdk/lnurl/src/routes.rs

+        let domain = host.trim().to_lowercase();
        let user = state
            .db
-            .get_user_by_name(&sanitize_domain(&state, &host)?, &username)
+            .get_user_by_name(&domain, &username)


Domain validation is missing in the get_lnurl_pay endpoint. The old sanitize_domain function validated the domain, but the new implementation directly uses the domain without validation. This could expose data for invalid domains. Consider adding domain validation similar to the register_lnurl_pay endpoint.

Copilot · 2025-11-13T13:46:38Z

crates/breez-sdk/lnurl/src/routes.rs

+        let domain = host.trim().to_lowercase();
        let user = state
            .db
            .get_user_by_name(&domain, &username)


Domain validation is missing in the get_lnurl_pay_callback endpoint. The old sanitize_domain function validated the domain, but the new implementation directly uses the domain without validation. This could allow unauthorized payment callbacks for invalid domains. Consider adding domain validation similar to the register_lnurl_pay endpoint.

Copilot · 2025-11-13T13:46:38Z

crates/breez-sdk/lnurl/src/routes.rs

+        let domain = host.trim().to_lowercase();
        state
            .db
-            .delete_user(&sanitize_domain(&state, &host)?, &pubkey.to_string())
+            .delete_user(&domain, &pubkey.to_string())


Domain validation is missing in the delete_lnurl_pay endpoint. The old sanitize_domain function validated the domain before allowing deletion, but the new implementation directly uses the domain without validation. This could allow unauthorized deletions on invalid domains. Consider adding domain validation similar to the register_lnurl_pay endpoint.

crates/fly-api/Cargo.toml

Copilot

Pull Request Overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-13T15:56:15Z

crates/fly-api/src/lib.rs

+    pub fn new(app_name: String, api_token: String) -> Self {
+        Self {
+            app_name,
+            api_token,


The constructor accepts String parameters which are moved into the struct. Consider accepting impl Into<String> or &str parameters to provide a more flexible API that doesn't require callers to own the strings.

Suggested change

pub fn new(app_name: String, api_token: String) -> Self {

Self {

app_name,

api_token,

pub fn new(app_name: impl Into<String>, api_token: impl Into<String>) -> Self {

Self {

app_name: app_name.into(),

api_token: api_token.into(),

Copilot · 2025-11-13T15:56:16Z

crates/breez-sdk/lnurl/src/state.rs

    pub session_manager: Arc<InMemorySessionManager>,
    pub service_provider: Arc<ServiceProvider>,
-    pub subscribed_keys: Arc<Mutex<HashSet<String>>>,
+    pub subscribed_keys: Arc<Mutex<std::collections::HashSet<String>>>,


The use of std::collections::HashSet here is inconsistent with the rest of the file where HashSet was previously imported. Since std::collections::HashSet was removed from imports (line 6), this should use the fully qualified path consistently or add back the import. The inconsistency makes the code harder to maintain.

Copilot · 2025-11-13T15:56:16Z

crates/domain-validator/src/lib.rs

+pub trait DomainValidator: Send + Sync {
+    async fn validate_domain(&self, domain: &str) -> Result<(), DomainValidatorError>;
+}


The DomainValidator trait lacks documentation explaining its purpose, expected behavior, and usage. Add doc comments to describe what domain validation means in this context and how implementers should handle the validation.

Copilot · 2025-11-13T15:56:16Z

crates/breez-sdk/lnurl/src/routes.rs

        Extension(state): Extension<State<DB>>,
    ) -> Result<Json<CheckUsernameAvailableResponse>, (StatusCode, Json<Value>)> {
        let username = sanitize_username(&identifier);
+        let domain = host.trim().to_lowercase();


The domain sanitization logic (host.trim().to_lowercase()) is duplicated across multiple route handlers (lines 98, 131, 178, 202, 242, 287). Consider extracting this into a helper function to improve maintainability and consistency.

Copilot · 2025-11-13T15:56:17Z

crates/fly-api/src/lib.rs

+        let response = self
+            .client
+            .post("https://api.fly.io/graphql")
+            .header("Authorization", format!("Bearer {}", self.api_token))


The API token is included in the Authorization header without any form of sanitization or validation. Consider validating the token format in the constructor to fail fast if an invalid token is provided, preventing potential security issues or misleading error messages.

Copilot · 2025-11-13T15:56:17Z

crates/domain-validator/src/lib.rs

+pub trait DomainValidator: Send + Sync {
+    async fn validate_domain(&self, domain: &str) -> Result<(), DomainValidatorError>;
+}
+


The ListDomainValidator struct lacks documentation explaining its purpose and usage. Add doc comments to describe that this is a simple implementation that validates against a predefined list of allowed domains.

Suggested change

/// A simple implementation of the `DomainValidator` trait that validates domains

/// against a predefined list of allowed domains.

JssDWt

This looks good. It certainly saves quite some effort adding domains to this service. I added one comment for performance, and there's the failing CI, but otherwise LGTM.

JssDWt · 2025-11-17T12:11:12Z

crates/fly-api/src/lib.rs

+        let allowed_domains = self
+            .get_certificate_domains()
+            .await
+            .map_err(|e| DomainValidatorError::DomainNotAllowed(e.to_string()))?;


I think it should be good enough to load these certificates at startup. You can always restart the service after adding a certificate. It saves an api call per request.

roeierez

only structural comments, otherwise LGTM

roeierez · 2025-11-23T06:26:58Z

crates/domain-validator/Cargo.toml

@@ -0,0 +1,21 @@
+[package]


I think it is better to move the domain-validator and fly-api crates under the breez sdk folder where lnurl crate is located.
Also would consider moving the domain-validator content into lnurl-models crate and get rid of the domain-validtor crate (I am not a fan of one trait crate).

dsbreez and others added 5 commits November 5, 2025 10:20

Update getting_started.md

2db3559

Update README.md

584bc2f

Update DNS configuration instructions for LNURL server

1a969f9

Clarify instructions for configuring DNS records for LNURL server.

Optionally use fly.io api to check that the domain is handled by a cert

44e27d2

The check is done only on register

Merge branch 'main' into domain-check-fly

52b46c7

yaslama requested review from JssDWt and Copilot November 13, 2025 12:50

Copilot started reviewing on behalf of yaslama November 13, 2025 12:50 View session

Copilot finished reviewing on behalf of yaslama November 13, 2025 12:53

Copilot AI reviewed Nov 13, 2025

View reviewed changes

Fix some issues after the merge

d476507

yaslama requested a review from Copilot November 13, 2025 13:31

Copilot started reviewing on behalf of yaslama November 13, 2025 13:33 View session

Copilot finished reviewing on behalf of yaslama November 13, 2025 13:34

Copilot AI reviewed Nov 13, 2025

View reviewed changes

yaslama requested a review from Copilot November 13, 2025 13:42

Copilot started reviewing on behalf of yaslama November 13, 2025 13:42 View session

Copilot finished reviewing on behalf of yaslama November 13, 2025 13:44

Copilot AI reviewed Nov 13, 2025

View reviewed changes

yaslama requested a review from Copilot November 13, 2025 15:50

Copilot started reviewing on behalf of yaslama November 13, 2025 15:50 View session

Copilot finished reviewing on behalf of yaslama November 13, 2025 15:52

Copilot AI reviewed Nov 13, 2025

View reviewed changes

JssDWt reviewed Nov 17, 2025

View reviewed changes

roeierez reviewed Nov 23, 2025

View reviewed changes

-                "No domains found in Fly.io certificates".to_string(),
+                format!(
+                    "No valid certificates with hostnames found for app '{}'. \
+                     Possible causes: no certificates exist, certificates have no hostnames, or the app name is incorrect. \
+                     Please check your Fly.io app configuration and certificate status.",
+                    self.app_name
+                ),

		#[async_trait::async_trait]
		pub trait DomainValidator: Send + Sync {

-#[async_trait::async_trait]
-pub trait DomainValidator: Send + Sync {
+#[async_trait::async_trait]
+/// A trait for validating whether a domain is allowed.
+///
+/// This trait should be implemented by external consumers to provide custom domain validation logic.
+/// The `validate_domain` method checks if the given domain is permitted according to the implementation's rules.
+/// If the domain is not allowed, it should return `Err(DomainValidatorError::DomainNotAllowed(domain.to_string()))`.
+/// Otherwise, it should return `Ok(())`.
+pub trait DomainValidator: Send + Sync {
+    /// Asynchronously validates whether the provided domain is allowed.
+    ///
+    /// # Arguments
+    ///
+    /// * `domain` - The domain name to validate.
+    ///
+    /// # Returns
+    ///
+    /// * `Ok(())` if the domain is allowed.
+    /// * `Err(DomainValidatorError::DomainNotAllowed)` if the domain is not permitted.
+    ///
+    /// Implementors should return `DomainNotAllowed` only when the domain fails validation according to their rules.

-        let domain = host.trim().to_lowercase();
+        let domain = host.trim().to_lowercase();
+        // Domain validation: ensure only allowed domains can request invoices
+        if let Some(allowed_domains) = &state.allowed_domains {
+            if !allowed_domains.contains(&domain) {
+                return Err((
+                    StatusCode::FORBIDDEN,
+                    Json(Value::String("domain not allowed".into())),
+                ));
+            }
+        }



	/// A simple implementation of the `DomainValidator` trait that validates domains
	/// against a predefined list of allowed domains.

Domain check using fly.io certificates #410

Are you sure you want to change the base?

Domain check using fly.io certificates #410

Uh oh!

Conversation

yaslama commented Nov 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025