mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-09-14 19:59:52 +08:00
chethanuk
7f8f254d84
`ocr session compare` had no equivalent in the web viewer (#1104). Adds GET /r/{repo}/compare?before=&after= rendering the same four buckets the CLI prints, plus a compare link on each session-list row pointing at the next-older session. Findings render with the existing/suggested code panels session.html already uses, so a finding whose fix is a patch shows the patch and not only the prose around it. The reviewed-path partition (manifest Completed + Reused) moves to an exported session.ReviewedPaths that the CLI now delegates to, so the two cannot drift. The viewer's own Summary.FilesReviewed is Coverage.Selected - the intended set - which would report files an interrupted run never reached as clean. Route registration moves out of StartServer into newMux, so the literal "compare" segment beating the {sessionID} wildcard is covered by a test that dispatches through the real ServeMux instead of calling the handler directly. The repo guard there now rejects "\" alongside "/": ServeMux unescapes each path segment, and "\" is a separator on Windows. encodeRepoPath maps both path separators to "-", so two distinct working directories ("/home/a/b" and "/home/a-b") can share one on-disk repo directory, and a compare request could otherwise pair sessions from different repos. handleCompare (internal/viewer/handler.go:185) rejects that case by comparing the two sessions' Summary.CWD, the same check the CLI does in cmd/opencodereview/session_cmd.go's runSessionCompare, tested at internal/viewer/compare_test.go:272. LoadSession (internal/viewer/store.go) now re-validates encodedRepo and sessionID itself through a new safeSegment helper and joins the value safeSegment returns rather than its own copy of the argument, instead of only trusting that every caller had already checked them with unsafeSegment. Both current callers (handleSession's route and handleCompare's before/after loop) already rejected traversal before reaching LoadSession, but a validate-then-use-the-original-argument shape across a function boundary is not something CodeQL's path-injection query can credit as a sanitizer, and it left LoadSession unsafe to call from anywhere that skipped that pre-check. ListSessions's sort by Timestamp alone was not a total order: two sessions with an equal (or both-zero) timestamp could swap position between calls, and sessions.html's "compare" link pairs row i with row i+1 as chronologically adjacent. The sort now breaks ties on SessionID so that pairing is deterministic. One divergence from the CLI is stated in the viewer docs: an empty bucket still renders as "New (0)" instead of being skipped. Entry points are plain links, never a form: the viewer's CSP sets form-action 'none', which blocks GET submissions with no server-side error. compare.html declares lang="en". The three older viewer templates still say lang="zh-CN" while serving English; fixing those is a separate change. Inherited limitation: session.Compare keys a finding on its current path, so a file renamed between the two runs reads as one resolved plus one new. The page shows what Compare returns and does not correct it. Signed-off-by: ChethanUK <chethanuk@outlook.com>