-
Notifications
You must be signed in to change notification settings - Fork 8
Domain check using fly.io certificates #410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2db3559
584bc2f
1a969f9
44e27d2
52b46c7
d476507
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -95,9 +95,10 @@ where | |||||||||||||||||||||||
| Extension(state): Extension<State<DB>>, | ||||||||||||||||||||||||
| ) -> Result<Json<CheckUsernameAvailableResponse>, (StatusCode, Json<Value>)> { | ||||||||||||||||||||||||
| let username = sanitize_username(&identifier); | ||||||||||||||||||||||||
| let domain = host.trim().to_lowercase(); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| let domain = host.trim().to_lowercase(); | |
| let domain = sanitize_domain_input(&host); |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check_username_available function does not validate the domain against allowed domains before querying the database. Unlike the register function (lines 134-137), this allows unauthorized domains to check username availability. Consider adding domain validation here to prevent information disclosure about existing usernames on unauthorized domains.
| let domain = host.trim().to_lowercase(); | |
| let domain = host.trim().to_lowercase(); | |
| // Domain validation: only allow allowed domains to check username availability | |
| if !state.allowed_domains.contains(&domain) { | |
| return Err(( | |
| StatusCode::FORBIDDEN, | |
| Json(json!({"error": "domain not allowed"})), | |
| )); | |
| } |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
yaslama marked this conversation as resolved.
Show resolved
Hide resolved
yaslama marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
yaslama marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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())), | |
| )); | |
| } | |
| } |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
yaslama marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,9 +1,9 @@ | ||||
| use spark::operator::OperatorConfig; | ||||
| use spark::operator::rpc::ConnectionManager; | ||||
| use spark::operator::OperatorConfig; | ||||
| use spark::session_manager::InMemorySessionManager; | ||||
| use spark::ssp::ServiceProvider; | ||||
| use spark_wallet::DefaultSigner; | ||||
| use std::{collections::HashSet, sync::Arc}; | ||||
| use std::sync::Arc; | ||||
|
||||
| use std::sync::Arc; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| [package] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better to move the domain-validator and fly-api crates under the breez sdk folder where lnurl crate is located. |
||
| name = "domain-validator" | ||
| edition = "2024" | ||
| version = "0.1.0" | ||
|
|
||
| [dependencies] | ||
| async-trait = "0.1.88" | ||
|
|
||
| thiserror = "2.0.12" | ||
|
|
||
yaslama marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| [lints] | ||
| clippy.suspicious = { level = "warn", priority = -1 } | ||
| clippy.complexity = { level = "warn", priority = -1 } | ||
| clippy.perf = { level = "warn", priority = -1 } | ||
| clippy.style = { level = "warn", priority = -1 } | ||
| clippy.pedantic = { level = "warn", priority = -1 } | ||
| clippy.missing_errors_doc = "allow" | ||
| clippy.missing_panics_doc = "allow" | ||
| clippy.must_use_candidate = "allow" | ||
| clippy.struct_field_names = "allow" | ||
| clippy.arithmetic_side_effects = "warn" | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| use std::collections::HashSet; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| use thiserror::Error; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| #[derive(Debug, Error)] | ||||||||||||||||||||||||||||||||||||||||||||||
| pub enum DomainValidatorError { | ||||||||||||||||||||||||||||||||||||||||||||||
| #[error("Domain {0} is not allowed")] | ||||||||||||||||||||||||||||||||||||||||||||||
| DomainNotAllowed(String), | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| #[async_trait::async_trait] | ||||||||||||||||||||||||||||||||||||||||||||||
| pub trait DomainValidator: Send + Sync { | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+11
to
+12
|
||||||||||||||||||||||||||||||||||||||||||||||
| #[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. |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| /// A simple implementation of the `DomainValidator` trait that validates domains | |
| /// against a predefined list of allowed domains. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| [package] | ||
| name = "fly-api" | ||
| edition = "2024" | ||
| version = "0.1.0" | ||
|
|
||
| [dependencies] | ||
| reqwest = { version = "0.12.10", features = ["json"] } | ||
| serde = { version = "1.0.219", features = ["derive"] } | ||
| serde_json = "1.0.140" | ||
| thiserror = "2.0.12" | ||
| tokio = { version = "1.45.1", features = ["rt-multi-thread", "macros", "signal"] } | ||
| domain-validator = { path = "../domain-validator" } | ||
| async-trait = "0.1.88" | ||
|
|
||
yaslama marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| [lints] | ||
| clippy.suspicious = { level = "warn", priority = -1 } | ||
| clippy.complexity = { level = "warn", priority = -1 } | ||
| clippy.perf = { level = "warn", priority = -1 } | ||
| clippy.style = { level = "warn", priority = -1 } | ||
| clippy.pedantic = { level = "warn", priority = -1 } | ||
| clippy.missing_errors_doc = "allow" | ||
| clippy.missing_panics_doc = "allow" | ||
| clippy.must_use_candidate = "allow" | ||
| clippy.struct_field_names = "allow" | ||
| clippy.arithmetic_side_effects = "warn" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant use of variable name in field initialization. This can be simplified from
domain_validator: domain_validator,to justdomain_validator,using Rust's field init shorthand.