Skip to content

fix(settings): drop a trailing host dot from the Public URL (abilityai/trinity#3881) - #3904

Merged
vybe merged 4 commits into
devfrom
feature/3881-public-url-trailing-dot
Oct 23, 2026
Merged

vybe merged 4 commits into
devfrom
feature/3881-public-url-trailing-dot

Conversation

@obasilakis

@obasilakis obasilakis commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • New settings_service.normalize_public_url: trims whitespace and a trailing / (as before) and drops a trailing dot from the host, keeping port and path. https://host.example.com./ → https://host.example.com.
  • PUT /api/settings/public_chat_url stores, and re-points Telegram webhooks at, the normalised value.
  • get_public_chat_url() (public links, paid chat) normalises on read too, and the webhook registrars that read the stored row directly (Telegram startup re-registration and connect, WhatsApp, VoIP) now call the new get_public_chat_url_setting(), the same row-only read, normalised. The portal's local resolver copy normalises too. So a row stored before this fix, or a dotted PUBLIC_CHAT_URL env value, is handed out clean. No migration needed. A scan test fails if a raw row read returns.

Chose "strip" over "refuse with 422" from the AC: an absolute name is the same name, and refusing would leave an existing dotted row in force.

Not in this PR: the optional follow-up to show Telegram's getWebhookInfo last_error_message in the Telegram panel.

Changes

  • src/backend/services/settings_service.py
  • src/backend/routers/settings/generic.py
  • tests/unit/test_3881_public_url_trailing_dot.py (new)

Test Plan

  • cd tests && pytest unit/test_3881_public_url_trailing_dot.py unit/test_2691_public_url_reachability.py unit/test_3215_public_base_url.py unit/test_2380_tls_ask_endpoint.py unit/test_2380_install_provenance.py unit/test_1028_settings_package.py — 219 passed (new test red on all 14 cases before the fix)
  • pytest unit -k "public or telegram or whatsapp or settings or paid" — 1706 passed; the 30 failures (IPv6-mapped URL validation, A2A paid gate) also fail on dev locally
  • Manual: save https://<tunnel>.trycloudflare.com., Settings shows it without the dot, Telegram getWebhookInfo has no SSL error

Fixes #3881

🤖 Generated with Claude Code

A Public URL like https://host.example.com. was stored as typed, and every
Telegram bot was re-registered there. The TLS certificate is for the name
without the dot, so Telegram's check failed and no message arrived while
the bot showed Connected.

normalize_public_url strips the dot (keeping port and path). The save
path stores and re-points with the clean value, and get_public_chat_url
normalises on read, so a row stored earlier or a dotted PUBLIC_CHAT_URL
env never reaches a webhook, public link or WhatsApp URL either.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/backend/services/settings_service.py Fixed
obasilakis and others added 2 commits October 22, 2026 17:24
CodeQL flagged the trailing-dot regex as polynomial on user input. Split
the authority into userinfo, host and port with rpartition and rstrip the
host instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…3881)

Telegram startup re-registration and connect, WhatsApp and VoIP read the
public_chat_url row directly and skipped normalisation, so a row stored
with a trailing dot before this fix was re-registered at every boot.
They now call settings_service.get_public_chat_url_setting(), which is
the same row-only read, normalised. The portal's local resolver copy
normalises too. A scan test fails if a raw row read comes back.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@obasilakis

Copy link
Copy Markdown
Contributor Author

/review + /validate-pr

Verdict: APPROVE after fixes (review requested changes for C1; fixed below). CodeQL alert #458 (py/polynomial-redos) is cleared.

Review

  • normalize_public_url edge cases checked: userinfo with :/@, ports (including host.:8443), IPv6 literals and zone ids, paths, query strings, fragments, several trailing dots, mixed case, no scheme, None.
  • Mutation: reverting the save line, the read-side change, or the strip itself turns 2, 2 and 9 tests red.
  • C1: Telegram startup re-registration (main.py), Telegram connect (routers/telegram.py), WhatsApp (main.py, routers/whatsapp.py ×2) and VoIP read the stored row directly and skipped the normalisation, so a dotted row stored before this fix was re-registered at every boot.

Fixed after review

  • New settings_service.get_public_chat_url_setting(): the same row-only read (no env fallback), normalised. All six sites use it; the client portal's local resolver copy normalises too.
  • A scan test fails if a raw get_setting("public_chat_url") read returns outside settings_service.
  • The trailing-dot regex was replaced with rpartition parsing (CodeQL).

Not changed, by design: the admin read of the raw row (settings/mcp_url.py) and config.PUBLIC_CHAT_URL as a CORS origin; neither registers a webhook.

Open: the manual tunnel + getWebhookInfo check, and the optional Telegram panel follow-up from the issue.

@github-actions

Copy link
Copy Markdown

⚠️ Nightly unit-suite found regressions when this PR is merged into dev (1 of 3 seeds).

Regression details (head_sha: `7bbf917fbad7e7940d469f0465911c608142d931`)

Seed 99999

Backend unit-suite regression diff

Per-XML totals

Side Path Total Pass Fail Error Skip
base junit-base-pr3904-99999.xml 28399 28319 0 0 80
head junit-head-pr3904-99999.xml 28415 28334 1 0 80

❌ New failures introduced by HEAD (1)

Tests failing under HEAD that did not fail under BASE in any seed:

  • [F] test_ent879_aauth_resource_route::test_resume_by_handle_polls_the_person_server_then_resends_the_stored_request

Legend: [F] = assertion failure, [E] = collection or fixture error.
Identity = (classname, name, kind); union taken across all input XMLs.

Reproduce locally: git merge dev && ( cd tests && python -m pytest unit/ -m "not slow" -p randomly --randomly-seed=99999 )

…; pin the portal mirror (#3881) — mechanical, per the merge-train note on the PR

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@vybe

vybe commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

merge-train (2026-10-23): pushed to your branch

Two mechanical fixes landed on feature/3881-public-url-trailing-dot as one commit, 6d181381c (on top of the validated head 7bbf917fb), approved at Gate 1.

1. normalize_public_url no longer raises on a malformed authority — src/backend/services/settings_service.py:1137-1144

The validator found that urlsplit raises ValueError: Invalid IPv6 URL on an input like https://[::1. On the save path (routers/settings/generic.py:531) the normaliser now runs before classify_advertised_url, which used to swallow that input and answer 422 — so the PR turned a 422 into an unhandled 500. The split is now wrapped in try/except ValueError: return url: the input is handed through unchanged and the downstream classifier keeps answering 422.

Tests added to tests/unit/test_3881_public_url_trailing_dot.py:

  • test_normalise_passes_a_malformed_authority_through_unchanged — normalize_public_url("https://[::1") returns the input and does not raise
  • test_save_refuses_a_malformed_authority_with_422_not_500 — drives the real update_setting handler (same harness as test_save_stores_and_repoints_without_the_dot) and asserts HTTPException(422)

Both were red before the guard (the exact ValueError above), green after.

2. The portal mirror is pinned — src/backend/client_portal/service.py:201-206

_public_chat_url (the client portal's local copy of get_public_chat_url()) had no test: reverting that hunk left the suite green. New tests/unit/test_3881_portal_mirror.py drives the real get_portal_base_url() with a dotted stored row and, separately, a dotted PUBLIC_CHAT_URL env, and asserts the dot is gone. Verified it bites: with the mirror hunk temporarily reverted it fails on both cases (https://host.example.com. vs expected https://host.example.com); restored, it passes.

Test run: pytest unit/test_3881_public_url_trailing_dot.py unit/test_3881_portal_mirror.py unit/test_2691_public_url_reachability.py unit/test_3215_public_base_url.py unit/test_2380_tls_ask_endpoint.py unit/test_2380_install_provenance.py unit/test_1028_settings_package.py — 225 passed. No unmerged paths, no conflict markers.

The PR body was also patched from Fixes abilityai/trinity#3881 to Fixes #3881 so status auto-promotion fires on merge.

No approval, label or merge from the train — that stays with the owner.

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train/20261023-2211 (#3930, all checks green)

@vybe
vybe merged commit 581c96d into dev Oct 23, 2026
24 checks passed
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.

4 participants