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 toPart 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: - If a PR crosses ~200 lines, split the spec edge cases into a follow-up - never thin out security validation to fit the budget. - 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. Top comments (0)