feat(levelcode): accept a development editor's callback scheme — off unless LEVELCODE_EXTRA_EDITOR_SCHEMES says so - #440
Merged
Conversation
Spec only. Two controllers decide whether a sign-in may hand its one-time code to a redirect_uri: Levelcode::WebController for the /ai flows, and the API's AuthController for /auth/login. Each keeps its own copy of the rule — scheme on a short list, host and path exact — and almost none of it was under test. Run against the code as it stands, every one of these passed the suite: the pre-rename scheme (atom-plus-plus) dropped web, api another scheme added to the list web, api the host no longer compared web, api the path no longer compared web, api The one refusal the suite did check was an https address, which fails on the scheme before the host or the path is looked at. Thirteen examples, one per address, at both gates: the shipped editor and a pre-rename build get the code with ?windowId kept; a scheme not on the list, the right scheme on another host, the right host on another path, https, and an address with no host get nothing. With them, each of the eight mutations above fails exactly the example that names it. One of them records something worth knowing before the list changes: at /ai/auth/verify an editor-shaped address on a scheme the server does not take is not refused — it is simply not an editor sign-in. The browser is signed in to the web account and the editor that asked hears nothing.
…EditorCallback Levelcode::WebController and the API's AuthController each decided for themselves whether a redirect_uri was the editor's: two lists of schemes, the host and the path written out twice. Nothing kept them in step, and adding a scheme to one would have left a sign-in that works through /ai and not through /auth/login, or the reverse. The rule moves to Levelcode::EditorCallback — the schemes, the extension id as host, the auth route as path — and both controllers ask it. No behaviour changes: the request examples from the previous commit are untouched and pass as they were. What that buys shows in the mutations. Before, loosening the rule in one controller failed that controller's example. Now each of them — the pre-rename scheme dropped, another scheme added, the host or the path not compared — fails the example at BOTH gates, and either controller going back to its own check fails its own. The class gets a spec of its own for what a request cannot easily show: a URI or a string, the scheme's case, a host that merely starts with the extension id, an address with no host, and input that is not an address at all — false, never an exception. WebController's EDITOR_CALLBACK and EDITOR_SCHEMES are gone; nothing else read them.
…o take its scheme
An editor run from source shares the installed app's identity: same bundle id,
same levelcode:// scheme. macOS therefore hands the sign-in callback to the
app in /Applications, and the editor that asked never hears back. The editor is
getting its own scheme for that, levelcode-dev (levelcodeai/levelcode), and a
server has to be willing to hand a code to it.
No server is, unless told:
LEVELCODE_EXTRA_EDITOR_SCHEMES comma list of further schemes to accept
Unset, the rule is exactly what it was: the shipped schemes and nothing else.
That is production. A development server sets it beside LEVELCODE_HOSTS.
What the setting may add is deliberately narrow: levelcode-<variant>, and
nothing else. The one-time code is appended to whatever passes this rule and
the browser is sent there, so a list that could be talked into `https` would
post the code to a web host, and one that took `javascript` would run script on
the account page. Saying what an entry must look like closes both without a
list of schemes to forbid. The host and the path stay exact for every scheme.
An entry that is not usable is dropped and named once in the log. Otherwise a
typo is invisible: its only symptom is the sign-in it was meant to allow ending
on the account page in the browser.
Extra schemes are added to the shipped ones, never in place of them, so the
setting cannot switch off the editor people have installed.
Specs:
- the rule itself: the comma list, what may and may not be added, entries
reported as written, .from_env on a hash, .current built once.
- through both gates, on a server told to take levelcode-dev: the code goes
to the development editor with ?windowId kept, double-encoded as the login
page passes it; the shipped editor still gets its own; another host on the
development scheme gets nothing; https gets nothing even when the setting
names it; no code_challenge still fails closed.
- the default stays pinned by the examples from two commits ago, now run on a
rule built with no setting whatever the machine has exported — a developer
who has the variable in their shell would otherwise see three of them fail.
Checked in a booted app, not only through the stand-in the request specs use:
unset gives the shipped list, levelcode-dev adds itself, and https beside it
is ignored and logged. Ten mutations of the rule each fail an example that
names them. Suite: 1119 examples, 0 failures.
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Target-version runtime verification was unavailable for this authentication-sensitive change, so human approval is recommended.
Review effort: Balanced
Findings: None
What changed in this PR
Adds opt-in development-editor callbacks while preserving default schemes and exact host/path validation.
Changes:
- Shares callback validation across both authentication controllers.
- Adds restricted extra schemes through
LEVELCODE_EXTRA_EDITOR_SCHEMES. - Tests default behavior, opt-in redirects, and rejected addresses.
| File | Description |
|---|---|
| spec/support/auth_helpers.rb | Adds explicit callback-policy setup. |
| spec/services/levelcode/editor_callback_spec.rb | Tests validation and configuration. |
| spec/requests/levelcode/web_spec.rb | Covers web authentication callbacks. |
| spec/requests/api/levelcode/v1/auth_spec.rb | Covers API authentication callbacks. |
| app/services/levelcode/editor_callback.rb | Defines the shared configurable policy. |
| app/controllers/levelcode/web_controller.rb | Delegates callback validation. |
| app/controllers/api/levelcode/v1/auth_controller.rb | Uses the shared callback policy. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Why
A LevelCode editor run from source shares the installed app's identity on macOS: same bundle id, same
levelcode://scheme. So the sign-in callback goes to the app in /Applications, and the editor that asked never hears back. The editor is getting its own scheme for development,levelcode-dev(levelcodeai/levelcode#99). A server has to be willing to hand a code to it.No server is, unless told. Production behaviour does not change.
What
levelcodeand the pre-renameatom-plus-plus.levelcode-<variant>can be added. The one-time code is appended to whatever passes this rule and the browser is sent there. A list that could be talked intohttpswould post the code to a web host, and one that tookjavascriptwould run script on the account page. Entries that are not usable are dropped and named once in the log.Set it on the server a development editor signs in to, beside
LEVELCODE_HOSTS.How it is built
Three commits, each runnable on its own.
Levelcode::EditorCallbackand both controllers ask it. The examples from commit 1 are untouched. Each mutation now fails at both gates, which is the evidence they share one rule.Levelcode::Hosts: a frozen value,.from_envon a hash,.currentbuilt once.Verification
zeitwerk:checkpasses.levelcode-devstill say refused, at both gates.?windowIdkept, double-encoded as the login page passes it. The shipped editor still gets its own. Another host on the dev scheme gets nothing.httpsgets nothing even when the setting names it. A missingcode_challengestill fails closed.levelcode-devadds itself, andhttpsbeside it is ignored and logged.One thing worth knowing
At
/ai/auth/verify, an editor-shaped address on a scheme the server does not take is not refused. It is treated as a web sign-in: the browser lands on the account page and the editor hears nothing. That is what a development editor's sign-in looks like against a server without this setting. It is pinned as it is, and the class comment says so.Not in this PR