Add trailing slash normalisation in host mode#5564
Open
backspace wants to merge 9 commits into
Open
Conversation
A host routing rule declared as "/pricing" only rendered for the trailing-slash form. The bare form 404'd for generic Accept headers (crawlers, link-unfurlers, curl): serve-index fell through to the module resolver when the path was not itself an indexed card instance, without first consulting the routing map. The trailing-slash form took the directory-index branch and reached the map, so it rendered. - Consult the routing map before falling through to the module resolver, so a bare routed sub-path whose target instance lives elsewhere still serves. - Canonicalize routed paths ahead of conditional-GET handling: redirect any non-canonical form (e.g. "/pricing/") to the realm-mount pathname joined with the rule's declared path via a 308, so bookmarks, shared links and crawlers converge on one URL and relative links in the served HTML resolve against a stable document base. - Add matchHostRoutingRule as the shared, trailing-slash-insensitive matcher, and a published-realm regression test covering both forms plus the redirect for a rule whose instance lives in a subdirectory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A rule authored as "/pricing/" never matched: request paths are compared slash-insensitively (RealmPaths.local strips trailing slashes) while the rule string kept its slash, so only the special-cased "/" root rule ever matched with a trailing slash. Strip trailing slashes from each rule path when building the routing map (preserving "/"), so an authored "/pricing/" behaves identically to "/pricing" for the server match, the canonical redirect, and the client-side routing map. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Preview deploymentsHost Test Results 1 files 1 suites 2h 55m 30s ⏱️ Results for commit ac48cd7. Realm Server Test Results 1 files ±0 1 suites ±0 12m 13s ⏱️ +13s Results for commit 1e8eb6e. ± Comparison against earlier commit ac48cd7. |
The routing rule path editor renders a fixed "/" accessory in front of a monospace input. The accessory kept the default sans font, so the slash read as a separate label rather than the first character of the path. Give the accessory the same monospace family as the input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A rule path is matched with its trailing slash stripped, so "/pricing/" behaves identically to "/pricing". The editor now surfaces that through the existing path advisory rather than silently normalizing the input: it shows the normalized form the route will match. The realm root "/" is exempt. Covered by the shared validateRoutingPath tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
backspace
marked this pull request as ready for review
July 21, 2026 20:31
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d79ce030ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Normalizing a rule path collapses "/pricing" and "/pricing/" to the same route, and the map resolves via .find(), so a second colliding rule's target was silently unreachable — but duplicate detection compared raw trimmed strings and never flagged it, leaving only the generic trailing-slash advisory. Extract the canonical normalization into normalizeRoutingPath and share it across the map builder, the editor's duplicate detection, and the path advisory, so a "/pricing" + "/pricing/" pair is reported as a duplicate route (in normalized form) and the editor's collision banner fires. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The canonical 308 redirect ran before the public-permission gate, so an unauthenticated request to a non-canonical form (e.g. /pricing/) on a non-public realm got a route-specific redirect — disclosing that the private routed path exists. Previously non-public HTML requests reached the generic Boxel shell without their routing configuration being consulted. Move the routing-map lookup and the canonical redirect after the public-permission check, so a non-public realm falls through to the generic shell before any route is revealed. The ETag/304 fast-path stays ahead of the permission check — it discloses nothing about routes — so cached revalidation remains cheap. Adds a regression test asserting a non-public realm emits no route-specific redirect. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
habdelra
reviewed
Jul 22, 2026
habdelra
left a comment
habdelra
left a comment
Contributor
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed with a focus on the request-routing state machine in serve-index.ts — specifically how the new bare-path branch interacts with the permission gate, the ETag fast-path, and the trailing-slash canonicalization — plus the shared normalization contract across the server map builder, the editor validators, and the client resolver.
Bottom line: the trailing-slash normalization and canonical-redirect work is solid and well-tested, but the new */* bare-path gate reintroduces the exact private-route disclosure this PR otherwise took care to close — on a non-public realm it now answers 200 for a real route and 404 for a non-route. That's the one issue I'd resolve before merge; everything else is follow-up or cleanup.
What lands right
- Moving the routing-map lookup + 308 behind
hasPublicPermissions(while keeping the ETag/304 fast-path ahead of it, since a revalidation discloses nothing) is the correct ordering, and the reasoning is captured in the comment at lines 361-367. My one caveat is that the bare-path branch doesn't yet honor that same ordering — see the inline thread. - Centralizing trailing-slash handling in
normalizeRoutingPathand reusing it acrossgetHostRoutingMap,findDuplicateRoutingPaths, andvalidateRoutingPathis the right shape: the editor's collision banner, the path advisory, and the server match now agree by construction. The.find()-makes-the-second-target-unreachable case the earlier bot review raised is genuinely handled by the duplicate detection now. matchHostRoutingRuleguardingRealmPaths.localagainst itsstatus:404throw is the right instinct — I only wish thetext/htmlpath reused it rather than re-implementing the match inline (inline thread).- Choosing 308 (not 302) so the method/body are preserved and the canonical form is treated as permanent is correct for this use.
Recommendations (detail in the inline threads)
- Gate the bare-path serve on public permissions so a non-public realm can't be probed via 200-vs-404. —
serve-index.ts, the!isCardInstanceblock. Should-fix. - Add the negative-space test for bare +
*/*on a non-public realm; the existing disclosure test only coverstext/html+ trailing slash, which never hits the leaky branch. —index-responses-test.ts. - Consolidate the canonicalization —
matchHostRoutingRulereturns a full match (rule +canonicalPathname) that the caller uses only as a boolean while the HTML path recomputes it; unify so the formula and thelocal()guard live once. —serve-index.ts. Follow-up. - Reconcile the "single source of truth" claim with the client's pre-existing
canonicalizeRoutingPathtwin inhost-mode-service.ts. —host-routing-validation.ts. Follow-up.
Adjacent, out of scope
- The
Host Memory Baselinecheck is red while Host and Realm test suites are green; it reads as the standalone baseline job rather than anything in this diff, but worth a glance before merge. getHostRoutingMapnormalizes an empty ('') authored path to/, so a hand-editedrealm.jsonrule with a blank path + a linked instance would become a realm-root rule. This actually aligns the server with the editor (which maps an unset path to/) and the client, so it's arguably a latent fix rather than a bug — noting it only so the empty→root behavior is a conscious choice.
None of these block from my side except at your discretion on #1; submitting as a comment so approve/request-changes stays with a human reviewer.
Generated by Claude Code
The bare-path branch (non-text/html, e.g. */* from crawlers) consulted the routing map before any permission check, so on a non-public realm a real route fell through to the generic 200 shell while a non-route 404'd — letting an unauthenticated caller enumerate configured routes by diffing 200 vs 404. This is the same disclosure class the redirect reorder closed, on a different signal. Require public read before honoring the matched rule, so routed and non-routed bare paths behave identically (both next() → 404) on a non-public realm. Adds the negative-space test: a routed and a non-routed bare path return the same status for a generic Accept header on a non-public realm (the existing text/html + trailing-slash test never exercised this branch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The HTML path re-derived the match and canonical form inline — re-reading the routing map, re-running the find, and recomputing canonicalPathname with the same formula that already lives in matchHostRoutingRule — so the canonicalization had two homes that could drift, and only the helper's copy guarded RealmPaths.local against its in-realm throw. Have the HTML path call matchHostRoutingRule and use its rule + canonicalPathname for the redirect and cardURL rewrite, so the match and canonical formula exist once. The full map is still fetched here for the config meta-tag injection. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`HostModeService` had its own `canonicalizeRoutingPath` twin doing the same job as `normalizeRoutingPath`, so the docstring's "single source of truth" claim wasn't backed by the tree, and the two could drift (they already differed on the unreachable `//` edge). Have `resolveRoutedPath` import and use `normalizeRoutingPath`, delete the twin, and update the docstring to name the client resolver as a consumer — now the claim holds. Map keys arrive server-normalized and `resolveRoutedPath`'s input is always slash-prefixed, so behavior is unchanged for every reachable path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
habdelra
approved these changes
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a host mode route map exists, a request for
/pricing/should be treated as one for/pricing. With this running locally:If a route map has a
/pricing/route, it should act as/pricing; the editor now warns about this. I also updated the leading slash to match the font of the field, because the mismatched/es were irksome.I chose HTTP 308 according to this flow chart.