From 00348f3ef58097051a0a6736d18934472fceb100 Mon Sep 17 00:00:00 2001 From: katayama8000 Date: Tue, 1 Sep 2026 09:41:56 +0800 Subject: [PATCH 1/4] fix: parse Content-Disposition instead of slicing it at the first '' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parseFileData` derived `filename` with disposition.substring(disposition.indexOf("''") + 2) which is wrong for every header shape a Backlog attachment arrives with. `indexOf` returns -1 when there is no RFC 5987 form — the common case, since a plain `filename="…"` contains no `''` — so `substring(1)` returned the whole header minus its first character. When the extended form was present the value came back still percent-encoded, which is every attachment with a non-ASCII name. Parse the header instead, following RFC 6266: `filename*` takes precedence over `filename`, its `''` prefix is dropped and the rest decoded; a quoted `filename` is matched before the unquoted one, because a quoted value may contain the `;` the unquoted form ends at. An absent header still yields an empty string. The one existing test used `filename*=UTF-8''test.png` — extended notation with nothing to decode, the single input the old code happened to get right. The added cases cover the shapes it did not; seven of the eight fail without this change. Fixes #195 Co-Authored-By: Claude Opus 5 (1M context) --- src/backlog.ts | 44 +++++++++++++++++++++++++++++++++++++++++--- test/test.ts | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/src/backlog.ts b/src/backlog.ts index 2c723be..294c942 100644 --- a/src/backlog.ts +++ b/src/backlog.ts @@ -3,6 +3,46 @@ import * as Entity from "./entity"; import Request from "./request"; import type { Fetch } from "./types"; +/** + * Extracts the filename from a `Content-Disposition` header. + * + * Per RFC 6266, `filename*` (the RFC 5987 extended notation, used for + * non-ASCII names) takes precedence over `filename` when a header carries + * both. Its value is `''`, so the prefix + * is dropped and the rest is decoded. A plain `filename` is never + * percent-encoded and is returned as written, with the quoted form unescaped. + * + * Returns an empty string when the header is absent or carries no filename. + */ +const parseContentDispositionFilename = (disposition: string | null): string => { + if (!disposition) { + return ""; + } + + const extended = /(?:^|;)\s*filename\*\s*=\s*([^;]+)/i.exec(disposition); + if (extended) { + const encoded = /^[^']*'[^']*'(.*)$/.exec(extended[1].trim()); + if (encoded) { + try { + return decodeURIComponent(encoded[1]); + } catch { + // Malformed percent-encoding: better the raw value than nothing. + return encoded[1]; + } + } + } + + // A quoted value may contain `;`, so it has to be matched before the + // unquoted form, which ends at the first one. + const quoted = /(?:^|;)\s*filename\s*=\s*"((?:[^"\\]|\\.)*)"/i.exec(disposition); + if (quoted) { + return quoted[1].replace(/\\(.)/g, "$1"); + } + + const plain = /(?:^|;)\s*filename\s*=\s*([^;]*)/i.exec(disposition); + return plain ? plain[1].trim() : ""; +}; + export default class Backlog extends Request { constructor(configure: { host: string; @@ -1408,12 +1448,10 @@ export default class Backlog extends Request { blob: () => response.blob(), }); } else { - const disposition = response.headers.get("Content-Disposition"); - const filename = disposition ? disposition.substring(disposition.indexOf("''") + 2) : ""; resolve({ body: (response).body, url: response.url, - filename: filename, + filename: parseContentDispositionFilename(response.headers.get("Content-Disposition")), }); } }); diff --git a/test/test.ts b/test/test.ts index 6ff4bf3..ee033e1 100644 --- a/test/test.ts +++ b/test/test.ts @@ -494,6 +494,51 @@ describe("Backlog API", () => { expect(data).toHaveProperty("filename", "test.png"); }); + // Every shape Backlog can send. The extended notation without any + // percent-encoding, covered above, is the one the previous `substring` + // implementation happened to get right. + it.each([ + ['attachment; filename="report.png"', "report.png"], + ["attachment; filename=report.png", "report.png"], + ["attachment; filename*=UTF-8''%E5%9B%B3%E9%9D%A2.png", "図面.png"], + // RFC 6266: `filename*` wins when a header carries both. + ["attachment; filename=\"fallback.png\"; filename*=UTF-8''%E5%9B%B3.png", "図.png"], + // A quoted value may contain the delimiter. + ['attachment; filename="quarter;summary.pdf"', "quarter;summary.pdf"], + // A plain value is not percent-encoded, so `%30` is two literal characters. + ['attachment; filename="20%30report.pdf"', "20%30report.pdf"], + ["attachment", ""], + ])("should read the filename from %j", async (disposition, expected) => { + const issueIdOrKey = "BLG-1"; + const attachmentId = 1; + mockRequest({ + method: "GET", + path: `/api/v2/issues/${issueIdOrKey}/attachments/${attachmentId}`, + reqHeaders: { "Backlog-API-Key": apiKey }, + status: 200, + data: "dummy", + headers: { "Content-Disposition": disposition }, + times: 1, + }); + const data = await backlog.getIssueAttachment(issueIdOrKey, attachmentId); + expect(data).toHaveProperty("filename", expected); + }); + + it("should return an empty filename when the header is absent.", async () => { + const issueIdOrKey = "BLG-1"; + const attachmentId = 2; + mockRequest({ + method: "GET", + path: `/api/v2/issues/${issueIdOrKey}/attachments/${attachmentId}`, + reqHeaders: { "Backlog-API-Key": apiKey }, + status: 200, + data: "dummy", + times: 1, + }); + const data = await backlog.getIssueAttachment(issueIdOrKey, attachmentId); + expect(data).toHaveProperty("filename", ""); + }); + it("should add a document.", async () => { mockRequest({ method: "POST", From 5556fb3558068c506ec47997184349cd48e1f7ab Mon Sep 17 00:00:00 2001 From: katayama8000 Date: Tue, 1 Sep 2026 10:12:24 +0800 Subject: [PATCH 2/4] fix: unquote a quoted ext-value, and say what the decoded name can contain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the parser found one shape it still got wrong: a server that quotes the RFC 5987 value — `filename*="UTF-8''%E5%9B%B3.png"` — left the closing quote on the name. RFC 5987 does not permit a quoted ext-value, but it is sent, and one stray character in a filename is exactly the class of thing this change exists to remove. The other note is about what the fix now lets through. Decoding is the point of it, and `%2E%2E%2F` decoding to `../` means a caller that writes `filename` straight to a path is exposed to something the old mangling hid. Say so on the function rather than leave it for a caller to discover. Also covers the shapes probed while reviewing: whitespace inside and outside quotes, an escaped quote, and no space after the semicolon. Co-Authored-By: Claude Opus 5 (1M context) --- src/backlog.ts | 14 +++++++++++++- test/test.ts | 7 +++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/backlog.ts b/src/backlog.ts index 294c942..fbecefe 100644 --- a/src/backlog.ts +++ b/src/backlog.ts @@ -12,7 +12,16 @@ import type { Fetch } from "./types"; * is dropped and the rest is decoded. A plain `filename` is never * percent-encoded and is returned as written, with the quoted form unescaped. * + * Only UTF-8 percent-encoding is decoded, which is what the charset token + * carries in practice; anything that does not decode is returned as it + * arrived rather than throwing. + * * Returns an empty string when the header is absent or carries no filename. + * + * The result is whatever the server put in the header, now actually decoded — + * so a caller that turns it into a path must sanitise it first. Before this + * parsed the header, `%2E%2E%2F` came back encoded and `..` could not reach a + * caller by that route; it can now. */ const parseContentDispositionFilename = (disposition: string | null): string => { if (!disposition) { @@ -21,7 +30,10 @@ const parseContentDispositionFilename = (disposition: string | null): string => const extended = /(?:^|;)\s*filename\*\s*=\s*([^;]+)/i.exec(disposition); if (extended) { - const encoded = /^[^']*'[^']*'(.*)$/.exec(extended[1].trim()); + // RFC 5987 does not allow a quoted ext-value, but servers send one, and + // leaving the quotes in makes them part of the name. + const value = extended[1].trim().replace(/^"(.*)"$/, "$1"); + const encoded = /^[^']*'[^']*'(.*)$/.exec(value); if (encoded) { try { return decodeURIComponent(encoded[1]); diff --git a/test/test.ts b/test/test.ts index ee033e1..efa6654 100644 --- a/test/test.ts +++ b/test/test.ts @@ -507,6 +507,13 @@ describe("Backlog API", () => { ['attachment; filename="quarter;summary.pdf"', "quarter;summary.pdf"], // A plain value is not percent-encoded, so `%30` is two literal characters. ['attachment; filename="20%30report.pdf"', "20%30report.pdf"], + // RFC 5987 does not allow a quoted ext-value, but servers send one. + ["attachment; filename*=\"UTF-8''%E5%9B%B3.png\"", "図.png"], + // A quoted value keeps its inner whitespace; an unquoted one is trimmed. + ['attachment; filename=" spaced.png "', " spaced.png "], + ["attachment; filename= spaced.png ", "spaced.png"], + ['attachment; filename="a\\"b.png"', 'a"b.png'], + ["attachment;filename=nospace.png", "nospace.png"], ["attachment", ""], ])("should read the filename from %j", async (disposition, expected) => { const issueIdOrKey = "BLG-1"; From 9278917dc2d5046cc81ba3e4d907c436f7ab0f8a Mon Sep 17 00:00:00 2001 From: katayama8000 Date: Tue, 1 Sep 2026 13:30:31 +0800 Subject: [PATCH 3/4] test: cover the headers Backlog actually sends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every case here was written by hand from what RFC 6266 permits. None of them came from a Backlog response, and that turned out to matter: measured against a live space, Backlog always uses the extended notation — even for a pure ASCII name — and percent-encodes anything that needs it. So the shape this client meets is only ever `filename*=UTF-8''…`, and the `indexOf` returning `-1` half of the bug is not reachable through Backlog. The decoding half is, and it is worse than "non-ASCII names": a space is `%20` and a `;` is `%3B`, so `a b.csv` and `q1;summary.csv` were mangled too. Four of the five measured headers fail against the old implementation. Added as their own group, with the general RFC cases kept and labelled — they cost nothing and stop the client from depending on one server's habits. Co-Authored-By: Claude Opus 5 (1M context) --- test/test.ts | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/test/test.ts b/test/test.ts index efa6654..55c4628 100644 --- a/test/test.ts +++ b/test/test.ts @@ -494,9 +494,37 @@ describe("Backlog API", () => { expect(data).toHaveProperty("filename", "test.png"); }); - // Every shape Backlog can send. The extended notation without any - // percent-encoding, covered above, is the one the previous `substring` - // implementation happened to get right. + // Headers measured against a live Backlog space, one attachment each. Backlog + // always uses the extended notation and percent-encodes everything that needs + // it, so these are the shapes this client actually meets — the group below + // covers what RFC 6266 permits more generally. + it.each([ + ["attachment; filename*=UTF-8''shot.png", "shot.png"], + ["attachment; filename*=UTF-8''%E5%9B%B3%E9%9D%A2.png", "図面.png"], + // A space is `%20`, so a perfectly ordinary filename arrives encoded. + ["attachment; filename*=UTF-8''a%20b.csv", "a b.csv"], + // `;` inside a name is `%3B`, never a literal delimiter. + ["attachment; filename*=UTF-8''q1%3Bsummary.csv", "q1;summary.csv"], + // A `%` in the name is itself encoded, so decoding once is correct. + ["attachment; filename*=UTF-8''20%2530report.csv", "20%30report.csv"], + ])("should read the filename Backlog sends in %j", async (disposition, expected) => { + const issueIdOrKey = "BLG-1"; + const attachmentId = 3; + mockRequest({ + method: "GET", + path: `/api/v2/issues/${issueIdOrKey}/attachments/${attachmentId}`, + reqHeaders: { "Backlog-API-Key": apiKey }, + status: 200, + data: "dummy", + headers: { "Content-Disposition": disposition }, + times: 1, + }); + const data = await backlog.getIssueAttachment(issueIdOrKey, attachmentId); + expect(data).toHaveProperty("filename", expected); + }); + + // Shapes RFC 6266 permits that Backlog does not currently send. Parsing them + // costs nothing and stops the client from depending on one server's habits. it.each([ ['attachment; filename="report.png"', "report.png"], ["attachment; filename=report.png", "report.png"], From 0514ecf7b567fb9058414d551858112d546bfc9b Mon Sep 17 00:00:00 2001 From: katayama8000 Date: Tue, 1 Sep 2026 14:19:02 +0800 Subject: [PATCH 4/4] docs: cut the comments back to what a later reader needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The paragraph about `%2E%2E%2F` described this change rather than the code: "before this parsed the header" means nothing to someone reading the file afterwards, when there is no before. It belongs in the PR, and it is there. The rest was explaining what the regexes already say. Kept the two notes that stop the code being "simplified" into a bug — quoted matched before unquoted, because a quoted value may contain the `;` that terminates an unquoted one, and the quoted ext-value servers send against RFC 5987 — plus the RFC 6266 precedence rule that decides the branch order, and one clause on sanitising, which is about the value rather than about the change. 24 comment lines to 9, and the same trim in the tests. Co-Authored-By: Claude Opus 5 (1M context) --- src/backlog.ts | 29 +++++++---------------------- test/test.ts | 16 +++++----------- 2 files changed, 12 insertions(+), 33 deletions(-) diff --git a/src/backlog.ts b/src/backlog.ts index fbecefe..fbd2435 100644 --- a/src/backlog.ts +++ b/src/backlog.ts @@ -4,24 +4,12 @@ import Request from "./request"; import type { Fetch } from "./types"; /** - * Extracts the filename from a `Content-Disposition` header. + * Extracts the filename from a `Content-Disposition` header, or `""` when it + * carries none. * - * Per RFC 6266, `filename*` (the RFC 5987 extended notation, used for - * non-ASCII names) takes precedence over `filename` when a header carries - * both. Its value is `''`, so the prefix - * is dropped and the rest is decoded. A plain `filename` is never - * percent-encoded and is returned as written, with the quoted form unescaped. - * - * Only UTF-8 percent-encoding is decoded, which is what the charset token - * carries in practice; anything that does not decode is returned as it - * arrived rather than throwing. - * - * Returns an empty string when the header is absent or carries no filename. - * - * The result is whatever the server put in the header, now actually decoded — - * so a caller that turns it into a path must sanitise it first. Before this - * parsed the header, `%2E%2E%2F` came back encoded and `..` could not reach a - * caller by that route; it can now. + * Per RFC 6266 the `filename*` extended notation wins over plain `filename`; + * its `''` prefix is dropped and the rest percent-decoded. + * The value is the server's, so sanitise it before using it as a path. */ const parseContentDispositionFilename = (disposition: string | null): string => { if (!disposition) { @@ -30,22 +18,19 @@ const parseContentDispositionFilename = (disposition: string | null): string => const extended = /(?:^|;)\s*filename\*\s*=\s*([^;]+)/i.exec(disposition); if (extended) { - // RFC 5987 does not allow a quoted ext-value, but servers send one, and - // leaving the quotes in makes them part of the name. + // Servers send a quoted ext-value even though RFC 5987 forbids it. const value = extended[1].trim().replace(/^"(.*)"$/, "$1"); const encoded = /^[^']*'[^']*'(.*)$/.exec(value); if (encoded) { try { return decodeURIComponent(encoded[1]); } catch { - // Malformed percent-encoding: better the raw value than nothing. return encoded[1]; } } } - // A quoted value may contain `;`, so it has to be matched before the - // unquoted form, which ends at the first one. + // Before the unquoted form: a quoted value may contain the `;` that ends one. const quoted = /(?:^|;)\s*filename\s*=\s*"((?:[^"\\]|\\.)*)"/i.exec(disposition); if (quoted) { return quoted[1].replace(/\\(.)/g, "$1"); diff --git a/test/test.ts b/test/test.ts index 55c4628..0758ef6 100644 --- a/test/test.ts +++ b/test/test.ts @@ -494,18 +494,14 @@ describe("Backlog API", () => { expect(data).toHaveProperty("filename", "test.png"); }); - // Headers measured against a live Backlog space, one attachment each. Backlog - // always uses the extended notation and percent-encodes everything that needs - // it, so these are the shapes this client actually meets — the group below - // covers what RFC 6266 permits more generally. + // Measured against a live Backlog space: it always uses the extended notation + // and percent-encodes anything that needs it, so `%20`, `%3B` and `%2530` are + // what an ordinary filename arrives as. it.each([ ["attachment; filename*=UTF-8''shot.png", "shot.png"], ["attachment; filename*=UTF-8''%E5%9B%B3%E9%9D%A2.png", "図面.png"], - // A space is `%20`, so a perfectly ordinary filename arrives encoded. ["attachment; filename*=UTF-8''a%20b.csv", "a b.csv"], - // `;` inside a name is `%3B`, never a literal delimiter. ["attachment; filename*=UTF-8''q1%3Bsummary.csv", "q1;summary.csv"], - // A `%` in the name is itself encoded, so decoding once is correct. ["attachment; filename*=UTF-8''20%2530report.csv", "20%30report.csv"], ])("should read the filename Backlog sends in %j", async (disposition, expected) => { const issueIdOrKey = "BLG-1"; @@ -523,8 +519,8 @@ describe("Backlog API", () => { expect(data).toHaveProperty("filename", expected); }); - // Shapes RFC 6266 permits that Backlog does not currently send. Parsing them - // costs nothing and stops the client from depending on one server's habits. + // Shapes RFC 6266 permits that Backlog does not send, so the client does not + // depend on one server's habits. it.each([ ['attachment; filename="report.png"', "report.png"], ["attachment; filename=report.png", "report.png"], @@ -533,11 +529,9 @@ describe("Backlog API", () => { ["attachment; filename=\"fallback.png\"; filename*=UTF-8''%E5%9B%B3.png", "図.png"], // A quoted value may contain the delimiter. ['attachment; filename="quarter;summary.pdf"', "quarter;summary.pdf"], - // A plain value is not percent-encoded, so `%30` is two literal characters. ['attachment; filename="20%30report.pdf"', "20%30report.pdf"], // RFC 5987 does not allow a quoted ext-value, but servers send one. ["attachment; filename*=\"UTF-8''%E5%9B%B3.png\"", "図.png"], - // A quoted value keeps its inner whitespace; an unquoted one is trimmed. ['attachment; filename=" spaced.png "', " spaced.png "], ["attachment; filename= spaced.png ", "spaced.png"], ['attachment; filename="a\\"b.png"', 'a"b.png'],