Skip to content

fix(security): restore canonical CSV control-prefix boundary - #1177

Draft
seonghobae wants to merge 3 commits into
developfrom
fix/csv-formula-control-prefix-1103-successor
Draft

fix(security): restore canonical CSV control-prefix boundary#1177
seonghobae wants to merge 3 commits into
developfrom
fix/csv-formula-control-prefix-1103-successor

Conversation

@seonghobae

Copy link
Copy Markdown
Collaborator

Canonical successor for #1103 after that branch accumulated unrelated workflow/supply-chain reversions and became non-mergeable.

Scope

This branch starts directly from protected develop@314ddeae7b775a4957594b599358c8255617eb2e and carries only the canonical desktop CSV-cell security delta:

  • apps/desktop/src/lib/export.test.ts: executable regressions for NUL/repeated-NUL/NUL-only cells, generic leading C0 controls such as ESC/BEL, and full-width =+-@ operators while preserving the existing ASCII/whitespace/structural quoting cases.
  • apps/desktop/src/lib/export.ts: escapeCsvField fails closed when the first significant token is any C0 control (\x00-\x1F), ASCII formula initiator, or the supported full-width operator variants; it prefixes the original value before ordinary CSV quoting.
  • .jules/sentinel.md: records the bounded parser-disagreement threat model without claiming that the mitigation alone proves arbitrary code execution or a severity upgrade.

RED → fix

  • RED a9c2913f9ea52ec3f654b88a902b3ee10a9a4c6c restores the C0/full-width regression contract on the clean protected base while production is still the narrower protected implementation.
  • Fix 1157ba9739cef0b72810cc02a95e91b1902f376c restores the production predicate.
  • Doctoring f9875aed9403d6372297d107f78b2f7a0807ec10 records the actual C0 trust boundary.

Succession

This clean successor preserves all valid semantic/test/documentation delta previously owned by #1103, including the earlier #1121 NUL regression, #941 full-width operator behavior, and #1171 generic-C0 regression. It intentionally does not inherit #1103's unrelated 19-file workflow/supply-chain divergence, predecessor checks, reviews, statuses, or mergeability claims.

#1175's NUL-only source/test delta is a strict subset of this branch. Its stronger remote-code-execution wording is not inherited because spreadsheet behavior is consumer-dependent and the repository evidence only supports a defense-in-depth parser-disagreement claim.

Merge gate

Keep Draft until this unchanged exact head has terminal applicable CI/security/SBOM/cross-platform evidence, zero actionable review findings, a qualifying independent non-author last-push approval, and the protected-context contract is satisfiable. No self-approval, force-push, destructive rebase, duplicate local CodeQL scanner, bypass, or gate weakening.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 43 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 24c87f51-0c05-42fb-bab2-ea16e12a5514

📥 Commits

Reviewing files that changed from the base of the PR and between 314ddea and f9875ae.

📒 Files selected for processing (3)
  • .jules/sentinel.md
  • apps/desktop/src/lib/export.test.ts
  • apps/desktop/src/lib/export.ts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

Please review exact head f9875aed9403d6372297d107f78b2f7a0807ec10, especially the bounded CSV parser-disagreement claim, C0/full-width initiator coverage, preservation of structural CSV quoting, and whether any valid security/test delta from superseded #1103/#1175 is missing. Do not treat predecessor checks/reviews as current-head evidence.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='f9875aed9403d6372297d107f78b2f7a0807ec10'
base='314ddeae7b775a4957594b599358c8255617eb2e'
repo='ContextualWisdomLab/bandscope'

echo '== Exact HEAD and base =='
git rev-parse HEAD
git show -s --format='%H%n%P%n%s' "$head"
git show -s --format='%H%n%s' "$base"

echo '== Current-head files changed from protected base =='
git diff --name-status "$base" "$head"
git diff --check "$base" "$head"
git diff --no-ext-diff --unified=80 "$base" "$head" -- \
  apps/desktop/src/lib/export.ts \
  apps/desktop/src/lib/export.test.ts \
  .jules/sentinel.md

echo '== Current source/test/documentation structure =='
ast-grep outline apps/desktop/src/lib/export.ts --items all
ast-grep outline apps/desktop/src/lib/export.test.ts --items all
nl -ba apps/desktop/src/lib/export.ts | sed -n '1,260p'
nl -ba apps/desktop/src/lib/export.test.ts | sed -n '1,360p'
nl -ba .jules/sentinel.md | sed -n '1,260p'

echo '== CSV escaping call sites and related tests =='
rg -n -C 4 --glob '!node_modules/**' \
  '\bescapeCsvField\s*\(|CSV|csv|full.?width|C0|NUL|formula' \
  apps/desktop/src .jules

echo '== PR `#1103` and `#1175` metadata, changed files, and patches =='
for pr in 1103 1175; do
  echo "-- PR #$pr --"
  gh pr view "$pr" --repo "$repo" --json number,state,isDraft,baseRefOid,headRefOid,title,body
  gh api "repos/$repo/pulls/$pr/files?per_page=100" \
    --jq '.[] | {filename,status,additions,deletions,patch}'
done

Length of output: 20647


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='f9875aed9403d6372297d107f78b2f7a0807ec10'
base='314ddeae7b775a4957594b599358c8255617eb2e'
repo='ContextualWisdomLab/bandscope'

echo '== Required current-head files =='
for path in \
  apps/desktop/src/lib/export.ts \
  apps/desktop/src/lib/export.test.ts \
  .jules/sentinel.md
do
  echo "-- $path --"
  sed -n '1,420p' "$path"
done

echo '== Exact current-head patch, with zero context for semantic inventory =='
git diff --no-ext-diff --unified=0 "$base" "$head" -- \
  apps/desktop/src/lib/export.ts \
  apps/desktop/src/lib/export.test.ts \
  .jules/sentinel.md

echo '== Escape function call sites and CSV-export construction =='
rg -n -C 5 --glob '!node_modules/**' \
  '\bescapeCsvField\s*\(|\bgenerateCueSheetCsv\s*\(' \
  apps/desktop/src

echo '== Predecessor PR metadata and relevant patch inventory =='
for pr in 1103 1175; do
  echo "-- PR #$pr metadata --"
  gh pr view "$pr" --repo "$repo" \
    --json number,state,isDraft,baseRefOid,headRefOid,title,body \
    --jq '{number,state,isDraft,baseRefOid,headRefOid,title,body}'

  echo "-- PR #$pr relevant changed files and patches --"
  gh api --paginate "repos/$repo/pulls/$pr/files?per_page=100" \
    --jq '.[] | select(.filename == "apps/desktop/src/lib/export.ts" or .filename == "apps/desktop/src/lib/export.test.ts" or .filename == ".jules/sentinel.md") | {filename,status,additions,deletions,patch}'
done

echo '== Current base implementation for comparison =='
git show "$base:apps/desktop/src/lib/export.ts" | sed -n '1,100p'
git show "$base:apps/desktop/src/lib/export.test.ts" | sed -n '35,150p'

Length of output: 61771


@seonghobae Reviewed exact head f9875aed9403d6372297d107f78b2f7a0807ec10.

No actionable findings.

  • The head matches the requested commit.
  • The diff from develop@314ddeae7b775a4957594b599358c8255617eb2e is limited to the three stated files.
  • escapeCsvField fails closed for every leading C0 control (\x00\x1F), ASCII =, +, -, @, and full-width , , , .
  • The function prefixes the original value before existing CSV quoting. Existing comma, quote, newline, and carriage-return behavior remains unchanged.
  • Tests cover NUL-prefixed, repeated-NUL, NUL-only, ESC-prefixed, BEL-prefixed, ESC-only, and full-width cases. The existing combined structural-quoting tests remain present.
  • The Sentinel entry keeps the claim bounded to downstream parser or representation disagreement. It does not assert consumer-independent code execution or a severity increase.
  • #1103’s valid three-file security, test, and documentation delta is present.
  • #1175 is a strict NUL-only subset. Its stronger remote-code-execution wording is correctly not inherited.

I did not use predecessor checks, reviews, statuses, or mergeability as evidence. Current-head CI, security, SBOM, cross-platform, and approval gates still need their own exact-head evidence.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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.

1 participant