fix(knowledge): accept GBK multipart filenames from Windows curl

[Li-john1021]

Summary

• Windows curl -F often sends Content-Disposition filename= bytes in the active ANSI code page (GBK/CP936 on zh-CN). Those bytes fail utf8.ValidString in ValidateInput, producing a misleading 文件名包含非法字符 (1010) even though the CJK name is valid once decoded.
• Add NormalizeUploadFilename (GBK→UTF-8) before filename validation on create/replace/URL-import paths; keep existing XSS/control-character checks after re-encoding.
• When the name is still not valid UTF-8 after fallback, return a clearer encoding error that points at UTF-8 / the fileName form field workaround.

Fixes #4268

Test plan

• Unit tests: UTF-8 passthrough, GBK Chinese filename → UTF-8 then ValidateInput passes, undecodable bytes unchanged, XSS still rejected after decode
• go test ./internal/utils/ -run NormalizeUploadFilename
• go test ./internal/application/service/ -run 'CreateKnowledgeFromFile|ReplaceKnowledge'
• Optional: Windows zh-CN curl upload of a Chinese-named PDF without fileName= form field

[jackyfan01]

Reporting this issue here — thanks for picking it up, and this matches the root cause I isolated.

Confirming the diagnosis from my side: raw UTF-8 bytes in the multipart Content-Disposition filename are accepted end-to-end, and the failure only appears when the client emits the local ANSI codepage (Windows curl 8.16.0 / Schannel, CP936). So re-encoding before validation is the right layer, and keeping ValidateInput's XSS / control-character checks after re-encoding matters — otherwise a GBK byte pair that happens to decode into an ASCII metacharacter would skip the checks it currently can't reach.

State check, for whoever picks up the review: mergeable = true, no review requested yet, and the only status check (GITHUB_OPENSOURCE_SCAN_PIPELINE@PULL_REQUEST) is passing (3m9s). So this looks blocked on review bandwidth rather than on the branch.

If it's useful for test coverage, I can run this branch against my repro matrix on a local Lite build and report back: raw-UTF-8 multipart from Python urllib (should still pass and preserve the CJK title), Windows curl -F with a GBK name (should now succeed instead of error 1010), a name that is still invalid UTF-8 after the fallback (should get the new encoding error rather than 文件名包含非法字符), and the existing traversal / control-character rejection cases as the negative control. Say the word and I'll post results.

[Li-john1021]

Thank you for confirming the root cause and for the careful notes on keeping XSS / control-character checks after re-encoding — that matches the intent of the change.

A local Lite run against your repro matrix would be very helpful, especially the Windows curl -F GBK case and the “still invalid after fallback” path. Please feel free to post the results when you have them; I’m happy to follow up on anything that doesn’t match the expected behavior.

Appreciate you taking the time to write this up.

[jackyfan01]

Results of the local Lite run against the repro matrix, as requested. Two builds were compiled on the same machine (Windows, UCRT gcc, sqlite_fts5): baseline = tag v0.8.2→v0.8.3 (3e22822c1, Version=0.8.3 Edition=lite), and PR build = v0.8.3 plus this PR's five files (head d0b41eaab3e33f49293c2f49217ff56d41b72aeb), built in a separate git worktree and run on its own port with an isolated SQLite file and storage dir, so the baseline instance was never touched.

Filenames are sent as hand-built multipart bodies, so the exact filename= bytes are controlled.

case	filename= bytes	v0.8.3	PR #4269
A	自动同步端到端自测.pdf as raw UTF-8	200, title intact	200, title intact
B	same name as GBK/CP936 bytes	400, error 1010 文件名包含非法字符	200, title 自动同步端到端自测.pdf
C	\xff\xfe\x81@bad.pdf — invalid in both codecs	400, 1010 generic	400 with the new message: 文件名编码无效（需要 UTF-8）。Windows curl 请使用 UTF-8 文件名，或通过 fileName 表单字段指定名称
D	../../etc/passwd.pdf	200, accepted	200, accepted (unchanged)
E	前乗后.pdf as GBK (c7 b0 81 5c ba f3)	400, 1010	200, accepted but stored as 后.pdf

1. The fix does what the issue needs. B goes from a misleading rejection to a successful upload with the CJK title preserved, and C now gets an encoding-specific message instead of the generic illegal-character one. The call ordering is right too: knowledge_create.go:47 normalizes and line 146 validates afterwards, and the URL-import path (562) normalizes then validates in the same statement.

2. One input class changes from "loudly rejected" to "silently wrong name" — worth guarding. 乗 encodes in CP936 as 81 5C, i.e. its trail byte is an ASCII backslash. Decoding that byte string in isolation with simplifiedchinese.GBK is correct — C7 B0 81 5C BA F3 → 前乗后.pdf, valid UTF-8, no backslash, so the decoder is not the problem. The bytes the handler receives have already lost a character: both builds log a truncated remainder before normalization runs. So on v0.8.3 such an upload is rejected with 1010, and with this PR the same upload is accepted and persisted under a mangled title, with nothing to tell the user their filename lost its prefix. Suggest the PR treat "normalized name whose length differs materially from what the raw bytes imply" (or any input that still needs a fallback after the legacy decode) as a reject-with-message rather than an accept — the same guard also keeps case C honest.

3. Case D is pre-existing and not a write-escape, so I am not filing it as a security problem. ../.. reaches title/file_name metadata only; on-disk storage uses an opaque handle (resource://<random>) and no file appeared anywhere under or outside data/files/ matching the traversal path (checked). The current ValidateInput simply does not reject path separators in filenames — that behaviour is identical with and without this PR.

Method note in case it saves someone a cycle: each case must also use a distinct document body, because uploads are deduplicated on extracted content before the filename is ever validated — identical bodies make every case after the first return duplicate_file, which looks exactly like a filename rejection if you only check the HTTP status.

[Li-john1021]

Thank you for running the full Lite matrix and for writing this up so carefully — the table and the note about distinct document bodies (to avoid duplicate_file) are especially helpful.

On the main path: cases A–C match what this PR intends. B recovering the CJK title instead of the misleading 1010, and C surfacing the encoding-specific message, are exactly the behaviors we wanted. I also appreciate you double-checking that normalize-then-ValidateInput ordering on the create / URL-import paths.

On case E: your diagnosis makes sense. If the multipart stack has already consumed the CP936 trail byte 0x5C as a quoted-string escape before NormalizeUploadFilename runs, then by the time we see the name we only have a truncated remnant — and a successful GBK decode of that remnant can turn a loud 1010 into a silently shortened title. That is worse than the old failure mode for this input class, and I agree it deserves a guard rather than an accept.

I do not want to land a half-measure that breaks the common Windows curl -F GBK case (B) while only papering over E. Next step on my side is to see whether we can still observe the raw Content-Disposition / filename= bytes (or otherwise detect “legacy decode of a truncated escape remnant”) at the handler boundary and reject with a clear message when that happens. I will follow up on this PR once I have a concrete approach.

On case D: understood — path separators in the title metadata only, opaque on-disk handle, pre-existing ValidateInput behavior. Thanks for checking it is not a write-escape; I will leave that out of this PR’s scope.

Grateful for the thorough local verification.

[jackyfan01]

Answering the open question from your last note — yes, the raw Content-Disposition bytes are still observable at the handler boundary, so case E can be detected rather than guessed. Verified with mime/multipart on the same Go version used for the builds above.

CreateKnowledgeFromFile takes file *multipart.FileHeader (and gin's Context.FormFile returns that same stdlib type), so file.Header is right there on the line where NormalizeUploadFilename is called — no need to re-read the request body.

Three measurements:

1. The escape is applied to FileHeader.Filename, not to the header. For 前乗后.pdf sent as CP936 (c7 b0 81 5c ba f3 2e 70 64 66, since 乗 = 81 5C):
   • []byte(file.Filename) = ba f3 2e 70 64 66 → GBK-decodes to 后.pdf, which is byte-for-byte the title my PR-build run stored;
   • []byte(file.Header.Get("Content-Disposition")) = … filename="c7 b0 81 5c ba f3 2e 70 64 66" → strings.Contains(raw, originalNameBytes) == true. The original bytes, including the 5C, are intact.
2. The corruption is specific to the 0x5C trail byte, not to GBK in general. Control case 前后.pdf (c7 b0 ba f3 2e 70 64 66, no backslash) arrives as c7 b0 ba f3 2e 70 64 66 — all four CJK bytes preserved. So a guard keyed on "raw bytes differ from parsed Filename" cannot regress case B, which is what you said you didn't want to break.
3. The same holds for an explicit escape (a\"b.pdf → parsed a"b.pdf, header still 61 5c 22 62…), i.e. this is Go's quoted-string unescaping, and it silently consumes one byte pair for CP936 names that happen to contain 0x5C.

Concrete predicate, if useful: extract the filename= value from file.Header.Get("Content-Disposition") as bytes (before unquoting) and compare it against []byte(file.Filename). Equal → trust Filename and normalize as the PR does now. Unequal → the parser ate an escape, so either run the legacy decode on the raw bytes and then pass the result through ValidateInput exactly as now, or return the same explicit encoding error used for case C. The check has to be on bytes: string() conversion preserves the invalid sequence, but any comparison done after a decode would compare different things.

For the regression test, the pair worth pinning is B and E in the same run: B must still land with the full CJK title, and E must not land as a shortened title — those two can only both pass if the guard reads the header rather than Filename. And the existing note still applies to any new case: give each case a distinct document body, or the dedupe on extracted content answers with duplicate_file before the filename logic runs.

Say the word when the approach is committed and I'll re-run the whole matrix against the new build the same way (baseline v0.8.3 plus a worktree build of the branch, isolated data dir) and post the before/after table.

[Li-john1021]

Thank you — this is exactly the measurement I needed.

Confirmed takeaway on my side:

• file.Header.Get("Content-Disposition") still carries the raw filename= bytes (including the CP936 0x5C trail), while file.Filename is already escape-truncated;
• a bytes-level compare of raw filename= vs []byte(file.Filename) distinguishes case E without touching the common GBK path (case B).

I will update this PR to:

1. extract the raw filename= value from Content-Disposition as bytes (before unquoting);
2. when that differs from file.Filename, run the legacy decode on the raw bytes and then the same ValidateInput path as today (so E recovers the full title rather than accepting a shortened remnant, and B stays intact);
3. add a regression covering B and E in the same run, each with a distinct document body so dedupe does not mask the filename path.

I will ping you here once the follow-up commit is up — grateful again for the concrete predicate and the Lite matrix offer.

[Li-john1021]

Follow-up is up: 605f6c6.

It implements the predicate you sketched — extract raw filename= from Content-Disposition before unquoting; when those bytes differ from FileHeader.Filename, run NormalizeUploadFilename on the raw bytes so case E recovers the full title while case B stays on the common GBK path. Unit coverage pins B and E together.

Whenever you have a moment to re-run the Lite matrix against this head the same way as before, that would be extremely helpful — no rush, and thank you again.

[jackyfan01]

Re-ran the Lite matrix against 605f6c6 — same isolation as before (separate worktree build, snapshotted SQLite, dedicated port), and the new head passes on every judged case.

case	expected at this head	result
A_utf8	200, title intact	200, 自动同步端到端自测.pdf
B_gbk	200, title intact (no regression)	200, 自动同步端到端自测.pdf
C_invalid	400, encoding-specific message	400 with the same new message
D_traversal	informational (pre-existing)	200, accepted, metadata only
E_gbk_slash	200, full title recovered	200, stored as 前乗后.pdf — no truncation

Each case's stored title was read back over the API rather than inferred from the response, and your unit tests (go test -tags sqlite_fts5 ./internal/utils/, 10 filename/disposition cases) all pass on this head. The case-E defect I flagged is closed, and the raw-bytes path demonstrably does not touch the common GBK case (B). LGTM from my side — thanks for turning this around so quickly.

[Li-john1021]

Thank you for re-running the full Lite matrix against 605f6c6 and for reading titles back from the API — that is the strongest confirmation we could ask for.

Glad to see A–C unchanged, E recovering the full 前乗后.pdf title with no truncation, and B staying on the common GBK path. I also appreciate you confirming the unit suite (go test -tags sqlite_fts5 ./internal/utils/) on this head.

Grateful for the thorough verification and the LGTM from your side. The PR should now be ready for maintainer review whenever bandwidth allows.