Skip to content

Commit a5968ff

Browse files
waleedlatif1claude
andcommitted
chore(security): tighten inline comments in CSV export and KB file authorization
Condense verbose comment blocks to concise TSDoc/single-line form; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 8b7dcab commit a5968ff

3 files changed

Lines changed: 11 additions & 25 deletions

File tree

apps/sim/app/api/files/authorization.test.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,7 @@ describe('verifyKBFileAccess', () => {
6767
})
6868

6969
it('denies access via an external URL that merely contains the key as a substring', async () => {
70-
// The demonstrated PoC: a planted document whose fileUrl is an unrelated
71-
// external URL containing the victim key. It must never authorize storage.
70+
// PoC: a planted external URL containing the victim key must never authorize storage.
7271
dbChainMockFns.limit.mockResolvedValueOnce([
7372
{
7473
workspaceId: 'ws-attacker',
@@ -83,8 +82,7 @@ describe('verifyKBFileAccess', () => {
8382
})
8483

8584
it('denies a later document planted in another workspace (ownership pins to earliest doc)', async () => {
86-
// db query orders by uploadedAt asc, so the owning (victim) document comes
87-
// first; the attacker's later internal-URL document must not grant access.
85+
// Ordered by uploadedAt asc: the victim owns the key, so the attacker's later doc is ignored.
8886
dbChainMockFns.limit.mockResolvedValueOnce([
8987
{ workspaceId: 'ws-victim', fileUrl: internalUrlFor(VICTIM_KEY) },
9088
{ workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) },
@@ -142,8 +140,7 @@ describe('verifyKBFileAccess', () => {
142140
})
143141

144142
it('does not let a later workspace document override a null-workspace owner', async () => {
145-
// The earliest (owning) document belongs to a workspace-less KB; a later
146-
// document planted in the attacker's workspace must not become the owner.
143+
// Earliest owner has no workspace; a later attacker-workspace doc must not become owner.
147144
dbChainMockFns.limit.mockResolvedValueOnce([
148145
{ workspaceId: null, fileUrl: internalUrlFor(VICTIM_KEY) },
149146
{ workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) },

apps/sim/app/api/files/authorization.ts

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -412,9 +412,7 @@ async function verifyKBFileAccess(
412412
customConfig?: StorageConfig
413413
): Promise<boolean> {
414414
try {
415-
// The `LIKE` predicate only narrows candidates cheaply; it never decides
416-
// authorization. Ordering by `uploadedAt` lets us pin ownership to the
417-
// earliest document, which is the one the file was originally uploaded for.
415+
// LIKE only narrows candidates; ownership is decided below, pinned to the earliest upload.
418416
const candidateDocuments = await db
419417
.select({
420418
workspaceId: knowledgeBase.workspaceId,
@@ -437,12 +435,8 @@ async function verifyKBFileAccess(
437435
.orderBy(asc(document.uploadedAt))
438436
.limit(50)
439437

440-
// The owner is the earliest document whose fileUrl resolves to EXACTLY this
441-
// storage key. Substring-only matches (e.g. an external URL that happens to
442-
// contain the key) and documents in other workspaces that merely reference
443-
// the key do not establish ownership. Pinning to the earliest match means a
444-
// later document planted in the caller's own workspace can never authorize
445-
// access to a file another workspace originally uploaded.
438+
// Owner is the earliest document whose fileUrl resolves to EXACTLY this key; substring
439+
// matches and cross-workspace references never establish ownership.
446440
const owningDocument = candidateDocuments.find(
447441
(doc) => resolveInternalKbKey(doc.fileUrl) === cloudKey
448442
)
@@ -477,9 +471,7 @@ async function verifyKBFileAccess(
477471
return false
478472
}
479473

480-
// KB file access must resolve through an active KB document that canonically
481-
// owns the key. Metadata alone is not enough because parent archives
482-
// intentionally keep the underlying file bytes around for history.
474+
// No owning document: metadata only lets us flag the deleted-file case; it never grants access.
483475
const fileRecord = await getFileMetadataByKey(cloudKey, 'knowledge-base', {
484476
includeDeleted: true,
485477
})

apps/sim/app/api/table/[tableId]/export/route.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -114,19 +114,16 @@ function sanitizeFilename(name: string): string {
114114
}
115115

116116
/**
117-
* Prefixes a single quote to values that spreadsheet applications would otherwise
118-
* interpret as formulas, preventing CSV formula injection when an exported file is
119-
* opened in Excel, LibreOffice, or Google Sheets. JSON exports preserve raw values.
117+
* Prefixes a single quote to values starting with a spreadsheet formula trigger
118+
* (`=`, `+`, `-`, `@`, tab, CR), neutralizing CSV injection in Excel/Sheets.
120119
*/
121120
function neutralizeCsvFormula(value: string): string {
122121
return /^[=+\-@\t\r]/.test(value) ? `'${value}` : value
123122
}
124123

125124
/**
126-
* Serializes a cell for CSV output. Only string cells are run through
127-
* {@link neutralizeCsvFormula}: numbers, booleans, dates, and JSON-serialized
128-
* objects can never stringify into a spreadsheet formula trigger, so they are
129-
* emitted verbatim to preserve fidelity (e.g. negative numbers stay numeric).
125+
* Serializes a cell for CSV. Only string cells are formula-neutralized; numbers,
126+
* booleans, dates, and JSON objects can never form a trigger and pass through verbatim.
130127
*/
131128
function formatCsvValue(value: unknown): string {
132129
if (value === null || value === undefined) return ''

0 commit comments

Comments
 (0)