Integrate pathplanner - #16
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01WbaJfBRdFvUft16ajR1Zgg
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
Review round 1: PathPlannerPane no longer shows a permanent "loading" spinner for console-only modules where no slug (and thus no iframe) will ever exist — it now renders a calm "not available" empty state instead. SimPaneSwitcher completes the WAI-ARIA tabs pattern started in task 6: tab/tabpanel aria-controls/aria-labelledby wiring, roving tabindex, and Left/Right arrow-key navigation between the two tabs. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
Filed as 039 rather than the plan's 038 since decision 038 (Java tooling compatibility) already landed on main and merged into this branch; the AGENTS.md decisions-active reference was stale at 037 and is corrected to 039 here. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
The PathPlanner deploy dir (src/main/deploy/pathplanner/) is bind-mounted read-write into the student's own container, so a student could plant a symlink there (final component or intermediate directory) to overwrite or delete any file the control-plane user can write, including another student's project or the shared database — a cross-tenant privilege escalation reachable with just a session cookie and curl. PUT now opens the target with O_NOFOLLOW (403 on ELOOP/EEXIST instead of following a planted symlink), and realpath-checks the deepest existing ancestor directory against the real project root both before and after mkdir -p, so mkdir -p itself can never walk through a planted directory symlink onto foreign disk. DELETE lstats the target (symlink semantics, so a planted symlink 403s instead of being followed-and-deleted) and realpath-checks the parent directory the same way. The read path (collectFiles' isFile()/isDirectory() lstat semantics) already skipped planted symlinks and is unchanged. Also, in the same file: bound the snapshot endpoint's aggregate size (25 MiB) and file count (2000), since nothing previously stopped a student from filling the deploy dir and forcing every snapshot read + JSON serialization to scale with it while serving other students concurrently. And split the "skipped oversized file" log from the "file vanished between readdir and stat" case, and catch-wrap the per-file read so an unreadable file is skipped instead of 500ing the whole snapshot. Added regression tests planting real symlinks (file as final PUT component, directory as intermediate PUT component, file as final DELETE component) that assert 403 and, more importantly, that the real file outside the project is unchanged afterward. Added a test seeding files past the aggregate byte cap to confirm the snapshot truncates instead of 5xxing. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
The write-side schema's allow-list ([A-Za-z0-9 ._()-]) rejected ordinary student-chosen filenames the read-side snapshot happily hydrated, e.g. "Bob's Auto.auto", "Score #1.path", "A+B.path", "Left, Right.path", and non-ASCII names — so a student could load a file into PathPlanner and then get a 400 on every save. Switch to a deny-list: total length 1..512, non-empty "/"-split segments (blocks "//" and a leading "/"), no segment may start with "." (the load-bearing traversal guard — blocks "..", ".", and dotfiles), and no segment may contain / \ : * ? " < > | or a control character. Non-ASCII letters are now accepted. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
- workspace-routes.ts: the deploy-files 405 now returns JSON
({ error: "Method not allowed." }) instead of a text/plain body,
matching every other error response in that route block.
- assets.ts: fix the operator-facing 503 string to reference the real
command (`bun run fetch:dist`) instead of the nonexistent
`bun scripts/fetch-dist.ts`.
- Dockerfile: the pathplanner-dist stage's URL was hardcoded to
/releases/latest/download/..., so once a release-less build caches its
"WARNING: ... will 503" layer, every later build reuses it and ships an
empty dist forever — including PathPlanner's debut release. Add
PATHPLANNER_DIST_TAG (default "latest", preserving current behavior)
and compose it into the download URL so pinning a real tag both fixes
the release fetched and busts the cache by changing the RUN command's
text. Documented the hazard in the stage's comment.
- docs/decisions/README.md: add the missing 039-pathplanner-integration.md
bullet and fix the stale "011–037" range to "011–039".
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
The symlink guards added in e46cc9e are path-based, which makes them check-then-use. The write root is bind-mounted read-write into the student's own container, so a student can swap a real directory for a symlink in the window between the realpath check and the open, and the open resolves somewhere else entirely. O_NOFOLLOW only protects the final path component, so an intermediate swap reproduced the original arbitrary overwrite. The window is ~10-100us but the attacker sets the swap rate and the endpoint has no rate limit, so retries make it practical rather than theoretical. Resolve the descriptor we already hold, via /proc/self/fd, and check that instead: it names the inode actually opened, which cannot be re-pointed underneath us. Drop O_TRUNC so a lost race leaves the target intact for that check to catch, and truncate explicitly afterwards. DELETE gets the same open-and-verify before the rm; Node exposes no unlinkat, so its residual is unlinking a leaf the caller already named. Hosts without /proc (macOS dev) keep the path-based checks rather than failing every write closed. Deployments are Linux, where the check is live. Verified the mechanism directly: with a directory symlink in place at open time, the descriptor resolves outside the project root and the victim file is byte-identical afterwards. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019191RPCd5owTF319Hp2si5
A lesson load or team import rewrites the deploy files on disk, but the PathPlanner iframe keeps the tree it hydrated at mount. Left alone its write-through sync queue would push that stale tree back over the newly imported project. Key the pane on `reloadNonce` so it remounts on swap and re-fetches the deploy-files snapshot — the same mechanism EditorPane already uses. Also drops a stale comment claiming the pane isn't mounted anywhere; it has been mounted via SimPaneSwitcher since 50b374d. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01ErsgDbpYrNavTaHeyufUY5
Trimmed both marks to their opaque bounds (the AdvantageScope source carried ~25px of padding plus a soft drop shadow reaching the canvas edge) and resampled both to a matching 64x64 — 4x the 16px render size, and 225KB/120KB down to ~6KB each. The base TabsTrigger already supplies inline-flex and gap-1.5, so the icons only need a fixed size. Empty alt keeps them decorative: the visible text is already each tab's accessible name. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01ErsgDbpYrNavTaHeyufUY5
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
1 participant
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.
Integrate forked & modified pathplanner from https://github.com/mathewdunne/pathplanner-web