From d63040b014822fcfc62b47e05b02f8d195a43860 Mon Sep 17 00:00:00 2001 From: Phil Skentelbery Date: Wed, 17 Dec 2025 15:40:39 -0700 Subject: [PATCH] fix(templates): handle optional PKCE in retry links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Request a new code" and "Try Again" links were outputting code_challenge=None when PKCE was not used, causing the retry to fail with validation errors. Now only includes PKCE parameters if code_challenge has a value. šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- client_id_validation_analysis.md | 187 ++++++++++++++++++ pyproject.toml | 2 +- src/gondulf/templates/verification_error.html | 2 +- src/gondulf/templates/verify_code.html | 2 +- test_client_id_spec_compliance.py | 93 +++++++++ uv.lock | 2 +- 6 files changed, 284 insertions(+), 4 deletions(-) create mode 100644 client_id_validation_analysis.md create mode 100644 test_client_id_spec_compliance.py diff --git a/client_id_validation_analysis.md b/client_id_validation_analysis.md new file mode 100644 index 0000000..719bfd0 --- /dev/null +++ b/client_id_validation_analysis.md @@ -0,0 +1,187 @@ +# Client ID Validation Compliance Analysis + +## W3C IndieAuth Specification Requirements (Section 3.2) + +According to the W3C IndieAuth specification, client identifiers MUST meet the following requirements: + +1. **Scheme**: MUST have either `https` or `http` scheme +2. **Path**: MUST contain a path component +3. **Path Segments**: MUST NOT contain single-dot or double-dot path segments +4. **Query String**: MAY contain a query string component +5. **Fragment**: MUST NOT contain a fragment component +6. **User Info**: MUST NOT contain username or password component +7. **Port**: MAY contain a port +8. **Hostname**: + - MUST be domain names or a loopback interface + - MUST NOT be IPv4 or IPv6 addresses except for 127.0.0.1 or [::1] + +## Current Gondulf Implementation Analysis + +### What We Currently Validate āœ… + +1. **Scheme Validation** (PARTIAL) + - āœ… We require HTTPS scheme in `normalize_client_id()` + - āŒ We reject HTTP completely, even for localhost (spec allows HTTP) + - Location: `/src/gondulf/utils/validation.py` lines 48-49 + +2. **Query String Preservation** + - āœ… We preserve query strings correctly + - Location: `/src/gondulf/utils/validation.py` lines 58-59 + +3. **Fragment Preservation** (BUT NOT VALIDATED) + - āš ļø We preserve fragments but don't reject them + - Location: `/src/gondulf/utils/validation.py` lines 60-61 + +### What We DON'T Validate āŒ + +1. **Path Component Requirement** + - āŒ We don't verify that a path component exists + - Spec requires at least "/" as a path + +2. **Path Segment Validation** + - āŒ We don't check for single-dot (.) or double-dot (..) path segments + - These should be rejected per spec + +3. **Fragment Component** + - āŒ We don't reject URLs with fragments (we actually preserve them) + - Spec says MUST NOT contain fragment + +4. **User Info Component** + - āŒ We don't check for or reject username:password in URLs + - Spec says MUST NOT contain username or password + +5. **Hostname/IP Validation** + - āŒ We don't validate that hostnames are domain names + - āŒ We don't reject IPv4/IPv6 addresses (except 127.0.0.1 and [::1]) + - āŒ We don't have special handling for localhost/127.0.0.1/[::1] + +6. **Port Support** + - āœ… We preserve ports correctly + - āš ļø But we don't allow HTTP for localhost with ports + +## Reference Implementation (IndieLogin) Comparison + +From examining `/home/phil/Projects/indielogin.com/app/Authenticate.php`: + +1. Uses `\p3k\url\is_url()` for basic URL validation (line 49) +2. Checks that client_id contains a dot OR is localhost (line 51) +3. Validates redirect_uri domain matches client_id domain (lines 75-95) +4. Does NOT appear to validate all spec requirements either + +## Non-Compliance Issues Found + +### Critical Issues + +1. **HTTP localhost support missing** + ```python + # Current code rejects all HTTP + if parsed.scheme != 'https': + raise ValueError("client_id must use https scheme") + ``` + Should allow HTTP for localhost/127.0.0.1/[::1] + +2. **Fragment rejection missing** + ```python + # Current code preserves fragments instead of rejecting them + if parsed.fragment: + normalized += f"#{parsed.fragment}" + ``` + +3. **Path component not validated** + - No check that path exists (at minimum "/") + +4. **IP address validation missing** + - Should reject IPs except 127.0.0.1 and [::1] + +### Security Implications + +1. **Fragment acceptance** - Could lead to confusion about actual client_id +2. **User info not rejected** - Could expose credentials in logs +3. **IP addresses allowed** - Could bypass domain validation +4. **Path traversal** - Accepting ../.. could be security issue + +## Recommended Fixes + +### Priority 1: Add proper client_id validation function + +Create a new `validate_client_id()` function that checks ALL spec requirements: + +```python +def validate_client_id(client_id: str) -> tuple[bool, str]: + """ + Validate client_id per W3C IndieAuth spec Section 3.2. + + Returns: + (is_valid, error_message) + """ + try: + parsed = urlparse(client_id) + + # 1. Check scheme + if parsed.scheme not in ['https', 'http']: + return False, "client_id must use https or http scheme" + + # 2. For HTTP, only allow localhost/loopback + if parsed.scheme == 'http': + if parsed.hostname not in ['localhost', '127.0.0.1', '[::1]', '::1']: + return False, "HTTP scheme only allowed for localhost/127.0.0.1/[::1]" + + # 3. Must have path component + if not parsed.path: + return False, "client_id must contain a path component" + + # 4. Check for . or .. segments + path_segments = parsed.path.split('/') + if '.' in path_segments or '..' in path_segments: + return False, "client_id must not contain . or .. path segments" + + # 5. Must NOT have fragment + if parsed.fragment: + return False, "client_id must not contain a fragment" + + # 6. Must NOT have user info + if parsed.username or parsed.password: + return False, "client_id must not contain username or password" + + # 7. Check hostname (no raw IPs except loopback) + if parsed.hostname: + # Check if it's an IP address + import ipaddress + try: + ip = ipaddress.ip_address(parsed.hostname) + # Only allow loopback IPs + if not ip.is_loopback: + return False, f"client_id must not use IP address {parsed.hostname}" + except ValueError: + # Not an IP, that's good (it's a domain) + pass + + return True, "" + + except Exception as e: + return False, f"Invalid URL: {e}" +``` + +### Priority 2: Update normalize_client_id() + +Should call validate_client_id() first, then normalize. + +### Priority 3: Add comprehensive tests + +Test all validation rules with both positive and negative cases. + +## Conclusion + +**Current Status: NOT COMPLIANT** āŒ + +Gondulf's client_id validation is currently **not compliant** with W3C IndieAuth specification Section 3.2. We have several missing validations that need to be implemented before v1.0.0 release. + +### Required Actions Before v1.0.0 + +1. Implement complete `validate_client_id()` function +2. Update `normalize_client_id()` to validate first +3. Add support for HTTP on localhost/127.0.0.1/[::1] +4. Add tests for all validation rules +5. Update authorization endpoint to use new validation + +This is a **BLOCKING ISSUE** for v1.0.0 release as it affects spec compliance. \ No newline at end of file diff --git a/pyproject.toml b/pyproject.toml index 2c69aa3..b1aca3b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "gondulf" -version = "1.0.1" +version = "1.0.2" description = "A self-hosted IndieAuth server implementation" readme = "README.md" requires-python = ">=3.10" diff --git a/src/gondulf/templates/verification_error.html b/src/gondulf/templates/verification_error.html index 724697e..c43d29b 100644 --- a/src/gondulf/templates/verification_error.html +++ b/src/gondulf/templates/verification_error.html @@ -33,7 +33,7 @@ {% endif %}

- + Try Again

diff --git a/src/gondulf/templates/verify_code.html b/src/gondulf/templates/verify_code.html index 24deca6..a1b18eb 100644 --- a/src/gondulf/templates/verify_code.html +++ b/src/gondulf/templates/verify_code.html @@ -36,7 +36,7 @@

Did not receive a code? Check your spam folder. - + Request a new code

diff --git a/test_client_id_spec_compliance.py b/test_client_id_spec_compliance.py new file mode 100644 index 0000000..17e92b6 --- /dev/null +++ b/test_client_id_spec_compliance.py @@ -0,0 +1,93 @@ +#!/usr/bin/env python3 +"""Test script to verify client_id validation compliance with W3C IndieAuth spec Section 3.2.""" + +import sys +from urllib.parse import urlparse + +# Import the validation function +sys.path.insert(0, '/home/phil/Projects/Gondulf/src') +from gondulf.utils.validation import normalize_client_id + + +def test_client_id(client_id: str, should_pass: bool, reason: str): + """Test a single client_id against the spec.""" + try: + result = normalize_client_id(client_id) + passed = True + print(f"āœ“ Accepted: {client_id} -> {result}") + except ValueError as e: + passed = False + print(f"āœ— Rejected: {client_id} ({str(e)})") + + if passed != should_pass: + print(f" ERROR: Expected {'pass' if should_pass else 'fail'}: {reason}") + return False + return True + + +def main(): + """Run comprehensive client_id validation tests.""" + print("Testing client_id validation against W3C IndieAuth spec Section 3.2") + print("=" * 70) + + tests = [ + # Valid client_ids per spec + ("https://example.com", True, "Valid HTTPS URL with domain"), + ("https://example.com/", True, "Valid HTTPS URL with path /"), + ("https://example.com/app", True, "Valid HTTPS URL with path"), + ("https://example.com/app/client", True, "Valid HTTPS URL with multiple path segments"), + ("https://example.com?foo=bar", True, "Valid HTTPS URL with query string"), + ("https://example.com:8080", True, "Valid HTTPS URL with port"), + ("https://example.com:8080/app", True, "Valid HTTPS URL with port and path"), + ("http://localhost", True, "Valid HTTP localhost (loopback allowed)"), + ("http://localhost:3000", True, "Valid HTTP localhost with port"), + ("http://127.0.0.1", True, "Valid HTTP IPv4 loopback"), + ("http://127.0.0.1:3000", True, "Valid HTTP IPv4 loopback with port"), + ("http://[::1]", True, "Valid HTTP IPv6 loopback"), + ("http://[::1]:3000", True, "Valid HTTP IPv6 loopback with port"), + + # Invalid client_ids per spec + ("http://example.com", False, "HTTP not allowed for non-loopback"), + ("https://example.com#fragment", False, "Fragment not allowed"), + ("https://user:pass@example.com", False, "Username/password not allowed"), + ("https://192.168.1.1", False, "IPv4 address not allowed (except 127.0.0.1)"), + ("https://[2001:db8::1]", False, "IPv6 address not allowed (except ::1)"), + ("https://10.0.0.1", False, "Private IPv4 not allowed"), + ("https://172.16.0.1", False, "Private IPv4 not allowed"), + ("example.com", False, "Missing scheme"), + ("ftp://example.com", False, "Invalid scheme (not http/https)"), + ("https://", False, "Missing hostname"), + ("https://example.com/../secret", False, "Path with .. segments not allowed"), + ("https://example.com/./secret", False, "Path with . segments not allowed"), + + # Edge cases that spec might not explicitly cover + ("HTTPS://EXAMPLE.COM", True, "Uppercase scheme/host should be normalized"), + ("https://example.com:443", True, "Default HTTPS port should be normalized"), + ("http://localhost:80", True, "Default HTTP port on localhost"), + ("https://xn--e1afmkfd.xn--p1ai", True, "Internationalized domain names (IDN)"), + ] + + all_passed = True + pass_count = 0 + fail_count = 0 + + for client_id, should_pass, reason in tests: + if test_client_id(client_id, should_pass, reason): + pass_count += 1 + else: + fail_count += 1 + all_passed = False + + print("\n" + "=" * 70) + print(f"Results: {pass_count} passed, {fail_count} failed") + + if all_passed: + print("\nāœ… All tests passed! client_id validation appears spec-compliant.") + else: + print("\nāŒ Some tests failed. Review the validation logic against the spec.") + + return 0 if all_passed else 1 + + +if __name__ == "__main__": + sys.exit(main()) \ No newline at end of file diff --git a/uv.lock b/uv.lock index 62a10da..d0f667b 100644 --- a/uv.lock +++ b/uv.lock @@ -431,7 +431,7 @@ wheels = [ [[package]] name = "gondulf" -version = "1.0.0rc1" +version = "1.0.1" source = { editable = "." } dependencies = [ { name = "aiosmtplib" },