diff --git a/CHANGELOG.md b/CHANGELOG.md index 90b71450..de20cfe0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,12 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). The format is based on [Keep a Changelog](http://keepachangelog.com/). -## Version 3.10.0 +## [Upcoming] Version 3.10.0 + +### Added + +- Emit the following security events on the attachments service: - AttachmentDownloadRejected, AttachmentSizeExceeded AttachmentUploadRejected +- If `@cap-js/audit-logging` is installed automatically trigger audit logs for the security events ### Fixed diff --git a/README.md b/README.md index 4c1248d6..8e53095d 100755 --- a/README.md +++ b/README.md @@ -242,6 +242,23 @@ According to the recommendation of the [Malware Scanning Service](http://help.sa By default, `scanExpiryMs` is set to `259200000` milliseconds (3 days). Downloading an attachment is not permitted unless its status is `Clean`. +### Audit logging + +The attachment service emits the following three events: + +- AttachmentDownloadRejected, +- AttachmentSizeExceeded, +- AttachmentUploadRejected + +When `@cap-js/audit-logging` is a dependency of your app, the three events will be automatically logged as security events in the audit log service. + +You can register custom handlers for the three events by writing: + +```js +const attachments = await cds.connect.to("attachments") +attachments.on("AttachmentDownloadRejected", (msg) => {}) +``` + ### Visibility Control for Attachments UI Facet Generation By setting the `@UI.Hidden` property to `true`, developers can hide the visibility of the plugin in the UI. This feature is particularly useful in scenarios where the visibility of the plugin needs to be dynamically controlled based on certain conditions. diff --git a/lib/generic-handlers.js b/lib/generic-handlers.js index 145be6e9..9d312889 100644 --- a/lib/generic-handlers.js +++ b/lib/generic-handlers.js @@ -151,6 +151,7 @@ async function validateAttachment(req) { if (scanEnabled) { if (status !== "Clean") { + const ipAddress = req.req?.socket?.remoteAddress cds.spawn(async () => { try { const srv = await cds.connect.to("attachments") @@ -158,6 +159,7 @@ async function validateAttachment(req) { target: req.target.name, keys: { ID: attachmentId }, status, + ipAddress, }) } catch (err) { LOG.error("Failed to emit AttachmentDownloadRejected", err) @@ -274,6 +276,22 @@ async function validateAttachmentSize(req, validateContentLength = false) { const attachmentRef = await SELECT.one("filename") .from(req.target) .where({ up__ID: req.data.up__ID }) + const ipAddress = req.req?.socket?.remoteAddress + cds.spawn(async () => { + try { + const AttachmentsSrv = await cds.connect.to("attachments") + await AttachmentsSrv.emit("AttachmentSizeExceeded", { + target: req.target.name, + keys: req.data.ID ? { ID: req.data.ID } : {}, + filename: attachmentRef?.filename ?? "n/a", + fileSize: length, + maxFileSize, + ipAddress, + }) + } catch (err) { + LOG.error("Failed to emit AttachmentSizeExceeded", err) + } + }) cds.spawn(async () => { try { @@ -316,6 +334,7 @@ function validateAttachmentMimeType(req) { const acceptableMediaTypes = req.target.elements.content["@Core.AcceptableMediaTypes"] || "*/*" if (!checkMimeTypeMatch(acceptableMediaTypes, mimeType)) { + const ipAddress = req.req?.socket?.remoteAddress cds.spawn(async () => { try { const AttachmentsSrv = await cds.connect.to("attachments") @@ -326,6 +345,7 @@ function validateAttachmentMimeType(req) { mimeType, acceptableMediaTypes, reason: `MIME type '${mimeType}' is not in @Core.AcceptableMediaTypes`, + ipAddress, }) } catch (err) { LOG.error("Failed to emit AttachmentUploadRejected", err) diff --git a/srv/azure-blob-storage.js b/srv/azure-blob-storage.js index 11e5128d..040fd26f 100644 --- a/srv/azure-blob-storage.js +++ b/srv/azure-blob-storage.js @@ -317,9 +317,18 @@ module.exports = class AzureAttachmentsService extends ( ) const blobClient = containerClient.getBlockBlobClient(blobName) - const response = await blobClient.delete() + let response + try { + response = await blobClient.delete() + } catch (error) { + if (error.statusCode === 404) { + response = error + } else { + throw error + } + } - if (response._response.status !== 202) { + if (response._response?.status !== 202) { LOG.warn("File has not been deleted from Azure Blob Storage", { blobName, containerName: containerClient.containerName, diff --git a/srv/basic.js b/srv/basic.js index 4ab063dc..0370b7d4 100644 --- a/srv/basic.js +++ b/srv/basic.js @@ -1,5 +1,6 @@ const cds = require("@sap/cds") const LOG = cds.log("attachments") +const DEBUG = cds.debug("attachments") const { computeHash, traverseEntity, @@ -46,6 +47,26 @@ class AttachmentsService extends cds.Service { ) } }) + + if (cds.env.requires["audit-log"]) { + DEBUG && DEBUG(`Register audit logging handlers for security events.`) + this.on( + [ + "AttachmentDownloadRejected", + "AttachmentSizeExceeded", + "AttachmentUploadRejected", + ], + async (msg) => { + const audit = await cds.connect.to("audit-log") + const { ipAddress, ...eventData } = msg.data + await audit.log("SecurityEvent", { + data: { event: msg.event, ...eventData }, + ip: ipAddress || undefined, + }) + }, + ) + } + return super.init() } @@ -276,12 +297,12 @@ class AttachmentsService extends cds.Service { for (const attachment of req.attachmentsToDelete) { if (attachment.url) { const attachmentsSrv = await cds.connect.to("attachments") - LOG.info( + LOG.debug( "[deleteAttachmentsWithKeys] Emitting DeleteAttachment for:", attachment.url, ) await attachmentsSrv.emit("DeleteAttachment", attachment) - LOG.info( + LOG.debug( "[deleteAttachmentsWithKeys] Emitted DeleteAttachment for:", attachment.url, ) @@ -292,7 +313,7 @@ class AttachmentsService extends cds.Service { ) } } - LOG.info("[deleteAttachmentsWithKeys] Finished") + LOG.debug("[deleteAttachmentsWithKeys] Finished") } /** diff --git a/srv/gcp.js b/srv/gcp.js index 209a791c..45477782 100644 --- a/srv/gcp.js +++ b/srv/gcp.js @@ -348,8 +348,17 @@ module.exports = class GoogleAttachmentsService extends ( ) const file = bucket.file(blobName) - const response = await file.delete() - if (response[0]?.statusCode !== 204) { + let response + try { + response = await file.delete() + } catch (error) { + if (error.statusCode === 404) { + response = error + } else { + throw error + } + } + if (response?.[0]?.statusCode !== 204) { LOG.warn("File has not been deleted from Google Cloud Storage", { blobName, bucketName: bucket.name, diff --git a/tests/incidents-app/package.json b/tests/incidents-app/package.json index 36914d46..6b612008 100644 --- a/tests/incidents-app/package.json +++ b/tests/incidents-app/package.json @@ -3,6 +3,7 @@ "version": "1.0.0", "dependencies": { "@cap-js/attachments": "file:../../.", + "@cap-js/audit-logging": "^1.2.0", "@cap-js/hana": "2.6.0", "@cap-js/postgres": "^2.2.0" }, @@ -13,6 +14,10 @@ } }, "requires": { + "queue": { + "parallel": true, + "chunkSize": 20 + }, "attachments": { "scanExpiryMs": 30000 }, diff --git a/tests/unit/auditLogging.test.js b/tests/unit/auditLogging.test.js new file mode 100644 index 00000000..be223bd1 --- /dev/null +++ b/tests/unit/auditLogging.test.js @@ -0,0 +1,108 @@ +require("../../lib/csn-runtime-extension") +const cds = require("@sap/cds") +cds.env.requires["audit-log"] = { + impl: "@cap-js/audit-logging/srv/log2console", + outbox: false, +} +const { join } = cds.utils.path +const app = join(__dirname, "../incidents-app") +cds.test(app) + +let attachmentsSvc + +beforeEach(async () => { + const svc = await cds.connect.to("attachments") + attachmentsSvc = cds.unboxed(svc) +}) + +describe("Audit logging for security events (audit-logging dependency present)", () => { + const log = cds.test.log() + + it("should log AttachmentDownloadRejected as SecurityEvent", async () => { + await attachmentsSvc.emit("AttachmentDownloadRejected", { + target: "AdminService.Incidents.attachments", + keys: { ID: "att-001" }, + status: "Infected", + ipAddress: "10.0.0.1", + }) + + expect(log.output).toContain("[audit-log] - SecurityEvent:") + expect(log.output).toContain("AttachmentDownloadRejected") + expect(log.output).toContain("Infected") + }) + + it("should log AttachmentSizeExceeded as SecurityEvent", async () => { + await attachmentsSvc.emit("AttachmentSizeExceeded", { + target: "AdminService.Incidents.attachments", + keys: { ID: "att-002" }, + filename: "large-file.pdf", + fileSize: 999999999, + maxFileSize: 5242880, + ipAddress: "192.168.1.10", + }) + + expect(log.output).toContain("[audit-log] - SecurityEvent:") + expect(log.output).toContain("AttachmentSizeExceeded") + expect(log.output).toContain("large-file.pdf") + expect(log.output).toContain("999999999") + expect(log.output).toContain("5242880") + }) + + it("should log AttachmentUploadRejected as SecurityEvent", async () => { + await attachmentsSvc.emit("AttachmentUploadRejected", { + target: "AdminService.Incidents.attachments", + keys: { ID: "att-003" }, + filename: "script.exe", + mimeType: "application/x-msdownload", + acceptableMediaTypes: ["image/jpeg", "image/png"], + reason: + "MIME type 'application/x-msdownload' is not in @Core.AcceptableMediaTypes", + ipAddress: "172.16.0.5", + }) + + expect(log.output).toContain("[audit-log] - SecurityEvent:") + expect(log.output).toContain("AttachmentUploadRejected") + expect(log.output).toContain("script.exe") + expect(log.output).toContain("application/x-msdownload") + }) +}) + +describe("Audit logging when audit-logging is disabled", () => { + const log = cds.test.log() + + it("should not register audit log handlers when hasAuditLogging returns false", async () => { + // Override hasAuditLogging to return false + const originalLog = cds.env.requires["audit-log"] + cds.env.requires["audit-log"] = false + + // Create a fresh AttachmentsService instance with audit logging disabled + const AttachmentsService = require("../../srv/basic") + const svc = new AttachmentsService() + svc.model = cds.model + // Stub super.init() to avoid full service bootstrap + const origInit = Object.getPrototypeOf(AttachmentsService.prototype).init + Object.getPrototypeOf(AttachmentsService.prototype).init = jest + .fn() + .mockResolvedValue(undefined) + + await svc.init() + + // Restore super.init + Object.getPrototypeOf(AttachmentsService.prototype).init = origInit + + // The service should have handlers for DeleteAttachment and DeleteInfectedAttachment + // but NOT for the security events routed to audit logging + const registeredEvents = (svc._handlers?.on || []).map((h) => + Array.isArray(h.for) ? h.for : [h.for], + ) + const flatEvents = registeredEvents.flat().filter(Boolean) + + expect(flatEvents).not.toContain("AttachmentDownloadRejected") + expect(flatEvents).not.toContain("AttachmentSizeExceeded") + expect(flatEvents).not.toContain("AttachmentUploadRejected") + + // Verify no audit log output was produced + expect(log.output).not.toContain("[audit-log] - SecurityEvent:") + cds.env.requires["audit-log"] = originalLog + }) +}) diff --git a/tests/unit/rejectionEvents.test.js b/tests/unit/rejectionEvents.test.js index e3348dcb..0da5b435 100644 --- a/tests/unit/rejectionEvents.test.js +++ b/tests/unit/rejectionEvents.test.js @@ -87,7 +87,7 @@ describe("AttachmentUploadRejected event", () => { ) }) - it("should not emit when MIME type is allowed", async () => { + it("should not emit when MIME type is allowed", () => { const req = { target: { name: "TestService.Attachments", @@ -107,7 +107,7 @@ describe("AttachmentUploadRejected event", () => { reject: jest.fn(), } - const result = await validateAttachmentMimeType(req) + const result = validateAttachmentMimeType(req) expect(result).toBe(true) expect(req.reject).not.toHaveBeenCalled() @@ -136,7 +136,7 @@ describe("AttachmentUploadRejected event", () => { reject: jest.fn(), } - const result = await validateAttachmentMimeType(req) + const result = validateAttachmentMimeType(req) expect(result).toBe(false) expect(req.reject).toHaveBeenCalledWith( @@ -172,7 +172,7 @@ describe("AttachmentUploadRejected event", () => { reject: jest.fn(), } - await validateAttachmentMimeType(req) + validateAttachmentMimeType(req) // cds.spawn is called, proving the emit runs in a separate transaction expect(cds.spawn).toHaveBeenCalledWith(expect.any(Function))