Skip to content

fix(cdp): block attachment file types hidden behind a trailing dot or space - #130685

Closed
Silthus wants to merge 2 commits into
masterfrom
fix/attachment-filename-trailing-dot
Closed

Silthus wants to merge 2 commits into
masterfrom
fix/attachment-filename-trailing-dot

Conversation

@Silthus

@Silthus Silthus commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • A code send can attach a blocked file type behind a trailing dot or space, such as setup.exe..
  • Windows drops the trailing characters on save, so the recipient gets a runnable setup.exe.
  • Reported on #128871.

Changes

  • cdp-api refuses these names with the 400 it gives setup.EXE.
  • The check reads the extension as Windows saves it. Django's check already does, so it needs no change.
  • Rejecting all trailing dots and spaces would also refuse report.pdf., which Django accepts.

Caution

🔴 Merge risk: high · two-way door (merge-risk rubric)

  • A bug in the check refuses code sends with attachments, on the email send path.
  • Django already refuses the same names, and the serial tests cover both outcomes.
  • Revert: git revert <merge commit>.

How did you test this code?

Test rationale: two rows in the existing refuses a code send with table. Both fail on master (202).

  • Live run: base cdp-api accepts both names, HEAD refuses them.
  • Not run: the Django suite: no Python changed.
Live run against cdp-api, base and HEAD

cdp-api ran in dev mode on an ephemeral box, with PersonHog pointed at an unused address, because validation runs before any person lookup. Each request is a code send with one attachment. "Team not found" means validation passed and the flow lookup ran, because the box has no team 1.

Filename Base HEAD
setup.exe. 404 "Team not found" 400 "mailbox providers block this file type"
setup.exe 404 "Team not found" 400 "mailbox providers block this file type"
setup.EXE 400 "mailbox providers block this file type" 400 "mailbox providers block this file type"
report.pdf 404 "Team not found" 404 "Team not found"

Release status

  • No feature flag controls this change

Docs update

  • None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5 (claude-opus-5-5)

Skills: /writing-tests, /writing-code-comments, /writing-pr-descriptions

More context for AI agents
  • Session: T3 Code side-fix thread, handed off from the scheduled-sends stack thread.
  • Decisions: strip trailing dots and spaces for the check instead of rejecting them, to match Django's _extension_as_windows_saves_it.
  • Decisions: strip in a loop rather than with /[. ]+$/, because Zod still runs refinements after max(255) fails.
  • Follow-ups: #128871 moves AttachmentSchema into its own module, so its stack carries this change on its next base merge.
  • Read closely: extensionAsWindowsSavesIt in nodejs/src/cdp/services/hogflows/transactional-email.ts.

🤖 Generated with Claude Code

… space

Windows drops trailing dots and spaces when it saves a file, so an
attachment named `setup.exe.` or `setup.exe ` passed the blocked file
type check and was saved as a runnable `setup.exe`. Read the extension
the saved file will have, the same way the Django code send check does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Silthus Silthus self-assigned this Nov 6, 2026
@trunk-io

trunk-io Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

🚫 This pull request was removed from the merge queue because it was canceled by @Silthus. See more details here.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane

This PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

@Silthus

Silthus commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@PostHog review

@posthog

posthog Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

@Silthus PostHog Review is reviewing this pull request.

@posthog posthog Bot 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.

PostHog Review · Standard · reviewed 638ddce: no findings.

@Silthus
Silthus marked this pull request as ready for review November 6, 2026 07:26
@parameterai

parameterai Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Risk: No findings

This increment merges current master into the branch; the PR's own change is still just the attachment-filename validator in transactional-email.ts, which now reads the extension the way Windows saves it (stripping trailing dots and spaces) before checking the blocked-extension list, plus two test rows. I traced the validator's edge cases (trailing dot/space chains, all-dot names, leading dots, case folding, no-extension names) against the blocked set and the parameterized attachment storage/SES header path, and found no bypass: the path-separator and control-character refines at transactional-email.ts:161 and the blocked-extension refine at :166 together cover every shape I could construct. The fieldNotAcceptedError helper the merge diff surfaces is master code with a single call site using a hardcoded literal, so no injection surface.

Sentinel reviewed 39c934a · Review settings

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team November 6, 2026 07:27
@Silthus

Silthus commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

/trunk merge

@veria-ai

veria-ai Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Reviews

@Silthus

Silthus commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

/trunk merge

@Silthus

Silthus commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

/trunk cancel

@Silthus

Silthus commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Closing: #131567 removed inline email attachments from cdp-api, so the filename check this PR fixes no longer exists. Django's attachment check already reads the extension the same way.

@Silthus Silthus closed this Nov 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants