fix: absolute download URL and resilient server version display#457
fix: absolute download URL and resilient server version display#457brandonrc wants to merge 1 commit into
Conversation
Resolves two v1.2.0 web bugs. #455: The artifact detail "Download URL" field showed a host-less path, so copying it produced a broken URL. Add artifactsApi.getAbsoluteDownloadUrl, which resolves the path against NEXT_PUBLIC_API_URL or the current window origin, and use it for the copyable detail field. #456: The sidebar hid the backend version whenever /health returned a non-2xx status, even though the version is present in the response body. adminApi.getHealth now detects the HealthResponse shape on the SDK error path (the backend returns 503 with a full body when degraded) and adapts it, so the version stays visible. Adds unit tests for both helpers and a Playwright spec under e2e/suites/interactions/repositories verifying the copied Download URL is absolute and well-formed and that the sidebar shows the backend version.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| // A degraded backend still includes the version and check details, so detect | ||
| // the HealthResponse shape and use it rather than discarding the version. | ||
| function isSdkHealthResponse(value: unknown): value is SdkHealthResponse { | ||
| if (typeof value !== 'object' || value === null) return false; |
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Test/E2E quality review (v1.2.0 batch). Verdict: real coverage, with one weak spot.
#455 download URL. The unit test getAbsoluteDownloadUrl returns a full, well-formed URL is the load-bearing assertion and it is honest: it stubs NEXT_PUBLIC_API_URL and asserts the exact absolute string plus new URL(url).protocol === "https:". The pre-fix getDownloadUrl returned a host-less /api/v1/... path, which would fail both new URL(url) (throws without a base) and the protocol check, so this assertion genuinely distinguishes fixed-vs-broken. Good.
#455 e2e. This one is softer than it looks. There are two test.skip(...) escape hatches (artifact not in table, Download URL label not visible). If either fires, the bug-fix assertion (/https?:\/\/\S+/ regex + new URL) never runs and the test still goes green. The skip on "Download URL field not visible" is the concerning one: a regression that broke the detail panel rendering would skip rather than fail. Since the unit test already nails the actual fix, this is acceptable, but consider asserting the row is present (hard fail) and only skipping on the upload/seed step.
#456 version. Strong. The unit test getHealth uses the body version on a non-2xx degraded response mocks { data: undefined, error: degraded } and asserts result.version === "1.2.0". Pre-fix getHealth did if (error) throw error, so this test would throw and fail. It directly proves the fix. The e2e grounds the assertion in the live /health body (expect(text).toContain('Server ${expectedVersion}')) only when a version is present, which is the right call.
No flaky arbitrary-ordering issues. Net: genuine coverage. Not requesting changes.
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Security review (open redirect / host injection / data trust lens).
getAbsoluteDownloadUrl — SAFE, no open redirect or host injection. The origin comes only from NEXT_PUBLIC_API_URL (a build-time operator-controlled env var) or window.location.origin (the browser's own trusted origin), never from user input or the artifact record. new URL(path, origin) resolves a fixed local path against that trusted origin, so an attacker who controls artifactPath cannot redirect to a foreign host: the path is the second-position relative ref and the origin is the base, so even an absolute-looking artifactPath stays under /api/v1/repositories/.../download/.... The value is rendered into a copyable text field (DetailRow ... copy mono), not used as a live href/redirect target, so there is no navigation sink. No SSRF either since this is client-side display only.
One note, not a defect: if artifactPath ever contained a ? or #, new URL would split it into query/fragment, but the copied string remains same-origin and harmless; backend routing already governs the real download.
getHealth trusting a non-2xx body — ACCEPTABLE. On a 503 the SDK surfaces the parsed body as error; isSdkHealthResponse narrows it (version is string, checks is a non-null object) before adaptHealth. The body is the backend's own /health payload over the same trusted channel, and the consumed fields are just version/status/check strings rendered as text in the sidebar. No injection sink, no privilege decision keyed off this. Worst case a malformed-but-shaped body shows a wrong version string. Low risk.
No request-changes. Looks good to ship.
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Frontend/React correctness review. No blockers. Both fixes are correct and well-tested.
Correctness notes
-
getAbsoluteDownloadUrlresolves againstNEXT_PUBLIC_API_URLthenwindow.location.origin, with anew URL(...)try/catch fallback. Solid. One edge: ifNEXT_PUBLIC_API_URLcarries a path prefix (e.g.https://host/registry),new URL("/api/v1/...", "https://host/registry")resolves against the origin and drops/registry. The catch-branch concatenation (origin + path) would keep it, but it only runs on a thrown error, whichnew URLwon't throw here. Likely a non-issue for current deployments (the SDK base is typically bare origin), just flagging. -
Multi-instance gap:
sdk-client.tsexposesgetActiveInstanceBaseUrl(), which prefixes/api/v1/instances/{id}/proxywhen a non-local instance is active.getAbsoluteDownloadUrlonly usesNEXT_PUBLIC_API_URL/origin, so when viewing an artifact through a proxied remote instance the copied URL points at the local base rather than the proxied path. The same-origingetDownloadUrldownload flow relies on the Next rewrite and is unaffected. If artifact detail can be opened under an active remote instance, the copied URL would be wrong there. Worth a follow-up; not blocking the primary same-origin case. -
The degraded-health adapter (
isSdkHealthResponse+ adapt-on-error inadminApi.getHealth) is a clean, well-scoped fix: it only treats the error body as a health response when it structurally looks like one (versionstring +checksobject), otherwise rethrows. Good. The unit test covers the 503-with-body path.
Nice, focused PR.
|
Shipped in v1.2.0. The changes from this PR were folded into the release integration branch #466, which was squash-merged to main and tagged as v1.2.0. The code is already on main, so closing this as superseded by #466 rather than merging it (re-applying would just duplicate what already shipped). No work lost. See #466 for the consolidated record. |
1 participant
Summary
Fixes two v1.2.0 web bugs.
#455 - The artifact detail "Download URL" field rendered a host-less path (e.g.
/api/v1/repositories/.../download/...), so copying it produced a broken URL. AddedartifactsApi.getAbsoluteDownloadUrl, which resolves the download path againstNEXT_PUBLIC_API_URL(falling back towindow.location.origin), matching how downloads are actually issued. The copyable "Download URL" detail row now uses it.getDownloadUrlis unchanged for the same-origin download flow.#456 - The sidebar hid the backend (Server) version whenever
/healthreturned a non-2xx status, becauseadminApi.getHealththrew on any SDK error. The backend returns 503 with a fullHealthResponsebody (including the version) when a dependency is degraded.getHealthnow detects theHealthResponseshape on the error path and adapts it, so the version stays visible even on a degraded health response.Closes #455
Closes #456
Test Checklist
Unit tests: added
getAbsoluteDownloadUrlcoverage inartifacts.test.tsand a degraded-503 case inadmin.test.ts. E2E: new spece2e/suites/interactions/repositories/download-url-and-version.spec.tsasserts the copied Download URL is absolute and parseable, and that the sidebar shows the backend version (cross-checked against the live/healthbody). Ran eslint, tsc, and vitest on the changed files; all green.UI Changes
The change is to the value of an existing copyable field and an existing sidebar version label; no layout, styling, or markup structure changed.