Skip to content

Commit 88c8c4d

Browse files
authored
fix: handle signed URL download failures (#66)
* fix: improve download failure handling * test: cover download failure handling
1 parent f146c90 commit 88c8c4d

5 files changed

Lines changed: 228 additions & 31 deletions

File tree

src-tauri/src/downloader.rs

Lines changed: 160 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -118,22 +118,7 @@ enum UrlExpiry {
118118
Expired(DateTime<Utc>),
119119
}
120120

121-
fn parse_url_expiry(url: &str, now: DateTime<Utc>) -> UrlExpiry {
122-
let Ok(parsed) = Url::parse(url) else {
123-
return UrlExpiry::Missing;
124-
};
125-
let Some(value) = parsed
126-
.query_pairs()
127-
.find_map(|(key, value)| (key == "Expires").then_some(value))
128-
else {
129-
return UrlExpiry::Missing;
130-
};
131-
let Ok(timestamp) = value.parse::<i64>() else {
132-
return UrlExpiry::Malformed;
133-
};
134-
let Some(expires_at) = DateTime::from_timestamp(timestamp, 0) else {
135-
return UrlExpiry::Malformed;
136-
};
121+
fn classify_url_expiry(expires_at: DateTime<Utc>, now: DateTime<Utc>) -> UrlExpiry {
137122
let confidently_expired = expires_at
138123
.checked_add_signed(chrono::Duration::seconds(CLOCK_SKEW_TOLERANCE_SECS))
139124
.is_some_and(|deadline| deadline < now);
@@ -145,6 +130,72 @@ fn parse_url_expiry(url: &str, now: DateTime<Utc>) -> UrlExpiry {
145130
}
146131
}
147132

133+
fn parse_relative_url_expiry(
134+
parsed: &Url,
135+
date_key: &str,
136+
lifetime_key: &str,
137+
now: DateTime<Utc>,
138+
) -> UrlExpiry {
139+
let signed_at = parsed
140+
.query_pairs()
141+
.find_map(|(key, value)| (key == date_key).then_some(value));
142+
let lifetime = parsed
143+
.query_pairs()
144+
.find_map(|(key, value)| (key == lifetime_key).then_some(value));
145+
match (signed_at, lifetime) {
146+
(None, None) => UrlExpiry::Missing,
147+
(Some(signed_at), Some(lifetime)) => {
148+
let Ok(signed_at) = chrono::NaiveDateTime::parse_from_str(&signed_at, "%Y%m%dT%H%M%SZ")
149+
else {
150+
return UrlExpiry::Malformed;
151+
};
152+
let Ok(lifetime_seconds) = lifetime.parse::<i64>() else {
153+
return UrlExpiry::Malformed;
154+
};
155+
if lifetime_seconds < 0 {
156+
return UrlExpiry::Malformed;
157+
}
158+
let Some(lifetime) = chrono::Duration::try_seconds(lifetime_seconds) else {
159+
return UrlExpiry::Malformed;
160+
};
161+
let Some(expires_at) = signed_at.and_utc().checked_add_signed(lifetime) else {
162+
return UrlExpiry::Malformed;
163+
};
164+
classify_url_expiry(expires_at, now)
165+
}
166+
_ => UrlExpiry::Malformed,
167+
}
168+
}
169+
170+
fn parse_url_expiry(url: &str, now: DateTime<Utc>) -> UrlExpiry {
171+
let Ok(parsed) = Url::parse(url) else {
172+
return UrlExpiry::Missing;
173+
};
174+
if let Some(value) = parsed
175+
.query_pairs()
176+
.find_map(|(key, value)| (key == "Expires").then_some(value))
177+
{
178+
let Ok(timestamp) = value.parse::<i64>() else {
179+
return UrlExpiry::Malformed;
180+
};
181+
let Some(expires_at) = DateTime::from_timestamp(timestamp, 0) else {
182+
return UrlExpiry::Malformed;
183+
};
184+
return classify_url_expiry(expires_at, now);
185+
}
186+
187+
for (date_key, lifetime_key) in [
188+
("X-Goog-Date", "X-Goog-Expires"),
189+
("X-Amz-Date", "X-Amz-Expires"),
190+
] {
191+
let expiry = parse_relative_url_expiry(&parsed, date_key, lifetime_key, now);
192+
if expiry != UrlExpiry::Missing {
193+
return expiry;
194+
}
195+
}
196+
UrlExpiry::Missing
197+
}
198+
148199
#[derive(Debug, Error, Clone, Copy, PartialEq, Eq)]
149200
enum UrlRejection {
150201
#[error("download URL is malformed")]
@@ -1059,6 +1110,99 @@ mod tests {
10591110
);
10601111
}
10611112

1113+
#[test]
1114+
fn parses_gcs_v4_expiry() {
1115+
let now = DateTime::parse_from_rfc3339("2026-02-01T19:45:00Z")
1116+
.unwrap()
1117+
.with_timezone(&Utc);
1118+
let expired_at = DateTime::parse_from_rfc3339("2026-02-01T19:40:47Z")
1119+
.unwrap()
1120+
.with_timezone(&Utc);
1121+
1122+
assert_eq!(
1123+
parse_url_expiry(
1124+
"https://storage.googleapis.com/bucket/image.jpg?X-Goog-Date=20260125T194047Z&X-Goog-Expires=604800&X-Goog-Signature=secret",
1125+
now,
1126+
),
1127+
UrlExpiry::Expired(expired_at)
1128+
);
1129+
assert_eq!(
1130+
parse_url_expiry(
1131+
"https://storage.googleapis.com/bucket/image.jpg?X-Goog-Expires=604800&X-Goog-Date=20260125T195000Z",
1132+
now,
1133+
),
1134+
UrlExpiry::Active(
1135+
DateTime::parse_from_rfc3339("2026-02-01T19:50:00Z")
1136+
.unwrap()
1137+
.with_timezone(&Utc)
1138+
)
1139+
);
1140+
assert_eq!(
1141+
parse_url_expiry(
1142+
"https://storage.googleapis.com/bucket/image.jpg?X-Goog-Date=invalid&X-Goog-Expires=604800",
1143+
now,
1144+
),
1145+
UrlExpiry::Malformed
1146+
);
1147+
assert_eq!(
1148+
parse_url_expiry(
1149+
"https://storage.googleapis.com/bucket/image.jpg?X-Goog-Date=20260125T194047Z",
1150+
now,
1151+
),
1152+
UrlExpiry::Malformed
1153+
);
1154+
assert_eq!(
1155+
parse_url_expiry(
1156+
"https://storage.googleapis.com/bucket/image.jpg?X-Goog-Date=20260125T194047Z&X-Goog-Expires=-1",
1157+
now,
1158+
),
1159+
UrlExpiry::Malformed
1160+
);
1161+
}
1162+
1163+
#[test]
1164+
fn parses_aws_v4_expiry() {
1165+
let now = DateTime::parse_from_rfc3339("2013-05-25T00:03:00Z")
1166+
.unwrap()
1167+
.with_timezone(&Utc);
1168+
let expired_at = DateTime::parse_from_rfc3339("2013-05-25T00:00:00Z")
1169+
.unwrap()
1170+
.with_timezone(&Utc);
1171+
1172+
assert_eq!(
1173+
parse_url_expiry(
1174+
"https://examplebucket.s3.amazonaws.com/test.txt?X-Amz-Date=20130524T000000Z&X-Amz-Expires=86400&X-Amz-Signature=secret",
1175+
now,
1176+
),
1177+
UrlExpiry::Expired(expired_at)
1178+
);
1179+
assert_eq!(
1180+
parse_url_expiry(
1181+
"https://examplebucket.s3.amazonaws.com/test.txt?X-Amz-Expires=86400&X-Amz-Date=20130524T120000Z",
1182+
now,
1183+
),
1184+
UrlExpiry::Active(
1185+
DateTime::parse_from_rfc3339("2013-05-25T12:00:00Z")
1186+
.unwrap()
1187+
.with_timezone(&Utc)
1188+
)
1189+
);
1190+
assert_eq!(
1191+
parse_url_expiry(
1192+
"https://examplebucket.s3.amazonaws.com/test.txt?X-Amz-Date=invalid&X-Amz-Expires=86400",
1193+
now,
1194+
),
1195+
UrlExpiry::Malformed
1196+
);
1197+
assert_eq!(
1198+
parse_url_expiry(
1199+
"https://examplebucket.s3.amazonaws.com/test.txt?X-Amz-Date=20130524T000000Z",
1200+
now,
1201+
),
1202+
UrlExpiry::Malformed
1203+
);
1204+
}
1205+
10621206
#[test]
10631207
fn aggregates_failures_with_safe_bounded_examples() {
10641208
let mut summary = FailureSummary::default();

src/components/__tests__/converter-screen.test.tsx

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ describe("DownloadFailureDetails", () => {
4040
/>,
4141
);
4242

43+
const copyStatus = screen.getByRole("status");
44+
expect(copyStatus).toBeEmptyDOMElement();
45+
expect(copyStatus).toHaveClass("sr-only");
4346
expect(screen.getByText("Download details")).toBeInTheDocument();
4447
expect(screen.getByText("1 download link expired — one.jpg")).toBeInTheDocument();
4548
await act(async () => {
@@ -49,13 +52,17 @@ describe("DownloadFailureDetails", () => {
4952
expect(writeText).toHaveBeenCalledWith(
5053
"expired_url: 1; one.jpg\nnot_found: 1; HTTP 404; two.jpg",
5154
);
52-
expect(screen.getByRole("status")).toHaveTextContent("Copied");
55+
expect(screen.getByRole("button", { name: "Copied" })).toBeInTheDocument();
56+
expect(copyStatus).toHaveTextContent("Copied");
5357

5458
act(() => {
5559
vi.advanceTimersByTime(2_000);
5660
});
5761

58-
expect(screen.queryByText("Copied")).not.toBeInTheDocument();
62+
expect(copyStatus).toBeEmptyDOMElement();
63+
expect(
64+
screen.getByRole("button", { name: "Copy details" }),
65+
).toBeInTheDocument();
5966
});
6067

6168
it("clears copied reset timer on unmount", async () => {
@@ -110,6 +117,9 @@ describe("DownloadFailureDetails", () => {
110117
expect(
111118
await screen.findByText("Couldn't copy details"),
112119
).toBeInTheDocument();
120+
expect(
121+
screen.getByRole("button", { name: "Copy failed" }),
122+
).toBeInTheDocument();
113123
});
114124

115125
it("renders structured hard failures through the error card", () => {
@@ -155,6 +165,7 @@ describe("DownloadFailureDetails", () => {
155165
expect(screen.getByTestId("error-message")).toHaveTextContent(
156166
"Your download links expired on 19 Jul 2026.",
157167
);
168+
expect(screen.getByTestId("error-card")).toHaveClass("overflow-hidden");
158169
expect(screen.getByText("Download details")).toBeInTheDocument();
159170
});
160171
});

src/components/converter-screen.tsx

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,18 @@ export function DownloadFailureDetails({
3535
const [copyStatus, setCopyStatus] = useState<
3636
"idle" | "copied" | "failed"
3737
>("idle");
38+
const copyButtonLabel =
39+
copyStatus === "copied"
40+
? "Copied"
41+
: copyStatus === "failed"
42+
? "Copy failed"
43+
: "Copy details";
44+
const copyAnnouncement =
45+
copyStatus === "copied"
46+
? "Copied"
47+
: copyStatus === "failed"
48+
? "Couldn't copy details"
49+
: "";
3850

3951
useEffect(() => {
4052
if (copyStatus !== "copied") {
@@ -85,18 +97,16 @@ export function DownloadFailureDetails({
8597
</ul>
8698
<button
8799
type="button"
88-
className="mt-2 underline underline-offset-2"
100+
className="mt-2 font-medium underline underline-offset-2"
89101
onClick={() => {
90102
void copyDetails();
91103
}}
92104
>
93-
Copy details
105+
{copyButtonLabel}
94106
</button>
95-
{copyStatus !== "idle" && (
96-
<span className="ml-2" role="status">
97-
{copyStatus === "copied" ? "Copied" : "Couldn't copy details"}
98-
</span>
99-
)}
107+
<span className="sr-only" role="status">
108+
{copyAnnouncement}
109+
</span>
100110
</details>
101111
)}
102112
</div>
@@ -316,7 +326,7 @@ export function ConverterScreen({ onBack }: { onBack: () => void }) {
316326
</>
317327
) : (
318328
<div className="space-y-4">
319-
<Card data-testid="success-card" className="bg-primary/5 p-6">
329+
<Card data-testid="success-card" className="overflow-hidden bg-primary/5 p-6">
320330
<div className="flex items-center gap-3 mb-4">
321331
<div className="flex h-10 w-10 items-center justify-center rounded-full bg-primary/20">
322332
<Check className="h-5 w-5 text-primary" />
@@ -358,7 +368,7 @@ export function ConverterScreen({ onBack }: { onBack: () => void }) {
358368

359369
{/* Error Display */}
360370
{error && (
361-
<Card data-testid="error-card" className="bg-destructive/10 p-4 text-center">
371+
<Card data-testid="error-card" className="overflow-hidden bg-destructive/10 p-4 text-center">
362372
{errorDownloadMessage ? (
363373
<DownloadFailureDetails
364374
message={errorDownloadMessage}

src/lib/__tests__/download-failures.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,33 @@ describe("buildDownloadFailureMessage", () => {
172172
).toBe(expected);
173173
});
174174

175+
it("keeps rejected-download remedies consistent", () => {
176+
const message = buildDownloadFailureMessage(
177+
summary(["http_error", 4, ["one.jpg", "two.jpg", "three.jpg"]]),
178+
4,
179+
true,
180+
);
181+
182+
expect(message?.primary).toBe(
183+
"The image server rejected 4 downloads. Export the dataset again or try later.",
184+
);
185+
expect(message?.breakdown).toEqual([
186+
"4 downloads were rejected by the image server, including one.jpg, two.jpg, three.jpg. Export the dataset again or try later.",
187+
]);
188+
});
189+
190+
it("marks truncated examples as non-exhaustive", () => {
191+
expect(
192+
buildDownloadFailureMessage(
193+
summary(["expired_url", 8, ["one.jpg", "two.jpg", "three.jpg"]]),
194+
8,
195+
true,
196+
)?.breakdown,
197+
).toEqual([
198+
"8 download links expired, including one.jpg, two.jpg, three.jpg. Export the dataset again.",
199+
]);
200+
});
201+
175202
it("uses generic mixed copy and only references existing details", () => {
176203
const message = buildDownloadFailureMessage(
177204
summary(

src/lib/download-failures.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,9 @@ function breakdownLabel(group: FailureGroup): string {
110110
case "unsupported_scheme":
111111
return `${count} download ${plural(count, "link")} used an unsupported format`;
112112
case "server_error":
113-
case "http_error":
114113
return `${count} image server ${plural(count, "error")}`;
114+
case "http_error":
115+
return `${count} ${plural(count, "download")} ${count === 1 ? "was" : "were"} rejected by the image server`;
115116
case "response_error":
116117
return `${count} incomplete ${plural(count, "download")}`;
117118
case "too_large":
@@ -139,8 +140,9 @@ function breakdownRemedy(kind: FailureKind): string {
139140
case "blocked_address":
140141
return "Try a different network or ask your network administrator.";
141142
case "server_error":
142-
case "http_error":
143143
return "Wait a moment and try again.";
144+
case "http_error":
145+
return "Export the dataset again or try later.";
144146
case "too_large":
145147
return "Use smaller images and try again.";
146148
}
@@ -149,8 +151,11 @@ function breakdownRemedy(kind: FailureKind): string {
149151
function formatBreakdown(group: FailureGroup): string {
150152
const examples = group.examples.slice(0, 3).map(safeExample);
151153
const label = breakdownLabel(group);
152-
const cause =
153-
examples.length > 0 ? `${label}${examples.join(", ")}` : label;
154+
const examplesPrefix =
155+
group.count > examples.length ? ", including " : " — ";
156+
const cause = examples.length > 0
157+
? `${label}${examplesPrefix}${examples.join(", ")}`
158+
: label;
154159
return `${cause}. ${breakdownRemedy(group.kind)}`;
155160
}
156161

0 commit comments

Comments
 (0)