fix: parse Content-Disposition instead of slicing it at the first ''

[katayama8000]

Fixes #195.

The bug

parseFileData derived FileData.filename with one substring:

const filename = disposition ? disposition.substring(disposition.indexOf("''") + 2) : "";

Two problems in that line:

1. indexOf returns -1 when there is no RFC 5987 form. substring(-1 + 2) is substring(1) — the whole header with its first character removed. That is the common case: a plain filename="…" contains no ''.
2. When the extended form is present, the value is left percent-encoded. Every attachment with a non-ASCII name came back as %E5%9B%B3….png.

Not one header shape produced a usable name. It has been this way since b191514 (2016-08-30), shipped in every release from 0.9.0 on. Affects every file-returning method: getIssueAttachment, getWikiAttachment, getPullRequestAttachment, downloadDocumentAttachment, getSharedFile, getSpaceIcon, getUserIcon, getProjectIcon, getTeamIcon.

The fix

A parseContentDispositionFilename helper that follows RFC 6266:

• filename* takes precedence over filename when a header carries both. Its <charset>'<language>' prefix is dropped and the remainder decoded; malformed percent-encoding falls back to the raw value rather than throwing.
• A quoted filename is matched before the unquoted one, because a quoted value may contain the ; that terminates an unquoted one, and the quoted-pair escapes are unwound.
• An unquoted filename is taken as written — RFC 6266 plain values are not percent-encoded, so %30 stays two characters.
• An absent header still yields "", unchanged.

Content-Disposition	before	after
attachment; filename="report.png"	ttachment; filename="report.png"	report.png
attachment; filename=report.png	ttachment; filename=report.png	report.png
attachment; filename*=UTF-8''%E5%9B%B3%E9%9D%A2.png	%E5%9B%B3%E9%9D%A2.png	図面.png
attachment; filename="fallback.png"; filename*=UTF-8''%E5%9B%B3.png	%E5%9B%B3.png	図.png
attachment; filename="quarter;summary.pdf"	ttachment; filename="quarter	quarter;summary.pdf
attachment; filename="20%30report.pdf"	ttachment; filename="20%30report.pdf"	20%30report.pdf
attachment	ttachment	""
absent	""	""

Why one test did not catch it

The existing coverage was a single case:

"Content-Disposition": "attachment; filename*=UTF-8''test.png",
...
expect(data).toHaveProperty("filename", "test.png");

Extended notation with nothing to decode — the one input the old code happened to get right. It still passes, and is left as it was. The added it.each covers the shapes it did not: seven of the eight new cases fail without this change (the eighth, an absent header, is behaviour this preserves).

Compatibility

This changes what filename returns, which is the point of the fix — but only from unusable values to correct ones. A caller doing nothing with filename is unaffected; a caller that repairs it downstream (see nulab/backlog-mcp-server#187, whose normalizeFilename exists solely for this) keeps working and can eventually drop the workaround.

FileData's shape is unchanged. No public API is added or removed.

Verified

npx oxlint, oxfmt --check, tsc --noEmit, npm run build all clean, and vitest run — 49 tests — passes on Node 22.23.2, 24 and 26.8.1, matching the CI matrix.

🤖 Generated with Claude Code

[katayama8000]

Self-reviewed the parser by extracting it from src/backlog.ts and driving it over header shapes the tests did not cover. Two things came out of it, both now pushed.

A shape it still got wrong. filename*="UTF-8''%E5%9B%B3.png" returned 図.png" — the closing quote survived. RFC 5987 does not allow a quoted ext-value, but servers send one, and a stray character in a filename is the exact class of defect this PR exists to remove. The ext-value is now unquoted before the charset'lang' prefix is stripped.

Something the fix lets through that the bug hid. Decoding is the point of this change, and it means filename*=UTF-8''%2E%2E%2Fetc%2Fpasswd now yields ../etc/passwd where it used to yield the inert %2E%2E%2Fetc%2Fpasswd. Same for a plain filename="../../etc/passwd", which used to come back as mangled header text. The parser does not sanitise — reporting the header and sanitising a path are different jobs, and content-disposition's parse does not either — but a caller writing this to disk is exposed to something it was not before, so the function now says so rather than leaving it to be discovered.

Shapes probed that behave correctly and are now covered by tests where useful: a similarly named parameter (xfilename=), a decoy filename= inside another parameter's quoted value, inline instead of attachment, whitespace before =, a lowercase charset token, an escaped quote inside a quoted value, no space after the semicolon, whitespace inside quotes (kept) versus unquoted (trimmed), a NUL byte, and a raw non-ASCII name.

54 tests pass; oxlint, oxfmt --check, tsc --noEmit and npm run build are clean.

[katayama8000]

Correcting the framing in this PR and in #195, after measuring against a live Backlog space instead of reasoning from the spec.

Every header in the original table was one I wrote by hand. None came from a Backlog response, and the difference matters.

What Backlog actually sends

One attachment each, headers read straight off the response:

uploaded as	Content-Disposition
shot.png	attachment; filename*=UTF-8''shot.png
図面.png	attachment; filename*=UTF-8''%E5%9B%B3%E9%9D%A2.png
data.csv	attachment; filename*=UTF-8''data.csv
a b.csv	attachment; filename*=UTF-8''a%20b.csv
q1;summary.csv	attachment; filename*=UTF-8''q1%3Bsummary.csv
20%30report.csv	attachment; filename*=UTF-8''20%2530report.csv

Backlog always uses the extended notation — even for a pure ASCII name — and percent-encodes everything that needs it.

What that changes

The indexOf returning -1 half of the bug is not reachable through Backlog. '' is always present, so substring never falls back to "the header minus its first character". #195 says the result is wrong for every header shape a Backlog attachment arrives with; that is wrong, and I should have measured before writing it. It stays a real defect for any other server, and for Backlog if the format ever changes, but it is latent rather than live.

The decoding half is live, and worse than I described it. I framed it as "every attachment with a non-ASCII name". A space is %20 and a ; is %3B, so a b.csv came back as a%20b.csv and q1;summary.csv as q1%3Bsummary.csv. Four of the six measured headers are mangled by the current implementation — a filename with a space in it is not an edge case.

The severity is unchanged; the reason for it is narrower and more common than stated.

What changed in the PR

9278917 adds the measured headers as their own group, labelled with where they came from. Two of them cover shapes the hand-written cases missed entirely: a %20 space, and a % that arrives double-encoded as %2530. Four of the five fail against the current implementation.

The RFC-general cases are kept, in a second group labelled as shapes Backlog does not currently send — parsing them costs nothing and keeps the client from depending on one server's habits.

The fix itself is unchanged and needs no revision — it is RFC 6266 parsing, so it handles what Backlog sends and what it does not. 59 tests, lint, format, tsc and build all pass.

[mmktomato]

@katayama8000
I added a comment 🙏

[mmktomato]

This paragraph won't make sense after this task is over.

Also, the comments are long overall, so please take another look 🙏

[katayama8000]

Both fair — fixed in 0514ecf.

You are right that the paragraph does not survive the merge. It said "before this parsed the header, %2E%2E%2F came back encoded" — which describes the change, not the code, and once merged there is no before for a reader to compare against. Removed; the point lives in the PR description where it belongs. What is left of it is one clause on the value being the server's, which is about the value rather than about this task.

On length: 24 comment lines down to 9, and the same trim in the tests. I kept only the notes that stop the code being "simplified" back into a bug —

• quoted matched before unquoted, because a quoted value may contain the ; that terminates an unquoted one
• the quoted ext-value servers send against RFC 5987

— plus the RFC 6266 precedence rule, which is what decides the branch order. Everything else was restating the regexes.

59 tests, lint, format and tsc still pass.

[katayama8000]

@mmktomato
I found a npm package but I do not think I need this for this time.
https://github.com/jshttp/content-disposition

[katayama8000]

Should have said in the description why this is hand-written rather than content-disposition (jshttp), since that is the obvious question.

I checked it properly: v3.0.0 has no dependencies, uses no Node built-ins, and parses all five measured Backlog headers correctly — including the decoding, which it exposes as parameters.filename alongside the raw filename*. It would reduce this to two lines. My assumption that it would drag in Buffer and break the browser bundle was wrong; v3 dropped that.

Reasons I still left it out, for you to overrule:

• dependencies is currently just qs. This adds a second for ~30 lines.
• ~13.5 KB unminified lands in dist/backlog.iife.min.js, which is the browser entry — and Enable tree-shaking #137 is about tree-shaking.
• Its engines is node >= 22, so it raises this library's floor.
• One behavioural difference: on malformed percent-encoding it omits filename entirely, where this returns the raw value. Neither is obviously right.

None of these is decisive and the trade is yours, not mine — happy to switch if you would rather have the dependency than the code.

[mmktomato]

@katayama8000

the trade is yours, not mine

Is this something for me to decide, or was that you AI agent asking you?
Either way, I don't think the npm library is necessary.

[katayama8000]

@mmktomato
AI asked me so I think We’re on the same page.
We do not need the npm library.

[mmktomato]

@katayama8000
LGTM!!!