Week 7: Creating phases for the tasks

This was the week the project got a plan it could actually execute, and the week a
reviewer found a security bug in a fix I had already convinced myself was correct.

The review

I had opened #7647 at the end
of last week — the small fix ensuring an LTI 1.1 grade is saved locally before being
pushed to the LMS.

On 14 July, anxkhn reviewed it. The first comment was housekeeping: rebase and look
at failing CI. I rebased, tests went green, and I said so.

The second comment is the one worth writing about:

storing the context is fixed, but it is still cleared before valid_request?. for
example, a fake launch could clear a teacher's valid grading session and prevent the
next grade passback. please clear it only after a valid launch too.

Let me unpack what that means, because it is genuinely subtle.

An LTI 1.1 launch stores some grading context in the session — the outcome service URL
the LMS wants scores posted to, and which assignment is being graded. My change made
sure that context is written after the launch signature is verified. Good.

But the clearing of stale context happened in a before_action, which runs before
valid_request? — before we know whether the launch is genuine at all.

So: a teacher launches from Canvas, gets a valid grading session, starts grading.
Meanwhile anyone can POST garbage at /lti/launch with no valid signature. That request
gets rejected — 401, correct — but on its way to being rejected it has already wiped
the teacher's grading context. Their next grade passback silently fails.

An unauthenticated request cannot sign in, cannot read anything, cannot write
anything — and can still degrade a legitimate user's session as a side effect of being
rejected.

I had read that code many times. I had written the fix for the adjacent bug in the same
method. I did not see it, because I was asking "is the write ordered correctly?" and the
bug was in the delete.

What I take from it: when you fix ordering around a security check, audit every
operation on that path, not just the one you came for. The invariant is not "the write
happens after verification." It is "nothing that affects state happens before
verification." I had fixed one instance of a general rule while leaving another in
place.

I also want to record that this is the value of review that no amount of care
substitutes for. Not because the reviewer knew something I did not, but because they
came to the code without my assumption about which line was interesting.

The other review comment

The same day, anxkhn reviewed
#7654, the LtiDeployment
model, and approved it — with a nitpick:

could you change Fixes #7405 to Part of #7405? this PR is the first foundation
step, so merging it should not close the larger issue.

Small, and it corrected a real misunderstanding. I had been writing Fixes #7405 on
every LTI pull request out of habit. GitHub's auto-close keywords mean the first one to
merge would have closed the umbrella issue tracking the entire LTI 1.3 upgrade — with
about 90% of it unbuilt.

Part of #7405 links without closing. I have used that phrasing on every LTI PR since.

Twenty-four pull requests

The bigger piece of the week was planning. On 14 July I wrote out the remaining work as
a sequence of pull requests, and the constraint I set was that no single one should
exceed roughly 200 hand-written lines. Generated files like Gemfile.lock and
db/schema.rb do not count, but get flagged for reviewers.

It came to twenty-four.

The number was startling and also clarifying. What used to be "implement LTI 1.3" became
a dependency-ordered queue, with phases:

• Phase 0 — the credibility work: fix the 1.1 bug that already affects real users.
• Phase 1 — the launch core, five PRs: gems and the deployment model; key manager and public endpoints; the JWT validator alone; OIDC login initiation; the launch itself; then the resource link model.
• Phase 2 — Assignment and Grade Services, so grades flow to the LMS gradebook.
• Phase 3 — Deep Linking, so an instructor can embed specific CircuitVerse content.
• Phase 4 — Names and Roles, so the roster syncs.
• Phase 5 — autograding.

The thing that surprised me is how much the constraint improved the design, not just
the delivery. Forced to make Lti::JwtValidator a standalone pull request, it has to be
a class that takes a token and a deployment and returns a payload — no controller, no
session, no Rails request. That is a better boundary than I would have drawn if I were
free to let it reach into whatever it wanted.

The same happened to the OIDC login. Made to stand alone, it has to hand back a state
that the launch can verify later with no shared memory between them — which is exactly
the signed-state design from last week. The constraint and the correct architecture
pointed the same way.

Two rules I wrote down alongside the plan:

1. If a PR crosses ~200 lines, split the spec edge cases into a follow-up — never thin out security validation to fit the budget.
2. Every PR description names which future PR depends on it, so a maintainer can see why a change with no user-visible behaviour deserves to merge.

That second rule is doing a lot of work. The hardest thing about this approach is that
most of the PRs do nothing on their own. A deployments table nothing reads. A validator
nothing calls. Without the dependency note, each looks like dead code, and "we don't
merge unused code" is a reasonable instinct for a maintainer to have.

Shipping against the plan

Two PRs went out under the new scheme:

#7654 — the LtiDeployment
model: issuer, client ID, deployment ID, and the platform's endpoint URLs, unique per
issuer + client + deployment. 97 lines. Storage only, nothing reads it yet. Approved
the day it was reviewed.

#7659 — the JWKS and tool
configuration endpoints, plus a key manager that owns CircuitVerse's own RSA keypair.
This is what makes CircuitVerse registerable: an LMS admin fetches the tool
configuration, and the platform can fetch our public key to verify anything we sign.
193 lines, behind the flag.

Both were reviewable in a sitting. Which, compared to a 2,000 line POC closed the next
day, is the whole difference.

Next week: waiting, and what I did with it.