Week 10: The Handshake Begins

This week the first leg of the LTI 1.3 handshake shipped as
#7746, a static analyser and I
disagreed about CSRF, and I spent an afternoon cleaning up a database mess I had made
for myself weeks earlier.

OIDC login initiation

The endpoint is /lti/login, and it is the URL that the tool configuration from
#7659 has been advertising as
oidc_initiation_url for three weeks while returning 404.

An LMS calls it with iss, login_hint, and optionally client_id and
lti_deployment_id. CircuitVerse resolves those to a registered deployment, mints a
one-time nonce and a signed state, and redirects the browser to the platform's
authorization endpoint:

302 Found
location: http://canvas.docker/api/lti/authorize_redirect?scope=openid
  &response_type=id_token&response_mode=form_post&prompt=none
  &client_id=...&redirect_uri=.../lti/launch&login_hint=demo-student
  &nonce=ddbf28cc...&state=eyJfcmFpbHMi...--e3a4d84925a8df11c8dc6a...

response_mode=form_post and prompt=none are what make the platform POST the token
straight back rather than showing a login screen. The -- in the state is the HMAC
separator — that is the signed-state design from week 6 finally in production form.

64 lines of controller code, about 150 lines of tests.

Arguing with CodeQL

CI came back with CodeQL failing: CSRF protection weakened or disabled.

It was pointing at this line, which I had edited to add the new action:

skip_before_action :verify_authenticity_token, only: %i[launch oidc_login]

My first reaction was that this is a false positive. Of course there is no CSRF token —
the request originates from Canvas. There is no way for an LMS to hold a token minted by
our forms.

My second reaction, which took longer, was that the scanner was pointing at something
real even if its conclusion did not fit. skip_before_action is a blunt instrument: it
turns the check off for the entire action, for every request shape, forever. What I
actually wanted to say is narrower — this particular kind of request does not need a
token, and everything else still does.

So instead of skipping, I overrode the predicate the check uses:

def verified_request?
  super || (action_name == "oidc_login" && params[:iss].present? && params[:login_hint].present?)
end

Token verification stays fully active for every other action and every other request
shape. Only a well-formed LTI initiation is treated as verified. And the justification
holds up: the action reads no session, writes nothing, and the signed state it returns is
what actually protects the launch that follows.

I checked this was real rather than incidental by removing the override and running the
tests — the cross-site POST spec fails with InvalidAuthenticityToken. Worth doing,
because a test that passes for the wrong reason is worse than no test. The test
environment disables forgery protection by default, so that spec has to switch it back
on explicitly or it proves nothing at all.

CodeQL went green, and the code is genuinely narrower than what I started with. The
scanner was more right than I initially gave it credit for.

What review caught

Three findings on the pull request, all worth having:

Ambiguous deployment matches. My lookup ended with scope.order(:id).first. Since
client_id is optional in an initiation, a platform with several registrations under
one issuer would silently resolve to whichever row happened to have the lowest ID — and
the launch would be sent to a different registration's client ID and authorization
endpoint. Now an ambiguous match is refused with a 404 rather than guessed at.

Non-HTTPS authorization endpoints. The LtiDeployment model only validates that
auth_login_url is present, not that it is a sane URL. Since the redirect uses
allow_other_host: true, a malformed or hostile value in that column goes straight to
the browser. A javascript: URL is now rejected before it can reach redirect_to, and
TLS is required in production while plain http still works locally.

Registered query parameters being discarded. Some platforms register an
authorization endpoint that already carries query parameters. I was replacing the query
string wholesale. The suggested fix merged instead of replaced — and introduced a subtle
bug of its own by merging symbol keys into a hash with string keys, so a platform's own
scope= would have survived alongside ours as a duplicate parameter. String keys
throughout fixed it.

That last one is a good reminder that a suggested patch is a suggestion. Both PRs this
week had a fix applied through the GitHub web UI that did not splice cleanly — one
duplicated the tail of a method, leaving four unreachable statements and a duplicate
rescue clause that failed lint. Convenient, but it still needs reading.

The database mess

Somewhere in this week I tried to run the new endpoint against my development database
and got a NOT NULL violation on a column that does not exist in the merged schema.

The cause was archaeology. My abandoned POC branch from week 2 had a migration creating
lti_deployments with a platform_id column. I ran it against my dev database in June.
The branch was closed; the migration was never rolled back. The merged version of that
table, from a completely different pull request, has no platform_id — so my dev
database had a table that matched no branch anyone was working on.

Tracing it was more interesting than expected. The physical column order gave it away:
platform_public_key sat after the timestamps, which meant it had been added by a
later add_column rather than declared inline — which pinned it to the March POC
migration rather than the June one. Twenty-six migration versions were recorded in my
dev database with no corresponding file on any branch, along with an orphaned
lti_platforms table whose migration no longer exists anywhere.

I cleaned it surgically rather than dropping the database: exported the two Canvas
registrations my demo depends on, dropped the drifted columns and tables, cleared the
stale migration stamps, ran the real migration, and re-imported the registrations
against the new schema.

The lesson is cheap to state and I had to learn it the expensive way: migrations you
run from a branch that never merges do not clean themselves up. A dev database is
long-lived state that accumulates the residue of every experiment.

Where the project stands

Merged: the 1.1 grade passback fix, and the deployment model. Approved and waiting: the
JWKS and tool configuration endpoints, the JWT validator, and now the OIDC login.

That is four of the twenty-four, with the whole of phase 1's protocol layer either
merged or in the queue.

Next is the launch itself — verify the state, validate the token through the validator
from week 9, bind the token's deployment to the one the state was issued for, and sign
the user in on their sub. It is the piece where all the separate bricks finally do
something a user can see: a Canvas click landing a signed-in user inside CircuitVerse.

It is also the one I need to be most careful with, and I have a list of open questions
to settle before writing it — what happens when a platform's privacy settings mean it
sends no email address at all, what happens when that email already belongs to a
CircuitVerse account, and where the nonce gets recorded so a state cannot be replayed
inside its five-minute window.

Ten weeks ago I would have just written it and found out.