Repository navigation
fix(levelcode): sign-in next refuses the /ai/ endpoints in every spelling the router answers - #456
Merged
Merged
Conversation
…lling the router answers From the review of the account-site half (onetime #100, comment 4308831990), which asked for the same exclusion here: safe_next refused /ai/auth/… and /ai/csrf as written, but Rails also answers /ai/csrf.json and /ai/csrf/, and /ai/%63srf named it too. The first segment after /ai/ is read as the router reads a name (lower case, up to the first character that is not a letter, digit or underscore), as written and percent-decoded, and refused when it is one of the "ai" scope's routes: auth, authorize_editor, checkout, billing, signout, csrf (NEXT_SERVER_NAMES). The SPA's isServerName is the same rule; the two validators agree on 40,000 generated values. Specs: eleven more refused spellings, and pages whose name only starts like an endpoint's or has one further down still land.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused validation change matches the stated rule, has regression coverage, and has no identified blocking issues.
No open issues 🎉
What changed in this PR
Hardens post-sign-in next validation so account-site redirects reject alternate spellings of server endpoints.
Changes:
- Checks endpoint names both as written and percent-decoded.
- Adds regression coverage for rejected variants and accepted look-alike page paths.
| File | Description |
|---|---|
| spec/requests/levelcode/web_spec.rb | Tests endpoint variants and valid page paths. |
| app/controllers/levelcode/web_controller.rb | Rejects server endpoint names during redirect validation. |
Review effort: Balanced
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
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.
What
A follow-up to #454 (merged). After a sign-in,
nextmust name a page of the account site. It already refused the server's own/ai/endpoints, but only in their exact spelling. Rails also answers/ai/csrf.jsonand/ai/csrf/, and/ai/%63srf(an encodedc) passed too, so a sign-in could land on the CSRF token's JSON instead of a page.The reviewer of the account-site half (systemu-net/onetime#100, comment 4308831990) asked for the same exclusion here.
The rule
The first segment after
/ai/is read the way the router reads a name: lower case, up to the first character that isn't a letter, digit or underscore (socsrf.jsonandcsrf;xread ascsrf). It is checked both as written and percent-decoded. It is refused if it is one of theaiscope's routes inconfig/routes.rb:authauthorize_editorcheckoutbillingsignoutcsrfA page whose name only starts like one (
/ai/csrfx), or that has an endpoint's name further down the path (/ai/docs/auth), is still accepted.The account site's
isServerNameis the same rule. Both validators were run over the same 40,000 generated values: 4,227 accepted by both, no disagreements.Verified
spec/requests/levelcode/web_spec.rbandsession_cookie_spec.rb: 114 examples, 0 failures. Eleven more refused spellings are in the table, plus a test that a look-alike name still lands on its page.rubocopis clean on the changed files.Note: locally,
spec/requests/api/levelcode/v1/admin_spec.rb:42fails (auth_failuresis 3, expected 1). That is unrelated to this change; it fails the same way ondevelop, before and after #454. The local test database holds two failed-sign-in rows from 21:30:59 UTC that no spec left behind; I checked that the sign-in specs leave no rows. CI starts from a clean database.🤖 Generated with Claude Code