diff --git a/CHANGELOG.md b/CHANGELOG.md index aa16a3d..d6a8dbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -315,6 +315,97 @@ 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 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 (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. + `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 …`), 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; + 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. 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 + 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 + UNIT/ACTIVATES when PASSED was a placeholder. The parser now + anchors on the trailing unit columns and treats placeholders + 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 + 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`; 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, 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 3151e5f..716f39a 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)") @@ -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,13 +392,21 @@ 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) } 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.") } @@ -408,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, }) } @@ -416,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, }) @@ -596,8 +619,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" + @@ -614,12 +637,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/cmd/download.go b/cmd/download.go index b17a572..cc080d3 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,7 @@ 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() { - 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/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/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/archive/archive.go b/internal/archive/archive.go index efd1370..fca7a76 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" @@ -82,8 +83,30 @@ 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" ) +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 @@ -219,9 +242,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 @@ -232,11 +257,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 @@ -245,6 +270,7 @@ type CourseDiff struct { TopicsNew int TopicsModified int TopicsUnchanged int + TopicsBroken int BodiesWouldFetch int WouldFetchIDs []int } @@ -576,6 +602,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 @@ -597,8 +625,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) } } @@ -676,6 +705,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) @@ -718,6 +748,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 != "" @@ -758,6 +792,7 @@ func diffCourse(opts Options, c Course) (CourseDiff, error) { type courseStats struct { stale, checked, fetched int bodiesNew, bodiesReused, bodiesErr int + failed, skipped int } // archiveCourse runs a single course's archive pass and returns the @@ -770,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 @@ -783,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 @@ -793,6 +841,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 != "" @@ -823,8 +875,23 @@ 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 abortWithIndex(ferr) + } + if errors.Is(ferr, errTransport) { + transportFails++ + if transportFails >= maxConsecutiveTransportFailures { + return abortWithIndex(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 @@ -866,12 +933,31 @@ 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) + } + if errors.Is(berr, errTransport) { + transportFails++ + if transportFails >= maxConsecutiveTransportFailures { + return abortWithIndex(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 @@ -897,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) } @@ -945,29 +1037,25 @@ 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 + } + 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 { + if res.StatusCode == 401 || res.StatusCode == 403 { + return nil, "", errSessionExpired + } body, _ := io.ReadAll(res.Body) msg := strings.TrimSpace(string(body)) if len(msg) > 200 { @@ -975,6 +1063,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 @@ -1024,6 +1119,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 @@ -1040,11 +1138,18 @@ 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 + } 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 @@ -1155,7 +1260,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)), }) } } @@ -1275,6 +1380,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 e7cc2d0..11dfc3f 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" @@ -10,6 +11,7 @@ import ( "reflect" "sort" "strings" + "sync" "testing" "time" @@ -100,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 { @@ -191,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: { @@ -424,13 +426,18 @@ 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 - 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 + tocStatus int + hijackTopics map[int]bool + topicCalls map[int]int + topicMu sync.Mutex + bodyHandler http.HandlerFunc // optional; serves file bodies on arbitrary paths } func newFakeD2L(t *testing.T) *fakeD2L { @@ -451,16 +458,37 @@ 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{}, + 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) { + if f.coursesStatus != 0 { + w.WriteHeader(f.coursesStatus) + return + } w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(d2lCoursesResponse{Courses: f.courses}) }) @@ -473,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) @@ -487,7 +518,28 @@ 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 + } + 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/body_state_test.go b/internal/archive/body_state_test.go new file mode 100644 index 0000000..a3bdece --- /dev/null +++ b/internal/archive/body_state_test.go @@ -0,0 +1,184 @@ +package archive + +import ( + "crypto/sha256" + "encoding/hex" + "os" + "path/filepath" + "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) + } +} + +// 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")) + 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) + } + 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 { + t.Errorf("topic %d: HasBody=%v, want %v", ft.TopicID, ft.HasBody, want) + } + } +} diff --git a/internal/archive/resilience_test.go b/internal/archive/resilience_test.go new file mode 100644 index 0000000..1b19ba2 --- /dev/null +++ b/internal/archive/resilience_test.go @@ -0,0 +1,403 @@ +package archive + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "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"}, + {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()) + 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 != 4 { + t.Errorf("TopicsChecked=%d, want 4", res.TopicsChecked) + } + 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) + } + if res.TopicsFailed != 1 { + t.Errorf("TopicsFailed=%d, want 1 (404 topic)", res.TopicsFailed) + } + 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) + } + + 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 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") + } + + 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 TestRunAbortsAfterConsecutiveTransportFailures(t *testing.T) { + f := newFakeD2L(t) + var topics []content.TocTopic + // 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}} + f.tocs["4700"] = content.TocResponse{Modules: []content.TocModule{{ModuleId: 1, Topics: topics}}} + 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 on consecutive transport failures") + } + if !strings.Contains(err.Error(), "consecutive transport failures") { + t.Errorf("err = %v, want a consecutive-transport-failure abort", err) + } + // 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 := 7003; i <= 7008; i++ { + attempts += f.topicCallCount(i) + } + if 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") + } +} + +// 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. + 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}} + 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 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}} + 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) + } +} diff --git a/internal/archive/store.go b/internal/archive/store.go index 4ef577d..dedd3a6 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" @@ -26,13 +28,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 +279,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 +302,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 +317,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) { @@ -354,12 +364,32 @@ 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) - return err == nil + 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 { @@ -432,7 +462,11 @@ func LoadStatus(root string) (Status, error) { if t.Type == "" || t.Type == "File" { st.FileTopics++ } - if 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++ } } 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/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) } } } diff --git a/internal/login/login.go b/internal/login/login.go index 0c552fe..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 { @@ -139,12 +139,12 @@ 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 } 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") @@ -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 } @@ -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) @@ -522,37 +522,42 @@ 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) } 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. +// 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") { - fmt.Fprintf(os.Stderr, " << Set-Cookie: %s\n", sc) + 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. +// 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 } - 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", @@ -560,11 +565,34 @@ 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) } } } +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..bd6286c --- /dev/null +++ b/internal/login/login_test.go @@ -0,0 +1,154 @@ +package login + +import ( + "io" + "net/http" + "net/http/httptest" + "net/url" + "os" + "strings" + "sync/atomic" + "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 + // 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. + // 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) + }() + func() { + defer func() { + _ = w.Close() + os.Stderr = old + }() + fn() + }() + return <-done +} + +func TestFetchPostDumpsOnlyWhenVerbose(t *testing.T) { + secret := "supersecretvalue-but-very-long-cookie-tail" + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + 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() { + 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 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() { + 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) + } + if strings.Contains(out, secret) { + t.Errorf("verbose fetchPost leaked the full cookie value: %q", out) + } + if got := requests.Load(); got != 2 { + t.Errorf("server saw %d requests, want 2", got) + } +} + +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 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) }) + 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) + } +} diff --git a/internal/systemd/systemd.go b/internal/systemd/systemd.go index 1df3651..e6725b4 100644 --- a/internal/systemd/systemd.go +++ b/internal/systemd/systemd.go @@ -6,6 +6,7 @@ package systemd import ( "embed" + "errors" "fmt" "os" "os/exec" @@ -98,10 +99,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 +125,7 @@ func legacyExecStart(svcPath string) bool { } } if !anyExec { - return false + return true } return !anyUpdate } @@ -190,9 +192,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 +208,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 +225,38 @@ 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) + 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 + } } - if err := os.Remove(tmrPath); err == nil { - removed = append(removed, tmrPath) + var rmErr error + if err := remove(svcPath); err != nil { + rmErr = errors.Join(rmErr, err) } - if _, err := runSystemctl("daemon-reload"); err != nil { - return Status{}, fmt.Errorf("daemon-reload after uninstall failed: %w", err) + if err := remove(tmrPath); err != nil { + rmErr = errors.Join(rmErr, err) } - if len(removed) == 0 { + if !removed && rmErr == nil { + // Nothing was installed: leave systemctl alone entirely. return Status{UserDir: dir}, nil } + if _, err := runSystemctl("daemon-reload"); err != nil { + // 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) + } return Status{UserDir: dir}, nil } @@ -261,7 +281,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 @@ -275,8 +295,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 @@ -298,9 +318,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: // -// 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. 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) < 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 dows[0] == 0 { + // 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 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 + } + 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 && isTimerPlaceholder(mid[0]) { + mid = nil + } + passed = strings.Join(mid, " ") } - next = strings.Join(f[0:4], " ") - if 4 < secondDOW { - left = f[4] + case 2: + if dows[1]+4 > len(rest) { + return } - } 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] + next = strings.Join(rest[:4], " ") + if l := rest[4:dows[1]]; !allPlaceholders(l) { + left = strings.Join(l, " ") } - } - - // 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 + last = strings.Join(rest[dows[1]:dows[1]+4], " ") + mid := rest[dows[1]+4:] + if len(mid) == 1 && isTimerPlaceholder(mid[0]) { + mid = nil } - } - 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 { + passed = strings.Join(mid, " ") + default: return } - unit = tail[0] - activates = tail[1] ok = true 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 new file mode 100644 index 0000000..559ac9d --- /dev/null +++ b/internal/systemd/systemd_test.go @@ -0,0 +1,225 @@ +package systemd + +import ( + "os" + "path/filepath" + "testing" +) + +func TestSplitTimerFieldsBothDatetimes(t *testing.T) { + f := []string{ + "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", + } + 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 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) + } + 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", "left", + "-", "-", + "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 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) + } + 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", "left", + "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 left" { + 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 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", + } + next, left, last, passed, unit, activates, ok := splitTimerFields(f) + if !ok { + t.Fatal("expected ok for a placeholder-NEXT row") + } + // 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) + } + 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 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{ + {}, + {"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) + } + } +} + +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") + } +} diff --git a/internal/urlutil/urlutil.go b/internal/urlutil/urlutil.go new file mode 100644 index 0000000..57d6219 --- /dev/null +++ b/internal/urlutil/urlutil.go @@ -0,0 +1,111 @@ +// 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. Relative +// references and absolute URLs are both percent-encoded first, so that +// 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 + } + b, err := url.Parse(base) + if err != nil { + return "", fmt.Errorf("parse base: %w", err) + } + 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) + } + return b.ResolveReference(ref).String(), nil +} + +// CleanPath percent-encodes a URL path, preserving escape sequences that +// are already valid. +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 { + lower := strings.ToLower(u) + return strings.HasPrefix(lower, "http://") || strings.HasPrefix(lower, "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) + 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): + fmt.Fprintf(&b, "%%%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..c77e319 --- /dev/null +++ b/internal/urlutil/urlutil_test.go @@ -0,0 +1,116 @@ +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"}, + {"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 { + 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 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 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 { + t.Fatalf("ResolveD2L: %v", err) + } + want := "https://files.example.com/a%20b%23c.pdf" + if got != want { + t.Errorf("ResolveD2L = %q, want %q", got, want) + } +} + +func TestResolveD2LBaseParseError(t *testing.T) { + if _, err := ResolveD2L("ht tp://bad", "/content/x.pdf"); err == nil { + t.Error("expected error for unparseable base") + } +}