From 9142ab8a0571f5be7cb427c490b871b343433297 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:17:31 -0600 Subject: [PATCH 01/27] fix(archive): percent-encode D2L file-body URLs before download MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D2L stores some /content/enforced/ URLs with characters that are not legal in a URI path: literal spaces, a bare '%' that is not the start of an escape ("5% (Vinegar).pdf"), and '#' in filenames ("Quiz #3 …"). These reached downloadFileBody (archive update) and resolveFileURL (download) verbatim, where url.Parse either failed outright ("invalid URL escape") or silently reinterpreted everything after '#' as a URL fragment, so the request hit a truncated path and 404'd. The new internal/urlutil package re-encodes the path portion of a reference (preserving already-valid %XX sequences, keeping query strings intact) before it is resolved against the LMS base URL. Both download paths now route through it. --- cmd/download.go | 16 +---- internal/archive/archive.go | 21 +++---- internal/archive/url_escape_test.go | 72 +++++++++++++++++++++++ internal/urlutil/urlutil.go | 90 +++++++++++++++++++++++++++++ internal/urlutil/urlutil_test.go | 81 ++++++++++++++++++++++++++ 5 files changed, 253 insertions(+), 27 deletions(-) create mode 100644 internal/archive/url_escape_test.go create mode 100644 internal/urlutil/urlutil.go create mode 100644 internal/urlutil/urlutil_test.go diff --git a/cmd/download.go b/cmd/download.go index b17a572..ff8f869 100644 --- a/cmd/download.go +++ b/cmd/download.go @@ -16,6 +16,7 @@ import ( "github.com/aarav/schooltools/internal/httpclient" "github.com/aarav/schooltools/internal/session" "github.com/aarav/schooltools/internal/ua" + "github.com/aarav/schooltools/internal/urlutil" ) var ( @@ -316,21 +317,10 @@ func resolveFileURL(rawURL string) (string, error) { if rawURL == "" { return "", fmt.Errorf("empty url") } - u, err := url.Parse(rawURL) - if err != nil { - return "", err - } - if u.IsAbs() { + if strings.HasPrefix(rawURL, "http://") || strings.HasPrefix(rawURL, "https://") { return rawURL, nil } - if u.Scheme == "" && u.Host == "" && (strings.HasPrefix(rawURL, "/") || strings.HasPrefix(rawURL, "./") || strings.HasPrefix(rawURL, "../")) { - base, perr := url.Parse(ua.D2LBase) - if perr != nil { - return "", perr - } - return base.ResolveReference(u).String(), nil - } - return rawURL, nil + return urlutil.ResolveD2L(ua.D2LBase, rawURL) } func pathExists(p string) (bool, error) { diff --git a/internal/archive/archive.go b/internal/archive/archive.go index efd1370..225b460 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -82,6 +82,7 @@ import ( "github.com/aarav/schooltools/internal/content" "github.com/aarav/schooltools/internal/httpclient" "github.com/aarav/schooltools/internal/ua" + "github.com/aarav/schooltools/internal/urlutil" ) const ( @@ -945,20 +946,12 @@ func downloadFileBody(jar *cookiejar.Jar, rawURL, base string) ([]byte, string, if rawURL == "" { return nil, "", fmt.Errorf("empty URL") } - if !strings.HasPrefix(rawURL, "http://") && !strings.HasPrefix(rawURL, "https://") { - b := base - if b == "" { - b = ua.D2LBase - } - parsed, err := url.Parse(b) - if err != nil { - return nil, "", fmt.Errorf("resolve base: %w", err) - } - rel, err := url.Parse(rawURL) - if err != nil { - return nil, "", fmt.Errorf("parse %s: %w", rawURL, err) - } - rawURL = parsed.ResolveReference(rel).String() + if base == "" { + base = ua.D2LBase + } + rawURL, rerr := urlutil.ResolveD2L(base, rawURL) + if rerr != nil { + return nil, "", fmt.Errorf("resolve %s: %w", rawURL, rerr) } res, err := httpclient.FollowRedirects(rawURL, jar, httpclient.FetchOptions{ Headers: map[string]string{"Accept": "*/*"}, diff --git a/internal/archive/url_escape_test.go b/internal/archive/url_escape_test.go new file mode 100644 index 0000000..d8a5046 --- /dev/null +++ b/internal/archive/url_escape_test.go @@ -0,0 +1,72 @@ +package archive + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/aarav/schooltools/internal/content" +) + +func TestRunDownloadsFileBodyWithUnescapedURL(t *testing.T) { + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("9999"), Name: "Chem", Code: "CHM", IsActive: true}} + rawURL := "/content/enforced/9999-CHM/Acetic Acid, 5% (Vinegar).pdf" + f.tocs["9999"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: []content.TocTopic{ + {TopicId: 9100, Title: "Acetic Acid", LastModifiedDate: "2026-03-01T00:00:00Z", TypeIdentifier: "File", Url: &rawURL}, + }}}} + f.topics[9100] = map[string]any{"TopicId": 9100, "Title": "Acetic Acid", "TypeIdentifier": "File", "Url": rawURL} + + var gotPath string + f.bodyHandler = func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + w.Header().Set("Content-Type", "application/pdf") + _, _ = w.Write([]byte("PDF-vinegar")) + } + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + res, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL, DownloadFiles: true}) + if err != nil { + t.Fatalf("run: %v", err) + } + if res.BodiesFetched != 1 { + t.Fatalf("BodiesFetched=%d, want 1", res.BodiesFetched) + } + wantPath := "/content/enforced/9999-CHM/Acetic Acid, 5% (Vinegar).pdf" + if gotPath != wantPath { + t.Errorf("server saw path %q, want %q (the filename must survive URL escaping)", gotPath, wantPath) + } +} + +func TestRunDownloadsFileBodyWithHashInURL(t *testing.T) { + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("9998"), Name: "Chem", Code: "CHM", IsActive: true}} + rawURL := "/content/enforced/9998-CHM/Quiz #3 Chemistry.pdf" + f.tocs["9998"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: []content.TocTopic{ + {TopicId: 9200, Title: "Quiz 3", LastModifiedDate: "2026-03-01T00:00:00Z", TypeIdentifier: "File", Url: &rawURL}, + }}}} + f.topics[9200] = map[string]any{"TopicId": 9200, "Title": "Quiz 3", "TypeIdentifier": "File", "Url": rawURL} + + var gotPath string + f.bodyHandler = func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + w.Header().Set("Content-Type", "application/pdf") + _, _ = w.Write([]byte("PDF-quiz")) + } + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + res, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL, DownloadFiles: true}) + if err != nil { + t.Fatalf("run: %v", err) + } + if res.BodiesFetched != 1 { + t.Fatalf("BodiesFetched=%d, want 1", res.BodiesFetched) + } + wantPath := "/content/enforced/9998-CHM/Quiz #3 Chemistry.pdf" + if gotPath != wantPath { + t.Errorf("server saw path %q, want %q (a raw # must not truncate the path into a fragment)", gotPath, wantPath) + } +} diff --git a/internal/urlutil/urlutil.go b/internal/urlutil/urlutil.go new file mode 100644 index 0000000..9c0cd2c --- /dev/null +++ b/internal/urlutil/urlutil.go @@ -0,0 +1,90 @@ +// Package urlutil repairs the D2L content URLs that the LMS emits with +// unescaped path characters (literal spaces, '#', stray '%') before they +// are handed to net/http. +package urlutil + +import ( + "fmt" + "net/url" + "strings" +) + +// ResolveD2L resolves a D2L content URL against a base URL. Absolute URLs +// are returned unchanged; relative references are percent-encoded first so +// that url.Parse cannot fail on them and no character is silently +// reinterpreted as a URL fragment. +func ResolveD2L(base, raw string) (string, error) { + if isAbs(raw) { + return raw, nil + } + b, err := url.Parse(base) + if err != nil { + return "", fmt.Errorf("parse base: %w", err) + } + ref, err := url.Parse(CleanPath(raw)) + if err != nil { + return "", fmt.Errorf("parse %s: %w", raw, err) + } + return b.ResolveReference(ref).String(), nil +} + +// CleanPath percent-encodes the path portion of a URL reference, +// preserving escape sequences that are already valid. A query string +// (everything from the first '?') is passed through untouched. +func CleanPath(raw string) string { + path, query := raw, "" + if i := strings.IndexByte(raw, '?'); i >= 0 { + path, query = raw[:i], raw[i:] + } + return encodePath(path) + query +} + +func isAbs(u string) bool { + return strings.HasPrefix(u, "http://") || strings.HasPrefix(u, "https://") +} + +func encodePath(p string) string { + var b strings.Builder + b.Grow(len(p) + 8) + for i := 0; i < len(p); i++ { + c := p[i] + switch { + case c == '%' && i+2 < len(p) && isHex(p[i+1]) && isHex(p[i+2]): + b.WriteString(p[i : i+3]) + i += 2 + case c == '%': + b.WriteString("%25") + case mustEscape(c): + b.WriteString(fmt.Sprintf("%%%02X", c)) + default: + b.WriteByte(c) + } + } + return b.String() +} + +func mustEscape(c byte) bool { + switch { + case c <= 0x20: + return true + case c >= 0x7f: + return true + } + switch c { + case '"', '#', '<', '>', '`', '{', '|', '}', '\\', '^': + return true + } + return false +} + +func isHex(c byte) bool { + switch { + case c >= '0' && c <= '9': + return true + case c >= 'a' && c <= 'f': + return true + case c >= 'A' && c <= 'F': + return true + } + return false +} diff --git a/internal/urlutil/urlutil_test.go b/internal/urlutil/urlutil_test.go new file mode 100644 index 0000000..8661997 --- /dev/null +++ b/internal/urlutil/urlutil_test.go @@ -0,0 +1,81 @@ +package urlutil + +import ( + "net/url" + "testing" +) + +func TestCleanPath(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"already valid", "/content/enforced/x/notes.pdf", "/content/enforced/x/notes.pdf"}, + {"literal spaces", "/c/Acetic Acid, 5% (Vinegar).pdf", "/c/Acetic%20Acid,%205%25%20(Vinegar).pdf"}, + {"hash in filename", "/c/Quiz #3 Chemistry.pdf", "/c/Quiz%20%233%20Chemistry.pdf"}, + {"existing escapes preserved", "/c/KEY%20Practice.pdf", "/c/KEY%20Practice.pdf"}, + {"trailing bare percent", "/c/100%", "/c/100%25"}, + {"bare percent not followed by hex", "/c/50% off.pdf", "/c/50%25%20off.pdf"}, + {"query string preserved", "/c/a b.pdf?download=1&x=2", "/c/a%20b.pdf?download=1&x=2"}, + {"percent then query", "/c/a%2fb.pdf?x=1", "/c/a%2fb.pdf?x=1"}, + {"non-ascii bytes", "/c/café menu.pdf", "/c/caf%C3%A9%20menu.pdf"}, + } + for _, tc := range cases { + if got := CleanPath(tc.in); got != tc.want { + t.Errorf("%s: CleanPath(%q) = %q, want %q", tc.name, tc.in, got, tc.want) + } + } +} + +func TestResolveD2L(t *testing.T) { + base := "https://d2l.cbe.ab.ca" + + got, err := ResolveD2L(base, "/content/enforced/1522695-x/Acetic Acid, 5% (Vinegar).pdf") + if err != nil { + t.Fatalf("ResolveD2L: %v", err) + } + want := "https://d2l.cbe.ab.ca/content/enforced/1522695-x/Acetic%20Acid,%205%25%20(Vinegar).pdf" + if got != want { + t.Errorf("ResolveD2L = %q, want %q", got, want) + } + + u, perr := url.Parse(got) + if perr != nil { + t.Fatalf("resolved url does not parse: %v", perr) + } + if u.Path != "/content/enforced/1522695-x/Acetic Acid, 5% (Vinegar).pdf" { + t.Errorf("decoded path = %q, want the original filename", u.Path) + } + if u.Fragment != "" { + t.Errorf("fragment = %q, want empty", u.Fragment) + } +} + +func TestResolveD2LHashFilename(t *testing.T) { + got, err := ResolveD2L("https://d2l.cbe.ab.ca", "/content/enforced/1527886-x/Quiz #3 Chemistry.pdf") + if err != nil { + t.Fatalf("ResolveD2L: %v", err) + } + want := "https://d2l.cbe.ab.ca/content/enforced/1527886-x/Quiz%20%233%20Chemistry.pdf" + if got != want { + t.Errorf("ResolveD2L = %q, want %q", got, want) + } +} + +func TestResolveD2LAbsolutePassthrough(t *testing.T) { + abs := "https://files.example.com/a%20b.pdf" + got, err := ResolveD2L("https://d2l.cbe.ab.ca", abs) + if err != nil { + t.Fatalf("ResolveD2L: %v", err) + } + if got != abs { + t.Errorf("absolute URL rewritten: %q", got) + } +} + +func TestResolveD2LBaseParseError(t *testing.T) { + if _, err := ResolveD2L("ht tp://bad", "/content/x.pdf"); err == nil { + t.Error("expected error for unparseable base") + } +} From 94ff8bb491e7d5d5820bdc51c556fc1a0f1d1597 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:19:42 -0600 Subject: [PATCH 02/27] fix(archive): skip broken topics and survive per-topic fetch failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single un-fetchable topic aborted the entire archive pass: archiveCourseV2 returned on the first fetchTopicFor error, which meant D2L's stale "broken" TOC entries (still listed, flagged isBroken, 404 on the topics API) permanently blocked every course queued after them — on the live store that was one dead 2022 dropbox link in Marketing stopping all six courses from converging. archiveCourseV2 now: - skips TOC topics D2L flags with IsBroken without an API call, - counts a failed metadata fetch and continues with the next topic (the topic is left unrecorded, so the next pass retries it), - still aborts the whole run on 401/403, reported as a clear "session expired; run 'schooltools login'" error instead of a raw status line. The update summary and --json envelope gain topicsFailed / topicsSkipped counters; --dry-run counts broken topics so the plan matches what a real pass will do. --- cmd/archive.go | 26 +++++-- internal/archive/archive.go | 48 ++++++++++--- internal/archive/archive_test.go | 14 ++-- internal/archive/resilience_test.go | 108 ++++++++++++++++++++++++++++ 4 files changed, 180 insertions(+), 16 deletions(-) create mode 100644 internal/archive/resilience_test.go diff --git a/cmd/archive.go b/cmd/archive.go index 3151e5f..79b2cd3 100644 --- a/cmd/archive.go +++ b/cmd/archive.go @@ -236,6 +236,10 @@ var archiveUpdateCmd = &cobra.Command{ "body is missing on disk, even when its metadata is unchanged. Pass\n" + "--no-bodies to skip body downloads. Pass --dry-run to preview without\n" + "writing.\n\n" + + "Topics D2L flags as broken, and topics whose metadata fetch fails, are\n" + + "skipped and counted instead of aborting the pass — one dead topic can no\n" + + "longer stop the rest of your courses from archiving. A 401/403 response\n" + + "still stops the run with a \"session expired\" error.\n\n" + "Examples:\n" + " schooltools archive update\n" + " schooltools archive update 12345 67890\n" + @@ -309,6 +313,8 @@ func runArchiveUpdate(args []string) error { "topicsChecked": result.TopicsChecked, "topicsFetched": result.TopicsFetched, "topicsStale": result.TopicsStale, + "topicsFailed": result.TopicsFailed, + "topicsSkipped": result.TopicsSkipped, "bodiesNew": result.BodiesFetched, "bodiesReused": result.BodiesReused, "bodiesFailed": result.BodiesFailed, @@ -321,8 +327,15 @@ func runArchiveUpdate(args []string) error { fmt.Printf("Archive root: %s\n", result.Root) fmt.Printf("Scanned %d course%s in %s\n", result.CoursesScanned, plural(result.CoursesScanned), elapsed.Round(time.Millisecond)) - fmt.Printf("Topics: %d checked, %d fetched, %d unchanged\n", - result.TopicsChecked, result.TopicsFetched, result.TopicsStale) + extra := "" + if result.TopicsSkipped > 0 { + extra += fmt.Sprintf(", %d skipped (broken)", result.TopicsSkipped) + } + if result.TopicsFailed > 0 { + extra += fmt.Sprintf(", %d failed", result.TopicsFailed) + } + fmt.Printf("Topics: %d checked, %d fetched, %d unchanged%s\n", + result.TopicsChecked, result.TopicsFetched, result.TopicsStale, extra) if !archiveUpdateNoBodies && result.BodiesFetched+result.BodiesReused+result.BodiesFailed > 0 { fmt.Printf("File bodies: %d new, %d unchanged (same SHA), %d errors\n", result.BodiesFetched, result.BodiesReused, result.BodiesFailed) @@ -367,6 +380,7 @@ func runArchiveUpdateDryRun(args []string, abs string) error { "topicsChecked": result.TopicsChecked, "topicsWouldFetch": result.TopicsWouldFetch, "topicsUnchanged": result.TopicsStale, + "topicsBroken": result.TopicsBroken, "bodiesWouldFetch": result.BodiesWouldFetch, "elapsedSeconds": elapsed.Seconds(), "perCourse": result.PerCourse, @@ -378,8 +392,12 @@ func runArchiveUpdateDryRun(args []string, abs string) error { printUpdateDryRunTable(result) fmt.Printf("\nWould scan %d course%s in %s\n", result.CoursesScanned, plural(result.CoursesScanned), elapsed.Round(time.Millisecond)) - fmt.Printf("Topics: %d checked, %d would fetch, %d unchanged\n", - result.TopicsChecked, result.TopicsWouldFetch, result.TopicsStale) + broken := "" + if result.TopicsBroken > 0 { + broken = fmt.Sprintf(" (%d broken topic%s never fetched)", result.TopicsBroken, plural(result.TopicsBroken)) + } + fmt.Printf("Topics: %d checked, %d would fetch, %d unchanged%s\n", + result.TopicsChecked, result.TopicsWouldFetch, result.TopicsStale, broken) if result.BodiesWouldFetch > 0 { fmt.Printf("Bodies: %d would fill (convergence)\n", result.BodiesWouldFetch) } diff --git a/internal/archive/archive.go b/internal/archive/archive.go index 225b460..c49e025 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -68,6 +68,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "net/http/cookiejar" @@ -85,6 +86,8 @@ import ( "github.com/aarav/schooltools/internal/urlutil" ) +var errSessionExpired = errors.New("session expired; run 'schooltools login'") + const ( // SchemaVersion is bumped when the on-disk layout changes in a way that // would confuse older readers. v3 added bodies (blobs/.bin) and @@ -220,9 +223,11 @@ type Result struct { TopicsChecked int TopicsFetched int TopicsStale int - BodiesFetched int // File topics whose body blob was newly saved (SHA changed or first-ever) - BodiesReused int // File topics whose body matched the saved hash — copy skipped - BodiesFailed int // File topics whose body download errored (metadata still saved) + TopicsFailed int + TopicsSkipped int + BodiesFetched int + BodiesReused int + BodiesFailed int } // DiffResult is the structured output of a Diff pass — what would a Run @@ -233,11 +238,11 @@ type DiffResult struct { TopicsChecked int TopicsWouldFetch int TopicsStale int + TopicsBroken int BodiesWouldFetch int PerCourse []CourseDiff } -// CourseDiff summarises a single course's planned work. type CourseDiff struct { CourseID string CourseName string @@ -246,6 +251,7 @@ type CourseDiff struct { TopicsNew int TopicsModified int TopicsUnchanged int + TopicsBroken int BodiesWouldFetch int WouldFetchIDs []int } @@ -577,6 +583,8 @@ func Run(opts Options) (Result, error) { result.TopicsChecked += stale.checked result.TopicsFetched += stale.fetched result.TopicsStale += stale.stale + result.TopicsFailed += stale.failed + result.TopicsSkipped += stale.skipped result.BodiesFetched += stale.bodiesNew result.BodiesReused += stale.bodiesReused result.BodiesFailed += stale.bodiesErr @@ -598,8 +606,9 @@ func Run(opts Options) (Result, error) { }) if opts.Verbose { - fmt.Fprintf(os.Stderr, "[archive] %s (%s): %d topics, %d fetched, %d stale, +%d bodies, %d reused, %d errors\n", + fmt.Fprintf(os.Stderr, "[archive] %s (%s): %d topics, %d fetched, %d stale, %d failed, %d broken, +%d bodies, %d reused, %d errors\n", c.OrgUnitId, c.Code, stale.checked, stale.fetched, stale.stale, + stale.failed, stale.skipped, stale.bodiesNew, stale.bodiesReused, stale.bodiesErr) } } @@ -677,6 +686,7 @@ func Diff(opts Options) (DiffResult, error) { result.TopicsChecked += cd.TopicsTotal result.TopicsWouldFetch += cd.TopicsNew + cd.TopicsModified result.TopicsStale += cd.TopicsUnchanged + result.TopicsBroken += cd.TopicsBroken result.BodiesWouldFetch += cd.BodiesWouldFetch result.PerCourse = append(result.PerCourse, cd) @@ -719,6 +729,10 @@ func diffCourse(opts Options, c Course) (CourseDiff, error) { } for _, t := range flattenTopics(toc.Modules) { cd.TopicsTotal++ + if t.IsBroken { + cd.TopicsBroken++ + continue + } newMod := parseD2LTime(t.LastModifiedDate) ti, existed := ci.Topics[t.TopicId] isFile := topicType(t) == "File" && t.Url != nil && *t.Url != "" @@ -757,8 +771,9 @@ func diffCourse(opts Options, c Course) (CourseDiff, error) { // when DownloadFiles is true — otherwise all three stay zero and the // summary line simply omits the bodies section. type courseStats struct { - stale, checked, fetched int - bodiesNew, bodiesReused, bodiesErr int + stale, checked, fetched int + bodiesNew, bodiesReused, bodiesErr int + failed, skipped int } // archiveCourse runs a single course's archive pass and returns the @@ -794,6 +809,10 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, var work []queued for _, t := range flattenTopics(toc.Modules) { s.checked++ + if t.IsBroken { + s.skipped++ + continue + } newMod := parseD2LTime(t.LastModifiedDate) ti, existed := ci.Topics[t.TopicId] isFile := topicType(t) == "File" && t.Url != nil && *t.Url != "" @@ -824,7 +843,14 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, }) body, ferr := fetchTopicFor(opts.baseURL(), opts.Jar, c.OrgUnitId, q.topicID) if ferr != nil { - return s, fmt.Errorf("fetch topic %d: %w", q.topicID, ferr) + if errors.Is(ferr, errSessionExpired) { + return s, ferr + } + s.failed++ + if opts.Verbose { + fmt.Fprintf(os.Stderr, "[archive] topic %d: fetch failed: %v (continuing)\n", q.topicID, ferr) + } + continue } uuid, size, werr := SaveBlob(opts.Root, body) if werr != nil { @@ -1017,6 +1043,9 @@ func fetchTocFor(base, orgUnitID string, jar *cookiejar.Jar) (content.TocRespons return content.TocResponse{}, err } if res.StatusCode != 200 { + if res.StatusCode == 401 || res.StatusCode == 403 { + return content.TocResponse{}, errSessionExpired + } return content.TocResponse{}, fmt.Errorf("D2L API returned %d %s for %s", res.StatusCode, res.Status, res.Request.URL) } var toc content.TocResponse @@ -1036,6 +1065,9 @@ func fetchTopicFor(base string, jar *cookiejar.Jar, orgUnitID string, topicID in return nil, err } if res.StatusCode != 200 { + if res.StatusCode == 401 || res.StatusCode == 403 { + return nil, errSessionExpired + } return nil, fmt.Errorf("D2L API returned %d %s for topic %d", res.StatusCode, res.Status, topicID) } body, err := httpclient.BodyBytes(res) diff --git a/internal/archive/archive_test.go b/internal/archive/archive_test.go index e7cc2d0..6396d55 100644 --- a/internal/archive/archive_test.go +++ b/internal/archive/archive_test.go @@ -428,6 +428,7 @@ type fakeD2L struct { courses []d2lCourse tocs map[string]content.TocResponse topics map[int]map[string]any + topicStatus map[int]int tocCalls int topicCalls map[int]int bodyHandler http.HandlerFunc // optional; serves file bodies on arbitrary paths @@ -451,10 +452,11 @@ func newFakeD2L(t *testing.T) *fakeD2L { }) d2lver.Reset() return &fakeD2L{ - t: t, - tocs: map[string]content.TocResponse{}, - topics: map[int]map[string]any{}, - topicCalls: map[int]int{}, + t: t, + tocs: map[string]content.TocResponse{}, + topics: map[int]map[string]any{}, + topicStatus: map[int]int{}, + topicCalls: map[int]int{}, } } @@ -488,6 +490,10 @@ func (f *fakeD2L) handler() http.Handler { } id := atoi(parts[7]) f.topicCalls[id]++ + if code, ok := f.topicStatus[id]; ok { + w.WriteHeader(code) + return + } topic, ok := f.topics[id] if !ok { http.NotFound(w, r) diff --git a/internal/archive/resilience_test.go b/internal/archive/resilience_test.go new file mode 100644 index 0000000..428dfe6 --- /dev/null +++ b/internal/archive/resilience_test.go @@ -0,0 +1,108 @@ +package archive + +import ( + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/aarav/schooltools/internal/content" +) + +func TestRunSurvivesBrokenAndFailingTopics(t *testing.T) { + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("4242"), Name: "Marketing", Code: "MKT", IsActive: true}} + goodURL := "/content/enforced/4242-MKT/ok.pdf" + f.tocs["4242"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: []content.TocTopic{ + {TopicId: 6001, Title: "Fine", LastModifiedDate: "2026-03-01T00:00:00Z", TypeIdentifier: "File", Url: &goodURL}, + {TopicId: 6002, Title: "Dead dropbox link", LastModifiedDate: "2022-03-15T16:25:36Z", IsBroken: true}, + {TopicId: 6003, Title: "Vanished", LastModifiedDate: "2026-03-02T00:00:00Z"}, + }}}} + f.topics[6001] = map[string]any{"TopicId": 6001, "Title": "Fine", "TypeIdentifier": "File", "Url": goodURL} + // 6002 is flagged broken and 6003 is never registered, so the topics API 404s it. + + srv := httptest.NewServer(f.handler()) + defer srv.Close() + dir := t.TempDir() + + res, err := Run(Options{Root: dir, Jar: newJar(t), BaseURL: srv.URL, DownloadFiles: false}) + if err != nil { + t.Fatalf("run should not abort on a broken/failing topic: %v", err) + } + if res.TopicsChecked != 3 { + t.Errorf("TopicsChecked=%d, want 3", res.TopicsChecked) + } + if res.TopicsFetched != 1 { + t.Errorf("TopicsFetched=%d, want 1 (only the healthy topic)", res.TopicsFetched) + } + if res.TopicsSkipped != 1 { + t.Errorf("TopicsSkipped=%d, want 1 (IsBroken topic)", res.TopicsSkipped) + } + if res.TopicsFailed != 1 { + t.Errorf("TopicsFailed=%d, want 1 (404 topic)", res.TopicsFailed) + } + + ci, err := LoadCourseIndex(dir, "4242") + if err != nil { + t.Fatalf("LoadCourseIndex: %v", err) + } + if ci.Topics[6001] == nil || ci.Topics[6001].Current == "" { + t.Error("healthy topic was not archived") + } + if ci.Topics[6002] != nil || ci.Topics[6003] != nil { + t.Error("broken/failing topics must not be recorded in the index") + } + + res2, err := Run(Options{Root: dir, Jar: newJar(t), BaseURL: srv.URL, DownloadFiles: false}) + if err != nil { + t.Fatalf("second run: %v", err) + } + if res2.TopicsFetched != 0 || res2.TopicsSkipped != 1 || res2.TopicsFailed != 1 { + t.Errorf("second run: fetched=%d skipped=%d failed=%d, want 0/1/1", res2.TopicsFetched, res2.TopicsSkipped, res2.TopicsFailed) + } +} + +func TestRunAbortsOnSessionExpired(t *testing.T) { + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("555"), Name: "Bio", Code: "BIO", IsActive: true}} + f.tocs["555"] = content.TocResponse{Modules: []content.TocModule{{ + ModuleId: 1, Topics: []content.TocTopic{{TopicId: 77, Title: "Cells", LastModifiedDate: "2026-03-01T00:00:00Z"}}, + }}} + f.topicStatus[77] = http.StatusUnauthorized + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + _, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL}) + if err == nil { + t.Fatal("expected the run to abort on 401") + } + if !errors.Is(err, errSessionExpired) { + t.Errorf("err = %v, want errSessionExpired", err) + } +} + +func TestDiffSkipsBrokenTopics(t *testing.T) { + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("777"), Name: "Hist", Code: "HIS", IsActive: true}} + f.tocs["777"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: []content.TocTopic{ + {TopicId: 11, Title: "New topic", LastModifiedDate: "2026-03-01T00:00:00Z"}, + {TopicId: 12, Title: "Broken topic", LastModifiedDate: "2022-01-01T00:00:00Z", IsBroken: true}, + }}}} + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + res, err := Diff(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL}) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if res.TopicsChecked != 2 { + t.Errorf("TopicsChecked=%d, want 2", res.TopicsChecked) + } + if res.TopicsWouldFetch != 1 { + t.Errorf("TopicsWouldFetch=%d, want 1 (broken topic excluded)", res.TopicsWouldFetch) + } + if res.TopicsBroken != 1 { + t.Errorf("TopicsBroken=%d, want 1", res.TopicsBroken) + } +} From 00fd5a3917edd1e30a086d19685a14ba96644845 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:20:42 -0600 Subject: [PATCH 03/27] fix(archive): report bodies missing from disk, not just missing pointers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Status and search trusted the index's currentBody pointer instead of the filesystem. On the live store that surfaced as "Bodies: 326 present, 8 missing" while 186 referenced blobs were actually gone from disk — the old prune bug's damage was invisible until archive update --dry-run reported 201 bodies to fill. LoadStatus now stats each blob, Search gains a WithBodiesOnDisk filter (used by find --with-bodies) and --missing-bodies now also matches topics whose pointer exists but whose blob does not. HasBody on SearchHit and FileTopic reflects on-disk state. Export keeps the old pointer semantics so its "skipped-missing-body" reporting is unchanged. --- cmd/archive.go | 20 ++--- internal/archive/archive.go | 2 +- internal/archive/body_state_test.go | 113 ++++++++++++++++++++++++++++ internal/archive/store.go | 28 ++++--- 4 files changed, 142 insertions(+), 21 deletions(-) create mode 100644 internal/archive/body_state_test.go diff --git a/cmd/archive.go b/cmd/archive.go index 79b2cd3..29ba559 100644 --- a/cmd/archive.go +++ b/cmd/archive.go @@ -102,8 +102,8 @@ func init() { archiveFindCmd.Flags().StringVar(&archiveFindExt, "ext", "", "Filter by URL extension (e.g. pdf, docx)") archiveFindCmd.Flags().StringVar(&archiveFindType, "type", "", "Filter by topic type (File, Link)") - archiveFindCmd.Flags().BoolVar(&archiveFindWith, "with-bodies", false, "Only show topics that have a file body archived") - archiveFindCmd.Flags().BoolVar(&archiveFindMiss, "missing-bodies", false, "Only show File topics whose body has never been archived") + archiveFindCmd.Flags().BoolVar(&archiveFindWith, "with-bodies", false, "Only show topics whose file body exists on disk") + archiveFindCmd.Flags().BoolVar(&archiveFindMiss, "missing-bodies", false, "Only show File topics whose file body is missing on disk") archiveShowCmd.Flags().StringVar(&archiveShowKind, "kind", "", "Force the ref kind: metadata|body (default: auto-detect)") @@ -614,8 +614,8 @@ var archiveFindCmd = &cobra.Command{ "type contains all whitespace-separated query tokens (case-insensitive).\n" + "All tokens must match (AND). Use --ext to filter by file extension,\n" + "--type File|Link to filter by topic type, --with-bodies for topics\n" + - "with an archived file body, --missing-bodies for File topics whose\n" + - "body has never been archived.\n\n" + + "whose file body exists on disk, --missing-bodies for File topics\n" + + "whose file body is missing on disk.\n\n" + "Examples:\n" + " schooltools archive find safety contract\n" + " schooltools archive find --type File --ext pdf\n" + @@ -632,12 +632,12 @@ func runArchiveFind(args []string) error { return err } q := archive.SearchQuery{ - Terms: strings.Join(args, " "), - Ext: archiveFindExt, - CourseIDs: archiveCourses, - Type: archiveFindType, - WithBodies: archiveFindWith, - MissingBodies: archiveFindMiss, + Terms: strings.Join(args, " "), + Ext: archiveFindExt, + CourseIDs: archiveCourses, + Type: archiveFindType, + WithBodiesOnDisk: archiveFindWith, + MissingBodies: archiveFindMiss, } hits := store.Search(q) diff --git a/internal/archive/archive.go b/internal/archive/archive.go index c49e025..d879de5 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -1180,7 +1180,7 @@ func FileTopics(root, ext string) ([]FileTopic, error) { Type: t.Type, LastModified: t.LastModified, CurrentBody: t.CurrentBody, - HasBody: t.CurrentBody != "", + HasBody: t.CurrentBody != "" && fileExists(BodyPath(root, t.CurrentBody)), }) } } diff --git a/internal/archive/body_state_test.go b/internal/archive/body_state_test.go new file mode 100644 index 0000000..4a3e73e --- /dev/null +++ b/internal/archive/body_state_test.go @@ -0,0 +1,113 @@ +package archive + +import ( + "crypto/sha256" + "encoding/hex" + "testing" + "time" +) + +func TestLoadStatusCountsBodiesMissingFromDisk(t *testing.T) { + dir := t.TempDir() + _, _, _ = SaveFileBody(dir, []byte("body")) + presentHash := sha256.Sum256([]byte("body")) + presentHex := hex.EncodeToString(presentHash[:]) + missingHash := sha256.Sum256([]byte("gone")) + missingHex := hex.EncodeToString(missingHash[:]) + + ci := &CourseIndex{CourseID: "1", Code: "SAF", Topics: map[int]*TopicIndex{ + 1: {TopicID: 1, Title: "on disk", Type: "File", URL: "/x.pdf", CurrentBody: presentHex, LastModified: time.Now().UTC()}, + 2: {TopicID: 2, Title: "pointer only", Type: "File", URL: "/y.pdf", CurrentBody: missingHex, LastModified: time.Now().UTC()}, + 3: {TopicID: 3, Title: "no pointer", Type: "File", URL: "/z.pdf", CurrentBody: "", LastModified: time.Now().UTC()}, + }} + if err := SaveCourseIndex(dir, "1", ci); err != nil { + t.Fatalf("SaveCourseIndex: %v", err) + } + + st, err := LoadStatus(dir) + if err != nil { + t.Fatalf("LoadStatus: %v", err) + } + if st.FileTopics != 3 { + t.Errorf("FileTopics=%d, want 3", st.FileTopics) + } + if st.BodiesPresent != 1 { + t.Errorf("BodiesPresent=%d, want 1 (only the blob that exists on disk)", st.BodiesPresent) + } + if st.MissingBodies != 2 { + t.Errorf("MissingBodies=%d, want 2 (pointer-without-blob plus empty pointer)", st.MissingBodies) + } +} + +func TestSearchBodyFiltersCheckDisk(t *testing.T) { + dir := t.TempDir() + _, _, _ = SaveFileBody(dir, []byte("body")) + presentHash := sha256.Sum256([]byte("body")) + presentHex := hex.EncodeToString(presentHash[:]) + missingHash := sha256.Sum256([]byte("gone")) + missingHex := hex.EncodeToString(missingHash[:]) + + ci := &CourseIndex{CourseID: "1", Code: "SAF", Topics: map[int]*TopicIndex{ + 1: {TopicID: 1, Title: "present", Type: "File", URL: "/a.pdf", CurrentBody: presentHex, LastModified: time.Now().UTC()}, + 2: {TopicID: 2, Title: "dangling", Type: "File", URL: "/b.pdf", CurrentBody: missingHex, LastModified: time.Now().UTC()}, + 3: {TopicID: 3, Title: "empty", Type: "File", URL: "/c.pdf", CurrentBody: "", LastModified: time.Now().UTC()}, + }} + if err := SaveCourseIndex(dir, "1", ci); err != nil { + t.Fatalf("SaveCourseIndex: %v", err) + } + + store, err := OpenStore(dir) + if err != nil { + t.Fatalf("OpenStore: %v", err) + } + + hits := store.Search(SearchQuery{WithBodiesOnDisk: true}) + if len(hits) != 1 || hits[0].TopicID != 1 { + t.Errorf("WithBodiesOnDisk: got %+v, want only topic 1", hits) + } + + hits = store.Search(SearchQuery{WithBodies: true}) + if len(hits) != 2 { + t.Errorf("WithBodies (pointer semantics): got %d hits, want 2 (pointer set)", len(hits)) + } + + hits = store.Search(SearchQuery{MissingBodies: true}) + if len(hits) != 2 || hits[0].TopicID != 2 || hits[1].TopicID != 3 { + t.Errorf("MissingBodies: got %+v, want topics 2 and 3 (dangling pointer counts as missing)", hits) + } + + if !store.Search(SearchQuery{Terms: "present"})[0].HasBody { + t.Error("HasBody should be true when the blob is on disk") + } + if store.Search(SearchQuery{Terms: "dangling"})[0].HasBody { + t.Error("HasBody should be false when the blob is missing from disk") + } +} + +func TestFileTopicsHasBodyChecksDisk(t *testing.T) { + dir := t.TempDir() + _, _, _ = SaveFileBody(dir, []byte("body")) + presentHash := sha256.Sum256([]byte("body")) + presentHex := hex.EncodeToString(presentHash[:]) + missingHash := sha256.Sum256([]byte("gone")) + missingHex := hex.EncodeToString(missingHash[:]) + + ci := &CourseIndex{CourseID: "1", Code: "SAF", Topics: map[int]*TopicIndex{ + 1: {TopicID: 1, Title: "present", Type: "File", URL: "/a.pdf", CurrentBody: presentHex, LastModified: time.Now().UTC()}, + 2: {TopicID: 2, Title: "dangling", Type: "File", URL: "/b.pdf", CurrentBody: missingHex, LastModified: time.Now().UTC()}, + }} + if err := SaveCourseIndex(dir, "1", ci); err != nil { + t.Fatalf("SaveCourseIndex: %v", err) + } + + topics, err := FileTopics(dir, "") + if err != nil { + t.Fatalf("FileTopics: %v", err) + } + for _, ft := range topics { + want := ft.TopicID == 1 + if ft.HasBody != want { + t.Errorf("topic %d: HasBody=%v, want %v", ft.TopicID, ft.HasBody, want) + } + } +} diff --git a/internal/archive/store.go b/internal/archive/store.go index 4ef577d..5971e48 100644 --- a/internal/archive/store.go +++ b/internal/archive/store.go @@ -26,13 +26,14 @@ type Resolved struct { } type SearchQuery struct { - Terms string - Ext string - CourseIDs []string - Type string - WithBodies bool - MissingBodies bool - Limit int + Terms string + Ext string + CourseIDs []string + Type string + WithBodies bool + WithBodiesOnDisk bool + MissingBodies bool + Limit int } type SearchHit struct { @@ -276,9 +277,12 @@ func (s *Store) Search(q SearchQuery) []SearchHit { if q.WithBodies && t.CurrentBody == "" { continue } + if q.WithBodiesOnDisk && !s.bodyOnDisk(t) { + continue + } if q.MissingBodies { isFileCompatible := t.Type == "" || t.Type == "File" - if !isFileCompatible || t.CurrentBody != "" { + if !isFileCompatible || s.bodyOnDisk(t) { continue } } @@ -296,7 +300,7 @@ func (s *Store) Search(q SearchQuery) []SearchHit { LastModified: t.LastModified, Current: t.Current, CurrentBody: t.CurrentBody, - HasBody: t.CurrentBody != "", + HasBody: s.bodyOnDisk(t), }) } sort.Slice(out, func(i, j int) bool { @@ -311,6 +315,10 @@ func (s *Store) Search(q SearchQuery) []SearchHit { return out } +func (s *Store) bodyOnDisk(t *TopicIndex) bool { + return t != nil && t.CurrentBody != "" && fileExists(BodyPath(s.Root, t.CurrentBody)) +} + func (te *topicEntry) hayMatches(tokens []string) bool { for _, tok := range tokens { if !strings.Contains(te.hay, tok) { @@ -432,7 +440,7 @@ func LoadStatus(root string) (Status, error) { if t.Type == "" || t.Type == "File" { st.FileTopics++ } - if t.CurrentBody != "" { + if t.CurrentBody != "" && fileExists(BodyPath(root, t.CurrentBody)) { st.BodiesPresent++ } } From 8a570807cbe4b53b5412cd62ae85e71322d6c7c1 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:22:37 -0600 Subject: [PATCH 04/27] fix(login): gate HTTP debug dumps behind --verbose, redact SAML and cookies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fetchPost unconditionally printed request headers and raw Set-Cookie values for every form POST, even with Verbose:false — and every auto-refresh path (cmd/whoami.go loginFn) hard-codes Verbose:false. A single `archive update` against an expired session therefore leaked the ADFS SAMLRequest URL and the d2lSessionVal/d2lSecureSessionVal cookie values to stderr, violating the SECURITY.md threat model. fetchPost now takes Options and only dumps when verbose, matching fetchGet. Additionally, even in verbose mode SAML parameters (SAMLRequest/SAMLResponse/RelayState) are replaced with a length marker and cookie values are truncated, mirroring the existing per-hop dump. --- internal/login/login.go | 38 +++++++++++---- internal/login/login_test.go | 93 ++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 8 deletions(-) create mode 100644 internal/login/login_test.go diff --git a/internal/login/login.go b/internal/login/login.go index 0c552fe..20acf94 100644 --- a/internal/login/login.go +++ b/internal/login/login.go @@ -139,7 +139,7 @@ func Run(opts Options) (Result, error) { "AuthMethod": {"FormsAuthentication"}, }.Encode() - res, err := fetchPost(loginPostURL, body, adfsURL, jar) + res, err := fetchPost(loginPostURL, body, adfsURL, jar, opts) if err != nil { return Result{}, err } @@ -285,7 +285,7 @@ stage = newStage hopsInStage = 0 } - res, err = fetchPost(postURL, postBody.Encode(), res.Request.URL.String(), jar) + res, err = fetchPost(postURL, postBody.Encode(), res.Request.URL.String(), jar, opts) if err != nil { return Result{}, err } @@ -522,14 +522,14 @@ func fetchGet(rawURL string, jar *cookiejar.Jar, referer string, opts Options) ( return out, err } -func fetchPost(rawURL, body, referer string, jar *cookiejar.Jar) (*http.Response, error) { +func fetchPost(rawURL, body, referer string, jar *cookiejar.Jar, opts Options) (*http.Response, error) { res, err := httpclient.Fetch(rawURL, jar, httpclient.FetchOptions{ Method: http.MethodPost, Headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"}, Body: body, Referer: referer, }) - if res != nil && res.Request != nil { + if res != nil && res.Request != nil && opts.Verbose { dumpReqHeaders(res.Request) dumpRespSetCookies(res) } @@ -538,21 +538,23 @@ func fetchPost(rawURL, body, referer string, jar *cookiejar.Jar) (*http.Response // dumpRespSetCookies prints raw Set-Cookie header values verbatim — useful // for confirming whether the server actually sent Expires / Max-Age that our -// cookie parser might be losing. +// cookie parser might be losing. Values are truncated like the per-hop dump. func dumpRespSetCookies(res *http.Response) { for _, sc := range res.Header.Values("Set-Cookie") { - fmt.Fprintf(os.Stderr, " << Set-Cookie: %s\n", sc) + name, value, _ := strings.Cut(sc, "=") + fmt.Fprintf(os.Stderr, " << Set-Cookie: %s=%s\n", name, truncateForLog(value)) } } // dumpReqHeaders prints the request headers in a compact form so verbose // mode shows what we actually sent to the server. Body is intentionally -// omitted from the dump. +// omitted from the dump, and SAML parameters are redacted from the URL +// because the assertion is a bearer credential. func dumpReqHeaders(r *http.Request) { if r == nil { return } - fmt.Fprintf(os.Stderr, " > %s %s\n", r.Method, r.URL.String()) + fmt.Fprintf(os.Stderr, " > %s %s\n", r.Method, redactSAML(r.URL.String())) keys := []string{"User-Agent", "Accept", "Accept-Language", "Accept-Encoding", "Referer", "Origin", "Content-Type", "Cache-Control", "sec-ch-ua", "sec-ch-ua-mobile", "sec-ch-ua-platform", @@ -565,6 +567,26 @@ func dumpReqHeaders(r *http.Request) { } } +func redactSAML(rawURL string) string { + u, err := url.Parse(rawURL) + if err != nil { + return "" + } + q := u.Query() + redacted := false + for _, k := range []string{"SAMLRequest", "SAMLResponse", "RelayState"} { + if v := q.Get(k); v != "" { + q.Set(k, fmt.Sprintf("", len(v))) + redacted = true + } + } + if !redacted { + return rawURL + } + u.RawQuery = q.Encode() + return u.String() +} + func isMFA(err error) bool { _, ok := err.(*stderrors.MfaRequiredError) return ok diff --git a/internal/login/login_test.go b/internal/login/login_test.go new file mode 100644 index 0000000..a59df25 --- /dev/null +++ b/internal/login/login_test.go @@ -0,0 +1,93 @@ +package login + +import ( + "io" + "net/http" + "net/http/httptest" + "net/url" + "os" + "strings" + "testing" + + "github.com/aarav/schooltools/internal/httpclient" +) + +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + old := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + os.Stderr = w + fn() + _ = w.Close() + os.Stderr = old + data, _ := io.ReadAll(r) + return string(data) +} + +func TestFetchPostDumpsOnlyWhenVerbose(t *testing.T) { + secret := "supersecretvalue-but-very-long-cookie-tail" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.SetCookie(w, &http.Cookie{Name: "d2lSessionVal", Value: secret, Path: "/"}) + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + jar, err := httpclient.NewJar() + if err != nil { + t.Fatalf("NewJar: %v", err) + } + + out := captureStderr(t, func() { + _, _ = fetchPost(srv.URL, "UserName=x", "", jar, Options{}) + }) + if out != "" { + t.Errorf("fetchPost without --verbose wrote to stderr: %q", out) + } + + out = captureStderr(t, func() { + _, _ = fetchPost(srv.URL, "UserName=x", "", jar, Options{Verbose: true}) + }) + if !strings.Contains(out, "Set-Cookie") { + t.Errorf("verbose fetchPost did not dump Set-Cookie: %q", out) + } + if strings.Contains(out, secret) { + t.Errorf("verbose fetchPost leaked the full cookie value: %q", out) + } +} + +func TestDumpReqHeadersRedactsSAML(t *testing.T) { + assertion := `secret-assertion` + target := "https://adfsfba.cbe.ab.ca/adfs/ls/?SAMLRequest=" + url.QueryEscape(assertion) + req := httptest.NewRequest(http.MethodPost, target, nil) + + out := captureStderr(t, func() { dumpReqHeaders(req) }) + if strings.Contains(out, assertion) { + t.Errorf("dumpReqHeaders leaked the SAML assertion: %q", out) + } + if !strings.Contains(out, "SAMLRequest") { + t.Errorf("dump should still name the parameter: %q", out) + } + if !strings.Contains(out, "redacted") { + t.Errorf("dump should mark the value redacted: %q", out) + } +} + +func TestDumpReqHeadersNonSAMLURLUnchanged(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "https://d2l.cbe.ab.ca/d2l/api/le/1.97/123/content/toc", nil) + out := captureStderr(t, func() { dumpReqHeaders(req) }) + if !strings.Contains(out, "/content/toc") { + t.Errorf("non-SAML URL should be printed verbatim: %q", out) + } + if strings.Contains(out, "redacted") { + t.Errorf("non-SAML URL should not be redacted: %q", out) + } +} + +func TestRedactSAMLUnparseableURL(t *testing.T) { + if got := redactSAML("ht tp://bad url"); got != "" { + t.Errorf("redactSAML = %q, want the full URL redacted", got) + } +} From 5008f7891bd971af691058e63fe56bd732d23cf0 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:24:18 -0600 Subject: [PATCH 05/27] fix(systemd): parse list-timers rows with placeholder LAST/PASSED columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit splitTimerFields required two day-of-week tokens, so any row whose LAST column was a placeholder (a timer that has never fired) was dropped entirely and status printed no "Next run" at all. When only PASSED was a placeholder, the tail anchoring consumed the "-" as the unit name, reporting Unit="-" and ACTIVATES=. The parse now anchors on the final UNIT/ACTIVATES tokens and treats placeholder columns explicitly; the CLI omits the "(…)" suffix when no relative time is available. --- cmd/systemd.go | 12 ++- internal/systemd/systemd.go | 105 +++++++++----------------- internal/systemd/systemd_test.go | 122 +++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 71 deletions(-) create mode 100644 internal/systemd/systemd_test.go diff --git a/cmd/systemd.go b/cmd/systemd.go index 2b500bf..c239bb0 100644 --- a/cmd/systemd.go +++ b/cmd/systemd.go @@ -136,7 +136,11 @@ func printSystemdStatus(action string, st systemd.Status) error { _, _ = fmt.Fprintf(tw, "Next run:\t%s (in %s)\n", st.Next, st.Left) } if st.Last != "" { - _, _ = fmt.Fprintf(tw, "Last run:\t%s (%s)\n", st.Last, st.Passed) + if st.Passed != "" { + _, _ = fmt.Fprintf(tw, "Last run:\t%s (%s)\n", st.Last, st.Passed) + } else { + _, _ = fmt.Fprintf(tw, "Last run:\t%s\n", st.Last) + } } writeWarnings() _, _ = fmt.Fprintln(tw, "") @@ -157,7 +161,11 @@ func printSystemdStatus(action string, st systemd.Status) error { _, _ = fmt.Fprintf(tw, "Next run:\t%s (in %s)\n", st.Next, st.Left) } if st.Last != "" { - _, _ = fmt.Fprintf(tw, "Last run:\t%s (%s)\n", st.Last, st.Passed) + if st.Passed != "" { + _, _ = fmt.Fprintf(tw, "Last run:\t%s (%s)\n", st.Last, st.Passed) + } else { + _, _ = fmt.Fprintf(tw, "Last run:\t%s\n", st.Last) + } } if len(st.TimerOutput) > 0 { _, _ = fmt.Fprintln(tw, "") diff --git a/internal/systemd/systemd.go b/internal/systemd/systemd.go index 1df3651..8a41541 100644 --- a/internal/systemd/systemd.go +++ b/internal/systemd/systemd.go @@ -321,89 +321,56 @@ func Query() (Status, error) { return st, nil } +const timerPlaceholder = "-" + // splitTimerFields groups a token slice from a `list-timers` data row into -// the six semantic columns. Robust to either layout: -// - NEXT is `-` (timer never fired): firstDOW is at the LAST position -// - NEXT is a real datetime: firstDOW is at position 0 +// the six semantic columns. The row layout is: +// +// // -// We anchor on the LAST datetime (second day-of-week token) which is always -// present, then derive everything else relative to it. +// The final two tokens are always UNIT and ACTIVATES, so they anchor the +// parse. LAST and PASSED are placeholders until the timer has fired at +// least once; a placeholder PASSED must not shift the UNIT/ACTIVATES +// columns. func splitTimerFields(f []string) (next, left, last, passed, unit, activates string, ok bool) { - if len(f) < 4 { + if len(f) < 6 { return } - // Locate the two day-of-week tokens. LAST always has 4 tokens - // (DayOfWeek YYYY-MM-DD HH:MM:SS TZ), so its DOW marks a clean split. - firstDOW := -1 - secondDOW := -1 - for i, tok := range f { + unit, activates = f[len(f)-2], f[len(f)-1] + rest := f[:len(f)-2] + + var dows []int + for i, tok := range rest { if isDayOfWeek(tok) { - if firstDOW == -1 { - firstDOW = i - } else if secondDOW == -1 { - secondDOW = i - break - } + dows = append(dows, i) } } - if firstDOW == -1 || secondDOW == -1 { - return - } - // last = 4 tokens starting at secondDOW - lastEnd := secondDOW + 4 - if lastEnd > len(f) { - return - } - last = strings.Join(f[secondDOW:lastEnd], " ") - - // next = tokens before secondDOW. Two sub-cases: - // - firstDOW == 0: NEXT is a full datetime at f[0:4], LEFT is f[4] - // - firstDOW > 0: NEXT is f[0:firstDOW] (placeholder or just "-" with - // nothing after), LEFT would be the next field after that - if firstDOW == 0 { - // Real datetime - if secondDOW < 4 { - return // malformed: NEXT datetime would overlap LAST + switch len(dows) { + case 1: + if len(rest) < 5 { + return } - next = strings.Join(f[0:4], " ") - if 4 < secondDOW { - left = f[4] + next = strings.Join(rest[:4], " ") + if rest[4] != timerPlaceholder { + left = rest[4] } - } else { - // Placeholder (or short). firstDOW > 0 means f[0] is the placeholder. - next = strings.Join(f[0:firstDOW], " ") - if firstDOW+1 < secondDOW { - left = f[firstDOW+1] + case 2: + if dows[1]+4 > len(rest) { + return } - } - - // passed = tokens after LAST, up to and including the "ago" sentinel. - // `systemctl list-timers` renders relative times in many shapes: - // "5s", "1min", "2h", "1min 42s", "3 days 2h", etc. The only stable - // boundary is the trailing "ago". - passedStart := lastEnd - passedEnd := -1 - for i := passedStart; i < len(f)-1; i++ { - if f[i+1] == "ago" { - passedEnd = i + 2 // include the "ago" - break + next = strings.Join(rest[:4], " ") + if rest[4] != timerPlaceholder { + left = rest[4] } - } - if passedEnd == -1 { - // No "ago" sentinel — leave Passed empty. - passed = "" - passedEnd = passedStart - } else { - passed = strings.Join(f[passedStart:passedEnd], " ") - } - - // unit, activates at the tail. - tail := f[passedEnd:] - if len(tail) < 2 { + last = strings.Join(rest[dows[1]:dows[1]+4], " ") + mid := rest[dows[1]+4:] + if len(mid) == 1 && mid[0] == timerPlaceholder { + mid = nil + } + passed = strings.Join(mid, " ") + default: return } - unit = tail[0] - activates = tail[1] ok = true return } diff --git a/internal/systemd/systemd_test.go b/internal/systemd/systemd_test.go new file mode 100644 index 0000000..ba8e185 --- /dev/null +++ b/internal/systemd/systemd_test.go @@ -0,0 +1,122 @@ +package systemd + +import "testing" + +func TestSplitTimerFieldsBothDatetimes(t *testing.T) { + f := []string{ + "Wed", "2026-10-14", "20:30:55", "MDT", "29min", + "Wed", "2026-10-14", "20:00:55", "MDT", "23s", "ago", + "schooltools-archive.timer", "schooltools-archive.service", + } + next, left, last, passed, unit, activates, ok := splitTimerFields(f) + if !ok { + t.Fatal("expected ok for a complete data row") + } + if next != "Wed 2026-10-14 20:30:55 MDT" { + t.Errorf("next = %q", next) + } + if left != "29min" { + t.Errorf("left = %q", left) + } + if last != "Wed 2026-10-14 20:00:55 MDT" { + t.Errorf("last = %q", last) + } + if passed != "23s ago" { + t.Errorf("passed = %q", passed) + } + if unit != "schooltools-archive.timer" || activates != "schooltools-archive.service" { + t.Errorf("unit/activates = %q/%q", unit, activates) + } +} + +func TestSplitTimerFieldsNeverFired(t *testing.T) { + f := []string{ + "Wed", "2026-10-14", "21:00:00", "MDT", "30min", + "-", "-", + "schooltools-archive.timer", "schooltools-archive.service", + } + next, left, last, passed, unit, activates, ok := splitTimerFields(f) + if !ok { + t.Fatal("expected ok for a never-fired row") + } + if next != "Wed 2026-10-14 21:00:00 MDT" { + t.Errorf("next = %q, want the upcoming run", next) + } + if left != "30min" { + t.Errorf("left = %q", left) + } + if last != "" || passed != "" { + t.Errorf("last/passed = %q/%q, want empty", last, passed) + } + if unit != "schooltools-archive.timer" || activates != "schooltools-archive.service" { + t.Errorf("unit/activates = %q/%q (placeholder LAST must not shift the tail)", unit, activates) + } +} + +func TestSplitTimerFieldsPlaceholderPassed(t *testing.T) { + f := []string{ + "Wed", "2026-10-14", "20:30:55", "MDT", "29min", + "Wed", "2026-10-14", "20:00:55", "MDT", "-", + "schooltools-archive.timer", "schooltools-archive.service", + } + next, left, last, passed, unit, activates, ok := splitTimerFields(f) + if !ok { + t.Fatal("expected ok") + } + if next != "Wed 2026-10-14 20:30:55 MDT" || left != "29min" { + t.Errorf("next/left = %q/%q", next, left) + } + if last != "Wed 2026-10-14 20:00:55 MDT" { + t.Errorf("last = %q", last) + } + if passed != "" { + t.Errorf("passed = %q, want empty for a placeholder", passed) + } + if unit != "schooltools-archive.timer" || activates != "schooltools-archive.service" { + t.Errorf("unit/activates = %q/%q, want the real unit names", unit, activates) + } +} + +func TestSplitTimerFieldsMultiTokenPassed(t *testing.T) { + f := []string{ + "Thu", "2026-10-15", "15:31:45", "MDT", "19h", + "Wed", "2026-10-14", "15:31:45", "MDT", "4h", "29min", "ago", + "launchpadlib-cache-clean.timer", "launchpadlib-cache-clean.service", + } + _, _, _, passed, unit, activates, ok := splitTimerFields(f) + if !ok { + t.Fatal("expected ok") + } + if passed != "4h 29min ago" { + t.Errorf("passed = %q, want %q", passed, "4h 29min ago") + } + if unit != "launchpadlib-cache-clean.timer" || activates != "launchpadlib-cache-clean.service" { + t.Errorf("unit/activates = %q/%q", unit, activates) + } +} + +func TestSplitTimerFieldsRejectsGarbage(t *testing.T) { + for _, f := range [][]string{ + {}, + {"NEXT", "LEFT", "LAST", "PASSED", "UNIT", "ACTIVATES"}, + {"-", "-", "schooltools-archive.timer", "schooltools-archive.service"}, + {"1", "timers", "listed."}, + } { + if _, _, _, _, _, _, ok := splitTimerFields(f); ok { + t.Errorf("splitTimerFields(%v) = ok, want not-ok", f) + } + } +} + +func TestIsDayOfWeek(t *testing.T) { + for _, s := range []string{"Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"} { + if !isDayOfWeek(s) { + t.Errorf("isDayOfWeek(%q) = false", s) + } + } + for _, s := range []string{"", "NEXT", "n/a", "Wed.", "1"} { + if isDayOfWeek(s) { + t.Errorf("isDayOfWeek(%q) = true", s) + } + } +} From ef3767e612c801f8a920b946957e6a6c5acca734 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:25:06 -0600 Subject: [PATCH 06/27] chore(systemd): drop dead code, surface install warnings via Status Uninstall built a `removed` slice it never read and had two identical return branches. Install printed renderExecStart's volatile-path warning straight to stderr instead of routing it through Status.Warnings, so --json output could not see it; it now goes through the same warning channel as the legacy-ExecStart warning. legacyExecStart also treats a unit file with no ExecStart line at all as needing an upgrade. --- internal/systemd/systemd.go | 32 ++++++++++---------------- internal/systemd/systemd_test.go | 39 +++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 21 deletions(-) diff --git a/internal/systemd/systemd.go b/internal/systemd/systemd.go index 8a41541..989ba23 100644 --- a/internal/systemd/systemd.go +++ b/internal/systemd/systemd.go @@ -98,10 +98,11 @@ func isVolatilePath(p string) bool { } // legacyExecStart reports whether the on-disk service unit still calls the -// pre-archive-UX-redesign form (e.g. `archive` / `prune`). Parses each -// ExecStart= directive (skipping comments and unrelated lines) and returns -// true when none of them invoke `archive update`. A bare mention of -// "archive update" in a Description= line cannot suppress the warning. +// pre-archive-UX-redesign form (e.g. `archive` / `prune`), or is missing an +// ExecStart line altogether. Parses each ExecStart= directive (skipping +// comments and unrelated lines) and returns true when none of them invoke +// `archive update`. A bare mention of "archive update" in a Description= +// line cannot suppress the warning. func legacyExecStart(svcPath string) bool { data, err := os.ReadFile(svcPath) if err != nil { @@ -123,7 +124,7 @@ func legacyExecStart(svcPath string) bool { } } if !anyExec { - return false + return true } return !anyUpdate } @@ -190,9 +191,6 @@ func Install(opts InstallOptions) (Status, error) { if err != nil { return Status{}, err } - for _, w := range warnings { - fmt.Fprintln(os.Stderr, "warning:", w) - } data := map[string]string{"ExecStart": execStart} if err := renderTemplate("schooltools-archive.service.tmpl", data, svcPath); err != nil { return Status{}, err @@ -209,7 +207,9 @@ func Install(opts InstallOptions) (Status, error) { return Status{}, fmt.Errorf("enable --now %s failed: %w\n%s", TimerName, err, out) } - return Query() + st, err := Query() + st.Warnings = append(warnings, st.Warnings...) + return st, err } // Uninstall disables+stops the timer, then removes the unit files. Safe to @@ -224,19 +224,11 @@ func Uninstall() (Status, error) { svcPath := filepath.Join(dir, ServiceName) tmrPath := filepath.Join(dir, TimerName) - removed := []string{} - if err := os.Remove(svcPath); err == nil { - removed = append(removed, svcPath) - } - if err := os.Remove(tmrPath); err == nil { - removed = append(removed, tmrPath) - } + _ = os.Remove(svcPath) + _ = os.Remove(tmrPath) if _, err := runSystemctl("daemon-reload"); err != nil { return Status{}, fmt.Errorf("daemon-reload after uninstall failed: %w", err) } - if len(removed) == 0 { - return Status{UserDir: dir}, nil - } return Status{UserDir: dir}, nil } @@ -261,7 +253,7 @@ func Query() (Status, error) { } if legacyExecStart(svcPath) { st.Warnings = append(st.Warnings, - "service unit uses a pre-archive-update ExecStart; rerun `schooltools systemd install --force` to upgrade in place") + "service unit has no ExecStart or uses a pre-archive-update one; rerun `schooltools systemd install --force` to upgrade in place") } // is-enabled diff --git a/internal/systemd/systemd_test.go b/internal/systemd/systemd_test.go index ba8e185..ed59c13 100644 --- a/internal/systemd/systemd_test.go +++ b/internal/systemd/systemd_test.go @@ -1,6 +1,10 @@ package systemd -import "testing" +import ( + "os" + "path/filepath" + "testing" +) func TestSplitTimerFieldsBothDatetimes(t *testing.T) { f := []string{ @@ -120,3 +124,36 @@ func TestIsDayOfWeek(t *testing.T) { } } } + +func TestLegacyExecStart(t *testing.T) { + dir := t.TempDir() + write := func(name, content string) string { + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", name, err) + } + return p + } + + cases := []struct { + name string + content string + want bool + }{ + {"current unit", "[Service]\nExecStart=/usr/bin/schooltools archive update --no-auto-refresh\n", false}, + {"bare archive", "[Service]\nExecStart=/usr/bin/schooltools archive\n", true}, + {"no exec start", "[Unit]\nDescription=schooltools archive\n", true}, + {"prune exec start", "[Service]\nExecStart=/usr/bin/schooltools archive prune --delete\n", true}, + {"description mention only", "[Unit]\nDescription=runs archive update\n[Service]\nExecStart=/usr/bin/schooltools archive\n", true}, + {"commented mention", "# ExecStart=/usr/bin/schooltools archive update\n[Service]\nExecStart=/usr/bin/schooltools archive\n", true}, + } + for _, tc := range cases { + if got := legacyExecStart(write(tc.name, tc.content)); got != tc.want { + t.Errorf("legacyExecStart(%s) = %v, want %v", tc.name, got, tc.want) + } + } + + if legacyExecStart(filepath.Join(dir, "does-not-exist")) { + t.Error("a missing unit file should not be reported as legacy") + } +} From 9c5938e9bba3b6e42619449cb4afc92ab8c0a8a0 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:25:28 -0600 Subject: [PATCH 07/27] docs: changelog entries for the robustness, body-state, and login-leak fixes --- CHANGELOG.md | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa16a3d..a8b9ef1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -315,6 +315,61 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `html.UnescapeString`, so the default text body decodes common named and numeric entities (`&`, `<`, `'`, `'`, …) rather than leaving them as raw HTML escape sequences. +- **P1:** `archive update` aborted the entire pass on the first topic + whose metadata fetch failed. D2L keeps stale "broken" TOC entries + (`IsBroken: true`, 404 on the topics API) — one dead 2022 dropbox + link in a single course blocked every course queued after it from + archiving. Topics D2L flags broken are now skipped without an API + call, failed fetches are counted and retried on the next pass, and + only a 401/403 stops the run (reported as `session expired; run + 'schooltools login'` instead of a raw status line). The update + summary and `--json` envelope gain `topicsFailed` / `topicsSkipped` + counters, and `--dry-run` reports broken topics so the plan matches + what a real pass does. +- **P1:** `archive` status and `archive find` trusted the index's + `currentBody` pointer instead of the filesystem, so a store with + 186 dangling pointers (see the 0.2.0 prune entry) reported + "Bodies: 326 present, 8 missing" while 186 blobs were actually + gone. `LoadStatus` now stats each blob; `find --with-bodies` gained + the `WithBodiesOnDisk` filter and `find --missing-bodies` also + matches topics whose pointer exists but whose blob does not. + `HasBody` on search hits and `FileTopic` reflects on-disk state. + `archive export` keeps the old pointer semantics so its + "skipped-missing-body" reporting is unchanged. +- **P2:** D2L file-body URLs that contain unescaped path characters — + literal spaces, a bare `%` (`Acetic Acid, 5% (Vinegar).pdf`), `#` in + filenames (`Quiz #3 …`) — either failed `url.Parse` with + `invalid URL escape` or were truncated at the `#` into a URL + fragment and 404'd. The new `internal/urlutil` re-encodes the path + portion of a reference (preserving already-valid `%XX` sequences + and query strings) before it is resolved against the LMS base URL; + both `archive update`'s body downloads and `download` route + through it. +- **P2:** `systemd status` dropped `systemctl list-timers` data rows + whose LAST column was a placeholder (a timer that never fired), so + fresh installs printed no next-run time at all, and mis-reported + UNIT/ACTIVATES when PASSED was a placeholder. The parser now + anchors on the trailing unit columns and treats placeholders + explicitly. +- **P2:** `systemd install` printed the volatile-binary-path warning + straight to stderr instead of routing it through `Status.Warnings` + (invisible to `--json`); `Uninstall` built a `removed` slice it + never read; a service unit with no `ExecStart=` line at all is now + flagged by the legacy-ExecStart warning. + +### Security +- **P1:** The login/auto-refresh HTTP debug dump (`<< Set-Cookie: …` + values, request URLs) was printed **unconditionally** on every form + POST — `fetchPost` didn't take `Options`, and every auto-refresh + path hard-codes `Verbose: false`. Any `archive update` or `login` + run against an expired session therefore leaked the ADFS + `SAMLRequest` URL and raw `d2lSessionVal` / `d2lSecureSessionVal` + cookie values to stderr (and into journald under the systemd + unit), violating the threat model in `SECURITY.md`. `fetchPost` now + dumps only behind `--verbose`, SAML parameters + (`SAMLRequest`/`SAMLResponse`/`RelayState`) are replaced with a + length marker even in verbose mode, and cookie values are + truncated the same way the per-hop dump truncates them. ## [0.2.0] - 2026-09-05 From b029da3bde0b7a5a7f1cda8e18b475da01666129 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:30:28 -0600 Subject: [PATCH 08/27] style(urlutil): use fmt.Fprintf on the builder (staticcheck QF1012) --- internal/urlutil/urlutil.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/urlutil/urlutil.go b/internal/urlutil/urlutil.go index 9c0cd2c..c9904ba 100644 --- a/internal/urlutil/urlutil.go +++ b/internal/urlutil/urlutil.go @@ -55,7 +55,7 @@ func encodePath(p string) string { case c == '%': b.WriteString("%25") case mustEscape(c): - b.WriteString(fmt.Sprintf("%%%02X", c)) + fmt.Fprintf(&b, "%%%02X", c) default: b.WriteByte(c) } From 152e69a548a5332c08306ec49b56c9fdafd27945 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:41:08 -0600 Subject: [PATCH 09/27] fix(urlutil): clean absolute URLs too and treat ? as a path character ResolveD2L returned absolute URLs unchanged, so a D2L URL with a space, bare % or # in its path still reached the HTTP client malformed; resolveFileURL (download) also short-circuited before the helper. Absolute URLs now go through the same path encoding, and both call sites route everything through ResolveD2L. A literal '?' in a filename was being treated as the start of a query string, truncating the requested path. D2L file URLs never carry a query, so '?' is now encoded as a path character (%3F). --- cmd/download.go | 3 --- internal/urlutil/urlutil.go | 41 +++++++++++++++++++------------- internal/urlutil/urlutil_test.go | 13 +++++----- 3 files changed, 31 insertions(+), 26 deletions(-) diff --git a/cmd/download.go b/cmd/download.go index ff8f869..cc080d3 100644 --- a/cmd/download.go +++ b/cmd/download.go @@ -317,9 +317,6 @@ func resolveFileURL(rawURL string) (string, error) { if rawURL == "" { return "", fmt.Errorf("empty url") } - if strings.HasPrefix(rawURL, "http://") || strings.HasPrefix(rawURL, "https://") { - return rawURL, nil - } return urlutil.ResolveD2L(ua.D2LBase, rawURL) } diff --git a/internal/urlutil/urlutil.go b/internal/urlutil/urlutil.go index c9904ba..f461e91 100644 --- a/internal/urlutil/urlutil.go +++ b/internal/urlutil/urlutil.go @@ -1,6 +1,6 @@ // Package urlutil repairs the D2L content URLs that the LMS emits with -// unescaped path characters (literal spaces, '#', stray '%') before they -// are handed to net/http. +// unescaped path characters (literal spaces, '#', stray '%', '?') before +// they are handed to net/http. package urlutil import ( @@ -9,13 +9,14 @@ import ( "strings" ) -// ResolveD2L resolves a D2L content URL against a base URL. Absolute URLs -// are returned unchanged; relative references are percent-encoded first so -// that url.Parse cannot fail on them and no character is silently -// reinterpreted as a URL fragment. +// ResolveD2L resolves a D2L content URL against a base URL. Relative +// references and absolute URLs are both percent-encoded first, so that +// url.Parse cannot fail on them and no character is silently +// reinterpreted as a URL fragment or query delimiter. A '?' is treated +// as a path character: D2L file URLs never carry a query string. func ResolveD2L(base, raw string) (string, error) { if isAbs(raw) { - return raw, nil + return cleanAbs(raw), nil } b, err := url.Parse(base) if err != nil { @@ -28,21 +29,29 @@ func ResolveD2L(base, raw string) (string, error) { return b.ResolveReference(ref).String(), nil } -// CleanPath percent-encodes the path portion of a URL reference, -// preserving escape sequences that are already valid. A query string -// (everything from the first '?') is passed through untouched. +// CleanPath percent-encodes a URL path, preserving escape sequences that +// are already valid. func CleanPath(raw string) string { - path, query := raw, "" - if i := strings.IndexByte(raw, '?'); i >= 0 { - path, query = raw[:i], raw[i:] - } - return encodePath(path) + query + return encodePath(raw) } func isAbs(u string) bool { return strings.HasPrefix(u, "http://") || strings.HasPrefix(u, "https://") } +func cleanAbs(raw string) string { + i := strings.Index(raw, "://") + if i < 0 { + return CleanPath(raw) + } + rest := raw[i+3:] + slash := strings.IndexByte(rest, '/') + if slash < 0 { + return raw + } + return raw[:i+3] + rest[:slash] + CleanPath(rest[slash:]) +} + func encodePath(p string) string { var b strings.Builder b.Grow(len(p) + 8) @@ -71,7 +80,7 @@ func mustEscape(c byte) bool { return true } switch c { - case '"', '#', '<', '>', '`', '{', '|', '}', '\\', '^': + case '"', '#', '<', '>', '?', '`', '{', '|', '}', '\\', '^': return true } return false diff --git a/internal/urlutil/urlutil_test.go b/internal/urlutil/urlutil_test.go index 8661997..2d9a19f 100644 --- a/internal/urlutil/urlutil_test.go +++ b/internal/urlutil/urlutil_test.go @@ -17,8 +17,7 @@ func TestCleanPath(t *testing.T) { {"existing escapes preserved", "/c/KEY%20Practice.pdf", "/c/KEY%20Practice.pdf"}, {"trailing bare percent", "/c/100%", "/c/100%25"}, {"bare percent not followed by hex", "/c/50% off.pdf", "/c/50%25%20off.pdf"}, - {"query string preserved", "/c/a b.pdf?download=1&x=2", "/c/a%20b.pdf?download=1&x=2"}, - {"percent then query", "/c/a%2fb.pdf?x=1", "/c/a%2fb.pdf?x=1"}, + {"question mark is a path character", "/c/What? Really.pdf?download=1", "/c/What%3F%20Really.pdf%3Fdownload=1"}, {"non-ascii bytes", "/c/café menu.pdf", "/c/caf%C3%A9%20menu.pdf"}, } for _, tc := range cases { @@ -63,14 +62,14 @@ func TestResolveD2LHashFilename(t *testing.T) { } } -func TestResolveD2LAbsolutePassthrough(t *testing.T) { - abs := "https://files.example.com/a%20b.pdf" - got, err := ResolveD2L("https://d2l.cbe.ab.ca", abs) +func TestResolveD2LAbsoluteURLIsCleaned(t *testing.T) { + got, err := ResolveD2L("https://d2l.cbe.ab.ca", "https://files.example.com/a b#c.pdf") if err != nil { t.Fatalf("ResolveD2L: %v", err) } - if got != abs { - t.Errorf("absolute URL rewritten: %q", got) + want := "https://files.example.com/a%20b%23c.pdf" + if got != want { + t.Errorf("ResolveD2L = %q, want %q", got, want) } } From 8160d714895d6819ae799f35595950fab0a59bce Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:41:17 -0600 Subject: [PATCH 10/27] fix(archive): report expired sessions for course list, login pages, and 401 bodies Three paths still mistook an expired session for ordinary data: - the my-courses listing 401'd and surfaced as a course-list failure instead of "session expired"; - a topic request that redirected to the D2L login page returned HTTP 200 with text/html; the pretty-JSON fallback stored the login HTML as the topic's metadata blob; - a 401/403 on a file-body download was counted as a body error, so the run completed with missing bodies and exit 0 while the session was dead. fetchCoursesPage, fetchTopicFor and downloadFileBody now map 401/403 (and login-page responses) to errSessionExpired; the body-error branch propagates it as a fatal run error. Also strengthens TestRunSurvivesBrokenAndFailingTopics: a healthy topic is now queued after the failing one (proving the pass continues), and the test asserts the broken topic is never requested. --- internal/archive/archive.go | 18 +++++-- internal/archive/archive_test.go | 29 +++++++--- internal/archive/resilience_test.go | 83 +++++++++++++++++++++++++++-- 3 files changed, 114 insertions(+), 16 deletions(-) diff --git a/internal/archive/archive.go b/internal/archive/archive.go index d879de5..7eba260 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -771,9 +771,9 @@ func diffCourse(opts Options, c Course) (CourseDiff, error) { // when DownloadFiles is true — otherwise all three stay zero and the // summary line simply omits the bodies section. type courseStats struct { - stale, checked, fetched int - bodiesNew, bodiesReused, bodiesErr int - failed, skipped int + stale, checked, fetched int + bodiesNew, bodiesReused, bodiesErr int + failed, skipped int } // archiveCourse runs a single course's archive pass and returns the @@ -893,6 +893,9 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, } bodyBytes, contentType, berr := downloadFileBody(opts.Jar, bodyURL, opts.baseURL()) if berr != nil { + if errors.Is(berr, errSessionExpired) { + return s, berr + } if opts.Verbose { fmt.Fprintf(os.Stderr, "[archive] body %d: %v (continuing)\n", q.topicID, berr) } @@ -987,6 +990,9 @@ func downloadFileBody(jar *cookiejar.Jar, rawURL, base string) ([]byte, string, } defer func() { _ = res.Body.Close() }() if res.StatusCode/100 != 2 { + if res.StatusCode == 401 || res.StatusCode == 403 { + return nil, "", errSessionExpired + } body, _ := io.ReadAll(res.Body) msg := strings.TrimSpace(string(body)) if len(msg) > 200 { @@ -1070,6 +1076,9 @@ func fetchTopicFor(base string, jar *cookiejar.Jar, orgUnitID string, topicID in } return nil, fmt.Errorf("D2L API returned %d %s for topic %d", res.StatusCode, res.Status, topicID) } + if ct := res.Header.Get("Content-Type"); strings.Contains(ct, "text/html") || ua.IsLoginURL(res.Request.URL.String()) { + return nil, errSessionExpired + } body, err := httpclient.BodyBytes(res) if err != nil { return nil, err @@ -1300,6 +1309,9 @@ func fetchCoursesPage(jar *cookiejar.Jar, rawURL string) (d2lCoursesResponse, er return d2lCoursesResponse{}, err } if res.StatusCode != 200 { + if res.StatusCode == 401 || res.StatusCode == 403 { + return d2lCoursesResponse{}, errSessionExpired + } return d2lCoursesResponse{}, fmt.Errorf("D2L my-courses API returned %d %s", res.StatusCode, res.Status) } ct := res.Header.Get("Content-Type") diff --git a/internal/archive/archive_test.go b/internal/archive/archive_test.go index 6396d55..92a1c44 100644 --- a/internal/archive/archive_test.go +++ b/internal/archive/archive_test.go @@ -2,6 +2,7 @@ package archive import ( "encoding/json" + "io" "net/http" "net/http/cookiejar" "net/http/httptest" @@ -424,14 +425,16 @@ func TestPruneKeepsLiveAcrossCourses(t *testing.T) { // ---- end-to-end with stubbed HTTP ----------------------------------------- type fakeD2L struct { - t *testing.T - courses []d2lCourse - tocs map[string]content.TocResponse - topics map[int]map[string]any - topicStatus map[int]int - tocCalls int - topicCalls map[int]int - bodyHandler http.HandlerFunc // optional; serves file bodies on arbitrary paths + t *testing.T + courses []d2lCourse + coursesStatus int + tocs map[string]content.TocResponse + topics map[int]map[string]any + topicRaw map[int]string + topicStatus map[int]int + tocCalls int + topicCalls map[int]int + bodyHandler http.HandlerFunc // optional; serves file bodies on arbitrary paths } func newFakeD2L(t *testing.T) *fakeD2L { @@ -455,6 +458,7 @@ func newFakeD2L(t *testing.T) *fakeD2L { t: t, tocs: map[string]content.TocResponse{}, topics: map[int]map[string]any{}, + topicRaw: map[int]string{}, topicStatus: map[int]int{}, topicCalls: map[int]int{}, } @@ -463,6 +467,10 @@ func newFakeD2L(t *testing.T) *fakeD2L { func (f *fakeD2L) handler() http.Handler { mux := http.NewServeMux() mux.HandleFunc("/d2l/le/manageCourses/api/mycourses", func(w http.ResponseWriter, r *http.Request) { + if f.coursesStatus != 0 { + w.WriteHeader(f.coursesStatus) + return + } w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(d2lCoursesResponse{Courses: f.courses}) }) @@ -494,6 +502,11 @@ func (f *fakeD2L) handler() http.Handler { w.WriteHeader(code) return } + if raw, ok := f.topicRaw[id]; ok { + w.Header().Set("Content-Type", "text/html") + _, _ = io.WriteString(w, raw) + return + } topic, ok := f.topics[id] if !ok { http.NotFound(w, r) diff --git a/internal/archive/resilience_test.go b/internal/archive/resilience_test.go index 428dfe6..54a868f 100644 --- a/internal/archive/resilience_test.go +++ b/internal/archive/resilience_test.go @@ -18,8 +18,10 @@ func TestRunSurvivesBrokenAndFailingTopics(t *testing.T) { {TopicId: 6001, Title: "Fine", LastModifiedDate: "2026-03-01T00:00:00Z", TypeIdentifier: "File", Url: &goodURL}, {TopicId: 6002, Title: "Dead dropbox link", LastModifiedDate: "2022-03-15T16:25:36Z", IsBroken: true}, {TopicId: 6003, Title: "Vanished", LastModifiedDate: "2026-03-02T00:00:00Z"}, + {TopicId: 6004, Title: "After the failure", LastModifiedDate: "2026-03-03T00:00:00Z"}, }}}} f.topics[6001] = map[string]any{"TopicId": 6001, "Title": "Fine", "TypeIdentifier": "File", "Url": goodURL} + f.topics[6004] = map[string]any{"TopicId": 6004, "Title": "After the failure"} // 6002 is flagged broken and 6003 is never registered, so the topics API 404s it. srv := httptest.NewServer(f.handler()) @@ -30,11 +32,11 @@ func TestRunSurvivesBrokenAndFailingTopics(t *testing.T) { if err != nil { t.Fatalf("run should not abort on a broken/failing topic: %v", err) } - if res.TopicsChecked != 3 { - t.Errorf("TopicsChecked=%d, want 3", res.TopicsChecked) + if res.TopicsChecked != 4 { + t.Errorf("TopicsChecked=%d, want 4", res.TopicsChecked) } - if res.TopicsFetched != 1 { - t.Errorf("TopicsFetched=%d, want 1 (only the healthy topic)", res.TopicsFetched) + if res.TopicsFetched != 2 { + t.Errorf("TopicsFetched=%d, want 2 (healthy topics on both sides of the failure)", res.TopicsFetched) } if res.TopicsSkipped != 1 { t.Errorf("TopicsSkipped=%d, want 1 (IsBroken topic)", res.TopicsSkipped) @@ -42,13 +44,19 @@ func TestRunSurvivesBrokenAndFailingTopics(t *testing.T) { if res.TopicsFailed != 1 { t.Errorf("TopicsFailed=%d, want 1 (404 topic)", res.TopicsFailed) } + if calls := f.topicCalls[6002]; calls != 0 { + t.Errorf("broken topic 6002 was fetched %d times, want 0 (must be skipped without an API call)", calls) + } ci, err := LoadCourseIndex(dir, "4242") if err != nil { t.Fatalf("LoadCourseIndex: %v", err) } if ci.Topics[6001] == nil || ci.Topics[6001].Current == "" { - t.Error("healthy topic was not archived") + t.Error("healthy topic 6001 was not archived") + } + if ci.Topics[6004] == nil || ci.Topics[6004].Current == "" { + t.Error("topic 6004, queued after the failure, was not archived") } if ci.Topics[6002] != nil || ci.Topics[6003] != nil { t.Error("broken/failing topics must not be recorded in the index") @@ -82,6 +90,71 @@ func TestRunAbortsOnSessionExpired(t *testing.T) { } } +func TestRunAbortsOnExpiredCourseList(t *testing.T) { + f := newFakeD2L(t) + f.coursesStatus = http.StatusUnauthorized + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + _, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL}) + if err == nil { + t.Fatal("expected the run to abort when the course list 401s") + } + if !errors.Is(err, errSessionExpired) { + t.Errorf("err = %v, want errSessionExpired", err) + } +} + +func TestRunAbortsOnLoginPageTopicResponse(t *testing.T) { + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("556"), Name: "Bio", Code: "BIO", IsActive: true}} + f.tocs["556"] = content.TocResponse{Modules: []content.TocModule{{ + ModuleId: 1, Topics: []content.TocTopic{{TopicId: 78, Title: "Cells", LastModifiedDate: "2026-03-01T00:00:00Z"}}, + }}} + f.topicRaw[78] = "Log Ind2l login page" + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + dir := t.TempDir() + _, err := Run(Options{Root: dir, Jar: newJar(t), BaseURL: srv.URL}) + if err == nil { + t.Fatal("expected the run to abort when a topic fetch returns the login page") + } + if !errors.Is(err, errSessionExpired) { + t.Errorf("err = %v, want errSessionExpired", err) + } + ci, lerr := LoadCourseIndex(dir, "556") + if lerr != nil { + t.Fatalf("LoadCourseIndex: %v", lerr) + } + if ci.Topics[78] != nil { + t.Error("the login page must not be archived as topic metadata") + } +} + +func TestRunAbortsOnExpiredBodyDownload(t *testing.T) { + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("557"), Name: "Bio", Code: "BIO", IsActive: true}} + urlStr := "/content/enforced/557-BIO/notes.pdf" + f.tocs["557"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: []content.TocTopic{ + {TopicId: 79, Title: "Notes", LastModifiedDate: "2026-03-01T00:00:00Z", TypeIdentifier: "File", Url: &urlStr}, + }}}} + f.topics[79] = map[string]any{"TopicId": 79, "Title": "Notes", "TypeIdentifier": "File", "Url": urlStr} + f.bodyHandler = func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + } + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + _, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL, DownloadFiles: true}) + if err == nil { + t.Fatal("expected the run to abort when a body download 401s") + } + if !errors.Is(err, errSessionExpired) { + t.Errorf("err = %v, want errSessionExpired", err) + } +} + func TestDiffSkipsBrokenTopics(t *testing.T) { f := newFakeD2L(t) f.courses = []d2lCourse{{OrgUnitId: json.Number("777"), Name: "Hist", Code: "HIS", IsActive: true}} From 8f71f3efaa5ec098ab78c104c1346c769385b0b6 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:41:22 -0600 Subject: [PATCH 11/27] test(archive): make the FileTopics HasBody test assert its fixture The loop over FileTopics ran zero times when the global index was missing, so the test passed vacuously. The test now seeds the global index and asserts the returned topic count. --- internal/archive/body_state_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/archive/body_state_test.go b/internal/archive/body_state_test.go index 4a3e73e..2700a52 100644 --- a/internal/archive/body_state_test.go +++ b/internal/archive/body_state_test.go @@ -99,11 +99,18 @@ func TestFileTopicsHasBodyChecksDisk(t *testing.T) { if err := SaveCourseIndex(dir, "1", ci); err != nil { t.Fatalf("SaveCourseIndex: %v", err) } - + if err := SaveIndex(dir, &Index{Version: SchemaVersion, Courses: map[string]*IndexEntry{ + "1": {OrgUnitId: "1", Code: "SAF"}, + }}); err != nil { + t.Fatalf("SaveIndex: %v", err) + } topics, err := FileTopics(dir, "") if err != nil { t.Fatalf("FileTopics: %v", err) } + if len(topics) != 2 { + t.Fatalf("FileTopics returned %d topics, want 2", len(topics)) + } for _, ft := range topics { want := ft.TopicID == 1 if ft.HasBody != want { From d1d18325833e1035e2d8d3a869f745f50c53bba0 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:41:22 -0600 Subject: [PATCH 12/27] fix(login): redact SAML in every verbose URL/header, redact cookie values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass redacted only the request URL. Verbose mode still printed the Referer header — which during the ADFS flow is the previous hop's URL carrying a SAMLRequest — plus Location headers, redirect targets, POST form actions and GET-failure URLs. All verbose URL prints now go through redactSAML. Cookie values were only truncated at 20 bytes, so short credentials printed in full. dumpRespSetCookies and the per-hop dump now redact the value entirely (keeping the name and attributes, which is what the Expires/Max-Age debugging actually needs). --- internal/login/login.go | 50 +++++++++++++++++++---------------- internal/login/login_test.go | 51 ++++++++++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 24 deletions(-) diff --git a/internal/login/login.go b/internal/login/login.go index 20acf94..fb32aff 100644 --- a/internal/login/login.go +++ b/internal/login/login.go @@ -89,7 +89,7 @@ func Run(opts Options) (Result, error) { return Result{}, stderrors.New("NO_LOCATION", "first redirect had no Location") } adfsURL := resolve(loc1, r1.Request.URL.String()) - verbose(opts, fmt.Sprintf("[stage %s] %d -> %s", StageD2LToADFS, r1.StatusCode, adfsURL)) + verbose(opts, fmt.Sprintf("[stage %s] %d -> %s", StageD2LToADFS, r1.StatusCode, redactSAML(adfsURL))) origins[originOf(adfsURL)] = struct{}{} r2, err := fetchGet(adfsURL, jar, r1.Request.URL.String(), opts) @@ -102,7 +102,7 @@ func Run(opts Options) (Result, error) { fmt.Sprintf("ADFS login page returned %d", r2.StatusCode)) } if opts.Verbose { - fmt.Fprintf(os.Stderr, "[2] %d %s\n", r2.StatusCode, r2.Request.URL.String()) + fmt.Fprintf(os.Stderr, "[2] %d %s\n", r2.StatusCode, redactSAML(r2.Request.URL.String())) } loginHTML, err := io.ReadAll(r2.Body) if err != nil { @@ -144,7 +144,7 @@ func Run(opts Options) (Result, error) { return Result{}, err } if opts.Verbose { - fmt.Fprintf(os.Stderr, "[3] %d %s\n", res.StatusCode, res.Request.URL.String()) + fmt.Fprintf(os.Stderr, "[3] %d %s\n", res.StatusCode, redactSAML(res.Request.URL.String())) } stage := StageADFSForms @@ -162,16 +162,16 @@ func Run(opts Options) (Result, error) { } hopsInStage++ hopsTotal++ -verbose(opts, fmt.Sprintf("[hop %d] %s #%d: %d %s", hopsTotal, stage, hopsInStage, res.StatusCode, res.Request.URL.String())) - if opts.Verbose { - if loc := res.Header.Get("Location"); loc != "" { - fmt.Fprintf(os.Stderr, " location: %s\n", loc) - } - for _, sc := range res.Cookies() { - fmt.Fprintf(os.Stderr, " set-cookie: %s=%s domain=%q path=%q expires=%v secure=%v httponly=%v\n", - sc.Name, truncateForLog(sc.Value), sc.Domain, sc.Path, sc.Expires, sc.Secure, sc.HttpOnly) +verbose(opts, fmt.Sprintf("[hop %d] %s #%d: %d %s", hopsTotal, stage, hopsInStage, res.StatusCode, redactSAML(res.Request.URL.String()))) + if opts.Verbose { + if loc := res.Header.Get("Location"); loc != "" { + fmt.Fprintf(os.Stderr, " location: %s\n", redactSAML(loc)) + } + for _, sc := range res.Cookies() { + fmt.Fprintf(os.Stderr, " set-cookie: %s= domain=%q path=%q expires=%v secure=%v httponly=%v\n", + sc.Name, len(sc.Value), sc.Domain, sc.Path, sc.Expires, sc.Secure, sc.HttpOnly) + } } - } origins[originOf(res.Request.URL.String())] = struct{}{} current := httpclient.CookiesForHost(jar, res.Request.URL.Host) @@ -199,7 +199,7 @@ verbose(opts, fmt.Sprintf("[hop %d] %s #%d: %d %s", hopsTotal, stage, hopsInStag return Result{}, stderrors.New("NO_LOCATION", fmt.Sprintf("Redirect %d with no Location", res.StatusCode)) } nextURL := resolve(loc, res.Request.URL.String()) - verbose(opts, " -> "+nextURL) + verbose(opts, " -> "+redactSAML(nextURL)) newStage := stageFor(nextURL, stage) if newStage != stage { stage = newStage @@ -271,7 +271,7 @@ stage = newStage postBody.Set(k, v) } postURL := resolve(formAction, res.Request.URL.String()) - verbose(opts, fmt.Sprintf(" POST %s (%d fields)", postURL, len(postBody))) + verbose(opts, fmt.Sprintf(" POST %s (%d fields)", redactSAML(postURL), len(postBody))) lower := strings.ToLower(postURL) isSAMLPush := strings.Contains(lower, "/saml/") || strings.Contains(lower, "samllogin") || strings.Contains(lower, "samlresponse") @@ -514,7 +514,7 @@ func fetchGet(rawURL string, jar *cookiejar.Jar, referer string, opts Options) ( func(err error) bool { return !isMFA(err) }, ) if err != nil { - verbose(opts, fmt.Sprintf("[net] GET %s failed: %v", rawURL, err)) + verbose(opts, fmt.Sprintf("[net] GET %s failed: %v", redactSAML(rawURL), err)) } else if opts.Verbose && out != nil && out.Request != nil { dumpReqHeaders(out.Request) dumpRespSetCookies(out) @@ -536,20 +536,23 @@ func fetchPost(rawURL, body, referer string, jar *cookiejar.Jar, opts Options) ( return res, err } -// dumpRespSetCookies prints raw Set-Cookie header values verbatim — useful -// for confirming whether the server actually sent Expires / Max-Age that our -// cookie parser might be losing. Values are truncated like the per-hop dump. +// dumpRespSetCookies prints one line per Set-Cookie with the value +// redacted — useful for confirming whether the server actually sent +// Expires / Max-Age that our cookie parser might be losing without +// putting the credential in a log. func dumpRespSetCookies(res *http.Response) { for _, sc := range res.Header.Values("Set-Cookie") { - name, value, _ := strings.Cut(sc, "=") - fmt.Fprintf(os.Stderr, " << Set-Cookie: %s=%s\n", name, truncateForLog(value)) + pair, attrs, _ := strings.Cut(sc, ";") + name, value, _ := strings.Cut(pair, "=") + fmt.Fprintf(os.Stderr, " << Set-Cookie: %s=%s\n", name, len(value), attrs) } } // dumpReqHeaders prints the request headers in a compact form so verbose // mode shows what we actually sent to the server. Body is intentionally -// omitted from the dump, and SAML parameters are redacted from the URL -// because the assertion is a bearer credential. +// omitted from the dump, SAML parameters are redacted from URLs, and the +// Referer is redacted because it carries the previous hop's URL, which +// during the ADFS flow contains a SAMLRequest. func dumpReqHeaders(r *http.Request) { if r == nil { return @@ -562,6 +565,9 @@ func dumpReqHeaders(r *http.Request) { "Upgrade-Insecure-Requests", "Pragma"} for _, k := range keys { if v := r.Header.Get(k); v != "" { + if k == "Referer" { + v = redactSAML(v) + } fmt.Fprintf(os.Stderr, " %s: %s\n", k, v) } } diff --git a/internal/login/login_test.go b/internal/login/login_test.go index a59df25..e26cf86 100644 --- a/internal/login/login_test.go +++ b/internal/login/login_test.go @@ -29,7 +29,9 @@ func captureStderr(t *testing.T, fn func()) string { func TestFetchPostDumpsOnlyWhenVerbose(t *testing.T) { secret := "supersecretvalue-but-very-long-cookie-tail" + requests := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ http.SetCookie(w, &http.Cookie{Name: "d2lSessionVal", Value: secret, Path: "/"}) w.WriteHeader(http.StatusOK) })) @@ -41,14 +43,25 @@ func TestFetchPostDumpsOnlyWhenVerbose(t *testing.T) { } out := captureStderr(t, func() { - _, _ = fetchPost(srv.URL, "UserName=x", "", jar, Options{}) + res, perr := fetchPost(srv.URL, "UserName=x", "", jar, Options{}) + if perr != nil { + t.Errorf("non-verbose fetchPost failed: %v", perr) + } + if res == nil || res.StatusCode != http.StatusOK { + t.Error("non-verbose fetchPost did not complete the request") + } }) if out != "" { t.Errorf("fetchPost without --verbose wrote to stderr: %q", out) } + if requests != 1 { + t.Errorf("server saw %d requests, want 1 (the non-verbose POST must still be sent)", requests) + } out = captureStderr(t, func() { - _, _ = fetchPost(srv.URL, "UserName=x", "", jar, Options{Verbose: true}) + if _, perr := fetchPost(srv.URL, "UserName=x", "", jar, Options{Verbose: true}); perr != nil { + t.Errorf("verbose fetchPost failed: %v", perr) + } }) if !strings.Contains(out, "Set-Cookie") { t.Errorf("verbose fetchPost did not dump Set-Cookie: %q", out) @@ -56,6 +69,9 @@ func TestFetchPostDumpsOnlyWhenVerbose(t *testing.T) { if strings.Contains(out, secret) { t.Errorf("verbose fetchPost leaked the full cookie value: %q", out) } + if requests != 2 { + t.Errorf("server saw %d requests, want 2", requests) + } } func TestDumpReqHeadersRedactsSAML(t *testing.T) { @@ -75,6 +91,37 @@ func TestDumpReqHeadersRedactsSAML(t *testing.T) { } } +func TestDumpReqHeadersRedactsReferer(t *testing.T) { + assertion := `secret` + referer := "https://adfsfba.cbe.ab.ca/adfs/ls/?SAMLRequest=" + url.QueryEscape(assertion) + req := httptest.NewRequest(http.MethodPost, "https://d2l.cbe.ab.ca/d2l/lp/auth/login/samlLogin.d2l", nil) + req.Header.Set("Referer", referer) + + out := captureStderr(t, func() { dumpReqHeaders(req) }) + if strings.Contains(out, assertion) { + t.Errorf("dumpReqHeaders leaked the SAML assertion via Referer: %q", out) + } + if !strings.Contains(out, "redacted") { + t.Errorf("Referer value should be redacted: %q", out) + } +} + +func TestDumpRespSetCookiesRedactsValue(t *testing.T) { + res := &http.Response{Header: http.Header{}} + res.Header.Add("Set-Cookie", "d2lSessionVal=abcdef0123456789abcdef0123456789; Path=/; Secure; HttpOnly") + + out := captureStderr(t, func() { dumpRespSetCookies(res) }) + if strings.Contains(out, "abcdef0123456789") { + t.Errorf("dumpRespSetCookies leaked the cookie value: %q", out) + } + if !strings.Contains(out, "d2lSessionVal=") { + t.Errorf("expected a redacted marker with the value length: %q", out) + } + if !strings.Contains(out, "Path=/") { + t.Errorf("cookie attributes should survive for debugging: %q", out) + } +} + func TestDumpReqHeadersNonSAMLURLUnchanged(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "https://d2l.cbe.ab.ca/d2l/api/le/1.97/123/content/toc", nil) out := captureStderr(t, func() { dumpReqHeaders(req) }) From db81ce1c82adbd04baeba7eca918b3e31055f9f6 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:41:25 -0600 Subject: [PATCH 13/27] fix(systemd): parse rows whose NEXT is a placeholder but LAST is real With a single day-of-week token the parser assumed it began a real NEXT datetime, so a NEXT="-" row (timer with no next elapse but a past run) produced a bogus next-run built from placeholders and lost the real LAST. Query also skipped such rows before parsing because it filtered on the first field being a weekday; it now filters on the line carrying the timer unit name, which distinguishes data rows from the header and summary lines directly. --- internal/systemd/systemd.go | 34 +++++++++++++++++++++++--------- internal/systemd/systemd_test.go | 27 +++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/internal/systemd/systemd.go b/internal/systemd/systemd.go index 989ba23..acdf184 100644 --- a/internal/systemd/systemd.go +++ b/internal/systemd/systemd.go @@ -290,9 +290,9 @@ func Query() (Status, error) { if fields[0] == "NEXT" { continue } - // Summary lines: "1 timers listed.", "Pass --all ...", "0 loaded ...". - // Skip anything whose first field doesn't look like a day-of-week. - if !isDayOfWeek(fields[0]) { + // Summary lines ("1 timers listed.", "Pass --all …") and the header + // never carry the unit name; data rows always do. + if !strings.Contains(line, TimerName) { continue } // We expect at least: len(rest) { + return + } + next = strings.Join(rest[:dows[0]-1], " ") + if rest[dows[0]-1] != timerPlaceholder { + left = rest[dows[0]-1] + } + last = strings.Join(rest[dows[0]:dows[0]+4], " ") + mid := rest[dows[0]+4:] + if len(mid) == 1 && mid[0] == timerPlaceholder { + mid = nil + } + passed = strings.Join(mid, " ") } case 2: if dows[1]+4 > len(rest) { diff --git a/internal/systemd/systemd_test.go b/internal/systemd/systemd_test.go index ed59c13..ca691e6 100644 --- a/internal/systemd/systemd_test.go +++ b/internal/systemd/systemd_test.go @@ -99,6 +99,33 @@ func TestSplitTimerFieldsMultiTokenPassed(t *testing.T) { } } +func TestSplitTimerFieldsPlaceholderNextRealLast(t *testing.T) { + f := []string{ + "-", "-", + "Wed", "2026-10-14", "20:00:55", "MDT", "23s", "ago", + "schooltools-archive.timer", "schooltools-archive.service", + } + next, left, last, passed, unit, activates, ok := splitTimerFields(f) + if !ok { + t.Fatal("expected ok for a placeholder-NEXT row") + } + if next != "-" { + t.Errorf("next = %q, want the placeholder", next) + } + if left != "" { + t.Errorf("left = %q, want empty", left) + } + if last != "Wed 2026-10-14 20:00:55 MDT" { + t.Errorf("last = %q, want the real last run", last) + } + if passed != "23s ago" { + t.Errorf("passed = %q", passed) + } + if unit != "schooltools-archive.timer" || activates != "schooltools-archive.service" { + t.Errorf("unit/activates = %q/%q", unit, activates) + } +} + func TestSplitTimerFieldsRejectsGarbage(t *testing.T) { for _, f := range [][]string{ {}, From 02d2db7c88e0f916097e8de599b7455d3bef5d21 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 21:41:36 -0600 Subject: [PATCH 14/27] docs: changelog updates for review-fix round (URL cleaning, session-expiry paths, redaction scope) --- CHANGELOG.md | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8b9ef1..bc1e5c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -338,13 +338,13 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). "skipped-missing-body" reporting is unchanged. - **P2:** D2L file-body URLs that contain unescaped path characters — literal spaces, a bare `%` (`Acetic Acid, 5% (Vinegar).pdf`), `#` in - filenames (`Quiz #3 …`) — either failed `url.Parse` with - `invalid URL escape` or were truncated at the `#` into a URL + filenames (`Quiz #3 …`), a literal `?` — either failed `url.Parse` + with `invalid URL escape` or were truncated at the `#` into a URL fragment and 404'd. The new `internal/urlutil` re-encodes the path - portion of a reference (preserving already-valid `%XX` sequences - and query strings) before it is resolved against the LMS base URL; - both `archive update`'s body downloads and `download` route - through it. + portion of a reference (preserving already-valid `%XX` sequences; + D2L file URLs never carry a query string, so `?` is a path + character) and applies the same cleaning to absolute URLs; both + `archive update`'s body downloads and `download` route through it. - **P2:** `systemd status` dropped `systemctl list-timers` data rows whose LAST column was a placeholder (a timer that never fired), so fresh installs printed no next-run time at all, and mis-reported @@ -366,10 +366,19 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `SAMLRequest` URL and raw `d2lSessionVal` / `d2lSecureSessionVal` cookie values to stderr (and into journald under the systemd unit), violating the threat model in `SECURITY.md`. `fetchPost` now - dumps only behind `--verbose`, SAML parameters - (`SAMLRequest`/`SAMLResponse`/`RelayState`) are replaced with a - length marker even in verbose mode, and cookie values are - truncated the same way the per-hop dump truncates them. + dumps only behind `--verbose`; every verbose URL print (request URL, + Referer, Location, redirect targets, POST form actions, GET + failures) redacts `SAMLRequest` / `SAMLResponse` / `RelayState` to + a length marker, and cookie values are redacted entirely rather + than truncated. +- **P1:** An expired session during an archive run was reported as + generic failures: the my-courses listing 401'd as a course-list + error, a topic request that redirected to the D2L login page + (HTTP 200 + `text/html`) archived the login HTML as the topic's + metadata blob via the non-JSON fallback, and a 401 on a file-body + download was counted as an ordinary body error so the run "succeeded" + with missing bodies. All three paths now map to `errSessionExpired` + and stop the run with `session expired; run 'schooltools login'`. ## [0.2.0] - 2026-09-05 From d2e378e94998f7e00ce5cc0dc2c9343498691050 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 22:42:16 -0600 Subject: [PATCH 15/27] fix(archive): bound network outages, reject login-page bodies, fix resolve errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups from CodeRabbit/cubic: - A transport-level failure (connection refused, DNS, timeout) is now wrapped as errTransport so archiveCourseV2 can tell it apart from a per-topic HTTP failure. A network outage no longer sends one doomed request per topic and exits 0: the pass aborts after 5 consecutive transport failures, in both the metadata and body loops. Per-topic HTTP failures (404 on a deleted topic) keep the existing continue behavior and do not consume the budget. - downloadFileBody now rejects a final login URL before reading the bytes: an expired session redirects the download to the D2L login page, which answers 200 with HTML that would otherwise be hashed and stored as the topic's body. Content-Type is deliberately not checked — D2L serves real .html topic files as text/html. - The resolve error message kept the resolved URL, not the input: "resolve https://…" was printed with the encoded value instead of what D2L actually supplied. - fetchTopicFor now closes the response body on its early returns; repeated 404s otherwise retain connections for the whole pass. - fakeD2L guards topicCalls with a mutex (the handler goroutine wrote it while the test read it) and gains a hijack knob so the transport path is exercised for real, plus tests: consecutive-transport abort, HTTP failures not consuming the budget, and a 403 TOC for both Run and Diff. --- internal/archive/archive.go | 51 ++++++++++++++++- internal/archive/archive_test.go | 63 ++++++++++++++++----- internal/archive/resilience_test.go | 87 ++++++++++++++++++++++++++++- internal/urlutil/urlutil.go | 16 ++++-- internal/urlutil/urlutil_test.go | 25 +++++++++ 5 files changed, 219 insertions(+), 23 deletions(-) diff --git a/internal/archive/archive.go b/internal/archive/archive.go index 7eba260..8cd5bdd 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -88,6 +88,25 @@ import ( var errSessionExpired = errors.New("session expired; run 'schooltools login'") +// errTransport marks a transport-level failure (connection refused, DNS +// failure, timeout, reset) rather than an HTTP status response. Callers +// use it to abort a pass that is only re-hitting a dead network, while +// continuing past per-topic HTTP failures such as a 404 on a deleted +// topic. +var errTransport = errors.New("transport failure") + +type transportError struct{ err error } + +func (e *transportError) Error() string { return fmt.Sprintf("transport: %v", e.err) } +func (e *transportError) Unwrap() error { return e.err } +func (e *transportError) Is(target error) bool { return target == errTransport } + +// maxConsecutiveTransportFailures bounds how many transport-level +// failures in a row archiveCourseV2 tolerates before aborting the run. +// Per-topic HTTP failures (a deleted topic, a stale TOC entry) do not +// count toward it. +const maxConsecutiveTransportFailures = 5 + const ( // SchemaVersion is bumped when the on-disk layout changes in a way that // would confuse older readers. v3 added bodies (blobs/.bin) and @@ -786,6 +805,7 @@ func archiveCourse(opts Options, c Course, now func() time.Time) (courseStats, e // archiveCourseV2 is the working implementation. func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, error) { var s courseStats + transportFails := 0 toc, err := fetchTocFor(opts.baseURL(), c.OrgUnitId, opts.Jar) if err != nil { return s, err @@ -846,12 +866,20 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, if errors.Is(ferr, errSessionExpired) { return s, ferr } + if errors.Is(ferr, errTransport) { + transportFails++ + if transportFails >= maxConsecutiveTransportFailures { + return s, fmt.Errorf("network unreachable: aborted after %d consecutive transport failures (last: %v)", + transportFails, ferr) + } + } s.failed++ if opts.Verbose { fmt.Fprintf(os.Stderr, "[archive] topic %d: fetch failed: %v (continuing)\n", q.topicID, ferr) } continue } + transportFails = 0 uuid, size, werr := SaveBlob(opts.Root, body) if werr != nil { return s, werr @@ -896,12 +924,20 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, if errors.Is(berr, errSessionExpired) { return s, berr } + if errors.Is(berr, errTransport) { + transportFails++ + if transportFails >= maxConsecutiveTransportFailures { + return s, fmt.Errorf("network unreachable: aborted after %d consecutive transport failures (last: %v)", + transportFails, berr) + } + } if opts.Verbose { fmt.Fprintf(os.Stderr, "[archive] body %d: %v (continuing)\n", q.topicID, berr) } s.bodiesErr++ continue } + transportFails = 0 sha := sha256.Sum256(bodyBytes) shaHex := hex.EncodeToString(sha[:]) // Re-save when the queued reason was "body-missing" (the existing @@ -978,15 +1014,16 @@ func downloadFileBody(jar *cookiejar.Jar, rawURL, base string) ([]byte, string, if base == "" { base = ua.D2LBase } - rawURL, rerr := urlutil.ResolveD2L(base, rawURL) + resolved, rerr := urlutil.ResolveD2L(base, rawURL) if rerr != nil { return nil, "", fmt.Errorf("resolve %s: %w", rawURL, rerr) } + rawURL = resolved res, err := httpclient.FollowRedirects(rawURL, jar, httpclient.FetchOptions{ Headers: map[string]string{"Accept": "*/*"}, }) if err != nil { - return nil, "", err + return nil, "", &transportError{err} } defer func() { _ = res.Body.Close() }() if res.StatusCode/100 != 2 { @@ -1000,6 +1037,13 @@ func downloadFileBody(jar *cookiejar.Jar, rawURL, base string) ([]byte, string, } return nil, "", fmt.Errorf("download %d %s for %s: %s", res.StatusCode, res.Status, rawURL, msg) } + // An expired session redirects the download to the D2L login page, + // which answers 200 with HTML; that page must never be hashed as a + // topic body. (Content-Type is deliberately not checked: D2L serves + // real .html topic files as text/html.) + if ua.IsLoginURL(res.Request.URL.String()) { + return nil, "", errSessionExpired + } body, err := io.ReadAll(res.Body) if err != nil { return nil, "", err @@ -1068,8 +1112,9 @@ func fetchTopicFor(base string, jar *cookiejar.Jar, orgUnitID string, topicID in Headers: map[string]string{"Accept": "application/json"}, }) if err != nil { - return nil, err + return nil, &transportError{err} } + defer func() { _ = res.Body.Close() }() if res.StatusCode != 200 { if res.StatusCode == 401 || res.StatusCode == 403 { return nil, errSessionExpired diff --git a/internal/archive/archive_test.go b/internal/archive/archive_test.go index 92a1c44..11dfc3f 100644 --- a/internal/archive/archive_test.go +++ b/internal/archive/archive_test.go @@ -11,6 +11,7 @@ import ( "reflect" "sort" "strings" + "sync" "testing" "time" @@ -101,8 +102,8 @@ func TestSortedCourseIDs(t *testing.T) { func TestParseD2LTime(t *testing.T) { cases := map[string]time.Time{ - "": time.Time{}, - "2026-01-02T03:04:05Z": time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC), + "": time.Time{}, + "2026-01-02T03:04:05Z": time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC), "2026-01-02T03:04:05.0000000Z": time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC), } for in, want := range cases { @@ -192,10 +193,10 @@ func TestCourseIndexRoundTrip(t *testing.T) { dir := t.TempDir() now := time.Now().UTC().Truncate(time.Second) ci := &CourseIndex{ - CourseID: "42", - Name: "Test Course", - Code: "TST 101", - IsActive: true, + CourseID: "42", + Name: "Test Course", + Code: "TST 101", + IsActive: true, UpdatedAt: now, Topics: map[int]*TopicIndex{ 100: { @@ -432,8 +433,10 @@ type fakeD2L struct { topics map[int]map[string]any topicRaw map[int]string topicStatus map[int]int - tocCalls int + tocStatus int + hijackTopics map[int]bool topicCalls map[int]int + topicMu sync.Mutex bodyHandler http.HandlerFunc // optional; serves file bodies on arbitrary paths } @@ -455,15 +458,30 @@ func newFakeD2L(t *testing.T) *fakeD2L { }) d2lver.Reset() return &fakeD2L{ - t: t, - tocs: map[string]content.TocResponse{}, - topics: map[int]map[string]any{}, - topicRaw: map[int]string{}, - topicStatus: map[int]int{}, - topicCalls: map[int]int{}, + t: t, + tocs: map[string]content.TocResponse{}, + topics: map[int]map[string]any{}, + topicRaw: map[int]string{}, + topicStatus: map[int]int{}, + hijackTopics: map[int]bool{}, + topicCalls: map[int]int{}, } } +// recordTopicCall counts a topics-API request. Called from the server +// goroutine; reads go through topicCallCount under the same mutex. +func (f *fakeD2L) recordTopicCall(id int) { + f.topicMu.Lock() + defer f.topicMu.Unlock() + f.topicCalls[id]++ +} + +func (f *fakeD2L) topicCallCount(id int) int { + f.topicMu.Lock() + defer f.topicMu.Unlock() + return f.topicCalls[id] +} + func (f *fakeD2L) handler() http.Handler { mux := http.NewServeMux() mux.HandleFunc("/d2l/le/manageCourses/api/mycourses", func(w http.ResponseWriter, r *http.Request) { @@ -483,7 +501,10 @@ func (f *fakeD2L) handler() http.Handler { course := parts[4] switch parts[6] { case "toc": - f.tocCalls++ + if f.tocStatus != 0 { + w.WriteHeader(f.tocStatus) + return + } toc, ok := f.tocs[course] if !ok { http.NotFound(w, r) @@ -497,7 +518,19 @@ func (f *fakeD2L) handler() http.Handler { return } id := atoi(parts[7]) - f.topicCalls[id]++ + f.recordTopicCall(id) + if f.hijackTopics[id] { + // Close the connection without a response so the + // client sees a transport-level error (EOF), not + // an HTTP status. + if hj, ok := w.(http.Hijacker); ok { + conn, _, herr := hj.Hijack() + if herr == nil { + _ = conn.Close() + } + } + return + } if code, ok := f.topicStatus[id]; ok { w.WriteHeader(code) return diff --git a/internal/archive/resilience_test.go b/internal/archive/resilience_test.go index 54a868f..53e6f2f 100644 --- a/internal/archive/resilience_test.go +++ b/internal/archive/resilience_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "testing" "github.com/aarav/schooltools/internal/content" @@ -44,7 +45,7 @@ func TestRunSurvivesBrokenAndFailingTopics(t *testing.T) { if res.TopicsFailed != 1 { t.Errorf("TopicsFailed=%d, want 1 (404 topic)", res.TopicsFailed) } - if calls := f.topicCalls[6002]; calls != 0 { + if calls := f.topicCallCount(6002); calls != 0 { t.Errorf("broken topic 6002 was fetched %d times, want 0 (must be skipped without an API call)", calls) } @@ -71,6 +72,90 @@ func TestRunSurvivesBrokenAndFailingTopics(t *testing.T) { } } +func TestRunAbortsAfterConsecutiveTransportFailures(t *testing.T) { + f := newFakeD2L(t) + var topics []content.TocTopic + for i := 7001; i <= 7006; i++ { + topics = append(topics, content.TocTopic{TopicId: i, Title: "T", LastModifiedDate: "2026-03-01T00:00:00Z"}) + f.hijackTopics[i] = true + } + f.courses = []d2lCourse{{OrgUnitId: json.Number("4700"), Name: "Bio", Code: "BIO", IsActive: true}} + f.tocs["4700"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: topics}}} + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + _, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL}) + if err == nil { + t.Fatal("expected the run to abort on consecutive transport failures") + } + if !strings.Contains(err.Error(), "consecutive transport failures") { + t.Errorf("err = %v, want a consecutive-transport-failure abort", err) + } + // Six topics were queued; the pass must stop at the bound, not + // grind through all of them during an outage. + attempts := 0 + for i := 7001; i <= 7006; i++ { + attempts += f.topicCallCount(i) + } + if attempts > maxConsecutiveTransportFailures { + t.Errorf("%d topic attempts, want at most %d", attempts, maxConsecutiveTransportFailures) + } +} + +func TestRunToleratesHTTPFailuresAfterTransportBounded(t *testing.T) { + // HTTP failures must not consume the transport budget: a mix of + // 404s and one transport error keeps the pass going. + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("4701"), Name: "Bio", Code: "BIO", IsActive: true}} + f.tocs["4701"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: []content.TocTopic{ + {TopicId: 7101, Title: "Deleted", LastModifiedDate: "2026-03-01T00:00:00Z"}, + {TopicId: 7102, Title: "Deleted", LastModifiedDate: "2026-03-01T00:00:00Z"}, + {TopicId: 7103, Title: "Deleted", LastModifiedDate: "2026-03-01T00:00:00Z"}, + {TopicId: 7104, Title: "Deleted", LastModifiedDate: "2026-03-01T00:00:00Z"}, + {TopicId: 7105, Title: "Deleted", LastModifiedDate: "2026-03-01T00:00:00Z"}, + {TopicId: 7106, Title: "Deleted", LastModifiedDate: "2026-03-01T00:00:00Z"}, + {TopicId: 7107, Title: "Deleted", LastModifiedDate: "2026-03-01T00:00:00Z"}, + }}}} + f.hijackTopics[7107] = true + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + res, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL}) + if err != nil { + t.Fatalf("HTTP failures must not abort the pass: %v", err) + } + if res.TopicsFailed != 7 { + t.Errorf("TopicsFailed=%d, want 7", res.TopicsFailed) + } +} + +func TestRunAbortsOnExpiredToc(t *testing.T) { + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("4610"), Name: "Bio", Code: "BIO", IsActive: true}} + f.tocs["4610"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: []content.TocTopic{ + {TopicId: 7201, Title: "Cells", LastModifiedDate: "2026-03-01T00:00:00Z"}, + }}}} + f.tocStatus = http.StatusForbidden + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + _, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL}) + if err == nil { + t.Fatal("expected the run to abort when the TOC request 403s") + } + if !errors.Is(err, errSessionExpired) { + t.Errorf("Run err = %v, want errSessionExpired", err) + } + + _, derr := Diff(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL, CourseIDs: []string{"4610"}}) + if derr == nil { + t.Fatal("expected Diff to abort when the TOC request 403s") + } + if !errors.Is(derr, errSessionExpired) { + t.Errorf("Diff err = %v, want errSessionExpired", derr) + } +} + func TestRunAbortsOnSessionExpired(t *testing.T) { f := newFakeD2L(t) f.courses = []d2lCourse{{OrgUnitId: json.Number("555"), Name: "Bio", Code: "BIO", IsActive: true}} diff --git a/internal/urlutil/urlutil.go b/internal/urlutil/urlutil.go index f461e91..75404e2 100644 --- a/internal/urlutil/urlutil.go +++ b/internal/urlutil/urlutil.go @@ -11,9 +11,13 @@ import ( // ResolveD2L resolves a D2L content URL against a base URL. Relative // references and absolute URLs are both percent-encoded first, so that -// url.Parse cannot fail on them and no character is silently -// reinterpreted as a URL fragment or query delimiter. A '?' is treated -// as a path character: D2L file URLs never carry a query string. +// no character is silently reinterpreted as a URL fragment or query +// delimiter. A '?' is treated as a path character: D2L file URLs never +// carry a query string. Relative references that are not already +// path-absolute (no leading '/', './' or '../') are resolved against the +// base's directory with a './' prefix, which keeps url.Parse from +// mistaking a colon in the first segment ("Week 1: Intro/…") for a URL +// scheme. func ResolveD2L(base, raw string) (string, error) { if isAbs(raw) { return cleanAbs(raw), nil @@ -22,7 +26,11 @@ func ResolveD2L(base, raw string) (string, error) { if err != nil { return "", fmt.Errorf("parse base: %w", err) } - ref, err := url.Parse(CleanPath(raw)) + p := CleanPath(raw) + if !strings.HasPrefix(p, "/") && !strings.HasPrefix(p, "./") && !strings.HasPrefix(p, "../") { + p = "./" + p + } + ref, err := url.Parse(p) if err != nil { return "", fmt.Errorf("parse %s: %w", raw, err) } diff --git a/internal/urlutil/urlutil_test.go b/internal/urlutil/urlutil_test.go index 2d9a19f..e1ce67b 100644 --- a/internal/urlutil/urlutil_test.go +++ b/internal/urlutil/urlutil_test.go @@ -62,6 +62,31 @@ func TestResolveD2LHashFilename(t *testing.T) { } } +func TestResolveD2LRelativeRefWithColon(t *testing.T) { + // url.Parse rejects a reference whose first segment contains a colon + // ("Week 1: Intro/…" looks like a scheme). The './' prefix keeps the + // colon inside a path segment. + got, err := ResolveD2L("https://d2l.cbe.ab.ca/d2l/le/content/123/view", "Week 1: Intro/notes.pdf") + if err != nil { + t.Fatalf("ResolveD2L: %v", err) + } + want := "https://d2l.cbe.ab.ca/d2l/le/content/123/Week%201:%20Intro/notes.pdf" + if got != want { + t.Errorf("ResolveD2L = %q, want %q", got, want) + } +} + +func TestResolveD2LRelativeRefWithSpaceBeforeColon(t *testing.T) { + got, err := ResolveD2L("https://d2l.cbe.ab.ca", "Unit 3: Ratios.pdf") + if err != nil { + t.Fatalf("ResolveD2L: %v", err) + } + want := "https://d2l.cbe.ab.ca/Unit%203:%20Ratios.pdf" + if got != want { + t.Errorf("ResolveD2L = %q, want %q", got, want) + } +} + func TestResolveD2LAbsoluteURLIsCleaned(t *testing.T) { got, err := ResolveD2L("https://d2l.cbe.ab.ca", "https://files.example.com/a b#c.pdf") if err != nil { From a3adc6740d2d2c795b11d4d6954eb97146f89e44 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 22:42:32 -0600 Subject: [PATCH 16/27] fix(archive): only treat a stat ErrNotExist as a missing body blob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fileExists returned false for any os.Stat error, so a permission or I/O error reported a present body as missing — LoadStatus, FileTopics and bodyNeeded all read that as "fetch it again". Only positive non-existence now counts as missing. LoadStatus.BodiesPresent also counted non-File topics that still hold a body while FileTopics/MissingBodies only cover file-compatible topics, so the two halves of the status line disagreed. The on-disk check is now restricted to the same topic set. --- internal/archive/store.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/archive/store.go b/internal/archive/store.go index 5971e48..845d33a 100644 --- a/internal/archive/store.go +++ b/internal/archive/store.go @@ -1,7 +1,9 @@ package archive import ( + "errors" "fmt" + "io/fs" "os" "path/filepath" "sort" @@ -367,7 +369,9 @@ func fileExists(p string) bool { return false } _, err := os.Stat(p) - return err == nil + // A permission or I/O error means the blob might well be there; + // only positive non-existence counts as missing. + return err == nil || !errors.Is(err, fs.ErrNotExist) } type Status struct { @@ -440,7 +444,11 @@ func LoadStatus(root string) (Status, error) { if t.Type == "" || t.Type == "File" { st.FileTopics++ } - if t.CurrentBody != "" && fileExists(BodyPath(root, t.CurrentBody)) { + // Only file-compatible topics are counted here so + // BodiesPresent matches MissingBodies' denominator: a + // non-File topic that still holds a body is reported by + // FileTopics/MissingBodies as missing. + if t.CurrentBody != "" && (t.Type == "" || t.Type == "File") && fileExists(BodyPath(root, t.CurrentBody)) { st.BodiesPresent++ } } From 86d33a613b4320773ea21edd2b7590f93c42d0eb Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 22:44:18 -0600 Subject: [PATCH 17/27] fix(systemd): report real removal errors, parse the whole LEFT field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Uninstall discarded both os.Remove errors, so a permission error left the unit files on disk while the caller was told everything was removed. Only os.ErrNotExist is ignored now; anything else is joined and returned. Uninstall is also a no-op again when nothing was installed — it no longer runs daemon-reload just to surface that command's error. splitTimerFields took LEFT as a single token, but systemctl prints it as display text ("29min left", "1h 42min left"): the word "left" was dropped and, with a multi-token LEFT, the parse could mis-align. LEFT is now the token range up to the next known column, joined with spaces. Placeholders are "-" everywhere but "n/a" on systemd 249+, so both are recognized, and an all-placeholder NEXT normalizes to empty so status renders no "Next run: n/a (in )" line. --- internal/systemd/systemd.go | 85 ++++++++++++++++++++++++++------ internal/systemd/systemd_test.go | 26 +++++----- 2 files changed, 83 insertions(+), 28 deletions(-) diff --git a/internal/systemd/systemd.go b/internal/systemd/systemd.go index acdf184..91d9329 100644 --- a/internal/systemd/systemd.go +++ b/internal/systemd/systemd.go @@ -6,6 +6,7 @@ package systemd import ( "embed" + "errors" "fmt" "os" "os/exec" @@ -224,11 +225,36 @@ func Uninstall() (Status, error) { svcPath := filepath.Join(dir, ServiceName) tmrPath := filepath.Join(dir, TimerName) - _ = os.Remove(svcPath) - _ = os.Remove(tmrPath) + removed := false + remove := func(p string) error { + err := os.Remove(p) + switch { + case err == nil: + removed = true + return nil + case errors.Is(err, os.ErrNotExist): + return nil + default: + return err + } + } + var rmErr error + if err := remove(svcPath); err != nil { + rmErr = errors.Join(rmErr, err) + } + if err := remove(tmrPath); err != nil { + rmErr = errors.Join(rmErr, err) + } + if !removed && rmErr == nil { + // Nothing was installed: leave systemctl alone entirely. + return Status{UserDir: dir}, nil + } if _, err := runSystemctl("daemon-reload"); err != nil { return Status{}, fmt.Errorf("daemon-reload after uninstall failed: %w", err) } + if rmErr != nil { + return Status{UserDir: dir}, fmt.Errorf("remove unit files: %w", rmErr) + } return Status{UserDir: dir}, nil } @@ -267,8 +293,8 @@ func Query() (Status, error) { // Parse `list-timers` output. The header line lists columns: // NEXT LEFT LAST PASSED UNIT ACTIVATES // and each data line is a fixed-width-ish sequence of: - // - // + // + // // // We don't try to be clever about column widths — we just locate the // header, then for each non-header, non-blank, non-summary line, pull @@ -313,17 +339,23 @@ func Query() (Status, error) { return st, nil } -const timerPlaceholder = "-" +// isTimerPlaceholder reports whether a list-timers token is an empty +// column. systemctl prints "-" on most versions and "n/a" on systemd +// 249+ for empty NEXT/LEFT/LAST/PASSED columns. +func isTimerPlaceholder(s string) bool { + return s == "-" || s == "n/a" +} // splitTimerFields groups a token slice from a `list-timers` data row into // the six semantic columns. The row layout is: // -// +// // // The final two tokens are always UNIT and ACTIVATES, so they anchor the // parse. LAST and PASSED are placeholders until the timer has fired at // least once; a placeholder PASSED must not shift the UNIT/ACTIVATES -// columns. +// columns. LEFT is human display text ("29min left"), not a fixed +// token count, so its slice runs up to the next known column. func splitTimerFields(f []string) (next, left, last, passed, unit, activates string, ok bool) { if len(f) < 6 { return @@ -340,24 +372,31 @@ func splitTimerFields(f []string) (next, left, last, passed, unit, activates str switch len(dows) { case 1: if dows[0] == 0 { - if len(rest) < 5 { + // NEXT is real; LAST and PASSED are single-token + // placeholders, so LEFT runs from the end of the NEXT + // datetime up to those two trailing tokens. + if len(rest) < 7 { return } next = strings.Join(rest[:4], " ") - if rest[4] != timerPlaceholder { - left = rest[4] + if l := rest[4 : len(rest)-2]; !allPlaceholders(l) { + left = strings.Join(l, " ") } } else { + // NEXT is a placeholder (timer with no next elapse); + // the sole DOW token starts LAST. if dows[0]+4 > len(rest) { return } - next = strings.Join(rest[:dows[0]-1], " ") - if rest[dows[0]-1] != timerPlaceholder { + if toks := rest[:dows[0]-1]; !allPlaceholders(toks) { + next = strings.Join(toks, " ") + } + if dows[0] >= 2 && !isTimerPlaceholder(rest[dows[0]-1]) { left = rest[dows[0]-1] } last = strings.Join(rest[dows[0]:dows[0]+4], " ") mid := rest[dows[0]+4:] - if len(mid) == 1 && mid[0] == timerPlaceholder { + if len(mid) == 1 && isTimerPlaceholder(mid[0]) { mid = nil } passed = strings.Join(mid, " ") @@ -367,12 +406,12 @@ func splitTimerFields(f []string) (next, left, last, passed, unit, activates str return } next = strings.Join(rest[:4], " ") - if rest[4] != timerPlaceholder { - left = rest[4] + if l := rest[4:dows[1]]; !allPlaceholders(l) { + left = strings.Join(l, " ") } last = strings.Join(rest[dows[1]:dows[1]+4], " ") mid := rest[dows[1]+4:] - if len(mid) == 1 && mid[0] == timerPlaceholder { + if len(mid) == 1 && isTimerPlaceholder(mid[0]) { mid = nil } passed = strings.Join(mid, " ") @@ -383,6 +422,20 @@ func splitTimerFields(f []string) (next, left, last, passed, unit, activates str return } +// allPlaceholders reports whether every token is an empty-column marker +// (or the slice is empty), i.e. the column carries no real value. +func allPlaceholders(toks []string) bool { + if len(toks) == 0 { + return true + } + for _, t := range toks { + if !isTimerPlaceholder(t) { + return false + } + } + return true +} + // splitNonEmptyLines trims trailing whitespace and drops empty lines. func splitNonEmptyLines(s string) []string { raw := strings.Split(strings.TrimRight(s, "\n"), "\n") diff --git a/internal/systemd/systemd_test.go b/internal/systemd/systemd_test.go index ca691e6..3aeb53c 100644 --- a/internal/systemd/systemd_test.go +++ b/internal/systemd/systemd_test.go @@ -8,7 +8,7 @@ import ( func TestSplitTimerFieldsBothDatetimes(t *testing.T) { f := []string{ - "Wed", "2026-10-14", "20:30:55", "MDT", "29min", + "Wed", "2026-10-14", "20:30:55", "MDT", "29min", "left", "Wed", "2026-10-14", "20:00:55", "MDT", "23s", "ago", "schooltools-archive.timer", "schooltools-archive.service", } @@ -19,8 +19,8 @@ func TestSplitTimerFieldsBothDatetimes(t *testing.T) { if next != "Wed 2026-10-14 20:30:55 MDT" { t.Errorf("next = %q", next) } - if left != "29min" { - t.Errorf("left = %q", left) + if left != "29min left" { + t.Errorf("left = %q, want the full multi-token LEFT field", left) } if last != "Wed 2026-10-14 20:00:55 MDT" { t.Errorf("last = %q", last) @@ -35,8 +35,8 @@ func TestSplitTimerFieldsBothDatetimes(t *testing.T) { func TestSplitTimerFieldsNeverFired(t *testing.T) { f := []string{ - "Wed", "2026-10-14", "21:00:00", "MDT", "30min", - "-", "-", + "Wed", "2026-10-14", "21:00:00", "MDT", "30min", "left", + "n/a", "n/a", "schooltools-archive.timer", "schooltools-archive.service", } next, left, last, passed, unit, activates, ok := splitTimerFields(f) @@ -46,8 +46,8 @@ func TestSplitTimerFieldsNeverFired(t *testing.T) { if next != "Wed 2026-10-14 21:00:00 MDT" { t.Errorf("next = %q, want the upcoming run", next) } - if left != "30min" { - t.Errorf("left = %q", left) + if left != "30min left" { + t.Errorf("left = %q, want the full multi-token LEFT field", left) } if last != "" || passed != "" { t.Errorf("last/passed = %q/%q, want empty", last, passed) @@ -59,7 +59,7 @@ func TestSplitTimerFieldsNeverFired(t *testing.T) { func TestSplitTimerFieldsPlaceholderPassed(t *testing.T) { f := []string{ - "Wed", "2026-10-14", "20:30:55", "MDT", "29min", + "Wed", "2026-10-14", "20:30:55", "MDT", "29min", "left", "Wed", "2026-10-14", "20:00:55", "MDT", "-", "schooltools-archive.timer", "schooltools-archive.service", } @@ -67,7 +67,7 @@ func TestSplitTimerFieldsPlaceholderPassed(t *testing.T) { if !ok { t.Fatal("expected ok") } - if next != "Wed 2026-10-14 20:30:55 MDT" || left != "29min" { + if next != "Wed 2026-10-14 20:30:55 MDT" || left != "29min left" { t.Errorf("next/left = %q/%q", next, left) } if last != "Wed 2026-10-14 20:00:55 MDT" { @@ -101,7 +101,7 @@ func TestSplitTimerFieldsMultiTokenPassed(t *testing.T) { func TestSplitTimerFieldsPlaceholderNextRealLast(t *testing.T) { f := []string{ - "-", "-", + "n/a", "n/a", "Wed", "2026-10-14", "20:00:55", "MDT", "23s", "ago", "schooltools-archive.timer", "schooltools-archive.service", } @@ -109,8 +109,10 @@ func TestSplitTimerFieldsPlaceholderNextRealLast(t *testing.T) { if !ok { t.Fatal("expected ok for a placeholder-NEXT row") } - if next != "-" { - t.Errorf("next = %q, want the placeholder", next) + // A placeholder NEXT must normalize to empty so status rendering + // prints no "Next run:" line at all instead of "Next run: n/a". + if next != "" { + t.Errorf("next = %q, want empty for a placeholder", next) } if left != "" { t.Errorf("left = %q, want empty", left) From 2859c6d9f4babd3d4ff5a59eb6609eabc5415a0c Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 22:44:40 -0600 Subject: [PATCH 18/27] test(login): panic-safe stderr capture, atomic request counter captureStderr restored os.Stderr with a plain statement, so a panic inside fn left stderr redirected for the rest of the test binary, and it read the pipe only after fn returned, so a write larger than the pipe buffer deadlocked. The writer now closes and stderr restores in a deferred func, and the reader drains concurrently. The httptest handler incremented a plain int the test goroutine read back, which -race flags. The counter is an atomic.Int32 now. --- internal/login/login_test.go | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/internal/login/login_test.go b/internal/login/login_test.go index e26cf86..b4c0acb 100644 --- a/internal/login/login_test.go +++ b/internal/login/login_test.go @@ -7,6 +7,7 @@ import ( "net/url" "os" "strings" + "sync/atomic" "testing" "github.com/aarav/schooltools/internal/httpclient" @@ -20,18 +21,29 @@ func captureStderr(t *testing.T, fn func()) string { t.Fatalf("pipe: %v", err) } os.Stderr = w - fn() - _ = w.Close() - os.Stderr = old - data, _ := io.ReadAll(r) - return string(data) + // Drain the pipe concurrently so a fn that writes more than the + // pipe buffer cannot block, and restore os.Stderr even if fn + // panics or calls t.Fatal. + done := make(chan string) + go func() { + data, _ := io.ReadAll(r) + done <- string(data) + }() + func() { + defer func() { + _ = w.Close() + os.Stderr = old + }() + fn() + }() + return <-done } func TestFetchPostDumpsOnlyWhenVerbose(t *testing.T) { secret := "supersecretvalue-but-very-long-cookie-tail" - requests := 0 + var requests atomic.Int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - requests++ + requests.Add(1) http.SetCookie(w, &http.Cookie{Name: "d2lSessionVal", Value: secret, Path: "/"}) w.WriteHeader(http.StatusOK) })) @@ -54,8 +66,8 @@ func TestFetchPostDumpsOnlyWhenVerbose(t *testing.T) { if out != "" { t.Errorf("fetchPost without --verbose wrote to stderr: %q", out) } - if requests != 1 { - t.Errorf("server saw %d requests, want 1 (the non-verbose POST must still be sent)", requests) + if got := requests.Load(); got != 1 { + t.Errorf("server saw %d requests, want 1 (the non-verbose POST must still be sent)", got) } out = captureStderr(t, func() { @@ -69,8 +81,8 @@ func TestFetchPostDumpsOnlyWhenVerbose(t *testing.T) { if strings.Contains(out, secret) { t.Errorf("verbose fetchPost leaked the full cookie value: %q", out) } - if requests != 2 { - t.Errorf("server saw %d requests, want 2", requests) + if got := requests.Load(); got != 2 { + t.Errorf("server saw %d requests, want 2", got) } } From fd9217e49f1d45ac8eb80ebc66c1ee5dd3ab6fe7 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Wed, 14 Oct 2026 22:46:16 -0600 Subject: [PATCH 19/27] fix(cmd): say so when only broken topics remain, add the dry-run Broken column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A --dry-run whose only leftovers are topics D2L flags broken printed "Nothing to do — your archive is up to date", which is not true: those topics will never be archived. It now prints a distinct no-fetchable-work line naming the broken topics. The dry-run per-course table counted broken topics in Total without a Broken column, so the row columns did not add up; the column is there now. Also adds a table test for resolveFileURL, the download route's URL handling (spaces, %, #, an existing escape, a relative reference with a colon, an absolute URL, and the empty string), matching the CHANGELOG claim that both routes were fixed. --- CHANGELOG.md | 41 ++++++++++++++++--------- cmd/archive.go | 11 +++++-- cmd/download_test.go | 61 +++++++++++++++++++++++++++++++++++++ internal/archive/archive.go | 4 +-- 4 files changed, 98 insertions(+), 19 deletions(-) create mode 100644 cmd/download_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index bc1e5c1..c3dcfa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -320,17 +320,23 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). (`IsBroken: true`, 404 on the topics API) — one dead 2022 dropbox link in a single course blocked every course queued after it from archiving. Topics D2L flags broken are now skipped without an API - call, failed fetches are counted and retried on the next pass, and - only a 401/403 stops the run (reported as `session expired; run - 'schooltools login'` instead of a raw status line). The update - summary and `--json` envelope gain `topicsFailed` / `topicsSkipped` - counters, and `--dry-run` reports broken topics so the plan matches - what a real pass does. + call and topic-level fetch failures are counted and retried on the + next pass; a 401/403 (or a login-page redirect) stops the run + directly with `session expired; run 'schooltools login'`. A burst of + consecutive transport failures (network down, DNS dead) now aborts + the pass too instead of sending one doomed request per topic and + reporting success — HTTP failures such as a 404 on a deleted topic + keep the continue behavior. Course-level fetch and storage errors + still stop the run. The update summary and `--json` envelope gain + `topicsFailed` / `topicsSkipped` counters, and `--dry-run` reports + broken topics so the plan matches what a real pass does. - **P1:** `archive` status and `archive find` trusted the index's `currentBody` pointer instead of the filesystem, so a store with - 186 dangling pointers (see the 0.2.0 prune entry) reported - "Bodies: 326 present, 8 missing" while 186 blobs were actually - gone. `LoadStatus` now stats each blob; `find --with-bodies` gained + 186 dangling pointers (from the `prune` entry under + Unreleased) reported "Bodies: 326 present, 8 missing" while + 186 blobs were actually gone. `LoadStatus` now stats each blob + (a stat error other than "not exist" no longer counts as + missing); `find --with-bodies` gained the `WithBodiesOnDisk` filter and `find --missing-bodies` also matches topics whose pointer exists but whose blob does not. `HasBody` on search hits and `FileTopic` reflects on-disk state. @@ -350,7 +356,11 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). fresh installs printed no next-run time at all, and mis-reported UNIT/ACTIVATES when PASSED was a placeholder. The parser now anchors on the trailing unit columns and treats placeholders - explicitly. + explicitly — both `-` and systemd 249's `n/a` — keeps the whole + multi-token LEFT field (`29min left`), and renders no next-run line + at all when the timer has no future elapse. `Uninstall` also + reports removal errors instead of silently claiming success, and is + a no-op again when nothing was installed. - **P2:** `systemd install` printed the volatile-binary-path warning straight to stderr instead of routing it through `Status.Warnings` (invisible to `--json`); `Uninstall` built a `removed` slice it @@ -358,6 +368,7 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). flagged by the legacy-ExecStart warning. ### Security + - **P1:** The login/auto-refresh HTTP debug dump (`<< Set-Cookie: …` values, request URLs) was printed **unconditionally** on every form POST — `fetchPost` didn't take `Options`, and every auto-refresh @@ -375,10 +386,12 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). generic failures: the my-courses listing 401'd as a course-list error, a topic request that redirected to the D2L login page (HTTP 200 + `text/html`) archived the login HTML as the topic's - metadata blob via the non-JSON fallback, and a 401 on a file-body - download was counted as an ordinary body error so the run "succeeded" - with missing bodies. All three paths now map to `errSessionExpired` - and stop the run with `session expired; run 'schooltools login'`. + metadata blob via the non-JSON fallback, a redirected file-body + download that ended at the login page hashed that HTML as the + topic's body, and a 401 on a body download was counted as an + ordinary body error so the run "succeeded" with missing bodies. + All of these paths now map to `errSessionExpired` and stop the run + with `session expired; run 'schooltools login'`. ## [0.2.0] - 2026-09-05 diff --git a/cmd/archive.go b/cmd/archive.go index 29ba559..716f39a 100644 --- a/cmd/archive.go +++ b/cmd/archive.go @@ -402,7 +402,11 @@ func runArchiveUpdateDryRun(args []string, abs string) error { fmt.Printf("Bodies: %d would fill (convergence)\n", result.BodiesWouldFetch) } if result.TopicsWouldFetch == 0 && result.BodiesWouldFetch == 0 { - fmt.Println("Nothing to do — your archive is up to date.") + if result.TopicsBroken > 0 { + fmt.Println("Nothing to fetch — every remaining topic is flagged broken on D2L (see above); they will never be archived.") + } else { + fmt.Println("Nothing to do — your archive is up to date.") + } } else { fmt.Println("Re-run without --dry-run to apply.") } @@ -426,6 +430,7 @@ func printUpdateDryRunTable(result archive.DiffResult) { fmt.Sprintf("%d", c.TopicsNew), fmt.Sprintf("%d", c.TopicsModified), fmt.Sprintf("%d", c.TopicsUnchanged), + fmt.Sprintf("%d", c.TopicsBroken), fetch, bodies, }) } @@ -434,11 +439,11 @@ func printUpdateDryRunTable(result archive.DiffResult) { return } out := table.Render(table.Options{ - Headers: []string{"ID", "Code", "Name", "Total", "New", "Modified", "Unchanged", "WouldFetch", "Bodies"}, + Headers: []string{"ID", "Code", "Name", "Total", "New", "Modified", "Unchanged", "Broken", "WouldFetch", "Bodies"}, Widths: table.Widths([]table.ColumnSpec{ table.Fixed(8), table.Fixed(8), table.Flex(20), table.Fixed(6), table.Fixed(5), table.Fixed(9), - table.Fixed(10), table.Fixed(10), table.Flex(8), + table.Fixed(10), table.Fixed(6), table.Fixed(10), table.Flex(8), }, table.TerminalWidth()), Rows: rows, }) diff --git a/cmd/download_test.go b/cmd/download_test.go new file mode 100644 index 0000000..263f0b7 --- /dev/null +++ b/cmd/download_test.go @@ -0,0 +1,61 @@ +package cmd + +import ( + "testing" +) + +func TestResolveFileURL(t *testing.T) { + cases := []struct { + name string + in string + want string + wantErr bool + }{ + { + name: "relative enforced path with space and percent", + in: "/content/enforced/1522695-x/Acetic Acid, 5% (Vinegar).pdf", + want: "https://d2l.cbe.ab.ca/content/enforced/1522695-x/Acetic%20Acid,%205%25%20(Vinegar).pdf", + }, + { + name: "hash in filename becomes an escape, not a fragment", + in: "/content/enforced/1527886-x/Quiz #3 Chemistry.pdf", + want: "https://d2l.cbe.ab.ca/content/enforced/1527886-x/Quiz%20%233%20Chemistry.pdf", + }, + { + name: "existing escape preserved", + in: "/content/enforced/1522695-x/KEY%20Practice.pdf", + want: "https://d2l.cbe.ab.ca/content/enforced/1522695-x/KEY%20Practice.pdf", + }, + { + name: "relative reference with a colon in the first segment", + in: "Week 1: Intro/notes.pdf", + want: "https://d2l.cbe.ab.ca/Week%201:%20Intro/notes.pdf", + }, + { + name: "absolute URL is cleaned too", + in: "https://files.example.com/a b#c.pdf", + want: "https://files.example.com/a%20b%23c.pdf", + }, + { + name: "empty URL", + in: "", + wantErr: true, + }, + } + for _, tc := range cases { + got, err := resolveFileURL(tc.in) + if tc.wantErr { + if err == nil { + t.Errorf("%s: resolveFileURL(%q) = %q, want an error", tc.name, tc.in, got) + } + continue + } + if err != nil { + t.Errorf("%s: resolveFileURL(%q): %v", tc.name, tc.in, err) + continue + } + if got != tc.want { + t.Errorf("%s: resolveFileURL(%q) = %q, want %q", tc.name, tc.in, got, tc.want) + } + } +} diff --git a/internal/archive/archive.go b/internal/archive/archive.go index 8cd5bdd..7ab3204 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -97,8 +97,8 @@ var errTransport = errors.New("transport failure") type transportError struct{ err error } -func (e *transportError) Error() string { return fmt.Sprintf("transport: %v", e.err) } -func (e *transportError) Unwrap() error { return e.err } +func (e *transportError) Error() string { return fmt.Sprintf("transport: %v", e.err) } +func (e *transportError) Unwrap() error { return e.err } func (e *transportError) Is(target error) bool { return target == errTransport } // maxConsecutiveTransportFailures bounds how many transport-level From 0dda22c2b37f5dcbd8214928460da974de52f11c Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 15 Oct 2026 19:25:19 -0600 Subject: [PATCH 20/27] fix(archive): persist the course index when a course pass aborts A fatal return from the topic/body loops skipped SaveCourseIndex, so every topic fetched earlier in that pass was written as a blob and then forgotten: unreferenced blobs in an append-only store, and index updates dropped on the floor. Both abort paths (session expired, consecutive transport failures) now persist the accumulated index first, joining any save error into the abort. TestRunAbortsAfterConsecutiveTransportFailures now fetches two topics cleanly before the outage and asserts both survive in the index while the unreachable ones stay absent. --- internal/archive/archive.go | 24 +++++++++++++++----- internal/archive/resilience_test.go | 35 +++++++++++++++++++++++------ 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/internal/archive/archive.go b/internal/archive/archive.go index 7ab3204..39ce1ee 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -819,6 +819,18 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, ci.Code = c.Code ci.IsActive = c.IsActive + // abortWithIndex persists whatever the pass has already archived + // before returning a fatal error. The store is append-only at the + // blob layer, so dropping the index would leave blobs on disk that + // nothing points at and silently discard the topics fetched so far. + abortWithIndex := func(cause error) (courseStats, error) { + ci.UpdatedAt = now() + if serr := SaveCourseIndex(opts.Root, c.OrgUnitId, ci); serr != nil { + return s, errors.Join(cause, serr) + } + return s, cause + } + type queued struct { topicID int topic content.TocTopic @@ -864,13 +876,13 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, body, ferr := fetchTopicFor(opts.baseURL(), opts.Jar, c.OrgUnitId, q.topicID) if ferr != nil { if errors.Is(ferr, errSessionExpired) { - return s, ferr + return abortWithIndex(ferr) } if errors.Is(ferr, errTransport) { transportFails++ if transportFails >= maxConsecutiveTransportFailures { - return s, fmt.Errorf("network unreachable: aborted after %d consecutive transport failures (last: %v)", - transportFails, ferr) + return abortWithIndex(fmt.Errorf("network unreachable: aborted after %d consecutive transport failures (last: %v)", + transportFails, ferr)) } } s.failed++ @@ -922,13 +934,13 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, bodyBytes, contentType, berr := downloadFileBody(opts.Jar, bodyURL, opts.baseURL()) if berr != nil { if errors.Is(berr, errSessionExpired) { - return s, berr + return abortWithIndex(berr) } if errors.Is(berr, errTransport) { transportFails++ if transportFails >= maxConsecutiveTransportFailures { - return s, fmt.Errorf("network unreachable: aborted after %d consecutive transport failures (last: %v)", - transportFails, berr) + return abortWithIndex(fmt.Errorf("network unreachable: aborted after %d consecutive transport failures (last: %v)", + transportFails, berr)) } } if opts.Verbose { diff --git a/internal/archive/resilience_test.go b/internal/archive/resilience_test.go index 53e6f2f..1b5c685 100644 --- a/internal/archive/resilience_test.go +++ b/internal/archive/resilience_test.go @@ -75,8 +75,13 @@ func TestRunSurvivesBrokenAndFailingTopics(t *testing.T) { func TestRunAbortsAfterConsecutiveTransportFailures(t *testing.T) { f := newFakeD2L(t) var topics []content.TocTopic - for i := 7001; i <= 7006; i++ { - topics = append(topics, content.TocTopic{TopicId: i, Title: "T", LastModifiedDate: "2026-03-01T00:00:00Z"}) + // Two topics fetch cleanly first, then the network goes away. + for i := 7001; i <= 7002; i++ { + topics = append(topics, content.TocTopic{TopicId: i, Title: "Good", LastModifiedDate: "2026-03-01T00:00:00Z"}) + f.topics[i] = map[string]any{"TopicId": i, "Title": "Good"} + } + for i := 7003; i <= 7008; i++ { + topics = append(topics, content.TocTopic{TopicId: i, Title: "Gone", LastModifiedDate: "2026-03-01T00:00:00Z"}) f.hijackTopics[i] = true } f.courses = []d2lCourse{{OrgUnitId: json.Number("4700"), Name: "Bio", Code: "BIO", IsActive: true}} @@ -84,21 +89,37 @@ func TestRunAbortsAfterConsecutiveTransportFailures(t *testing.T) { srv := httptest.NewServer(f.handler()) defer srv.Close() - _, err := Run(Options{Root: t.TempDir(), Jar: newJar(t), BaseURL: srv.URL}) + dir := t.TempDir() + _, err := Run(Options{Root: dir, Jar: newJar(t), BaseURL: srv.URL}) if err == nil { t.Fatal("expected the run to abort on consecutive transport failures") } if !strings.Contains(err.Error(), "consecutive transport failures") { t.Errorf("err = %v, want a consecutive-transport-failure abort", err) } - // Six topics were queued; the pass must stop at the bound, not - // grind through all of them during an outage. + // Six broken topics were queued; the pass must stop at the bound, + // not grind through all of them during an outage. attempts := 0 - for i := 7001; i <= 7006; i++ { + for i := 7003; i <= 7008; i++ { attempts += f.topicCallCount(i) } if attempts > maxConsecutiveTransportFailures { - t.Errorf("%d topic attempts, want at most %d", attempts, maxConsecutiveTransportFailures) + t.Errorf("%d transport attempts, want at most %d", attempts, maxConsecutiveTransportFailures) + } + // Whatever was fetched before the outage must stay archived: the + // index is persisted on the abort path so the blobs already on disk + // keep a reference. + ci, lerr := LoadCourseIndex(dir, "4700") + if lerr != nil { + t.Fatalf("LoadCourseIndex: %v", lerr) + } + for _, id := range []int{7001, 7002} { + if ci.Topics[id] == nil || ci.Topics[id].Current == "" { + t.Errorf("topic %d fetched before the outage was not persisted in the index", id) + } + } + if ci.Topics[7003] != nil { + t.Error("a topic that never arrived must not be in the index") } } From cd7cbbcae57c0e1f8d9e48e18ec851e83a8d93c8 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 15 Oct 2026 19:25:19 -0600 Subject: [PATCH 21/27] fix(archive): report an unreadable blob in verify instead of passing it Making fileExists tolerant of non-not-exist stat errors was right for the status/search/refetch paths (a permission error must not read as "missing body"), but it also applied to archive verify, which would have passed blobs it could not actually stat. verify now uses statOK, which surfaces the stat error: a blob that is present but unreadable is reported as unreadable-*-blob rather than missing-*-blob, and verify no longer reports OK for it. Adds a test that reproduces a non-not-exist stat failure deterministically (the blobs path is a regular file, so stat returns ENOTDIR). --- internal/archive/body_state_test.go | 64 ++++++++++++++++++++++++++ internal/archive/store.go | 22 ++++++++- internal/archive/verify.go | 70 +++++++++++++++++------------ 3 files changed, 125 insertions(+), 31 deletions(-) diff --git a/internal/archive/body_state_test.go b/internal/archive/body_state_test.go index 2700a52..a3bdece 100644 --- a/internal/archive/body_state_test.go +++ b/internal/archive/body_state_test.go @@ -3,6 +3,8 @@ package archive import ( "crypto/sha256" "encoding/hex" + "os" + "path/filepath" "testing" "time" ) @@ -39,6 +41,68 @@ func TestLoadStatusCountsBodiesMissingFromDisk(t *testing.T) { } } +// A stat failure that is not not-existence (here: the blobs directory is +// a regular file, so stat returns ENOTDIR) must not read as "missing": +// the status/search paths stay quiet, while `archive verify` reports it. +func TestStatErrorsAreNotMissingButVerifyReportsThem(t *testing.T) { + dir := t.TempDir() + if err := SaveIndex(dir, &Index{Version: SchemaVersion, Courses: map[string]*IndexEntry{ + "1": {OrgUnitId: "1", Code: "SAF"}, + }}); err != nil { + t.Fatalf("SaveIndex: %v", err) + } + hash := sha256.Sum256([]byte("body")) + hexSum := hex.EncodeToString(hash[:]) + ci := &CourseIndex{CourseID: "1", Code: "SAF", Topics: map[int]*TopicIndex{ + 1: {TopicID: 1, Title: "unstatable", Type: "File", URL: "/a.pdf", CurrentBody: hexSum, LastModified: time.Now().UTC()}, + }} + if err := SaveCourseIndex(dir, "1", ci); err != nil { + t.Fatalf("SaveCourseIndex: %v", err) + } + if err := os.RemoveAll(filepath.Join(dir, blobsDirName)); err != nil { + t.Fatalf("remove blobs dir: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, blobsDirName), []byte("not a directory"), 0o600); err != nil { + t.Fatalf("replace blobs dir with a file: %v", err) + } + + st, err := LoadStatus(dir) + if err != nil { + t.Fatalf("LoadStatus: %v", err) + } + if st.MissingBodies != 0 || st.BodiesPresent != 1 { + t.Errorf("BodiesPresent=%d MissingBodies=%d, want 1/0 (an unstattable blob is not a missing body)", + st.BodiesPresent, st.MissingBodies) + } + topics, err := FileTopics(dir, "") + if err != nil { + t.Fatalf("FileTopics: %v", err) + } + if len(topics) != 1 || !topics[0].HasBody { + t.Errorf("FileTopics HasBody = %+v, want true (stat error must not read as missing)", topics) + } + + res, err := Verify(dir, false) + if err != nil { + t.Fatalf("Verify: %v", err) + } + found := false + for _, is := range res.Issues { + if is.Code == "missing-body-blob" { + t.Errorf("verify reported a missing body for an unstattable blob: %+v", is) + } + if is.Code == "unreadable-body-blob" { + found = true + } + } + if !found { + t.Errorf("verify did not report unreadable-body-blob; issues: %+v", res.Issues) + } + if res.OK { + t.Error("verify must not pass a store whose blobs cannot be statted") + } +} + func TestSearchBodyFiltersCheckDisk(t *testing.T) { dir := t.TempDir() _, _, _ = SaveFileBody(dir, []byte("body")) diff --git a/internal/archive/store.go b/internal/archive/store.go index 845d33a..dedd3a6 100644 --- a/internal/archive/store.go +++ b/internal/archive/store.go @@ -364,16 +364,34 @@ func isHex(s string) bool { return true } +// fileExists reports whether p is present, treating only positive +// non-existence as absent. A permission or I/O error means the blob may +// well be there, and callers that would otherwise re-download or +// re-report it must not act on a failed stat. `archive verify` uses +// statOK instead so an unreadable blob is reported, not passed. func fileExists(p string) bool { if p == "" { return false } _, err := os.Stat(p) - // A permission or I/O error means the blob might well be there; - // only positive non-existence counts as missing. return err == nil || !errors.Is(err, fs.ErrNotExist) } +// statOK reports whether p is present and surfaces the stat error when +// it failed for a reason other than not-existence. +func statOK(p string) (present bool, err error) { + if p == "" { + return false, nil + } + if _, err := os.Stat(p); err != nil { + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + return false, err + } + return true, nil +} + type Status struct { Root string `json:"root"` Exists bool `json:"exists"` diff --git a/internal/archive/verify.go b/internal/archive/verify.go index 20b4646..71f3f15 100644 --- a/internal/archive/verify.go +++ b/internal/archive/verify.go @@ -51,6 +51,29 @@ func Verify(root string, deep bool) (VerifyResult, error) { blobsDir := filepath.Join(root, blobsDirName) seenBlobs := map[string]struct{}{} + // checkBlob distinguishes "absent" from "present but unreadable": a + // stat failure that is not not-existence is reported as an issue + // instead of being passed as a present blob. + checkBlob := func(path, name, missingCode, unreadableCode, missingMsg, courseID string, topicID int) bool { + present, err := statOK(path) + if err != nil { + res.Issues = append(res.Issues, VerifyIssue{ + Code: unreadableCode, + CourseID: courseID, TopicID: topicID, Blob: name, + Message: fmt.Sprintf("cannot stat %s: %v", name, err), + }) + return false + } + if !present { + res.Issues = append(res.Issues, VerifyIssue{ + Code: missingCode, + CourseID: courseID, TopicID: topicID, Blob: name, + Message: missingMsg, + }) + } + return present + } + courseIDs := make([]string, 0, len(idx.Courses)) for cid := range idx.Courses { courseIDs = append(courseIDs, cid) @@ -104,37 +127,29 @@ func Verify(root string, deep bool) (VerifyResult, error) { res.CheckedTopics++ if t.Current != "" { name := t.Current + blobExt - path := MetadataBlobPath(root, t.Current) - if !fileExists(path) { - res.Issues = append(res.Issues, VerifyIssue{ - Code: "missing-metadata-blob", - CourseID: cid, TopicID: t.TopicID, Blob: name, - Message: fmt.Sprintf("current metadata pointer %s does not exist on disk", name), - }) - } else { + ok := checkBlob(MetadataBlobPath(root, t.Current), name, + "missing-metadata-blob", "unreadable-metadata-blob", + fmt.Sprintf("current metadata pointer %s does not exist on disk", name), + cid, t.TopicID) + if ok { seenBlobs[name] = struct{}{} } } for _, v := range t.Versions { name := v.UUID + blobExt - if !fileExists(MetadataBlobPath(root, v.UUID)) { - res.Issues = append(res.Issues, VerifyIssue{ - Code: "dangling-metadata-version", - CourseID: cid, TopicID: t.TopicID, Blob: name, - Message: fmt.Sprintf("versions[] entry %s not found on disk", name), - }) - } + checkBlob(MetadataBlobPath(root, v.UUID), name, + "dangling-metadata-version", "unreadable-metadata-version", + fmt.Sprintf("versions[] entry %s not found on disk", name), + cid, t.TopicID) } if t.CurrentBody != "" { name := t.CurrentBody + bodyExt path := BodyPath(root, t.CurrentBody) - if !fileExists(path) { - res.Issues = append(res.Issues, VerifyIssue{ - Code: "missing-body-blob", - CourseID: cid, TopicID: t.TopicID, Blob: name, - Message: fmt.Sprintf("currentBody %s does not exist on disk", name), - }) - } else { + ok := checkBlob(path, name, + "missing-body-blob", "unreadable-body-blob", + fmt.Sprintf("currentBody %s does not exist on disk", name), + cid, t.TopicID) + if ok { seenBlobs[name] = struct{}{} if deep { if err := verifyBodyHash(path, t.CurrentBody); err != nil { @@ -149,13 +164,10 @@ func Verify(root string, deep bool) (VerifyResult, error) { } for _, b := range t.BodyVersions { name := b.SHA256 + bodyExt - if !fileExists(BodyPath(root, b.SHA256)) { - res.Issues = append(res.Issues, VerifyIssue{ - Code: "dangling-body-version", - CourseID: cid, TopicID: t.TopicID, Blob: name, - Message: fmt.Sprintf("bodyVersions[] entry %s not found on disk", name), - }) - } + checkBlob(BodyPath(root, b.SHA256), name, + "dangling-body-version", "unreadable-body-version", + fmt.Sprintf("bodyVersions[] entry %s not found on disk", name), + cid, t.TopicID) } } } From 56c3a3eff2a50ade1b6305aee2d4f814045bd8f0 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 15 Oct 2026 19:25:23 -0600 Subject: [PATCH 22/27] fix(systemd): don't hide a failed removal behind the daemon-reload error When both a unit-file removal and daemon-reload failed, the reload branch returned first and the caller never learned a unit file was still on disk. The removal error is now joined into the reload error (errors.Join ignores the nil case, so a reload-only failure reads exactly as before). --- internal/systemd/systemd.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/systemd/systemd.go b/internal/systemd/systemd.go index 91d9329..e6725b4 100644 --- a/internal/systemd/systemd.go +++ b/internal/systemd/systemd.go @@ -250,7 +250,9 @@ func Uninstall() (Status, error) { return Status{UserDir: dir}, nil } if _, err := runSystemctl("daemon-reload"); err != nil { - return Status{}, fmt.Errorf("daemon-reload after uninstall failed: %w", err) + // Join so a removal failure isn't hidden by the reload error: + // a unit file that could not be removed is still on disk. + return Status{}, fmt.Errorf("daemon-reload after uninstall failed: %w", errors.Join(err, rmErr)) } if rmErr != nil { return Status{UserDir: dir}, fmt.Errorf("remove unit files: %w", rmErr) From 70bc9c9fd270a43c81a6742817f1b51577bed7c7 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 15 Oct 2026 19:25:23 -0600 Subject: [PATCH 23/27] fix(urlutil): treat the scheme case-insensitively when detecting absolute URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isAbs matched only the lowercase "http://" / "https://" prefixes, so "HTTPS://files.example.com/a.pdf" fell through to the relative path and got a "./" prefix — the download would then have targeted the D2L base instead of the file host. Schemes are case-insensitive per RFC 3986; isAbs lowercases before matching. --- internal/urlutil/urlutil.go | 6 +++++- internal/urlutil/urlutil_test.go | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/urlutil/urlutil.go b/internal/urlutil/urlutil.go index 75404e2..57d6219 100644 --- a/internal/urlutil/urlutil.go +++ b/internal/urlutil/urlutil.go @@ -43,8 +43,12 @@ func CleanPath(raw string) string { return encodePath(raw) } +// isAbs reports whether raw already carries an HTTP(S) scheme. Schemes +// are case-insensitive, so "HTTPS://host/x" is absolute too and must +// never be treated as a relative reference. func isAbs(u string) bool { - return strings.HasPrefix(u, "http://") || strings.HasPrefix(u, "https://") + lower := strings.ToLower(u) + return strings.HasPrefix(lower, "http://") || strings.HasPrefix(lower, "https://") } func cleanAbs(raw string) string { diff --git a/internal/urlutil/urlutil_test.go b/internal/urlutil/urlutil_test.go index e1ce67b..c77e319 100644 --- a/internal/urlutil/urlutil_test.go +++ b/internal/urlutil/urlutil_test.go @@ -87,6 +87,17 @@ func TestResolveD2LRelativeRefWithSpaceBeforeColon(t *testing.T) { } } +func TestResolveD2LUppercaseSchemeStaysAbsolute(t *testing.T) { + got, err := ResolveD2L("https://d2l.cbe.ab.ca", "HTTPS://files.example.com/a b.pdf") + if err != nil { + t.Fatalf("ResolveD2L: %v", err) + } + want := "HTTPS://files.example.com/a%20b.pdf" + if got != want { + t.Errorf("ResolveD2L = %q, want %q (an uppercase scheme must not resolve under the D2L base)", got, want) + } +} + func TestResolveD2LAbsoluteURLIsCleaned(t *testing.T) { got, err := ResolveD2L("https://d2l.cbe.ab.ca", "https://files.example.com/a b#c.pdf") if err != nil { From ca4cf0b6a133de870f5b0a0f358a94d110714fe3 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 15 Oct 2026 19:25:23 -0600 Subject: [PATCH 24/27] test(systemd): cover both placeholder spellings in both row shapes The updated fixtures had drifted to using only "n/a", leaving the pre-249 "-" form untested on a passing parse. A table test now runs the never-fired and placeholder-NEXT shapes against both spellings. --- internal/systemd/systemd_test.go | 39 +++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/internal/systemd/systemd_test.go b/internal/systemd/systemd_test.go index 3aeb53c..559ac9d 100644 --- a/internal/systemd/systemd_test.go +++ b/internal/systemd/systemd_test.go @@ -36,7 +36,7 @@ func TestSplitTimerFieldsBothDatetimes(t *testing.T) { func TestSplitTimerFieldsNeverFired(t *testing.T) { f := []string{ "Wed", "2026-10-14", "21:00:00", "MDT", "30min", "left", - "n/a", "n/a", + "-", "-", "schooltools-archive.timer", "schooltools-archive.service", } next, left, last, passed, unit, activates, ok := splitTimerFields(f) @@ -128,6 +128,43 @@ func TestSplitTimerFieldsPlaceholderNextRealLast(t *testing.T) { } } +func TestSplitTimerFieldsPlaceholderSpellings(t *testing.T) { + // "-" is the pre-249 form and "n/a" the systemd 249+ form; both + // must parse identically in both shapes. + for _, ph := range []string{"-", "n/a"} { + neverFired := []string{ + "Wed", "2026-10-14", "21:00:00", "MDT", "30min", "left", + ph, ph, + "schooltools-archive.timer", "schooltools-archive.service", + } + next, left, last, passed, _, _, ok := splitTimerFields(neverFired) + if !ok { + t.Errorf("%s never-fired: not ok", ph) + continue + } + if next != "Wed 2026-10-14 21:00:00 MDT" || left != "30min left" || last != "" || passed != "" { + t.Errorf("%s never-fired: next=%q left=%q last=%q passed=%q", ph, next, left, last, passed) + } + + noNext := []string{ + ph, ph, + "Wed", "2026-10-14", "20:00:55", "MDT", "23s", "ago", + "schooltools-archive.timer", "schooltools-archive.service", + } + next, left, last, passed, _, _, ok = splitTimerFields(noNext) + if !ok { + t.Errorf("%s placeholder-NEXT: not ok", ph) + continue + } + if next != "" || left != "" { + t.Errorf("%s placeholder-NEXT: next=%q left=%q, want empty", ph, next, left) + } + if last != "Wed 2026-10-14 20:00:55 MDT" || passed != "23s ago" { + t.Errorf("%s placeholder-NEXT: last=%q passed=%q", ph, last, passed) + } + } +} + func TestSplitTimerFieldsRejectsGarbage(t *testing.T) { for _, f := range [][]string{ {}, From 2613bbaae6688966646b4ab2192f3f5e1ac25481 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 15 Oct 2026 19:25:23 -0600 Subject: [PATCH 25/27] test(login): buffer the stderr capture channel The reader goroutine sends on an unbuffered channel; if fn panics, nobody receives, so the goroutine leaks with the captured bytes still in flight. A one-slot buffer lets it finish after the deferred cleanup. --- internal/login/login_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/login/login_test.go b/internal/login/login_test.go index b4c0acb..bd6286c 100644 --- a/internal/login/login_test.go +++ b/internal/login/login_test.go @@ -24,7 +24,9 @@ func captureStderr(t *testing.T, fn func()) string { // Drain the pipe concurrently so a fn that writes more than the // pipe buffer cannot block, and restore os.Stderr even if fn // panics or calls t.Fatal. - done := make(chan string) + // Buffered so the reader can finish and exit after the deferred + // cleanup even when fn panics and nobody receives from it. + done := make(chan string, 1) go func() { data, _ := io.ReadAll(r) done <- string(data) From 04ff969c683fc4cf2b4f6c45b2ffb36e3890adbf Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 15 Oct 2026 19:25:29 -0600 Subject: [PATCH 26/27] docs: changelog entries for the abort-persistence and verify-stat fixes --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3dcfa3..ac98e93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -351,6 +351,16 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). D2L file URLs never carry a query string, so `?` is a path character) and applies the same cleaning to absolute URLs; both `archive update`'s body downloads and `download` route through it. +- **P2:** A course pass that hit a fatal error (expired session, or the + network going away mid-pass) returned before writing the per-course + index, so blobs fetched earlier in that pass were left on disk with + nothing pointing at them and the index updates were dropped. The + accumulated index is now persisted on the abort paths. +- **P2:** `archive verify` treated any stat failure other than "not + exist" as proof a blob was present (a side effect of making the + status/search "is this body missing?" check tolerant of permission + errors). It now reports a present-but-unstattable blob as + `unreadable-*-blob` and no longer reports `ok` for it. - **P2:** `systemd status` dropped `systemctl list-timers` data rows whose LAST column was a placeholder (a timer that never fired), so fresh installs printed no next-run time at all, and mis-reported From ebbf46a4516581662038a4beed97235906703288 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 15 Oct 2026 19:39:26 -0600 Subject: [PATCH 27/27] fix(archive): clear a stale body pointer when a modified topic's body fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the abort-persistence fix, and a hole that predates it: when a topic is queued as "modified" and its body download then fails, the index recorded the new LastModified (and new metadata blob) while keeping the old currentBody. The next pass then saw matching metadata plus an existing blob and skipped the changed body forever — silent staleness with nothing in the counters to show for it. The pointer is now cleared whenever the body isn't saved for a modified topic (download failure and blob-save failure alike). The blob is never removed and BodyVersions still names it, so nothing is lost; the topic simply stays eligible for a body retry, which the next pass performs as body-missing convergence work. TestModifiedTopicWithFailedBodyStaysEligible seeds six modified File topics whose bodies 404, asserts every pointer is cleared while the new metadata timestamp sticks, and then asserts a recovery pass re-fetches all six bodies. It fails without this change. --- CHANGELOG.md | 6 +- internal/archive/archive.go | 14 ++++ internal/archive/resilience_test.go | 116 ++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac98e93..d6a8dbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -355,7 +355,11 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). network going away mid-pass) returned before writing the per-course index, so blobs fetched earlier in that pass were left on disk with nothing pointing at them and the index updates were dropped. The - accumulated index is now persisted on the abort paths. + accumulated index is now persisted on the abort paths. A topic whose + metadata advanced but whose body could not be fetched now clears its + stale `currentBody` pointer (the blob itself is never removed), so the + next pass still sees it as needing a body instead of treating the old + bytes as current. - **P2:** `archive verify` treated any stat failure other than "not exist" as proof a blob was present (a side effect of making the status/search "is this body missing?" check tolerant of permission diff --git a/internal/archive/archive.go b/internal/archive/archive.go index 39ce1ee..fca7a76 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -933,6 +933,14 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, } bodyBytes, contentType, berr := downloadFileBody(opts.Jar, bodyURL, opts.baseURL()) if berr != nil { + // The metadata for a topic whose reason was new/modified is + // about to be recorded as current, but its body is not. Clear + // the stale pointer so bodyNeeded queues the topic again on + // the next pass instead of pinning the old bytes forever (the + // blob itself is never removed; BodyVersions still names it). + if q.reason == "modified" { + ti.CurrentBody = "" + } if errors.Is(berr, errSessionExpired) { return abortWithIndex(berr) } @@ -975,6 +983,12 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, continue } if _, _, sErr := SaveFileBody(opts.Root, bodyBytes); sErr != nil { + // Same reasoning as the download-failure branch: metadata + // advanced, body did not, so the topic must stay eligible for + // a retry. + if q.reason == "modified" { + ti.CurrentBody = "" + } if opts.Verbose { fmt.Fprintf(os.Stderr, "[archive] body %d save: %v (continuing)\n", q.topicID, sErr) } diff --git a/internal/archive/resilience_test.go b/internal/archive/resilience_test.go index 1b5c685..1b19ba2 100644 --- a/internal/archive/resilience_test.go +++ b/internal/archive/resilience_test.go @@ -3,10 +3,12 @@ package archive import ( "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" "strings" "testing" + "time" "github.com/aarav/schooltools/internal/content" ) @@ -123,6 +125,120 @@ func TestRunAbortsAfterConsecutiveTransportFailures(t *testing.T) { } } +// A topic whose metadata advanced must not keep an old body pointer: the +// next pass would see matching metadata plus an existing blob and skip +// the changed body forever. The pointer is cleared so the topic stays +// eligible for a retry. +func TestModifiedTopicWithFailedBodyStaysEligible(t *testing.T) { + dir := t.TempDir() + for i := 7301; i <= 7306; i++ { + seedTopicBody(t, dir, "1", i) + } + + f := newFakeD2L(t) + f.courses = []d2lCourse{{OrgUnitId: json.Number("1"), Name: "Bio", Code: "BIO", IsActive: true}} + var topics []content.TocTopic + for i := 7301; i <= 7306; i++ { + u := fmt.Sprintf("/content/enforced/1-BIO/note%d.pdf", i) + topics = append(topics, content.TocTopic{ + TopicId: i, Title: "Notes", LastModifiedDate: "2026-03-01T00:00:00Z", + TypeIdentifier: "File", Url: &u, + }) + f.topics[i] = map[string]any{"TopicId": i, "Title": "Notes", "TypeIdentifier": "File", "Url": u} + } + f.tocs["1"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: topics}}} + f.bodyHandler = func(w http.ResponseWriter, r *http.Request) { + http.NotFound(w, r) + } + srv := httptest.NewServer(f.handler()) + defer srv.Close() + + res, err := Run(Options{Root: dir, Jar: newJar(t), BaseURL: srv.URL, DownloadFiles: true}) + if err != nil { + t.Fatalf("a 404 on a body must not abort the pass: %v", err) + } + if res.BodiesFailed != 6 { + t.Errorf("BodiesFailed=%d, want 6", res.BodiesFailed) + } + + ci, err := LoadCourseIndex(dir, "1") + if err != nil { + t.Fatalf("LoadCourseIndex: %v", err) + } + for i := 7301; i <= 7306; i++ { + ti := ci.Topics[i] + if ti == nil { + t.Fatalf("topic %d is missing from the persisted index", i) + } + if ti.CurrentBody != "" { + t.Errorf("topic %d kept body %q after a failed download; the next pass would treat it as converged", i, ti.CurrentBody) + } + if ti.LastModified.IsZero() || ti.LastModified.Before(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) { + t.Errorf("topic %d did not keep the new metadata timestamp: %v", i, ti.LastModified) + } + } + + // With the bodies reachable again, every cleared pointer is refetched. + f2 := newFakeD2L(t) + f2.courses = f.courses + f2.tocs = f.tocs + f2.topics = f.topics + f2.bodyHandler = func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("new body for " + r.URL.Path)) + } + srv2 := httptest.NewServer(f2.handler()) + defer srv2.Close() + + res2, err := Run(Options{Root: dir, Jar: newJar(t), BaseURL: srv2.URL, DownloadFiles: true}) + if err != nil { + t.Fatalf("recovery run: %v", err) + } + if res2.BodiesFetched != 6 { + t.Errorf("BodiesFetched=%d, want 6 (every cleared pointer must be re-fetched)", res2.BodiesFetched) + } + ci, err = LoadCourseIndex(dir, "1") + if err != nil { + t.Fatalf("LoadCourseIndex after recovery: %v", err) + } + for i := 7301; i <= 7306; i++ { + if ti := ci.Topics[i]; ti == nil || ti.CurrentBody == "" { + t.Errorf("topic %d has no body after the recovery run", i) + } + } +} + +// seedTopicBody writes an archived File topic with one body blob on disk +// and returns that body's SHA. +func seedTopicBody(t *testing.T, dir, courseID string, topicID int) string { + t.Helper() + sha, _, err := SaveFileBody(dir, []byte(fmt.Sprintf("old body %d", topicID))) + if err != nil { + t.Fatalf("SaveFileBody: %v", err) + } + ci, err := LoadCourseIndex(dir, courseID) + if err != nil { + t.Fatalf("LoadCourseIndex: %v", err) + } + if ci.Topics == nil { + ci.Topics = map[int]*TopicIndex{} + } + ci.CourseID = courseID + ci.Topics[topicID] = &TopicIndex{ + TopicID: topicID, Title: "Notes", Type: "File", + URL: fmt.Sprintf("/content/enforced/%s-BIO/note%d.pdf", courseID, topicID), + CurrentBody: sha, LastModified: time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC), + } + if err := SaveCourseIndex(dir, courseID, ci); err != nil { + t.Fatalf("SaveCourseIndex: %v", err) + } + if err := SaveIndex(dir, &Index{Version: SchemaVersion, Courses: map[string]*IndexEntry{ + courseID: {OrgUnitId: courseID}, + }}); err != nil { + t.Fatalf("SaveIndex: %v", err) + } + return sha +} + func TestRunToleratesHTTPFailuresAfterTransportBounded(t *testing.T) { // HTTP failures must not consume the transport budget: a mix of // 404s and one transport error keeps the pass going.