Skip to content

Commit ee22760

Browse files
committed
fix(microsoft-word): fail closed when a document reports no version to compare
1 parent 0511698 commit ee22760

4 files changed

Lines changed: 96 additions & 12 deletions

File tree

apps/sim/app/api/tools/microsoft_word/append/route.test.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,65 @@ describe('POST /api/tools/microsoft_word/append', () => {
172172
)
173173
})
174174

175+
it('refuses to write when Graph reports no version to compare against', async () => {
176+
const untagged = {
177+
id: 'doc-abc',
178+
name: 'notes.docx',
179+
file: {
180+
mimeType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
181+
},
182+
}
183+
mockSecureFetchWithPinnedIP.mockResolvedValueOnce({
184+
ok: true,
185+
status: 200,
186+
statusText: '',
187+
headers: new Headers(),
188+
body: null,
189+
text: async () => JSON.stringify(untagged),
190+
json: async () => untagged,
191+
arrayBuffer: async () => new ArrayBuffer(0),
192+
})
193+
194+
const response = await POST(createMockRequest('POST', baseBody))
195+
196+
expect(response.status).toBe(409)
197+
const data = (await response.json()) as { error: string }
198+
expect(data.error).toMatch(/did not report a version/)
199+
expect(mockSecureFetchWithPinnedIP.mock.calls.some((call) => call[2]?.method === 'PUT')).toBe(
200+
false
201+
)
202+
})
203+
204+
it('refuses to write when the re-read reports no version', async () => {
205+
const untagged = {
206+
id: 'doc-abc',
207+
name: 'notes.docx',
208+
file: {
209+
mimeType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
210+
},
211+
}
212+
mockSecureFetchWithPinnedIP
213+
.mockResolvedValueOnce(itemResponse('tag-1'))
214+
.mockResolvedValueOnce(await docxResponse())
215+
.mockResolvedValueOnce({
216+
ok: true,
217+
status: 200,
218+
statusText: '',
219+
headers: new Headers(),
220+
body: null,
221+
text: async () => JSON.stringify(untagged),
222+
json: async () => untagged,
223+
arrayBuffer: async () => new ArrayBuffer(0),
224+
})
225+
226+
const response = await POST(createMockRequest('POST', baseBody))
227+
228+
expect(response.status).toBe(409)
229+
expect(mockSecureFetchWithPinnedIP.mock.calls.some((call) => call[2]?.method === 'PUT')).toBe(
230+
false
231+
)
232+
})
233+
175234
it('surfaces a 412 from the upload precondition as the same conflict', async () => {
176235
mockSecureFetchWithPinnedIP
177236
.mockResolvedValueOnce(itemResponse('tag-1'))

apps/sim/app/api/tools/microsoft_word/append/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
assertContentUnchanged,
1111
downloadDocumentContent,
1212
fetchDocumentItem,
13-
getContentTag,
13+
requireContentTag,
1414
toDocumentMetadata,
1515
uploadDocumentContent,
1616
} from '@/lib/microsoft-word/graph.server'
@@ -45,7 +45,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
4545
try {
4646
const basePath = getDocumentBasePath(documentId, driveId ?? undefined)
4747
const existingItem = await fetchDocumentItem(basePath, accessToken)
48-
const contentTag = getContentTag(existingItem)
48+
const contentTag = requireContentTag(existingItem)
4949

5050
const existingBuffer = await downloadDocumentContent(basePath, accessToken)
5151
const { buffer, paragraphsAppended } = await appendParagraphsToDocx(existingBuffer, content)

apps/sim/app/api/tools/microsoft_word/replace-text/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
assertContentUnchanged,
1111
downloadDocumentContent,
1212
fetchDocumentItem,
13-
getContentTag,
13+
requireContentTag,
1414
toDocumentMetadata,
1515
uploadDocumentContent,
1616
} from '@/lib/microsoft-word/graph.server'
@@ -46,7 +46,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
4646
try {
4747
const basePath = getDocumentBasePath(documentId, driveId ?? undefined)
4848
const existingItem = await fetchDocumentItem(basePath, accessToken)
49-
const contentTag = getContentTag(existingItem)
49+
const contentTag = requireContentTag(existingItem)
5050

5151
const existingBuffer = await downloadDocumentContent(basePath, accessToken)
5252
const { buffer, occurrencesChanged } = await replaceTextInDocx(

apps/sim/lib/microsoft-word/graph.server.ts

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,24 +191,49 @@ export function documentChangedError(): GraphRequestError {
191191
return new GraphRequestError(CONFLICT_MESSAGE, 409)
192192
}
193193

194+
/** Message shown when the document carries no version to compare against. */
195+
const UNVERIFIABLE_MESSAGE =
196+
'Microsoft Graph did not report a version for this document, so Sim cannot confirm the edit would not overwrite someone else’s change and did not apply it. Use Replace Content if you intend to overwrite the document outright.'
197+
198+
/**
199+
* Raised when there is no version to compare, rather than writing unguarded.
200+
*
201+
* Graph returns `cTag` for every file and `eTag` for every drive item, so this
202+
* should not be reachable in practice — but a read-modify-write that silently
203+
* degrades to no protection is the failure this whole guard exists to prevent,
204+
* so the missing-version path fails closed instead of proceeding.
205+
*/
206+
function unverifiableDocumentError(): GraphRequestError {
207+
return new GraphRequestError(UNVERIFIABLE_MESSAGE, 409)
208+
}
209+
210+
/**
211+
* Returns the content tag an edit must be based on, refusing the edit outright
212+
* when the item carries none.
213+
*/
214+
export function requireContentTag(item: GraphDriveItem): string {
215+
const tag = getContentTag(item)
216+
if (!tag) {
217+
throw unverifiableDocumentError()
218+
}
219+
return tag
220+
}
221+
194222
/**
195223
* Aborts a read-modify-write when the document's content changed since it was
196224
* read. Without this, two overlapping edits both succeed and the later upload
197225
* silently discards the earlier one.
198226
*
199-
* `expected` being undefined means Graph returned neither tag for this item, so
200-
* there is nothing to compare and the caller proceeds unguarded — the
201-
* `if-match` header on the upload is the remaining line of defense.
227+
* Every branch fails closed: a changed tag, and equally a re-read that reports
228+
* no tag at all, both refuse the write rather than fall through to it.
202229
*/
203230
export async function assertContentUnchanged(
204231
basePath: string,
205232
accessToken: string,
206-
expected: string | undefined
233+
expected: string
207234
): Promise<void> {
208-
if (!expected) return
209-
210-
const current = getContentTag(await fetchDocumentItem(basePath, accessToken))
211-
if (current && current !== expected) {
235+
const current = requireContentTag(await fetchDocumentItem(basePath, accessToken))
236+
if (current !== expected) {
212237
throw documentChangedError()
213238
}
214239
}

0 commit comments

Comments
 (0)