Repository navigation
fix(channels): hand chat users the web app address when a setup ceremony can't run in chat #7897
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
2197317
e8ba225
cc7686f
b2f36fd
e1db46b
bbd8a76
f842ec0
0d99188
ad1a047
8a1c85b
775ac91
d7562e4
4564647
a5ef722
ba2cc0d
37ca96e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| //! Validation for the deployment-configured public web origin used to build | ||
| //! extension connect/setup links (`/extensions`, `/chat?connect=…`) shown to | ||
| //! chat users. | ||
| //! | ||
| //! The origin's source is `IRONCLAW_REBORN_WEBUI_BASE_URL` | ||
| //! (`ironclaw_composition::extension_host_assembly::connect_link_base_url_from_env`), | ||
| //! read independently by three call sites — the deployment-channel notice | ||
| //! (`ironclaw_extension_host::channel_host::configured_origin`), the | ||
| //! device-link-unavailable chat prompt | ||
| //! (`ironclaw_assistant::run_delivery::prompts::extensions_page_link`), and | ||
| //! the personal-account setup nudge | ||
| //! (`ironclaw_extension_manager::install_guidance::personal_setup_link`) — | ||
| //! that used to trim-and-check-non-empty only, never confirming the value was | ||
| //! an absolute origin at all. A deployment misconfigured as | ||
| //! `IRONCLAW_REBORN_WEBUI_BASE_URL=app.example.com` (no scheme) rendered the | ||
| //! relative `app.example.com/extensions` into a customer conversation, and a | ||
| //! value carrying a query or fragment could redirect the link away from the | ||
| //! Extensions page. This module is the one place that decides an origin is | ||
| //! safe to render. | ||
|
|
||
| /// Validate `base_url` as an absolute `http`/`https` origin with no query | ||
| /// string or fragment, and return it with any trailing slash trimmed. | ||
| /// | ||
| /// Returns `None` for anything that is not a safely renderable origin: no | ||
| /// value, blank/whitespace, a scheme-less (relative) value, a non-http(s) | ||
| /// scheme, or a value carrying a `?query` or `#fragment` (either would change | ||
| /// where the rendered link actually goes). `None` here means "ship the | ||
| /// link-free copy" at every call site — never a startup failure. That | ||
| /// deliberately differs from the OAuth callback consumer of the same | ||
| /// environment variable, which fails startup on a blank value; see | ||
| /// `connect_link_base_url_from_env`'s own doc comment for why the two | ||
| /// consumers of one variable are allowed to disagree on how unusable costs. | ||
| /// | ||
| /// `https://x.test/` and `https://x.test` are equivalent — the trailing | ||
| /// slash is trimmed, not treated as part of the origin's identity. | ||
| pub fn validated_connect_link_origin(base_url: Option<&str>) -> Option<&str> { | ||
| let trimmed = base_url?.trim(); | ||
| if trimmed.is_empty() { | ||
| return None; // silent-ok: blank/whitespace origin means "unset"; every caller ships link-free | ||
| } | ||
|
|
||
| let scheme_end = trimmed.find("://")?; // silent-ok: no scheme means a relative value, never safe to render as a link | ||
| // Schemes are case-insensitive (RFC 3986 §3.1), and `.claude/rules/types.md` | ||
| // requires normalizing case-insensitive external values at the boundary — | ||
| // an operator writing `HTTPS://` means the same origin. | ||
| let scheme = &trimmed[..scheme_end]; | ||
| if !scheme.eq_ignore_ascii_case("http") && !scheme.eq_ignore_ascii_case("https") { | ||
| return None; // silent-ok: only http(s) origins are safe to render as a clickable link | ||
| } | ||
|
|
||
| let authority = &trimmed[scheme_end + 3..]; | ||
| if authority.contains(['?', '#']) { | ||
| return None; // silent-ok: a query string or fragment can redirect the link away from its intended page | ||
| } | ||
|
|
||
| let authority = authority.trim_end_matches('/'); | ||
| if authority.is_empty() { | ||
| return None; // silent-ok: a scheme with no host is not a usable origin | ||
| } | ||
|
|
||
| Some(&trimmed[..scheme_end + 3 + authority.len()]) | ||
|
Comment on lines
+42
to
+61
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Reject malformed URLs and non-origin URLs. Line 42 accepts Parse the value and require a valid host, optional port, and an empty or root path. Add regression cases for malformed authorities and path-bearing values. As per coding guidelines, “Treat every listener, route, product adapter, runtime lane, container, and external service as untrusted until a typed boundary establishes otherwise.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::validated_connect_link_origin; | ||
|
|
||
| #[test] | ||
| fn accepts_absolute_https_origin() { | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("https://app.example.com")), | ||
| Some("https://app.example.com") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn accepts_an_uppercase_scheme() { | ||
| // Fail-safe rejection would have shipped link-free for a perfectly | ||
| // valid origin. | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("HTTPS://app.example.com")), | ||
| Some("HTTPS://app.example.com") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn accepts_absolute_http_origin() { | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("http://app.example.com")), | ||
| Some("http://app.example.com") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn trims_trailing_slash() { | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("https://app.example.com/")), | ||
| validated_connect_link_origin(Some("https://app.example.com")) | ||
| ); | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("https://app.example.com/")), | ||
| Some("https://app.example.com") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_scheme_less_value() { | ||
| // The exact CodeRabbit-reported case: no scheme renders the RELATIVE | ||
| // `app.example.com/extensions` into a customer conversation. | ||
| assert_eq!(validated_connect_link_origin(Some("app.example.com")), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_non_http_scheme() { | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("ftp://app.example.com")), | ||
| None | ||
| ); | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("javascript://app.example.com")), | ||
| None | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_query_string() { | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("https://x.test/?a=1")), | ||
| None | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_fragment() { | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("https://x.test#f")), | ||
| None | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_query_and_fragment_together() { | ||
| assert_eq!( | ||
| validated_connect_link_origin(Some("https://x.test/?a=1#f")), | ||
| None | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_blank_whitespace_and_none() { | ||
| assert_eq!(validated_connect_link_origin(None), None); | ||
| assert_eq!(validated_connect_link_origin(Some("")), None); | ||
| assert_eq!(validated_connect_link_origin(Some(" ")), None); | ||
| assert_eq!(validated_connect_link_origin(Some("/")), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_scheme_with_no_host() { | ||
| assert_eq!(validated_connect_link_origin(Some("https://")), None); | ||
| assert_eq!(validated_connect_link_origin(Some("https:///")), None); | ||
| } | ||
| } | ||
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.
Medium — Add coverage for composed origin plumbing.
The new device-link delivery tests set
HarnessOptions.connect_link_base_urldirectly, bypassingconnect_link_base_url_from_env()and the production composition-to-workflow wiring changed here. If this environment read or either forwarding step regresses, the tests remain green while deployed chat auth notices lose their URL.Fix: Add a caller-level test that configures the production origin source and asserts the posted message contains the Extensions URL.
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.
Verified against the code. Your comment bundles two things, and they came out differently — so partly not-addressing, partly valid.
The env read is not bypassed. You're describing the e2e tests (
slack_dm_device_link_auth_prompt_*), which do setHarnessOptions.connect_link_base_urldirectly. But the integration tests added in0d991880euseWebUiBaseUrlEnvGuard(tests/integration/extension_delivery.rs:1543-1582), which sets the real process env varIRONCLAW_REBORN_WEBUI_BASE_URLand drivesstart_channel_host_assembly_for_test→start_channel_host_from_stores→start_channel_host— the same functionservecalls, readingconnect_link_base_url_from_env()atextension_host_assembly.rs:589. Unstubbed, through production assembly. If that env read regressed,slack_unserviceable_auth_gate_*fails.channel_workflow.rs:474is also covered — the live-observer forwarding is what those tests traverse to reachobserver.rs:1052-1055.channel_workflow.rs:200is genuinely uncovered, and you're right about the consequence. The only existing test touching the triggered unserviceable path (triggered_manual_token_auth_cancels_and_notifies_all_targets) hand-setsauth_url: Nonefrom a literal and asserts only that the phrase "Ironclaw web app" appears — never URL presence or absence.What I did about it, and what I didn't. I'm not building a background-run harness for this. The two consumption sites were byte-for-byte identical, so instead of testing a duplication I removed it: both now go through one method on
RunDeliveryServices, so the triggered path cannot diverge from the live one at the point where the message is built. That closes the half that has actually bitten this PR twice — a helper reachable from only one path.What remains genuinely untested is the forwarding at
:200— two adjacentRunDeliveryServicesliterals in one file, both reading the sameself.services.setup_link_base_url. A regression there means changing one and not its neighbour. I judged a new test-support entry point for the background/triggered notifier assembly disproportionate to that risk; saying so explicitly rather than implying coverage I don't have.