Skip to content

feat(file): expand upload extension whitelist with low-risk formats (closes #460)#461

Open
pkuWMH wants to merge 1 commit into
Mininglamp-OSS:mainfrom
pkuWMH:feat/file-ext-whitelist-460
Open

feat(file): expand upload extension whitelist with low-risk formats (closes #460)#461
pkuWMH wants to merge 1 commit into
Mininglamp-OSS:mainfrom
pkuWMH:feat/file-ext-whitelist-460

Conversation

@pkuWMH

@pkuWMH pkuWMH commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

What

Expands the server-side upload extension whitelist (allowedExtensions in modules/file/const.go) to cover common low/no-execution-risk formats that were previously rejected with 不支持上传<ext>类型的文件 (400).

Closes #460.

Why

The real upload gate is a whitelist — anything not listed is rejected. It was missing a lot of safe everyday formats, most notably Apple iWork files (.key triggered this) and the iPhone-default .heic photo format.

Added (low/no execution risk)

  • Apple iWork (zip-based documents): .key .numbers .pages
  • Modern images: .heic .heif .tiff .tif
  • OpenDocument presentation (odt/ods were present, odp missing): .odp
  • Ebooks: .epub .mobi
  • Text / data: .toml .ini .log .tsv .ndjson
  • Subtitles: .srt .vtt .ass
  • Audio: .opus .aiff

Intentionally NOT added (execution / XSS risk)

.svg (embeddable <script>, stored-XSS), .xlsm / .docm / .pptm (Office macros), .iso / .img (mountable disk images). These need separate security review and are out of scope here.

Tests

  • Updated TestIsAllowedExtension_AllEntries to include the new extensions.
  • Added TestIsAllowedExtension_NewlyAddedExtensions: asserts each new extension is allowed and not blocked, and that the deliberately-excluded risky ones return false.
  • go build ./... → OK
  • go test ./modules/file/... → ok

Notes

None of the added extensions collide with blockedExtensions. The change is confined to the whitelist map + its tests; no unrelated refactoring.

Add Apple iWork (.key/.numbers/.pages), modern images (.heic/.heif/.tiff/.tif),
.odp, ebooks (.epub/.mobi), text/data (.toml/.ini/.log/.tsv/.ndjson),
subtitles (.srt/.vtt/.ass) and audio (.opus/.aiff) to allowedExtensions.

Risky formats (.svg/.xlsm/.docm/.pptm/.iso/.img) are intentionally excluded.

Closes Mininglamp-OSS#460
@pkuWMH pkuWMH requested a review from a team as a code owner June 25, 2026 05:16
@github-actions github-actions Bot added the size/M PR size: M label Jun 25, 2026

@OctoBoooot OctoBoooot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat(file): expand upload extension whitelist with low-risk formats (closes #460) (#461)

Verdict: Request changes — check-sprint / check-sprint is FAILURE (process gate; despite the body containing Closes #460, check-sprint is the same gate flagged on prior PRs that requires additional sprint metadata). Code itself is clean — no blockers, no majors, no minors, one minor security note + one informational. Once CI clears this is an APPROVE candidate without code changes.

Verified — author's load-bearing claims hold

Applied [description-vs-diff-crosscheck] discipline to the description's three claims:

  • "None of the added extensions collide with blockedExtensions" — verified by enumeration. Added: .heic .heif .tiff .tif .key .numbers .pages .epub .mobi .toml .ini .log .tsv .ndjson .srt .vtt .ass .opus .aiff .odp. Blocked: .exe .bat .sh .cmd .msi .dll .com .scr .pif .vbs .vbe .js .jse .wsf .wsh .ps1 .sys .cpl .inf .reg .apk .ipa .php .jsp .asp .aspx .cgi .py .rb .pl. Disjoint set. (.ini.inf; .opus ≠ none-similar; etc.)
  • Extension check is uniform across all upload sitesIsAllowedExtension/IsBlockedExtension are called consistently at modules/bot_api/file.go:172, :274, modules/file/api.go:183-188, :486, modules/robot/api.go:1567. All paths use both checks (blocked-first inside IsAllowedExtension + outer IsBlockedExtension). Adding to whitelist takes effect everywhere.
  • Case-insensitive check (const.go:244 ext = strings.ToLower(ext)) — .KEY / .HEIC / .Tiff all work as expected.
  • Tests pin both positive and negative directionsTestIsAllowedExtension_NewlyAddedExtensions asserts each new extension is allowed AND not blocked, AND that .svg .xlsm .docm .pptm .iso .img (the deliberately-excluded risky ones) return false. This is the [test-the-negative] discipline applied correctly — a future "let's just add .svg too" would fail the negative-direction pin. TestIsAllowedExtension_AllEntries updated to mirror the full map.

Minor — stored-XSS surface (informational, not regression)

.html and .htm were ALREADY in the whitelist (const.go:139 pre-PR). This PR adds .epub and .mobi which are also nominally HTML-containers (epub = zipped HTML; mobi = similar). If the file-serving path returns these inline (no Content-Disposition: attachment or X-Content-Type-Options: nosniff), a malicious uploader could craft an epub/mobi that triggers stored-XSS when a victim previews it inline.

This is not a regression.html is the dominant existing risk on the same surface — but the new additions widen it slightly. The right defense is at the serve-side (force-attachment headers for these MIME types), not at the upload-allowlist side. Out of scope for this PR unless the team treats "expanding allowlist requires re-auditing serve-side" as a precondition.

The author's explicit exclusion of .svg with rationale ("embeddable <script>, stored-XSS") shows awareness of the XSS threat model — and .svg is the highest-risk SVG-as-image case where the file_kind discriminator most easily fools the renderer. Excluding .svg while including .epub/.html is internally consistent if the serve-side does force-attachment on container types (worth confirming, but pre-existing).

Praise

  • Description structure is exactly the right shape for a security-touching change: "Added (low/no execution risk)" + "Intentionally NOT added (execution / XSS risk)" + per-category grouping with rationale. Reviewer can scan the threat model in 30 seconds — what's let in, what's deliberately not, what the principle is. .svg/.xlsm/.docm/.pptm/.iso/.img are exactly the categories I'd want to see explicitly excluded with a one-liner why.
  • Test pins the deliberately-excluded set (const_test.go:201-205) — without this, a future PR that adds .svg to the allowlist wouldn't fail any test; with it, the assertion IsAllowedExtension(".svg") == false breaks loudly and forces the author to either remove the test or document the security review. Same discipline pattern as PR #423's mdLinkText negative tests.
  • Comment grouping at const.go:131-152 preserves the file's existing visual structure (// 图片 / 文档 / 音频 / 视频) and adds new categories (// Apple iWork, // 电子书, etc.) inline rather than appending an unstructured block. Future maintainer adding a .heic-equivalent format hits the right place by category.
  • Disjoint sets verified by description — author's "None of the added extensions collide with blockedExtensions" claim is exactly the assertion I'd want a reviewer to make at the description level so I can grep-verify in 5 seconds. Compare to "trust me, I checked" which forces full re-verification.

Out of scope (informational)

  • Pre-existing .html / .htm allowlist entry is the dominant stored-XSS surface on this code path. Worth a separate threat-review of the file serve path (force-attachment headers? CSP? Content-Type sniffing?) but that's a different PR.
  • CI check-sprint failure: PR body has Closes #460 and linked-issues API confirms #460 is linked, but check-sprint is still red. Likely the gate requires additional sprint metadata on #460 itself (sprint label / milestone). Same gate seen on other PRs in this repo.

@Jerry-Xin Jerry-Xin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is in scope for octo-server and cleanly expands the file upload extension whitelist with focused tests.

💬 Non-blocking

🔵 Suggestion: modules/file/const.go:141 says Apple iWork files have "no execution risk." That is probably fine operationally, but the wording is stronger than necessary for complex document containers. Consider softening it to "low execution risk" to match the PR framing.

🔵 Suggestion: modules/file/const.go:135 adds several binary formats that currently skip ValidateMagicNumber because no signatures are registered. That matches existing behavior for many allowed formats and is not blocking, but adding checks later for easy cases like TIFF, HEIC/HEIF, AIFF, and Opus would improve mismatch detection.

✅ Highlights

The blocked-list priority remains intact, and the new test at modules/file/const_test.go:179 covers both newly allowed extensions and deliberately excluded risky extensions (.svg, .xlsm, .docm, .pptm, .iso, .img).

Verification: go test ./modules/file/... passed. Byte-verified at head fd7c7b4b: every added extension is a passive format (modern images, iWork/OpenDocument/ebook containers, plain-text/data, subtitles, audio) — none resolve to a scriptable browser content-type. The two XSS-capable types are NOT introduced here (.html/.htm pre-existing; .svg deliberately excluded). The serve path (getFile) 302-redirects to a presigned backend URL and forces attachment for unknown MIME types, so this diff adds no stored-XSS surface.

@yujiawei yujiawei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #461 (octo-server)

Summary

This PR expands the server-side upload extension whitelist (allowedExtensions in modules/file/const.go) with 20 low/no-execution-risk formats, closing #460. The change is tightly scoped (+48 / -2 across const.go and const_test.go only), matches the agreed scope exactly, and introduces no new attack surface. Approved.

This PR is security-sensitive (upload-path change), so it received an extra-careful review of the serving path and the MIME resolution of each new extension.

1. Spec compliance

The added set matches the approved list in #460 exactly — 20 extensions, no more, no less:

  • Apple iWork: .key .numbers .pages (const.go:142)

  • Modern images: .heic .heif .tiff .tif (const.go:135)

  • OpenDocument presentation: .odp (const.go:140)

  • Ebooks: .epub .mobi (const.go:144)

  • Text/data: .toml .ini .log .tsv .ndjson (const.go:146)

  • Subtitles: .srt .vtt .ass (const.go:148)

  • Audio: .opus .aiff (const.go:152)

  • No under-build: every requested extension is present.

  • No over-build: nothing beyond the approved list; no new flags, fields, or functions.

  • Forbidden set correctly excluded: .svg, .xlsm, .docm, .pptm, .iso, .img are absent from allowedExtensions, and const_test.go:201-206 explicitly guards that they are not allowed.

  • blockedExtensions is untouched.

2. Security assessment (the core concern for a whitelist expansion)

The decisive question is whether any newly allowed extension can be served back as active content (stored XSS / script execution). I verified by running Go's mime.TypeByExtension for every new extension against the build toolchain:

  • None of the new extensions resolve to text/html or image/svg+xml — the two MIME types a browser will execute script from.
  • The genuinely risky .svg (image/svg+xml) is correctly excluded.
  • .ini and .ndjson resolve to an empty MIME, which the serve path (api.go:351-352) forces to Content-Disposition: attachment (download-only).
  • The new text/* subtitle/log/data types carry explicit non-text/plain content types, so browser MIME-sniffing will not upgrade them to HTML. No new sniffing surface is created (the only sniff-susceptible .txt = text/plain was already whitelisted).
  • .epub / .mobi are zip/binary blobs served as a single opaque object; there is no archive-member routing that could reach inner HTML — same posture as the pre-existing .docx / .odt.

Magic-number validation (ValidateMagicNumber, const.go:74-99) is skipped for these new formats because they have no fileMagicNumbers entry — this is identical to the existing behavior for .txt / .json / .csv / .md, consistent with the "no execution risk" classification, and not a regression.

Conclusion: this diff introduces no new stored-XSS, script-execution, or content-sniffing-escalation risk.

3. Code quality & tests

  • No blacklist collision: none of the 20 new keys intersect blockedExtensions (const.go:166-175), so no dead/contradictory entries.
  • Cross-file propagation is correct: all six upload gates (api.go:188, api.go:486, bot_api/file.go:172, bot_api/file.go:274, robot/api.go:1567, robot/api.go:1670) call the shared IsAllowedExtension / IsBlockedExtension against the same package-global maps, so the expansion reaches every entrypoint with no per-site change needed.
  • Tests are meaningful, not tautological: TestIsAllowedExtension_NewlyAddedExtensions asserts each new extension is allowed and not blocked, and that the excluded risky set is genuinely absent from both maps — these negative assertions would fire if a future edit ever added .svg et al.
  • No env-test map pollution: TestLoadExtensionsFromEnv snapshots both maps and restores them via t.Cleanup, so the .svg it injects cannot leak into the new .svg-negative assertion.
  • go vet and go test ./modules/file/... both pass.

4. Non-blocking note (pre-existing, out of scope for this PR)

The file-serving path (getFile, api.go:343-360) honors ?disposition=inline and sets no X-Content-Type-Options: nosniff / CSP header. The only genuinely script-capable whitelist entries (.html / .htm / .xml) were added before this PR, not here. A durable hardening would be to add nosniff (and ideally a restrictive CSP) on this serving path, but that is independent of this change and must not block it. Worth a separate follow-up issue.

Verdict

Spec-compliant, no net-new security risk, clean tests. Approved.

@mochashanyao mochashanyao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).


octo-server PR#461 Review Report

Reviewer: Octo-Q (automated review)
Head SHA: fd7c7b4b39a17e484dc829d112e03a6b87d21e93
PR: feat(file): expand upload extension whitelist with low-risk formats (closes #460)
Files changed: modules/file/const.go (+11/-1), modules/file/const_test.go (+37/-1)


1. Verification Summary

  • ✅ Build: go build ./... passes
  • ✅ Tests: go test ./modules/file/... passes (0.079s)
  • ✅ No overlap between new extensions and blockedExtensions (verified by grep + test TestIsAllowedExtension_NewlyAddedExtensions)
  • ✅ All new extensions asserted in tests (both positive allowed + negative not-blocked)
  • ✅ Risky formats (.svg, .xlsm, .docm, .pptm, .iso, .img) explicitly excluded and tested as not-allowed
  • IsAllowedExtension blocked-takes-priority logic unchanged

2. Findings

P2 — Minor: No magic number entries for new binary formats

File: modules/file/const.go fileMagicNumbers map (lines 23-70)
Diff-scope: pre-existing pattern, amplified by this PR

New binary extensions (.heic, .heif, .tiff, .tif, .epub, .mobi, .opus, .aiff, .key, .numbers, .pages, .odp) have no entries in fileMagicNumbers. ValidateMagicNumber will return true for these (permissive fallback at line ~90: "该扩展名没有魔数定义,跳过验证").

Assessment: This is consistent with many pre-existing allowed extensions (.txt, .json, .xml, .yaml, .csv, .rtf, etc. also have no magic numbers). Not a blocker — the magic number system is defense-in-depth, not the primary gate. Adding magic numbers for these formats would be a nice follow-up but is not required for landing.

P2 — Minor: .ass subtitle format has limited markup capability

File: modules/file/const.go line ~151
Diff-scope: new (this PR)

.ass (Advanced SubStation Alpha) supports override tags in {} braces and, in some players, limited Lua/ASS scripting. However:

  • Browsers do not natively render .ass files; Go's mime.TypeByExtension returns text/plain or application/octet-stream
  • No browser-side execution risk
  • The override tags are confined to subtitle rendering context, not general-purpose script execution

Assessment: Low risk. Not a blocker. Noted for completeness.

3. Suggestions (non-blocking)

  1. Future follow-up: Consider adding magic number entries for high-value binary formats (.epub = PK\x03\x04 like ZIP; .tiff = II*\x00 or MM\x00*). This would strengthen content validation on the main uploadFile path.

4. Additional Findings (pre-existing, outside PR scope)

During the data-flow trace I noted pre-existing gaps that are not introduced or amplified by this PR and thus not blockers:

  • modules/robot/api.go:botUploadFile (~line 1491) only checks ext == "" — no IsBlockedExtension or IsAllowedExtension call.
  • modules/bot_api/file.go:botUploadFile (~line 59) has zero extension validation.
  • These are separate security concerns for a future PR.

5. Data Flow Trace

Consumer: IsAllowedExtension(ext)modules/file/api.go:uploadFile (~line 188), getUploadCredentials (~line 493), botUploadCredentials and botUploadPresigned in both robot/api.go and bot_api/file.go.

Data source: allowedExtensions map (package-level var, mutated only by init()loadExtensionsFromEnv()). After init, the map is read-only.

Flow verification: New entries added to allowedExtensions map literal at compile time → available at init() completion → consumed by IsAllowedExtension on every upload request. No intermediate transformation, no getter that could return stale/empty data. Flow is correct.

Extension extraction: filepath.Ext(fileName)strings.ToLower() → passed to IsAllowedExtension. New extensions are all lowercase in the map, matching the ToLower normalization. Correct.

6. Blind Spot Checklist (R5)

  • C1 双路径 parity: N/A — no dual/symmetric paths touched. This is a map-literal expansion.
  • C2 control-flow ordering / 嵌套复用: N/A — no control flow changed. IsAllowedExtension logic unchanged.
  • C3 授权边界 ≠ 能力边界: N/A — no authorization/permission changes.
  • C4 授权生命周期 / 容器-成员状态级联: N/A — no auth changes.
  • C5 build/note ≠ 运行期路径: N/A — no build artifacts, relative paths, or packaging changes. Go map literal is identical at build and runtime.
  • C6 治理/策略/安全文档自洽性: N/A — no documentation/policy changes.

7. Cross-round Blocker Review (R6)

N/A — first review of this PR.


[Octo-Q] verdict: APPROVE

All 20 newly added extensions are genuinely low/no execution risk (text-based data formats, image containers, audio codecs, zip-based documents). None collide with blockedExtensions. Tests properly assert both allowed and not-blocked for each new entry, plus negative tests for deliberately excluded risky formats. Build and tests pass. The two P2 findings are non-blocking observations.

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Review — VERDICT: PASS

Persona: qa-engineer
Scope reviewed: modules/file/const.go (+11/-1), modules/file/const_test.go (+37/-1)
Verify path: CI Test job SUCCESS on commit (run 28148551004); no local checkout needed for a whitelist-map delta.

Coverage check (per linked issue #460)

New entries claimed in PR body Asserted in TestIsAllowedExtension_NewlyAddedExtensions Re-asserted in TestIsAllowedExtension_AllEntries
.key .numbers .pages (iWork)
.heic .heif .tiff .tif
.odp
.epub .mobi
.toml .ini .log .tsv .ndjson
.srt .vtt .ass
.opus .aiff

Every entry is double-asserted (allowed + not-blocked). The deliberately-excluded risky set (.svg .xlsm .docm .pptm .iso .img) is negatively asserted — this is the right shape; it guards against an accidental "add everything from #460" follow-up that would silently widen the surface.

Boundary / flaky / completeness

  • Map-key drift guard: TestIsAllowedExtension_AllEntries is regenerated to match the new map. Good — this is the drift sentinel for future additions.
  • Negative tests present: explicit assert.False on excluded set — catches the "I'll just add svg too while I'm here" regression.
  • Case sensitivity: not tested for the new entries. IsAllowedExtension likely normalises to lower-case already (otherwise the existing .jpg/.JPG distinction would have been caught elsewhere) — out of scope for this PR, but worth a follow-up if normalisation is not provably done at the call site.
  • Env override interaction (DM_FILE_EXTRA_ALLOWED, const.go:175): not directly tested here. The change is to the static defaults map, so env-merge behaviour is orthogonal — fine.
  • Flakiness: pure table-driven tests on a static map. Zero non-determinism.

Verdict rationale

PR is a focused, low-risk whitelist addition with positive + negative coverage for every new entry. CI Build/Test/Vet/Lint all green. check-sprint failure is unrelated metadata, not a code-correctness signal. PASS.

@lml2468 lml2468 added review:done:qa:approve qa-engineer PASS and removed review:running:qa qa-engineer review in progress labels Jun 27, 2026

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review — VERDICT: CLEARED-WITH-RISK

Persona: security-engineer
Threat model basis: STRIDE on upload pipeline + OWASP "Unrestricted File Upload"; per-extension execution / parser / XSS risk.
Diff inspected: modules/file/const.go whitelist additions only — no auth/validation/routing changes.

Per-extension risk classification

Category Extensions added Execution risk Parser/CVE history Stored-XSS risk Verdict
iWork bundles .key .numbers .pages None (zip-packaged) Negligible if not auto-extracted server-side None ✅ safe
Modern image .heic .heif None libheif had decode CVEs but only matters if server thumbnails None ✅ safe (see ⚠ below)
Legacy image .tiff .tif None libtiff has long CVE history (CVE-2022-3597/3598/3599/3626/3627 …) None ⚠ depends on processing
OpenDocument .odp None aligns with existing .odt/.ods None ✅ safe (consistent)
Ebook .epub .mobi None (zip / proprietary) Negligible if served as attachment None ✅ safe
Plain text/data .toml .ini .log .tsv .ndjson None None None inline (text/plain) ✅ safe
Subtitles .srt .vtt .ass None on upload libass CVE-2017-14227 etc. if server renders None ⚠ depends on processing
Audio .opus .aiff None Decoder CVEs possible if server transcodes None ✅ safe (likely no server decode)

Explicit exclusions — concur

The PR correctly withholds: .svg (script-in-SVG stored-XSS), .xlsm/.docm/.pptm (Office macros), .iso/.img (mountable disk images). This matches issue #460's recommended scope and is the right call. A negative test (TestIsAllowedExtension_NewlyAddedExtensions excluded set) locks this in against silent re-introduction.

Risk notes (not blocking, for ops/follow-up)

  1. TIFF parser surface: any server-side path that decodes uploaded files (thumbnail, preview, EXIF strip) and uses Go's golang.org/x/image/tiff or shells out to libtiff carries TIFF parser CVE exposure. Whitelisting upload ≠ adding processing, so net new risk depends on whether such a pipeline exists. Recommend: grep for image-decode entry points before this lands in a release.
  2. libass / subtitle rendering: same caveat — if subtitles ever feed into a rendering pipeline (rare on a chat server, but worth confirming), libass parser CVEs apply. Pure storage + download → no risk.
  3. Content-type / sniff guarantees: this PR doesn't change Content-Disposition or sniffing. Assuming existing upload handlers serve user files with Content-Disposition: attachment and a non-sniff content-type, none of the added extensions widen the served-back attack surface. If any inline-render path exists for "preview", .html/.htm (already in whitelist, pre-existing) is the larger risk than anything this PR adds.
  4. AuthZ / quota / rate-limit: not touched by this PR. Assuming existing per-user quota and size limits already cap blast radius for media types like .heic/.tiff.
  5. Env-override surface (DM_FILE_EXTRA_ALLOWED, const.go:175): unchanged. Operators retain the same escape hatch.

Verdict rationale

The whitelist deltas themselves are inert containers / text / standard media. The risks above are pre-existing, conditional on downstream processing, and not introduced by this PR. The exclusion list and its negative test are the right defensive shape. CLEARED with the ⚠ items called out for ops awareness; not blocking.

@lml2468 lml2468 added review:done:security:comment security-engineer CLEARED-WITH-RISK and removed review:running:security security-engineer review in progress labels Jun 27, 2026

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — VERDICT: APPROVE

Persona: code-reviewer
Lens: correctness / readability / maintainability / design-fit
Scope: 2 files, +48/-2; pure data + test additions.

Correctness

  • Additions are placed inside allowedExtensions (const.go:132) at the existing literal — no path collision, no shadowing. The map remains a map[string]bool of lowercase extensions with leading .; every new key follows that convention.
  • Both test tables (TestIsAllowedExtension_AllEntries and TestIsAllowedExtension_NewlyAddedExtensions) are updated in the same commit, so the drift-guard test cannot silently lag the production map.
  • .odp is correctly placed next to existing .odt/.ods (was the only ODF gap).
  • No collision with blockedExtensions (const.go:156) — the new tests assert this explicitly via assert.False(t, IsBlockedExtension(ext)).
  • CI Build/Test/Vet/Lint all green; only unrelated check-sprint workflow failed (sprint metadata, not code).

Readability

  • New entries are grouped by category with Chinese comments matching the file's existing style (// 图片, // 文档, // 音频, // 视频).
  • Added an // Apple iWork(基于 zip 的文档,无执行风险) annotation — useful because that group isn't obvious from the extensions alone, and documents why they're considered safe.
  • Test file mirrors the same category grouping; reviewer can map test entries 1:1 to const entries.

Maintainability

  • The pattern of "add to map → add to AllEntries drift guard → add positive+negative case to a dedicated test" is preserved. Future additions follow the same recipe.
  • No new abstractions introduced. The change resists the temptation to refactor the map into per-category sub-maps — correct restraint for a 20-entry whitelist.
  • The negative test for .svg/.xlsm/.docm/.pptm/.iso/.img doubles as living documentation of "things we explicitly considered and rejected" — saves the next contributor from re-litigating issue #460.

Design fit

  • Aligns with linked issue #460's recommended scope verbatim. PR body lists added vs. deliberately-excluded matching the issue's risk classification.
  • Env override (DM_FILE_EXTRA_ALLOWED, const.go:175) is left untouched — operators retain the same escape hatch; defaults just got more sensible.
  • The "front-end blacklist vs back-end whitelist" inconsistency raised in #460's §2 is not addressed here, which is fine — the PR scope is "fix defaults", not "unify the two gates". A follow-up issue for the front-end alignment would be reasonable but isn't a blocker.

Nits (non-blocking)

  • The // 现代图片 comment is implicit in the diff (the new image entries land between the // 图片 block and the next group). Adding an inline // 现代图片 would mirror the iWork annotation. Trivial.
  • TestIsAllowedExtension_NewlyAddedExtensions could share a single ext-list constant with TestIsAllowedExtension_AllEntries for the new additions to avoid two places to update. Trade-off (clarity vs DRY) — current shape is fine and arguably clearer.

Verdict rationale

Single-purpose, well-tested, well-scoped change that does exactly what the linked issue asked for and nothing else. APPROVE.

@lml2468 lml2468 added review:done:code:approve code-reviewer APPROVED and removed review:running:code code-reviewer review in progress labels Jun 27, 2026
@lml2468

lml2468 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Aggregate Verdict: RISKED — needs-human-review

Three reviewer verdicts are in; no CHANGES_REQUESTED / BLOCKED / FAIL, but one CLEARED-WITH-RISK from security ⇒ verdict is RISKED, not auto-APPROVED. A human must review the residual risk before merge.

Verdict matrix

Role Persona Verdict Signal
QA qa-engineer PASS All new/excluded extensions covered by TestIsAllowedExtension_NewlyAddedExtensions + roster updated in TestIsAllowedExtension_AllEntries; CI Test job SUCCESS.
Security security-engineer CLEARED-WITH-RISK Whitelist additions pass STRIDE/OWASP-upload review; risky formats (.svg, Office macros, disk images) explicitly excluded. Residual risk: server is the last trust boundary — accepting .heic/.tiff/iWork/.epub widens the attack surface for downstream parsers/renderers. Out-of-scope follow-ups (no decoder hardening here) listed in security verdict.
Code code-reviewer APPROVE Pure data + test additions in modules/file/const.go (+11/-1) and const_test.go (+37/-1); no collisions with blockedExtensions; tests assert both allow-set and excluded-set behaviour.

Residual risk (from security verdict — please review before merge)

  • New formats are accepted as bytes server-side; any rendering/preview/thumbnailing path (e.g. HEIC→JPEG transcode, TIFF preview, EPUB unpack) must remain isolated. This PR does not introduce parsers, but downstream consumers should be audited if they exist.
  • .epub/.mobi/iWork (.key/.numbers/.pages) are zip-based containers — depth/size limits in any unzip path apply.
  • Magic-byte verification / content-type sniffing is not added here; whitelist is filename-extension-only, same as before for existing entries. Acceptable for this scope; tracked as broader follow-up.

Pre-existing failing check (NOT a review verdict)

check-sprint / check-sprint is FAILURE — this is a separate process gate (sprint scoping) flagged by the repo's OctoBoooot bot, not by the 3-persona review. Out of scope for this aggregate; human merger must satisfy the gate or get a sprint override before merging.

Decision

  • review:complete applied (3 verdicts aggregated).
  • 🟣 needs-human-review already present — retained.
  • No merge by automation. A human must (a) accept the security with-risk notes and (b) clear the check-sprint gate.

@lml2468 lml2468 added the review:complete 3 verdicts aggregated, awaiting human merge label Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human-review review:complete 3 verdicts aggregated, awaiting human merge review:done:code:approve code-reviewer APPROVED review:done:qa:approve qa-engineer PASS review:done:security:comment security-engineer CLEARED-WITH-RISK size/M PR size: M stage:review Review phase - QA/security/code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(file): 扩充上传扩展名白名单 — 补全 Apple/.heic 等无风险格式,对齐前后端口径

6 participants