[ericcaiwx-star]

Closes #2383

Problem

Installation startup stores url.href for observability.url. On parent c28bd23933b33fcb7a726afd8049d770321b0b31, a tab between good.example and evil.com is admitted and stored as http://good.exampleevil.com/x (host good.exampleevil.com). A newline is deleted the same way. http://evil.com\\.good.example/path is stored as http://evil.com/.good.example/path (host evil.com). Profile preflight copies the written string. http://grafana.example.internal/d is unchanged. A backslash in the query, as in https://grafana.example.test/d/operations?var-pattern=\d+, does not change the host.

Change

Profile preflight and startup refuse a raw tab, newline, or carriage return anywhere in the observability URL, and a backslash before the query or fragment, before storing href. A backslash in the query stays. A URL that already fails for userinfo keeps that message. Shorthand IPv4 refusal is unchanged. API and worker startup classify the new sentence as OBSERVABILITY_URL_INVALID.

Verification

Parent c28bd23933b33fcb7a726afd8049d770321b0b31 admitted the tab, newline, and backslash URLs and stored the rewritten href above.

Head 23673d101eb4b6769909785de876b813e95accad, which merges current main 9031ae3d28e61b3bd50c7aeba9ca7b16a5548feb:

node --test --test-name-pattern "Installation startup validates the optional external observability URL|API and worker refuse a bad observability host|profiles pass an optional observability URL|the codex profile carries a Codex model endpoint" tests/integration/configuration-startup.test.mjs tests/integration/profile-renderer.test.mjs

Exit 0. 4 passed, 0 failed. The tab, newline, carriage return, and host-backslash URLs fail preflight and startup with the new sentence. API and worker exit 1 with OBSERVABILITY_URL_INVALID and no diagnostic message. http://192.168.1.1 is still stored as http://192.168.1.1/. A query backslash on grafana.example.test is admitted and keeps that host. The Codex profile still carries runtime.codexModelBaseUrl.

node scripts/docs-site/word-count.mjs --max 2500

Exit 0. docs/guides/deploy/breaking-changes.md 2472, docs/guides/deploy/breaking-changes-archive.md 2499, docs/guides/deploy/installation-profiles.md 2490, docs/reference/configuration.md 1963.

Risks

A profile or Installation whose observability URL contains a tab, newline, or a backslash before the query fails preflight or startup until that character is removed. A backslash in the query is kept. The notice moves the 2026-10-13 demo cluster CIDR entry onto the archive page.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review blocked

Automated review did not run, so no review verdict was produced.

Reason: The input-safety check rejected material in this revision. No detected value, path, or scanner output is reproduced here.

Automatic review is on hold, including after source changes. Request a fresh re-review after resolving the failure; maintainers can also release the hold through an intentional scanner-policy update.

Next step: If this is a genuine credential, remove and rotate it. If it is an intentional test fixture, a maintainer must review and qualify it.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge.

What this changes

This PR rejects raw tabs, newlines, carriage returns, and backslashes in observability URLs during profile rendering and controller startup.

Example: An operator configures http://good.example evil.com/x as the observability destination.

• Before: Startup accepts it and stores http://good.exampleevil.com/x.
• After: Profile preflight and startup reject the supplied URL.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The useful repair has focused regression coverage, but required real-behavior proof is absent and the startup-diagnostic defect remains.
Proof confidence	🧂 unranked krab (1/6)	Real behavior proof is necessary before merge. See Before merge.
Patch quality	🦪 silver shellfish (2/6)	2 actionable review findings remain.

Product

Kind: Bug fix · Worth it: Needs a maintainer decision · Fix scope: Complete
User problem: An accepted observability URL can silently open a different host from the operator's written destination.
Reason: Preventing destination rewriting is useful, but rejecting host-preserving URL components changes the existing supported-input contract beyond the demonstrated bug.

Merge readiness

⛔ Blocked before merge - 5 items remain

Keep this PR open: it addresses a current-main host-rewriting bug, but both prior findings remain and the broader rejection of host-preserving URLs needs an owner decision.

Priority: P2
Reviewed head: a6718c96ba4dce7543bb5064e0fa57d6d6561dc5
Owner decision: Required. See Decision needed.

Decision needed

• Question: Should observability URLs reject these characters everywhere, including a backslash in a query value that does not change the destination host?
• Recommendation: Keep the repair focused on host rewriting: Preserve host-stable URL components and reject spellings that change the written destination.
• Why: The whole-string guard rejects previously accepted inputs such as https://grafana.example.com/d?filter=a\b, while the reported bug concerns destination-host rewriting; no owner approval of that wider restriction appears in this PR.

Before merge

• Add real behavior proof - For head a6718c9, the complete body supplies test results for the renderer and configuration loader, but no after-fix terminal output or logs from the renderer CLI and shipped API/worker exercising these new spellings; provide that output with revision and environment identified, and redact IPs, API keys, and non-public endpoints. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Classify the new refusal in both shipped startup entry points (P2) - With observability.url containing the reported DNS-host tab or backslash, this new exception does not match either startup classifier's observability patterns. The API logs STARTUP_FAILED and the worker logs WORKER_STARTUP_FAILED, suppressing the setting-specific diagnosis and contradicting the new breaking-change notice. Extend both classifiers and the existing API/worker startup integration case to cover this refusal. This prior finding remains unresolved.
• Update the owning configuration reference and runtime flow (P3) - The guide gains the new restriction, but docs/reference/configuration.md still describes the older accepted-input contract and docs/flows/platform-console/observability-link.md omits this new startup failure path. Operators consulting those owners cannot discover the newly refused values or their recovery behavior. Update both existing pages alongside this behavior change, as root AGENTS.md requires. This prior finding remains unresolved.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
• Add data-model compatibility proof - The review found that existing stored data may not work after upgrade. Show that existing data still loads and works with this change.

Findings

• [P2] Classify the new refusal in both shipped startup entry points — apps/controller/src/composition/installation-config.ts:287-290
• [P3] Update the owning configuration reference and runtime flow — docs/guides/deploy/installation-profiles.md:192

Tests

• Missing end-to-end proof: Supply after-fix renderer and shipped API/worker output for the new refusals and correction of existing YAML with unrelated settings intact; a base-fail/head-pass comparison for these added cases is not shown.

Agent review details

How this fits together

The profile renderer produces Installation YAML, startup validates its observability destination, and an Installation-admin-authorized API read supplies the console's external link.

flowchart TB
A[Operator profile] --> B[Profile renderer]
B --> C[Installation YAML]
C --> D[Startup URL validation]
D --> E[Authorized observability API]
E --> F[Console external link]

Loading

Technical review

Best possible solution:

Reject host-changing spellings through the existing validators, preserve approved host-stable inputs, and provide consistent startup diagnostics and recovery guidance.

Do we have a high-confidence way to reproduce the issue?

Current-main source admits parsed DNS destinations and returns normalized url.href without rejecting the reported characters; this read-only review ran no reproduction or tests.

Is this the best way to solve the issue?

Extending the existing renderer and startup validators addresses the right owners, but whole-string rejection exceeds the demonstrated host-rewriting case and startup classification remains incomplete.

Full review comments:

• [P2] Classify the new refusal in both shipped startup entry points — apps/controller/src/composition/installation-config.ts:287-290
  With observability.url containing the reported DNS-host tab or backslash, this new exception does not match either startup classifier's observability patterns. The API logs STARTUP_FAILED and the worker logs WORKER_STARTUP_FAILED, suppressing the setting-specific diagnosis and contradicting the new breaking-change notice. Extend both classifiers and the existing API/worker startup integration case to cover this refusal. This prior finding remains unresolved.
  Confidence: 0.99
• [P3] Update the owning configuration reference and runtime flow — docs/guides/deploy/installation-profiles.md:192
  The guide gains the new restriction, but docs/reference/configuration.md still describes the older accepted-input contract and docs/flows/platform-console/observability-link.md omits this new startup failure path. Operators consulting those owners cannot discover the newly refused values or their recovery behavior. Update both existing pages alongside this behavior change, as root AGENTS.md requires. This prior finding remains unresolved.
  Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 20b99cdc87a2.

Provenance checked

• Trusted external observability destination keeps the original intent (3935a45: Provide Installation administrators a trusted external observability link while keeping destination authentication separate.)
• Written-host validation in profile rendering and startup keeps the original intent (63d93f2: Prevent URL parsing from silently substituting another IPv4 address; later shared-helper refactoring preserved that rule.)
• Breaking-change archive placement keeps the original intent (c742f82: Move older notices into linked archive pages to keep documentation within the repository's length limit.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

• Introduced change and current-main gap: The pinned diff adds character rejection to both existing validators; main still parses the URL and returns its normalized href without this rejection. (apps/controller/src/composition/installation-config.ts:287, a6718c96ba4d)
• Startup classification remains incomplete: The API and worker recognize specific observability error prefixes but omit the new character-refusal sentence, producing STARTUP_FAILED and WORKER_STARTUP_FAILED instead of the documented OBSERVABILITY_URL_INVALID. (apps/controller/src/server.mjs:63, a6718c96ba4d)
• Existing contract and trust boundary: The configuration reference and runtime flow describe an operator-configured external destination, Installation-admin authorization, and independent destination authentication; the value is not persisted as an Installation resource or used as an OTLP target. (docs/flows/platform-console/observability-link.md:41, a6718c96ba4d)
• Proof supplied for this revision: The complete current PR body reports focused test passes and a documentation word-count check; contributor discussion supplies no additional after-fix entry-point output or media. (a6718c96ba4d)
• Related repair and original purpose: feat(console): add admin Observability link and Grafana overview #495 established the trusted administrator link; fix(profiles): refuse observability URLs that parse as another host #2329 added canonical IPv4 rejection and classified startup failures, rather than addressing the DNS-host spellings tracked by Observability URLs with a tab or backslash store a different host #2383. (apps/controller/src/composition/installation-config.ts:284, 63d93f2e39ef)
• Policy and history coverage: The complete root AGENTS.md applies; no nested AGENTS.md or maintainer-note entries were found. Scoped history, current-main blame, and GitHub commit records establish feature contributors; some deeper historical blob searches failed because local promisor objects were unavailable. (AGENTS.md, a6718c96ba4d)

Likely related people:

• Russell Bryant: Raw commit 3935a45 adds apps/controller/src/composition/installation-config.ts:250 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 3935a4545342; files: apps/controller/src/composition/installation-config.ts)
• Free Wortley: Raw commit f3fdbf5 adds apps/controller/src/composition/installation-config.ts:282 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: f3fdbf5f1cf7; files: apps/controller/src/composition/installation-config.ts)

Root-cause cluster

Relationship: same_root_cause
Canonical: #2383
Summary: The linked issue owns DNS-host character rewriting, this PR proposes its repair, and the earlier merged repair covered distinct IPv4 spellings.

Members:

• canonical: Observability URLs with a tab or backslash store a different host #2383 - Its reported inputs and normalization path match the two validators changed here.
• adjacent_distinct: fix(profiles): refuse observability URLs that parse as another host #2329 - Its canonical IPv4 guard does not reject the reported DNS-host spellings.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Labels

Label changes:

No label changes.

Label justifications:

• P2: The reported bug affects an optional operator-configured observability link with limited blast radius.
• merge-risk: 🚨 compatibility: Previously accepted startup YAML can now stop API and worker startup, including URLs whose query backslash does not change their host.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (2 earlier review cycles)

• reviewed 2026-10-14T03:22:12.126Z sha 9c4a3a1 :: needs real behavior proof before merge. :: [P2] Classify the new refusal in both shipped startup entry points | [P3] Update the owning configuration reference and runtime flow
• reviewed 2026-10-14T03:56:18.661Z sha 90595d2 :: needs real behavior proof before merge. :: [P2] Classify the new refusal in both shipped startup entry points | [P3] Update the owning configuration reference and runtime flow

Reviewed October 14, 2026, 12:15 AM ET / 04:15 UTC (Revision 3).

[freeqaz]

Free's clanker here (an AI agent working on Free's behalf).

Thanks. I reviewed head c6a12c08a. The query-backslash carve-out and the OBSERVABILITY_URL_INVALID classifier update in server.mjs/worker.mjs cover what I was going to raise. It also fits with #2362, which added the userinfo refusal and the classifier this extends.

The one blocker is a conflict. #2385 just landed (9031ae3) and added its notice at the same spot in docs/guides/deploy/breaking-changes.md. Could you merge main? Please keep the page under the docs word cap: your archive move should still leave room, but Static Checks has the final say. Once it's green on the merged head, it's ready to land from our side.

Optional: docs/flows/platform-console/observability-link.md lists the startup refusals, so it could mention the tab/newline/backslash rule too.

The touched tests fail on main (configuration-startup 19/20, profile-renderer 66/67) and pass on the merged tree (20/20, 67/67), checked at the earlier head.

[freeqaz]

Free's clanker here (an AI agent working on Free's behalf).

Landed as 8ccb33b. Thanks, Eric. The main merge kept #2385's notice next to yours, and CI was green on the merged head.